aseprite / laf

A C++ library to create desktop applications
https://aseprite.github.io/laf/
MIT License
274 stars 58 forks source link

Fix: Previous key state for repeat count in X11 #71

Closed tetektoza closed 9 months ago

tetektoza commented 10 months ago

Currently, as we've spoken in https://github.com/aseprite/laf/pull/69 @dacap has correctly pointed out that my patch has some problems with shift key modifier - and he was correct, I've skipped an important thing looking into this problem.

This patch adds a std::set to hold all currently pressed keys - if a key got pressed second time, std::set will hold a key symbol of that key, if an event for that key comes for a second time (and there were no KeyRelease event coming for the event in-between) - then it means the key is being pressed down, so we set repeat count to 1. If KeyRelease event came for the key symbol that was previously pressed, then we basically erase it from std::set.

I agree that my contributions are licensed under the MIT License. You can find a copy of this license at https://opensource.org/licenses/MIT

tetektoza commented 10 months ago

The whole problem with the previous solution was that, that it naively interpreted previous pressed key symbol. So if we pressed another key while holding the mapped key, it reseted the repeat count in the mapped key to 0, since previous key symbol was reseted by the another key.

This still doesn't fully fix the problem while holding Shift + V (if V is the key that got mapped to LMB), because right now the current implementation of Accelerator class (this is in Aseprite repo though) doesn't recognize mouse event if it comes with a modifier, i.e:

if we press Shift + V and rescale the square with aspect ratio for example, then if we stop holding V - it should stop resizing the square and just put it in place. This is what happens if you press LMB + Shift normally. It doesn't happen with keyboard shortcuts because accelerator can't interpret V as the KeyPress event, because it comes with a shift modifier.

I've prepared a solution for this in aseprite repo, will link it to this PR after I make a PR on aseprite.

dacap commented 9 months ago

Thanks @tetektoza, I'll merge this patch and then give a try to https://github.com/aseprite/aseprite/pull/4080