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 an option to exact-match keybindings #1493

merged 15 commits into from May 8, 2018


2 participants

smoogipoo commented Apr 2, 2018

Fixes ppy/osu#2315 .

The cause was that Shift+Delete was being handled as a Delete keybind due to permissive matching. I discussed possible solutions with @peppy and this is the best we've come up with at the moment - passing OnKeyDown/OnAction simultaneously wouldn't solve the issue as the elements are at different depths anyway.

I still think there is some validity to passing OnKeyDown/OnAction simultaneously , but that's a future consideration and wouldn't negate this change.

@peppy peppy added the resolves issue label Apr 2, 2018


This comment has been minimized.


peppy commented Apr 9, 2018

This probably deserves to be added to TestCaseKeyBindings.

smoogipoo and others added some commits Apr 12, 2018

Don't exact match framework actions
This caused problems with Ctrl+F1 on mac :(
Fix incorrect display of TestCaseKeyBindings
Because all OnPressed methods returned false, the input was propagating down to all buttons, which resulted in incorrect values for SimultaneousMode = None.

E.g. SimultaneousMode = None with Ctrl+A:
1. Pass down Ctrl+A, the Ctrl+A button responds to it but doesn't handle it.
2. Since Ctrl+A wasn't handled, A is passed down. A button gets lit up, Ctrl+A button gets released due to the simultaneous mode.

This comment has been minimized.


peppy commented Apr 18, 2018

Can trigger an assert fail:

  • Press Ctrl
  • Press D
  • Press F
  • Release D


peppy added some commits Apr 25, 2018


peppy approved these changes May 8, 2018

@peppy peppy merged commit 11e4535 into ppy:master May 8, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment