Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intents on start & exit #2265

Closed
wants to merge 5 commits into from

Conversation

brunoais
Copy link
Contributor

@brunoais brunoais commented Apr 18, 2021

Send intents when scrcpy starts and when scrcpy exits

Still missing (will be added if feature accepted)

  • Mention feature in README
  • Create command line option to choose whether to send the intents (defaulting to off)

Extra: video showing a visual use:

https://www.mediafire.com/file/ktcxs0exclvoqha/intent_demo.mp4/file

(I was unable to attach the file in this post)

@brunoais brunoais changed the base branch from master to dev April 18, 2021 18:53
@brunoais brunoais changed the title Code for discussion: Intents on start exit Code for discussion: Intents on start & exit Apr 19, 2021
@brunoais brunoais force-pushed the intents_on_start_exit branch from 5fa9fb2 to 9105660 Compare April 19, 2021 18:57
@brunoais
Copy link
Contributor Author

brunoais commented May 5, 2021

@rom1v Anything that can be discussed about this already?

Comment on lines 23 to 33
ListIterator<String> iter = list.listIterator(list.size());
for (String e = iter.previous(); iter.hasPrevious(); e = iter.previous()) {
if(e == null){
// If there's no value, remove the entry
iter.remove();
iter.previous();
iter.remove();
iter.previous();
iter.remove();
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it's easier to read if just adding all and then remove the unused ones than to just add the ones provided.
Doing the other way is fine too

@rom1v
Copy link
Collaborator

rom1v commented Jun 6, 2021

Sorry, I have no much time to work on scrcpy presently.

I'm ok with sending a new intent on start and close, but IMO:

  • it should be enabled only with an explicit parameter
  • the implementation should send the intent directly (without executing a new program am broadcast …, but calling the functions used by am directly); see this old WIP commit: 160ae02

Regards

@brunoais
Copy link
Contributor Author

brunoais commented Jun 6, 2021

Sorry, I have no much time to work on scrcpy presently.

Sad but understood. Must be really bad that can't even comment. Thanks for taking the time for that comment.

it should be enabled only with an explicit parameter

Updated the top comment with that in mind.

the implementation should send the intent directly see this old WIP commit: 160ae02

Got it! I'll keep that in mind. Thank you for that commit. It should be enough for what I'm aiming at. I didn't know what could be done.

Thank you for giving me what I wanted and needed to make this into scrcpy content. (I.e: if approved, if going a right way and what way to go if not approved)

@rom1v
Copy link
Collaborator

rom1v commented Jun 6, 2021

Must be really bad that can't even comment.

I still can (and often) answer to comments/issues.

About this one, I didn't answer immediately because in my TODO list I wanted to improve the drag&drop behavior by sending an intent after "adb push" a file (the equivalent of adb shell am broadcast -a android.intent.action.MEDIA_SCANNER_SCAN_FILE -d file:///sdcard/Download for example), so I initially wanted to implement it quickly, then answer with sendBroadcast() already available in the codebase. But I didn't have time to implement it, and I forgot to comment.

@brunoais
Copy link
Contributor Author

brunoais commented Jun 6, 2021

Ty for clarification. Those things happen.

@brunoais brunoais force-pushed the intents_on_start_exit branch from 9105660 to fc1e549 Compare June 13, 2021 09:29
@brunoais
Copy link
Contributor Author

Must be really bad that can't even comment.

I still can (and often) answer to comments/issues.

About this one, I didn't answer immediately because in my TODO list I wanted to improve the drag&drop behavior by sending an intent after "adb push" a file (the equivalent of adb shell am broadcast -a android.intent.action.MEDIA_SCANNER_SCAN_FILE -d file:///sdcard/Download for example), so I initially wanted to implement it quickly, then answer with sendBroadcast() already available in the codebase. But I didn't have time to implement it, and I forgot to comment.

Want me to finish that implementation?

@rom1v
Copy link
Collaborator

rom1v commented Jun 13, 2021

Want me to finish that implementation?

Thank you for the proposal, but I started implementing it few days ago (i just pushed the WIP: broadcast), but I discovered that ACTION_MEDIA_SCANNER_SCAN_FILE is now deprecated, so I finally abandoned the idea. 😞

EDIT: Refs #2383

@brunoais
Copy link
Contributor Author

It's deprecated but lots of people use older phones. It can still make sense to go forward with it.

@brunoais brunoais force-pushed the intents_on_start_exit branch from fc1e549 to 6a06f60 Compare June 19, 2021 15:34
@brunoais brunoais changed the title Code for discussion: Intents on start & exit Intents on start & exit Jun 19, 2021
@brunoais
Copy link
Contributor Author

@rom1v
I think I finished this feature.
There was 1 detail I had problems deciding how to implement. Which is the "scrcpy" prefix. I decided to end up with a field and a method in the Intents enum but I'm not fully sure of it.

@@ -188,6 +219,9 @@ private static Options createOptions(String... args) {
boolean powerOffScreenOnClose = Boolean.parseBoolean(args[15]);
options.setPowerOffScreenOnClose(powerOffScreenOnClose);

EnumSet<Intents> broadcastIntents = Intents.fromBitSet(BitSet.valueOf(new long[]{Long.parseLong(args[16])}));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a roundabout way of translating but it is the way that seemed to be most direct without implementing manually

@brunoais brunoais force-pushed the intents_on_start_exit branch 2 times, most recently from 51122a5 to 6c53b25 Compare June 19, 2021 16:12
@rom1v
Copy link
Collaborator

rom1v commented Jun 19, 2021

Hi,

Thank you for your work.

IMO, this feature should be kept simple:

  • two intents only: STARTED and STOPPED (no CLEANED, which is an implementation detail)
  • STOPPED would be sent by the CleanUp process only (so that it is sent even on adb disconnection)
  • a simple option --send-broadcasts without additional parameters: either the user wants the broadcasts, either they don't, IMO there is no need for enabling individual broadcasts
  • no intent extras: currently, AFAIU, they provide no more information than the broadcast action itself (but they might be added later to provide scrcpy info, like capture resolution?)

Regards

@brunoais
Copy link
Contributor Author

Here's my counter-opinions:

  1. Two intents only where STOPPED is sent when CLEANED is currently sent. <- Nothing against this one
  2. When I did it like that, I had into account needs such as not announcing when the connection happens (possible app that learned about it and is now listening) but still wanting to be announced when the disconnection happens. One kind of app I was thinking is a game that checks for scrcpy and blocks running if scrcpy is detected as connected but the user still wants to be able to pause the game automatically in case a disconnection happens. Do you think it would be bad to maintain such code?
  3. The intent extras are mostly useful when using automation tools, instead of custom code. For custom code, they don't add anything because you can do pretty much anything. With automation tools such as Tasker, having such variables helps having less programming steps and profiles+tasks easier to maintain, specially as scrcpy evolves.

@rom1v
Copy link
Collaborator

rom1v commented Jun 20, 2021

When I did it like that, I had into account needs such as not announcing when the connection happens (possible app that learned about it and is now listening) but still wanting to be announced when the disconnection happens.

The STARTED and STOPPED broadcasts could be sent, but the app would only react to STOPPED.

The intent extras are mostly useful when using automation tools, instead of custom code.

I don't know Tasker, but I guess that the automation tools could probably react to broadcasts without duplicate information in the extras. I don't want custom hacks specific to Tasker (which is not even open source).

@brunoais
Copy link
Contributor Author

The STARTED and STOPPED broadcasts could be sent, but the app would only react to STOPPED.

In this case, it would be to hide scrcpy. It's more about apps you don't control than apps you do control.

I don't know Tasker, but I guess that the automation tools could probably react to broadcasts without duplicate information in the extras. I don't want custom hacks specific to Tasker (which is not even open source).

It's not a feature specific for Tasker, I only mentioned it as an example, but fair enough.

@rom1v
Copy link
Collaborator

rom1v commented Jun 20, 2021

In this case, it would be to hide scrcpy. It's more about apps you don't control than apps you do control.

I'm not sure to understand the problem.

IIRC, in your initial version, broadcasts were sent unconditionally. The purpose of the cli option was to disable broadcasts by default because they are not used in 99.9% of users. But if a user needs them, why configuring which broadcasts are sent by scrcpy would help?

Scrcpy could just send both STARTED and STOPPED, and apps (or Tasker or whatever) could just do actions on reception of one of them.

@brunoais
Copy link
Contributor Author

IIRC, in your initial version, broadcasts were sent unconditionally.

That's because I wanted to confirm implementation direction. Such as:

  1. Whether OK to do such kind of feature
  2. If doing on the server is sth you accept
  3. If done on the server, if it should work by calling the shell or if I'm missing sth.

As I mention in the title, it was code as a base for discussion ;).
I already had the two checkmarks you see on the top post since the very beginning. I guess I should have mentioned that fact more explicitly.

If I were to do all the cli API by then and you didn't want to accept intent sending, I'd wasted my time on the C code when it would be just thrown away.

Scrcpy could just send both STARTED and STOPPED, and apps (or Tasker or whatever) could just do actions on reception of one of them.

I'll try to explain with different words. About selecting which intents to send, my idea is more directed towards apps (most commonly, games but also some bank apps) that try to detect everything and then refuse to work when some parameter is different than expected. If you still insist on a "no" then I can remove the selection part with no hard feelings...

@brunoais
Copy link
Contributor Author

Given what you mentioned and also not continuing the discussion, I decided my reasons are weak and I'll remove the ability to choose which intents to use completely

@rom1v
Copy link
Collaborator

rom1v commented Jul 10, 2021

Given the specific needs related to the intent format and their intent extra, maybe a more generic approach would be preferable:

scrcpy --exec-on-start='am broadcast -a ...;settings ...' --exec-on-exit='am broadcast -a ...;settings ...'

What do you think?

@brunoais
Copy link
Contributor Author

brunoais commented Jul 10, 2021

Even though that approach is flexible, it can lead to any command being run and then will come the need to accommodate for it by what different people want, etc, etc... Then there's the users making typos and then ho to tell there were typos and then check the status code.... Arbitrary code ends up being messy.
It's flexible, good willed but I think, in the long run, it's worse than just removing the ability to choose which intents.
In other words: IMO, it makes sense for scrcpy to announce what it's doing but it doesn't make sense for scrcpy to execute arbitrary code. Even if, technically, it's not creating vulnerabilities.

I'm almost done removing the options :)

@brunoais brunoais force-pushed the intents_on_start_exit branch from 3fb9ac2 to 99b82b4 Compare July 10, 2021 17:03
@brunoais
Copy link
Contributor Author

brunoais commented Jul 10, 2021

Now I just need to test if all is good (later)

@Genymobile Genymobile deleted a comment from DanEdens Nov 22, 2021
@brunoais
Copy link
Contributor Author

Oh... More conflicts to fix... Maybe next weekend... I'll put in my calendar.

@rom1v
Copy link
Collaborator

rom1v commented Nov 22, 2021

IMO, it makes sense for scrcpy to announce what it's doing but it doesn't make sense for scrcpy to execute arbitrary code.

There are many requests/use cases to execute arbitrary scripts on the device on start/stop: #96 #101 #1413 #1449
Start/stop intents is a very specific need IMO.

scrcpy --exec-on-start='am broadcast -a ...;settings ...' --exec-on-exit='am broadcast -a ...;settings ...'

Maybe not on the command line, but a hook-script:

scrcpy --hook-script yourhook.bash
# yourhook.bash (executed on the device on start/stop)
if [[ "$1" == start ]]
then
    # your commands on start
elif [[ "$1" == stop ]]
    # your commands on stop
fi

@brunoais
Copy link
Contributor Author

brunoais commented Nov 23, 2021

A hook script from a file is more acceptable than direct text as command line options.

However, how can we properly tell the user that the stop is not guaranteed to run?
The only real way to make sure a "stop" script is run is by having something running by an app installed on the phone which checks for when scrcpy closes and then ensures the cleanup is actually done.
I'd like to provide that but I understand you don't want it.

Intents on start and exit don't solve that but I think a solution should be worked on at some point. I have my own proposal with the ServerSocket, which is what I use locally. @rom1v, maybe you can come up with a better solution.

@rom1v
Copy link
Collaborator

rom1v commented Nov 23, 2021

The only real way to make sure a "stop" script is run is by having something running by an app installed on the phone which checks for when scrcpy closes and then ensures the cleanup is actually done.

There is. See CleanUp.java

@brunoais
Copy link
Contributor Author

brunoais commented Nov 23, 2021

There is. See CleanUp.java

That one works most of the time... Not even nearly every time.
For me, it works about 90% of the time (Do note: way better than the expected ~40% without it, though!).
However, it's still flawed. Sometimes I have to reset the android system settings myself because CleanUp.java didn't do it.
There's also a very easy way to force the cleanup code not to run after everything is setup so it is run.

@rom1v
Copy link
Collaborator

rom1v commented Nov 24, 2021

For me, it works about 90% of the time

If this doesn't work, my understanding is that it's a bug on the device which kills the child process after few hundreds of milliseconds. On a Nexus 5, i have this problem. On a OnePlus 7 Pro, it works 100℅ reliably (even if I had sleep(5 seconds)), for example when I unplug the device.

@brunoais
Copy link
Contributor Author

brunoais commented Nov 24, 2021

@rom1v The easiest way for me to reproduce is to just turn off debug mode automatically.
In my case, for some reason, just leaving it there and wait, my phone (for some reason) decides that debug mode has been on for too long and turns it off. I can't control that. I went to the battery saver app (the one from the manufacturer) and I can't find any way to control the timer for debug.

@rom1v
Copy link
Collaborator

rom1v commented Nov 24, 2021

The easiest way for me to reproduce is to just turn off debug mode automatically.

It seems to work as expected for me.

For example:

# enable show touches
scrcpy -t

When I turn USB debugging off, show touches is correctly disabled.

@brunoais
Copy link
Contributor Author

brunoais commented Nov 24, 2021

In my phone, if I turn debugging off, everything running as adb is immediately killed (as in kill -9). I don't know if it's manufacturer, android, both... I don't know.
What I know is that when I run scrcpy like that and I turn off debugging, none of the cleanup is run. Same happens on my friend's running Android 8

@brunoais
Copy link
Contributor Author

Replaced by #3191

@brunoais brunoais closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants