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

Fix keys scrolling stuck (+ some function double calls) #13115

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Projects
None yet
5 participants
@rob-v
Contributor

rob-v commented Apr 12, 2017

After each pressed hotkey for saving viewport bookmark or jumping to bookmark/edge, it was executed twice.
I found it while looking at #13111 which is also solved by this PR.
With pressed a control key (ctrl/shift/alt) + up/down/left/right, depending on order of up/down events of these keys, it can happen that despite releasing all keys, the viewport is still scrolling to one of edges and doesn't react anymore properly to scroll keys.

Fixes #13111.
Fixes #2075.
Fixes #10828

@rob-v rob-v changed the title from Fix Viewport HandleKey - double calls and broken scrolling #13111 to Fix Viewport HandleKey - double calls and locked scrolling #13111 Apr 20, 2017

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 20, 2017

Contributor

Fixes the same issue #2075

Contributor

rob-v commented Apr 20, 2017

Fixes the same issue #2075

@rob-v rob-v referenced this pull request Apr 20, 2017

Closed

scroll key sometimes stuck #2075

@rob-v rob-v changed the title from Fix Viewport HandleKey - double calls and locked scrolling #13111 to Fix frozen scrolling (+ some function double calls) #13111 #2075 Apr 20, 2017

@pchote

pchote approved these changes Apr 22, 2017 edited

Confirmed that this fixes the sticky scroll bug, although it does feel a bit jarring to have it stop when pressing shift while scrolling. My repro case is to press down an arrow key, press shift, release arrow, release shift.

Anyone trying to test the keydown/keyup change for bookmarks should beware the keyrepeat.

Code looks good, just one minor code style nit:

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 23, 2017

Contributor

style updated.

small note:

to have it stop when pressing shift while scrolling

isn't clear to me. Shift+Up doesn't scroll before/now. Up + shift..... does not stop scrolling. Nobody will use Ctrlkey + a key to scroll, so we could support only single keys for scrolling and ignore control keys so also Shift+Up would scroll.

Contributor

rob-v commented Apr 23, 2017

style updated.

small note:

to have it stop when pressing shift while scrolling

isn't clear to me. Shift+Up doesn't scroll before/now. Up + shift..... does not stop scrolling. Nobody will use Ctrlkey + a key to scroll, so we could support only single keys for scrolling and ignore control keys so also Shift+Up would scroll.

@Phrohdoh

Up+Shift does stop scrolling ingame.

[15:00:37]	<Phrohdoh> Is #13115 supposed to fix the issue where you hold an arrow, open a menu (esc for example), release the arrow, close the menu and the game continues to scroll?
[15:00:37]	<orabot> Pull request #13115 (open) by rob-v: Fix frozen scrolling (+ some function double calls) #13111 #2075 | http://bugs.openra.net/13115
[15:00:48]	<pchote> yeah
[15:00:58]	<Phrohdoh> I just reprod that
[15:01:05]	<pchote> maybe not the menu case, but it definitely fixes the modifier case

LGTM so long as this wasn't intended to fix ^.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 29, 2017

Contributor

@Phrohdoh Up+Shift does NOT stop scrolling ingame (on Windows).

re scroll key + ESC see my explanation in #2075

Contributor

rob-v commented Apr 29, 2017

@Phrohdoh Up+Shift does NOT stop scrolling ingame (on Windows).

re scroll key + ESC see my explanation in #2075

@Phrohdoh

This comment has been minimized.

Show comment
Hide comment
@Phrohdoh

Phrohdoh Apr 29, 2017

Member

@rob-v It does stop scrolling on macOS.

Member

Phrohdoh commented Apr 29, 2017

@rob-v It does stop scrolling on macOS.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 29, 2017

Contributor

On Windows Up+Shift does not stop scrolling in current release and also in this PR. The behaviour of Up+Shift wasn't changed. I wonder how it works in Linux.
Could you try it in current release on macOS? I assume it will also stop scrolling so this feature is then not caused by this PR.

I don't know if it should stop or not. Because when I try to hold 'a' in notepad(++) or OpenRA Text field it stops writing on Shift press. so the standard behaviour seems to 'stop', but it doesn't in OpenRA ingame scrolling on Windows, anyway this behaviour wasn't changed by this PR.

Contributor

rob-v commented Apr 29, 2017

On Windows Up+Shift does not stop scrolling in current release and also in this PR. The behaviour of Up+Shift wasn't changed. I wonder how it works in Linux.
Could you try it in current release on macOS? I assume it will also stop scrolling so this feature is then not caused by this PR.

I don't know if it should stop or not. Because when I try to hold 'a' in notepad(++) or OpenRA Text field it stops writing on Shift press. so the standard behaviour seems to 'stop', but it doesn't in OpenRA ingame scrolling on Windows, anyway this behaviour wasn't changed by this PR.

@rob-v rob-v changed the title from Fix frozen scrolling (+ some function double calls) #13111 #2075 to Fix keys stuck scrolling (+ some function double calls) #13111 #2075 May 16, 2017

@rob-v rob-v changed the title from Fix keys stuck scrolling (+ some function double calls) #13111 #2075 to Fix keys scrolling stuck (+ some function double calls) #13111 #2075 May 16, 2017

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 19, 2017

Contributor

Updated - I had an idea after noticing another issue #10828 solved by this PR.
This PR solves stuck scrolling and in addition allows scrolling with Shift (a modifier) pressed so you can e.g. scroll while queueing commands,...

Contributor

rob-v commented May 19, 2017

Updated - I had an idea after noticing another issue #10828 solved by this PR.
This PR solves stuck scrolling and in addition allows scrolling with Shift (a modifier) pressed so you can e.g. scroll while queueing commands,...

@rob-v rob-v changed the title from Fix keys scrolling stuck (+ some function double calls) #13111 #2075 to Fix keys scrolling stuck (+ some function double calls) May 21, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 28, 2017

Contributor

@pchote @Phrohdoh Could you revisit this and update your reviews?

Contributor

reaperrr commented May 28, 2017

@pchote @Phrohdoh Could you revisit this and update your reviews?

@obrakmann

👍 Looks good to me. Pressing shift does not stop scrolling on Linux, btw.

@obrakmann obrakmann merged commit fc0495a into OpenRA:bleed Jun 1, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann
Contributor

obrakmann commented Jun 1, 2017

@reaperrr reaperrr referenced this pull request Jul 28, 2017

Closed

Interface locked #13727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment