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

statistics dialog is shown after closing "Current status" dialog (with hotkey) #1736

Closed
ePaul opened this Issue May 28, 2017 · 23 comments

Comments

Projects
None yet
7 participants
@ePaul
Contributor

ePaul commented May 28, 2017

Trying out the 1.13 series first, with 1.13.8+dev (a25f3d3, compiled myself and run on Linux).

When I use Alt+S to open the "game overview" dialog (dialog title is either "Current status" or "Szenrioeinstellungen" (szenario settings in German)), after closing that dialog the statistics dialog (which you normally get with just the S key) opens.

When opening the game overview dialog from the menu, that doesn't happen.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 28, 2017

Member

Probably related to #1711

Member

CelticMinstrel commented May 28, 2017

Probably related to #1711

@Vultraz Vultraz added this to the 1.13.9 milestone May 29, 2017

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel May 29, 2017

Member

I can't reproduce this, unfortunately. However I'm getting another bug that seems similar, so I think I can probably fix it... assuming I'm right that they're related.

Member

CelticMinstrel commented May 29, 2017

I can't reproduce this, unfortunately. However I'm getting another bug that seems similar, so I think I can probably fix it... assuming I'm right that they're related.

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Jun 1, 2017

Member

I couldn't replicate this either, though it does sound like the keys are not being 'debounced' properly - or rather the keys are registering multiple times incorrectly.

Member

Wedge009 commented Jun 1, 2017

I couldn't replicate this either, though it does sound like the keys are not being 'debounced' properly - or rather the keys are registering multiple times incorrectly.

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Jun 1, 2017

Member

Strangely enough I can confirm this now, albeit only on Linux.

Member

Wedge009 commented Jun 1, 2017

Strangely enough I can confirm this now, albeit only on Linux.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Jun 2, 2017

Member

The similar case I've experienced with opening the Lua console appears to be caused by the action being triggered on both "key down" and "text input" events. Aginor implemented the use of text input events in order to better support arbitrary keyboard layouts, so we can't just drop that outright; I think what we need is some way for "key down" to record that the event was handled so that "text input" can skip it. Either that or some way for "key down" to recognize that a "text input" will be generated from this event, but... I think that's actually impossible here.

Member

CelticMinstrel commented Jun 2, 2017

The similar case I've experienced with opening the Lua console appears to be caused by the action being triggered on both "key down" and "text input" events. Aginor implemented the use of text input events in order to better support arbitrary keyboard layouts, so we can't just drop that outright; I think what we need is some way for "key down" to record that the event was handled so that "text input" can skip it. Either that or some way for "key down" to recognize that a "text input" will be generated from this event, but... I think that's actually impossible here.

@sevu

This comment has been minimized.

Show comment
Hide comment
@sevu

sevu Sep 11, 2017

Member

I have the issue from above with alt+s

Member

sevu commented Sep 11, 2017

I have the issue from above with alt+s

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 12, 2017

Member

On latest master?

Member

CelticMinstrel commented Sep 12, 2017

On latest master?

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Sep 12, 2017

Member

Confirming I also still have this issue on Linux as of 20eb424. Still can't get it on Windows...

Member

Wedge009 commented Sep 12, 2017

Confirming I also still have this issue on Linux as of 20eb424. Still can't get it on Windows...

@sevu

This comment has been minimized.

Show comment
Hide comment
@sevu

sevu Sep 12, 2017

Member

yes

Member

sevu commented Sep 12, 2017

yes

@Vultraz Vultraz modified the milestones: 1.13.9, 1.14.0 Feb 14, 2018

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Mar 5, 2018

Member

Can someone re-text this? @jyrkive 's recent changes might have fixed it.

Member

Vultraz commented Mar 5, 2018

Can someone re-text this? @jyrkive 's recent changes might have fixed it.

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Mar 5, 2018

Member

Unfortunately it's still the same for me, as of 900a159. Issue is present in Linux build but not in Windows.

Member

Wedge009 commented Mar 5, 2018

Unfortunately it's still the same for me, as of 900a159. Issue is present in Linux build but not in Windows.

@stevecotton

This comment has been minimized.

Show comment
Hide comment
@stevecotton

stevecotton Mar 17, 2018

Contributor

If both "key" and "alt+key" are assigned as hotkeys, it seems that pressing alt+key triggers both commands. The bug also happens with the unit status table (alt+u) which also triggers an undo (u).

Build version: Linux f6428ed

Contributor

stevecotton commented Mar 17, 2018

If both "key" and "alt+key" are assigned as hotkeys, it seems that pressing alt+key triggers both commands. The bug also happens with the unit status table (alt+u) which also triggers an undo (u).

Build version: Linux f6428ed

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Mar 17, 2018

Member

That's actually really helpful, thanks!

Member

Vultraz commented Mar 17, 2018

That's actually really helpful, thanks!

@stevecotton

This comment has been minimized.

Show comment
Hide comment
@stevecotton

stevecotton Mar 17, 2018

Contributor

One more finding - if you are still holding the 'alt' key when closing the first dialog, the second thing does not happen (you can close the first dialog with alt+enter).

IIUC, the hotkey_keyboard::matches_helper is reading the current keyboard state via SDL_GetModState(), ignoring the modifiers that are in the SDL_Event itself.

