Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upIgnore navigation input if key state is Released #20628
Conversation
highfive
commented
Apr 12, 2018
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. |
highfive
commented
Apr 12, 2018
|
Heads up! This PR modifies the following files:
|
|
We need to do the same thing for the other bindings too. Like reload, loadurl, Cmd-Q, … |
| if let Some(id) = self.browser_id { | ||
| let event = WindowEvent::Navigation(id, TraversalDirection::Forward(1)); | ||
| self.event_queue.push(event); | ||
| if state != KeyState::Released { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
piotr-szpetkowski
Apr 12, 2018
Author
Contributor
I've followed the suggestion of @cbrewster that both KeyState::Repeated and KeyState::Pressed might be a valid state and therefore to check for state not being equal to KeyState::Released - #20478 (comment)
|
@paulrouget is it desirable to disallow the |
|
r? @paulrouget |
|
@piotr-szpetkowski You'll see that in For the record: Servo will never receive these key events ever. Even if 1) it's no-op (if we press a goForward binding, but we know we can't go forward), and 2) on Release. |
|
@piotr-szpetkowski let me know if you need help moving forward. |
piotr-szpetkowski
force-pushed the
piotr-szpetkowski:master
branch
2 times, most recently
from
2ba7c25
to
270fddb
|
@paulrouget I've updated my PR and it should (hopefully) comply with your review. I've changed the if expression into the match guard, which seems cleaner to me and I've also added it to every other match branch except the |
|
|
piotr-szpetkowski
force-pushed the
piotr-szpetkowski:master
branch
from
270fddb
to
f45c1be
|
Looks better. Can you rebase and use |
|
@piotr-szpetkowski Will you have time to address Paul's last comment? |
|
Hi @jdm - I'll be able to take care of it on this Friday's evening or Saturday. Also, I've finally finished dealing with real life stuff, so I should have more of a spare time |
piotr-szpetkowski
force-pushed the
piotr-szpetkowski:master
branch
from
f45c1be
to
03819a5
piotr-szpetkowski
force-pushed the
piotr-szpetkowski:master
branch
from
03819a5
to
3840687
|
Hey @paulrouget - I've applied your change request and rebased from master. I've already done it few days ago, but I've forgot to poke you about it, so here, I've rebased once again now and awaiting review. |
|
Thank you for the update. |
| self.event_queue.push(WindowEvent::Quit); | ||
| } | ||
| (_, Some('3'), _) if mods ^ KeyModifiers::CONTROL == KeyModifiers::SHIFT => { | ||
| (_, Some('3'), _) => | ||
| if state == KeyState::Pressed && mods ^ KeyModifiers::CONTROL == KeyModifiers::SHIFT { |
This comment has been minimized.
This comment has been minimized.
paulrouget
Jun 1, 2018
Contributor
Because you moved the modifier check after =>, it means that just pressing "3" will never make it to Servo.
Keep the modifier check before =>.
|
|
|
#21250 fixed this. |
piotr-szpetkowski commentedApr 12, 2018
•
edited by SimonSapin
Hi. As my first PR in Servo and also my first commit in any Rust project I've fixed issue with
Navigationevent being sent twice on a single shortcut press. I'm not entirely happy about nested ifs, but AFAIK it's not possible to useif letand a conditional in one line.I'm a newcomer to Rust, so I'd be grateful for feedback for improvements.
./mach build -ddoes not report any errors./mach build-geckolibdoes not report any errors./mach test-tidydoes not report any errorsThese changes fix #20478
There are tests for these changes OR
These changes do not require tests because the change is trivial.
This change is