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

Keypad Keymap improvement and inclusion of more intuitive key combinations #44

Merged
merged 15 commits into from
May 8, 2016

Conversation

denisps
Copy link
Contributor

@denisps denisps commented May 2, 2016

  • Reimplemented the keymap, mapping all the keys to as intuitive key combinations as I could think of, while also preserving the existing Alt combinations. (Some of the Alt combinations were and still aren't working on windows due to collision with application menu and duplication)
  • Reimplemented keypad key look-up to be O(1) instead of O(n)

@denisps
Copy link
Contributor Author

denisps commented May 2, 2016

It resolves issue #27 and partially issue #14

switch (button)
{
// Touchpad left buttons
case Qt::Key_Escape: case Qt::Key_Escape | ALT: return esc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alt-Esc was On/Home before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Fixed. I can't use that combo anyway, as it's a reserved combo, on windows at least.

@Vogtinator
Copy link
Member

Vogtinator commented May 3, 2016

Redesigning the keypad implementation was overdue and this looks very promising so far!
I couldn't try it yet, but I'll do so today, thanks!

@adriweb
Copy link
Member

adriweb commented May 3, 2016

Looks nice - I'll try on Mac hopefully very soon :)

@Vogtinator
Copy link
Member

Vogtinator commented May 5, 2016

Works really well except for one issue:

  • Hold Shift and 7 -> Shift press event + '(' press event
  • Release Shift -> Shift release event + 7 press event
  • Release 7 -> 7 release event

Result: "(7)" + ( key stuck

Stuck keys are really annoying as the OS behaves quite weird and it persists resets and restarts.

@denisps
Copy link
Contributor Author

denisps commented May 5, 2016

OK, That's a challenging issue. This is how the keyboard events work. That is what qt gives us, and that is what system gives to qt. I assume you are on Mac, but I'm experiencing the same on windows.

I've worked around that problem by releasing last pressed key. Let's assume the user is not going to hold multiple shift modifiable keys simultaneously while releasing the shift.

There is also a possibility of user pressing shift while holding a shift modifiable key, and then releasing the key while holding the shift, which almost never happens in the real world usage, so, I think, we can safely ignore this scenario.

@Vogtinator
Copy link
Member

It is indeed a challenging issue and the main reason I kept the initial implementation so simple.
I picked only keys that are accessible without modifiers on all major layouts, for example.

Let's assume the user is not going to hold multiple shift modifiable keys simultaneously while releasing the shift.

How hard is it to remove that constraint too? Stuck keys are a nuisance.

I have an idea how the issue can be solved completely:
Keep a list of all currently pressed (host) keys. On events, update that list and derive to which calc keys they map. Then set the calc keymap to the result. This ensures that if no host key is pressed, no calc key is pressed either.

@denisps
Copy link
Contributor Author

denisps commented May 5, 2016

The problem with that is that event->key() gives us virtual keys, modified with shift, ctrl, etc at the time of the event, which is what it gets directly from the system. The press event is being modified by shift, while release event is being delivered directly. We don't know what layout the keyboard has, and so we don't really know what virtual keys map to what physical keys. For instance, Shift+7 is ? on a keyboard with Russian layout. But even if we knew the layout, and were willing to implement it ourselves, nativeScanCode is not working on Mac.

@Vogtinator
Copy link
Member

The press event is being modified by shift, while release event is being delivered directly.

I see, X11 itself does it the same way, apparently. If we don't even have the guarantee that Qt KeyEvents match up, I can't see any way to implement it :-(

@adriweb
Copy link
Member

adriweb commented May 5, 2016

Yeah, so, works fine on Mac as well, but I may have the same kind of issues you are talking about (tried with the latest change as of this date/time):

Hold a key (let's say a), then Hold command, then release a.
But a is still active on the Firebird's keyboard (and also after releasing command)

This only happens with command (and probably control on Windows), though - no such issues with shift.

Edit: I actually have this behaviour without these changes, so at least it's not a regression :)

@denisps
Copy link
Contributor Author

denisps commented May 6, 2016

Solved.
I'm keeping track of all pressed keys in relation to their corresponding virtual keys (or native scan codes, if available) and if a keyboard event arrives with a virtual key that is already pressed, then it is interpreted as a release event for the key that was previously pressed.
I'm ignoring auto-repeat as it's the job of the calc-os and is messing up with my state machine.

Also, I went from switch case to hash since it's true O(1) independent of compiler optimization, looks better, and can potentially be configurable in the settings.

@Vogtinator
Copy link
Member

Vogtinator commented May 7, 2016

Sadly the issue still remains:

  • Press and hold 9
  • Press and hold shift
  • Wait a few moments until "(" is pressed
  • Let shift go
  • Let 9 go

-> ( stuck.

@adriweb
Copy link
Member

adriweb commented May 7, 2016

I don't seem to have the issue @Vogtinator is describing - everything gets pressed and released as expected when trying the sequence (and some variants)

However, the issue with command I was describing earlier is still there for me.
Oh well - it's still very good :P

int row = key / 11;
int col = key % 11;

if (state) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an assert for row here, like in qmlbridge.cpp?

@Vogtinator
Copy link
Member

As it's highly unlikely to be ever fixed completely, I'll merge this when the two nitpicks are solved.

@denisps
Copy link
Contributor Author

denisps commented May 7, 2016

Done.

I experience neither of those on windows 7, neither with ctrl, nor with alt, nor with shift.
I'm not even sure how are you getting "(". If you've pressed 9 before shift, the only thing you should get is "9". Unless qt is reporting press event twice with non-matching nativeVirtualKey and with no repeat flag.

@Vogtinator
Copy link
Member

Done.

Thanks! If you want to, you can add yourself in the author list in mainwindow.cpp.

I'm not even sure how are you getting "(". If you've pressed 9 before shift, the only thing you should get is "9". Unless qt is reporting press event twice with non-matching nativeVirtualKey and with no repeat flag.

On X11, there's a special "parenleft" key, so the same issue with Qt, just a layer below as well.

@denisps
Copy link
Contributor Author

denisps commented May 8, 2016

Added. Thanks!

@Vogtinator Vogtinator merged commit e54035a into nspire-emus:master May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants