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

Add no-repeat cli option #1623

Closed
wants to merge 1 commit into from
Closed

Conversation

xeropresence
Copy link
Contributor

@xeropresence xeropresence commented Jul 27, 2020

Still need documentation, not sure what you would want to put in there.

This modifies event_loop to pass down the option struct instead of passing the individual options as it seemed silly to expand the function to add a third param.

Not sure of the point of

xeropresence@434ffbd#diff-4dd0bcc81fa9404d5dd8572e7682d3c5R245

@Genymobile Genymobile deleted a comment Jul 27, 2020
@Genymobile Genymobile deleted a comment Jul 27, 2020
rom1v added a commit that referenced this pull request Aug 2, 2020
This avoids to pass additional options to some input manager functions.

Refs #1623 <#1623>
rom1v pushed a commit that referenced this pull request Aug 2, 2020
This avoids to pass specific options values individually. Since these
function are static (internal to the file), this is not a problem to
make them depend on scrcpy_options.

Refs #1623 <#1623>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v pushed a commit that referenced this pull request Aug 2, 2020
Add an option to avoid forwarding repeated key events.

Refs #1013 <#1013>
PR #1623 <#1623>

Signed-off-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented Aug 2, 2020

Thank you. 👍

This modifies event_loop to pass down the option struct instead of passing the individual options as it seemed silly to expand the function to add a third param.

I moved the control flag to the initialization (the same way as it worked for prefer_text): 74079ea
Then I split your commit to first pass the whole scrcpy_options in scrcpy.c: 65d06a3
and implement the option in a separate commit: 94269cc

(I renamed to --no-key-repeat)

Still need documentation, not sure what you would want to put in there.

I added documentation in that last commit.

The branch is pr1623. Could you review and test, please?


Not sure of the point of xeropresence@434ffbd#diff-4dd0bcc81fa9404d5dd8572e7682d3c5R245

if the #ifdef CONTINUOUS_RESIZING_WORKAROUND block was not included, then display is not used, and it generates a warning:

../app/src/scrcpy.c: In function ‘event_loop’:
../app/src/scrcpy.c:248:17: warning: unused parameter ‘display’ [-Wunused-parameter]
  248 | event_loop(bool display, bool control) {
      |                 ^

@xeropresence
Copy link
Contributor Author

Seems to work fine.

I see you inverted the naming scheme of the option.

434ffbd#diff-1e7437254ef4ba8f538e75a3bd53846fR414

options->ignore_key_repeat

vs

94269cc#diff-1e7437254ef4ba8f538e75a3bd53846fR465

!im->forward_key_repeat

I think ignore_key_repeat is easier to understand at first glance rather then !im->forward_key_repeat, but other then that it all looks good.

@xeropresence
Copy link
Contributor Author

With display no longer being a variable then that should no longer be needed as well right?

@rom1v
Copy link
Collaborator

rom1v commented Aug 3, 2020

Thank you for the feedbacks.

I think ignore_key_repeat is easier to understand at first glance rather then !im->forward_key_repeat

The idea is to use boolean flags with a "positive" meaning. For example --no-display, --no-control, --no-mipmaps are stored as booleans display, control and mipmaps (to avoid conditions like if (!no_display)). For consistency, --no-key-repeat could be stored in a bool key_repeat (but I added forward to make it clear that we don't forward them even if they are received, and --no-forward-key-repeat is too long).

Here, the condition in the block is negative because it's an early return (an early return is often used in the negative form):

if (im->forward_key_repeat) {
    // forward event
}

vs

if (!im->forward_key_repeat) {
    return;
}
// forward_event

ignore_key_repeat is not bad , but I think forward_key_repeat is more consistent.

With display no longer being a variable then that should no longer be needed as well right?

It's removed: 65d06a3#diff-4dd0bcc81fa9404d5dd8572e7682d3c5L248

rom1v pushed a commit that referenced this pull request Aug 6, 2020
Add an option to avoid forwarding repeated key events.

PR #1623 <#1623>
Refs #1013 <#1013>

Signed-off-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented Aug 6, 2020

Merged 84f1d9e

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