Contributor

stevecotton commented Mar 17, 2018

One more finding - if you are still holding the 'alt' key when closing the first dialog, the second thing does not happen (you can close the first dialog with alt+enter).

IIUC, the hotkey_keyboard::matches_helper is reading the current keyboard state via SDL_GetModState(), ignoring the modifiers that are in the SDL_Event itself.

@sevu

This comment has been minimized.

Show comment
Hide comment
@sevu

sevu Apr 8, 2018

Member

May I mark this as blocker?

Member

sevu commented Apr 8, 2018

May I mark this as blocker?

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Apr 9, 2018

Member

@CelticMinstrel hasn't worked on this for a long time. Unassigning.

Member

jyrkive commented Apr 9, 2018

@CelticMinstrel hasn't worked on this for a long time. Unassigning.

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Apr 10, 2018

Member

Debugging this on GNU/Linux shows that the "current status" dialog is shown in response to a SDL_KEYDOWN event, whereas the statistics dialog is shown in response to a SDL_TEXTINPUT event for the same keypress.

That's what we get for using two distinct events for hotkeys, I guess...

Member

jyrkive commented Apr 10, 2018

Debugging this on GNU/Linux shows that the "current status" dialog is shown in response to a SDL_KEYDOWN event, whereas the statistics dialog is shown in response to a SDL_TEXTINPUT event for the same keypress.

That's what we get for using two distinct events for hotkeys, I guess...

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Apr 10, 2018

Member

Okay, the difference on Windows is that SDL_TEXTINPUT events are not generated when Alt is held down.

Member

jyrkive commented Apr 10, 2018

Okay, the difference on Windows is that SDL_TEXTINPUT events are not generated when Alt is held down.

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Apr 10, 2018

Member

I'm away from home at the moment so I can't check for myself - it seems like there's different handling for the state of Alt being held down... in OS or SDL I wonder. That's a good find, though,

Member

Wedge009 commented Apr 10, 2018

I'm away from home at the moment so I can't check for myself - it seems like there's different handling for the state of Alt being held down... in OS or SDL I wonder. That's a good find, though,

@jyrkive jyrkive closed this in c42401a Apr 10, 2018

jyrkive added a commit that referenced this issue Apr 10, 2018

Fix #1736: on GNU/Linux, a hotkey can trigger multiple commands
SDL_TEXTINPUT events aren't generated on Windows when Alt is held down.
Let's ensure that the behavior is the same on all platforms.
@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Apr 10, 2018

Member

That doesn't look like a good fix to me; can you still type eg "é" on a Mac with this fix?

Member

CelticMinstrel commented Apr 10, 2018

That doesn't look like a good fix to me; can you still type eg "é" on a Mac with this fix?

jyrkive added a commit that referenced this issue Apr 11, 2018

Revert "Fix #1736: on GNU/Linux, a hotkey can trigger multiple commands"
This reverts commit f80bc42.

As @CelticMinstrel pointed out, that commit made it impossible to input characters which require
AltGr to type (e.g. @, € and $ in Finnish keyboard).

Reopens #1736.

jyrkive added a commit that referenced this issue Apr 11, 2018

Revert "Fix #1736: on GNU/Linux, a hotkey can trigger multiple commands"
This reverts commit c42401a.

As @CelticMinstrel pointed out, that commit made it impossible to input characters which require
AltGr to type (e.g. @, € and $ in Finnish keyboard).

Reopens #1736.
@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Apr 11, 2018

Member

Reopening because I reverted the fix.

Member

jyrkive commented Apr 11, 2018

Reopening because I reverted the fix.

@jyrkive jyrkive reopened this Apr 11, 2018

jyrkive added a commit that referenced this issue Apr 11, 2018

Hotkey manager: drop duplicate commands
We use both SDL_KEYDOWN and SDL_TEXTINPUT events for hotkeys. It's possible
for both events (caused by the same keypress) to trigger the hotkey command
and we don't want that. Hence, let's drop duplicate commands.

Fixes #1736.

jyrkive added a commit that referenced this issue Apr 11, 2018

Hotkey manager: drop duplicate commands
We use both SDL_KEYDOWN and SDL_TEXTINPUT events for hotkeys. It's possible
for both events (caused by the same keypress) to trigger the hotkey command
and we don't want that. Hence, let's drop duplicate commands.

Fixes #1736.

@jyrkive jyrkive closed this in 8667e5b Apr 12, 2018

@ePaul

This comment has been minimized.

Show comment
Hide comment
@ePaul

ePaul Apr 14, 2018

Contributor

I can confirm that the issue doesn't appear anymore on my system with a fresh build on the 1.4 branch (76cf5ba). Thanks!

Contributor

ePaul commented Apr 14, 2018

I can confirm that the issue doesn't appear anymore on my system with a fresh build on the 1.4 branch (76cf5ba). Thanks!

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 Apr 26, 2018

Member

Sorry for the late confirmation, but this seems okay to me too. Tested within a few commits of the recently tagged 1.14.0.

Member

Wedge009 commented Apr 26, 2018

Sorry for the late confirmation, but this seems okay to me too. Tested within a few commits of the recently tagged 1.14.0.

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