Fix: Previous key state for repeat count in X11 #71
Conversation
tetektoza
force-pushed
the
fix_previous_key_state_x11
branch
2 times, most recently
from
4e4a0cb to
83d1f4a
Compare
|
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. |
os/x11/window.h
Outdated
| @@ -128,8 +128,7 @@ class WindowX11 : public Window { | |||
| bool m_resizable = false; | |||
| bool m_transparent = false; | |||
|
|
|||
| KeySym m_pressedKeySym = 0; | |||
|
|
|||
| std::map<KeySym, bool> map_keys; | |||
There was a problem hiding this comment.
I think it would be better if this is a globa/static/defined in the anonymous namespace of window.cpp, and it's called g_pressedKeys. And it might be an std::set.
There was a problem hiding this comment.
Done, also please check out aseprite/aseprite#4080 - this fixes a behaviour when we for example press Shift + Key that got mapped to mouse trigger button, and then we release the key that got mapped but never release Shift. I've explained it in the PR's comments.
os/x11/window.cpp
Outdated
| if (event.type == KeyPress) { | ||
| if (keysym == m_pressedKeySym) | ||
| if (map_keys[keysym]) | ||
| ev.setRepeat(1); | ||
| m_pressedKeySym = keysym; | ||
| } | ||
| else | ||
| m_pressedKeySym = 0; | ||
|
|
||
| map_keys[keysym] = event.type == KeyPress ? true : false; |
There was a problem hiding this comment.
I'm not sure if the map should be set to false when the key is released or just remove the element from the map.
I think we can have something like:
if (event.type == KeyPress) {
if (g_pressedKeys.find(keysym) != g_pressedKeys.end())
ev.setRepeat(1);
else
g_pressedKeys.insert(keysym);
}
else
g_pressedKeys.erase(keysym);There was a problem hiding this comment.
Done, also please check out aseprite/aseprite#4080 - this fixes a behaviour when we for example press Shift + Key that got mapped to mouse trigger button, and then we release the key that got mapped but never release Shift. I've explained it in the PR's comments.
tetektoza
force-pushed
the
fix_previous_key_state_x11
branch
from
83d1f4a to
1289341
Compare
os/x11/window.h
Outdated
| @@ -24,7 +24,7 @@ | |||
| #include <X11/Xatom.h> | |||
| #include <X11/Xlib.h> | |||
| #include <X11/Xutil.h> | |||
|
|
|||
| #include <set> | |||
There was a problem hiding this comment.
This #include can go to the window.cpp file (because we aren't using std::set here in the .h)
There was a problem hiding this comment.
Done, I've just seen it after force pushing first commit :D
tetektoza
force-pushed
the
fix_previous_key_state_x11
branch
from
1289341 to
ae89a0c
Compare
os/x11/window.h
Outdated
| @@ -24,7 +24,6 @@ | |||
| #include <X11/Xatom.h> | |||
| #include <X11/Xlib.h> | |||
| #include <X11/Xutil.h> | |||
|
|
|||
Currently, as we've spoken in aseprite#69 @dcap 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.
tetektoza
force-pushed
the
fix_previous_key_state_x11
branch
from
ae89a0c to
7bd5b65
Compare
|
Thanks @tetektoza, I'll merge this patch and then give a try to aseprite/aseprite#4080 |
Currently, as we've spoken in #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