-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Actions on start exit #3191
Open
brunoais
wants to merge
5
commits into
Genymobile:dev
Choose a base branch
from
brunoais:actions_on_start_exit
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Actions on start exit #3191
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
2056a21
Add shell script execution to the server
brunoais 3e69f69
Create: sc_str_find_replace()
brunoais d5156df
Add hookScript ability to the server
brunoais 2ea9cdd
Add hookScript options to scrcpy
brunoais 4287759
Run server with escaped hook_script argument
brunoais File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike what would be expected, adb shell just concatenates the arguments as strings same
as if they were just separated by spaces. Sending the commands in one argument
or sending as multiple arguments doesn't preduce any different outcome.
Sad but that's how adb works.
try typing this in your local shell (you may need the
-s
argument for adb):adb shell 'am broadcast'
vs
adb shell am broadcast
If they both work, means there's no reasonable way to deliver the arguments as-is, even when using
execvp
(which scrcpy is using).Sadly, they have to be escaped.
Whether the escape be done here or before the calling stack to call adb shell is up for debate. However, no doubt it's needed and no doubt the behavior adb has is improper when intending to provide arguments as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it depends on the adb version and the platform. It's a total mess. You can't rely on any specific behavior here. Adding quotes might break on some platforms and some adb versions.
Anyway, in practice, the command line should not exceed 256 chars (it crashes on some devices), so the script content could not be passed as a server argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, should I add sending the script to
control_msg.c
by queueing a message usingsc_controller_push_msg()
sometime in the beginning of the program execution while also implementing a new type atsc_control_msg_serialize()
for that new feature (and then adding the counter-part code in the server)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what to do. I have mixed feelings about this feature.
This would require to start the
CleanUp
process only after this message is received :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about sending much of the first data through stdin? It can be encoded in some way and then just sent to stdin when
adb shell
is run.With that, we can also deal with the issue of limited size of args, the argv inconsistency, etc... and just send all arguments through a stream in stdin as soon as the stdin stream exists. Maybe just encoding as base64 is enough and then ending with maybe the '\0'
Only if a script exists but yes, if a script was set, it would need to wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rom1v Any answers you can give before I try again? It's been a few weeks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rom1v May I proceed to try to implement what I proposed? So far I can tell it appears to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this. IMO this feature is secondary, it adds complexity and requires to change how the parameters are passed, and apparently it does not behave the same way on all adb/devices. So TBH I'm not very keen it on for the moment :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. In that case this and the alternative way that was already shot down are shot down.
I'll see if I can find an alternative way to process this.
Maybe it can be sent as a
ControlMessage
message like most features are done. Possibly in a similar way that the clipboard message is sent.However, it would be convenient to increase the limit to
1«19
512KB. That way a script can be ~400KB which would allow a lot of flexibility.I think that the downside of using the
ControlMessage
API is acceptable with that upside of stability between versions.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rom1v ?