GNOME Bugzilla – Bug 781285
Key repeat cancel under Wayland should depend on which key is repeating
Last modified: 2017-06-02 17:19:53 UTC
Created attachment 349836 [details] [review] [PATCH] wayland: selectively cancel key repeat on key release Under Wayland, when multiple keys are pressed and the user releases a key, key repeat should continue unless the key released is the one currently repeating. In the case of: - key1 press - key1 repeat - key2 press -> key1 repeat stopped - key2 repeats - key2 release The behavior should be to cancel keyboard repeat, though key1 is still held down. This is consistent with prior X11/XWayland behavior. The following also must work: - key1 press - key2 press - key2 release - key2 press - key1 release - key2 should continue to repeat The fix for bug #778019 should continue to work: - key1 press - key1 repeat - key2 press -> key1 repeat stopped - key1 release - key2 should repeat The choice to change the counter nkeys to the flag repeat_active helps to solve the second test case. Attached is a proposed patch to fix this behavior.
That looks very similar to bug 781896 and attachment 350636 [details] [review] Can you see if this works for you?
(In reply to Olivier Fourdan from comment #1) Hi Olivier, Glad to hear back! > That looks very similar to bug 781896 and attachment 350636 [details] [review] Indeed bug 781896 and this one both address the same underlying problem. > > Can you see if this works for you? Mixed results: attachment 350636 [details] [review] does solve bug 781896, but it doesn't produce expected behavior in the second case of this bug. To rewrite that case as in your (clearer) steps in bug 781896: Steps to reproduce: 1. Press key "a" and keep the key pressed. 2. Press key "b" and release. 3. Press key "b" and keep the key pressed. 4. Release key "a". Actual result: Key "a" is repeated. Once key "b" is pressed and released, all repeats stop. Then key "b" is repeated when it is pressed again, but all repeat stops when key "a" is released. Expected result: Once key "a" is released, key "b" should continue to repeat, as it is still depressed and was one repeating. The expected result may well be debatable, and is the proverbial corner case! But I put it in to suggest that counting the number of keys pressed is not actually pertinent. And this counting is easy to fool. So in the case above, in your patch, I believe that this is happening: 1. Press key "a" and keep the key pressed [nkeys = 1] 2. Press key "b" [nkeys = 2] and release [nkeys = 0, as have released the repeated key] 3. Press key "b" and keep the key pressed [nkeys = 1] 4. Release key "a". [nkeys = 0, and hence repeating stops, though "b" is still pressed and was repeating] I'd argue that what really matters here isn't the count of keys pressed, but rather whether the key released is the currently repeated key. Hence my attachment 349836 [details] [review] entirely eliminates the nkeys counter, and just uses a flag (repeat_active). As an aside, in your patch, you init repeat_key to 0. In my patch, I didn't initialize repeat_key, but do test it, assuming that, as it was only tested after a key release, it would by then have already been set by a keypress. Your approach is certainly safer! This bug report came from a problem with darktable to under Wayland (https://redmine.darktable.org/issues/11535#note-14), and both your attachment 350636 [details] [review] and my attachment 349836 [details] [review] solve that problem. I don't have the big picture of Wayland devices which you have, and would hate to blithely advocate removal of nkeys, but do want to put it out there for consideration! Thank you, Dan
Created attachment 351038 [details] [review] [PATCH v2] wayland: selectively cancel key repeat on key release (In reply to Dan Torop from comment #2) > Glad to hear back! Sorry, it's just that I didn't see your bug report initially. > Mixed results: attachment 350636 [details] [review] [review] does solve bug 781896, > but it doesn't produce expected behavior in the second case of this bug. To > rewrite that case as in your (clearer) steps in bug 781896: > > Steps to reproduce: > > 1. Press key "a" and keep the key pressed. > 2. Press key "b" and release. > 3. Press key "b" and keep the key pressed. > 4. Release key "a". > [...] Yeah, I see what you mean. I don't have strong opinions there, whatever works and is close enough to Xorg behavior will be fine with me, and Xorg behaves as described here. > As an aside, in your patch, you init repeat_key to 0. In my patch, I didn't > initialize repeat_key, but do test it, assuming that, as it was only tested > after a key release, it would by then have already been set by a keypress. > Your approach is certainly safer! Well it's easy enough to add. > This bug report came from a problem with darktable to under Wayland > (https://redmine.darktable.org/issues/11535#note-14), and both your > attachment 350636 [details] [review] [review] and my attachment 349836 [details] [review] > [review] solve that problem. I don't have the big picture of Wayland devices > which you have, and would hate to blithely advocate removal of nkeys, but do > want to put it out there for consideration! Again I don't have strong opinions, we could go with your patch, it makes sense. I am attaching an updated version of your patch with some changes (it needed updating after commit 83322a4 had landed), including the repeat_key value (I am not sure this is actually needed, just the over cautious part of me asking for that) - I also change the type of repeat_active to be a boolean, not hat it makes much difference eventually, but in essence it is a boolean so klet's use the appropriate type. Lastly, I added a link to this bugzilla in your commit message, but if it was me, I would shorten and simplify the commit message though.
Let's see what Carlos think of this :)
*** Bug 781896 has been marked as a duplicate of this bug. ***
Created attachment 351053 [details] [review] [PATCH v3] wayland: selectively cancel key repeat on key release Ah wait, following your logic, you don't even need a "repeat_key" at all, the attached patch works just as well, no? And it's waaaaay simpler imho.
Ah no sorry, my bad, I forgot we do need the "repeat key" flag or some other counter such as nkeys because we won't get a key release from the compositor on focus change (otherwise we'll end up re-introducing bug 779374) so attachment 351038 [details] [review] is better.
Created attachment 351063 [details] [review] [PATCH v4] wayland: selectively cancel key repeat on key release Ok, last try and then I put it at rest, promise... My take is that we don't even need a "repeat_key" flag, we could simply use the existing seat->repeat_key for that, as 0 is not a valid value there.
(In reply to Olivier Fourdan from comment #8) > Created attachment 351063 [details] [review] [review] > [PATCH v4] wayland: selectively cancel key repeat on key release > > Ok, last try and then I put it at rest, promise... > > My take is that we don't even need a "repeat_key" flag, we could simply use > the existing seat->repeat_key for that, as 0 is not a valid value there. Hi Olivier, The last version attachment 351063 [details] [review] looks pretty ideal! (Excepting my somewhat verbose commit message...). It fixes all the test cases for this bug, as well as taking care of the original darktable issue which brought me to this. With regards, Dan
Anyone to review attachment 351063 [details] [review]? Carlos maybe?
Comment on attachment 351063 [details] [review] [PATCH v4] wayland: selectively cancel key repeat on key release Makes sense, and looks simpler :). Please push to gtk-3-22 and master.
Comment on attachment 351063 [details] [review] [PATCH v4] wayland: selectively cancel key repeat on key release attachment 351063 [details] [review] pushed to git master as commit a23ad61 - wayland: selectively cancel key repeat on key release attachment 351063 [details] [review] pushed to branch gtk-3-22 as commit d9a9530 - wayland: selectively cancel key repeat on key release