Keyboard: avoid additional key stroke on hold release

[poire-z]

Added some forgotten return true on hold release event handlers: the event would otherwise trigger
a tap (as we generate a tap normally only at release time) and cause duplicate key strokes.
Details in #5555 (comment). Closes #5555.

(I included your suggested fix for the keyboard popup, as it's more related to my other fixes. You might want to remove it from #5569).

[Frenzie]

The behavior is still incorrect though.

1. Hold.
2. Move over to another key (outside the popup).
3. Release.
4. Now it's pressed!

If the modal behavior were as it should be (i.e., as it was prior to #5452) then there would be nothing to "fix."

[Frenzie]

Anyway, this will do for now I suppose.

[Frenzie]

This is @#$@# ugly though. ;-)

[Frenzie]

Actually I rescind that, I really hate this workaround. It hurts my eyes. :-P

[poire-z]

I don't understand much about that modal stuff - and hardly a bit much about the inner working of your key popups.
But the absence of these return true just looks like a bug - and adding them like the natural fix.

And having a quick look at that uimanager stuff, it look to me that #5452 is naturally correct. Before it, may be the events didn't reach the keyboard because something there in uimanager was (incorrectly) not sending them. But now they are, as I think they should.
For my backspace issue, which is the only one having a hold_callback (with your keypopups), it does some immediate action (delToStartOfLine). On hold release, like for all other keys, onHoldRelease is called, and is supposed to do the tap-like action associated with the key (delChar, or addChar). And it is doing that since #5452 (and I don't know why it wasn't before :). Preventing it from doing that (so, "ignore key release" :) looks to me like the correct way to fix the issue (and I don't see an other way that with this ugly flag :)
(If we get the Touch/press/pan event, it's not for uimanager modal handling to not send us also the pan/host release event).
Your key popup issue is a lot less understandable to me.

Having that fixed so far away in uimanager looks like far fetched side effect :)
But if that really make sense for you, I'm fine not merging this.

[Frenzie]

You can think of a modal dialog as a fullscreen widget for most intents and purposes. You wouldn't expect to be able to interact with anything below the fullscreen widget, quite the contrary. However, modal dialogs are arguably overused, including in KOReader. Does the wifi dialog even need to be modal? (The answer is probably affirmative within our context, because it might get lost behind other widgets.)

But the absence of these return true just looks like a bug - and adding them like the natural fix.

Not a bug exactly, at least not here, but more of a good practice to possibly prevent a few wasted CPU cycles.

Preventing it from doing that (so, "ignore key release" :) looks to me like the correct way to fix the issue (and I don't see an other way that with this ugly flag :)

But how are you overriding it to remove the line on hold?

[poire-z]

Not a bug exactly,

On my 2 unwanted delChar was solved by the return true. The other one by the self.ignore_key_release.

But how are you overriding it to remove the line on hold?

The removal of the line is done by the hold_callback, which is done on the "hold" event by:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 129 to 134 in f1f75c5

	HoldSelect = {
	GestureRange:new{
	ges = "hold",
	range = self.dimen,
	},
	},

handled by:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 206 to 214 in f1f75c5

	function VirtualKey:onHoldSelect()
	Device:performHapticFeedback("LONG_PRESS")
	if self.flash_keyboard and not self.skiphold then
	self[1].inner_bordersize = self.focused_bordersize
	self:update_keyboard(false, true)
	-- Don't refresh the key region if we're going to show a popup on top of it ;).
	if self.hold_callback then
	self[1].inner_bordersize = 0
	self.hold_callback()

A tap on a normal key is handled by:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 123 to 128 in f1f75c5

	TapSelect = {
	GestureRange:new{
	ges = "tap",
	range = self.dimen,
	},
	},

which calls:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 187 to 195 in f1f75c5

	function VirtualKey:onTapSelect(skip_flash)
	Device:performHapticFeedback("KEYBOARD_TAP")
	-- just in case it's not flipped to false on hold release where it's supposed to
	self.keyboard.ignore_first_hold_release = false
	if self.flash_keyboard and not skip_flash and not self.skiptap then
	self[1].inner_bordersize = self.focused_bordersize
	self:update_keyboard(false, true)
	if self.callback then
	self.callback()

But this one is also called when hold on a normal key (so, a slow tap):

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 135 to 140 in f1f75c5

	HoldReleaseKey = {
	GestureRange:new{
	ges = "hold_release",
	range = self.dimen,
	},
	},

handled by:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 243 to 250 in f1f75c5

	function VirtualKey:onHoldReleaseKey()
	Device:performHapticFeedback("LONG_PRESS")
	if self.keyboard.ignore_first_hold_release then
	self.keyboard.ignore_first_hold_release = false
	return true
	end
	self:onTapSelect()
	end

So, 2 way to handle tap: quick tap, and slow tap that appears like a hold.
We just want to not handle the hold_release when it's not a slow tap, but a real hold that has its action handled before the hold_release.
So, for the backspace, I'm not overriding anything.

[Frenzie]

I simply didn't realize that hold to remove a line was built-in behavior.

Anyway, note that #5452 resulted in three removals as opposed to what looks like the intended two.

So this PR could still make sense if you only want one.

[poire-z]

as opposed to what looks like the intended two.

I intended only one: the removal to start of line, not the removal of the \n (by that unwanted delChar) :) as without #5452.

Regarding this modal stuff handling:

koreader/frontend/ui/uimanager.lua

Lines 683 to 707 in f1f75c5

	-- if the event is not consumed, active widgets (from top to bottom) can
	-- access it. NOTE: _window_stack can shrink on close event
	local checked_widgets = {top_widget}
	for i = #self._window_stack, 1, -1 do
	local widget = self._window_stack[i]
	if checked_widgets[widget] == nil then
	-- active widgets has precedence to handle this event
	-- Note: ReaderUI currently only has one active_widget: readerscreenshot
	if widget.widget.active_widgets then
	checked_widgets[widget] = true
	for _, active_widget in ipairs(widget.widget.active_widgets) do
	if active_widget:handleEvent(event) then return end
	end
	end
	if widget.widget.is_always_active or widget.widget.modal then
	-- active widgets will handle this event
	-- Note: is_always_active widgets currently are widgets that want to show a keyboard
	-- and readerconfig
	-- By default modal widgets are always on top but if there is more than one modal
	-- widget, only last one will be top_widget. e.g. keyboard and confirmbox.
	checked_widgets[widget] = true
	if widget.widget:handleEvent(event) then return end
	end
	end
	end

We have modal:

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 502 to 503 in f1f75c5

	local VirtualKeyboard = FocusManager:new{
	modal = true,

koreader/frontend/ui/widget/virtualkeyboard.lua

Lines 270 to 271 in f1f75c5

	VirtualKeyPopup = FocusManager:new{
	modal = true,

So, if your modal popup key don't consume the event, it looks normal (but may be not correct) with this code that some other modal will get it: the keyboard, which I guess will give it to some VirtualKey, so your bug.
So, may be, in that uimanager code, when a modal is met, whether it's handleEvent() has consumed it or not (return true or false), we should stop checking other modal or any other widget (but still the is_always_active?).
Would that be the expected behaviour when having 2 modal, even when they are not overlapping?

[Frenzie]

Would that be the expected behaviour when having 2 modal, even when they are not overlapping?

No, absolutely not! That's why #5452 is dead wrong. A modal dialog should always be modal. The very idea of "some other modal" makes no sense.

[Frenzie]

If the wifi dialog pops up first and then another modal dialog opens on top of it then the wifi dialog should not be accessible.

The problem is that the caller there called two modal dialogs in a row. Only modal dialogs themselves are ever supposed to call other modal dialogs.

[poire-z]

OK.
In uimanager, a widget having self.modal = true gets itself put on top in the window_stack. There's no other check/use of this modal attribute.
So, if at some point we have may have 2 widgets with modal=true in the stack.
Now, some event is send and not handled by the 1st modal in the stack.
Should it be send to the other lower leve widgets (so, possibly send to another widget that had/has modal=true) - or meeting a widget with modal=true should just stop the propagation to lower level widgets?
From what you said, and logically, the propagation should be stopped. Our current code does not do that.

[poire-z]

OK, with #5574 and this one, I don't notice any issue. So I hope we're fine on the keyboard side.

[Frenzie]

Could you clarify that? The behavior described in #5573 (comment) and #5415 suggests that it behaves correctly.

[poire-z]

Yes, with your added return true in virtual_key.onHoldReleaseKey, the event is explicitely consumed and not propagated (even if it did not match the key, like tap outside it). So we're fine.

But if it is set to modal, and if we had forgotten this return true, it would be propagated to other widgets (modal or not). Shouldn't the stopping of the propagation be handled generically by UIManager because this widget is modal?
I'm wondering that generically, not thinking about these keyboard issues.

[Frenzie]

But that return true isn't responsible for that behavior.

[poire-z]

OK, forget about that keyboard and that return true. I have enough of it :)

But generically: sendEvent => modal widget handler not returning true => propagated to lower non-modal widgets. Correct behaviour or bug?

[Frenzie]

If that happened, that would be a bug. (Assuming that modal isn't a misnomer for always_on_top.)