VirtualKeyboard: Revamp visibility handling #10852
Conversation
It never made sense to me for it to be handled anywhere else.
I don't think it really matters, because we don't care about its own weird visibility tracking, but it is technically saner this way.
InputDialog instance bakc to a fullscreen one
It's Close => onCloseWidget, so that should already have happened
as fullscreen Not super fond of doing this here, but InputDialog can run init so much at runtime that it could be brittle in there...
Just have to double-check whether we should actually lock in toggle if nav_bar but not fullscreen...
| function InputDialog:onTap() | ||
| -- This is slightly more fine-grained than VK's own visibility lock, hence the duplication... | ||
| if self.deny_keyboard_hiding then | ||
| return | ||
| end | ||
| if self._input_widget.onCloseKeyboard then | ||
| self._input_widget:onCloseKeyboard() | ||
| end | ||
| self:onCloseKeyboard() | ||
| end |
There was a problem hiding this comment.
Wondered why we were closing the keyboard on Tap! :)
I guess that's because a real Tap in the text field is handled in the sub InputText - and if it reaches InputDialog, it is that we tapped outside a text field, so unfocused from a text entry, and so it makes sense to have that closing the keyboard.
Might be worth a comment - if I am right :)
There was a problem hiding this comment.
I haven't traced that one precisely, but that sounds right (the handler is defined a few lines above that, and it indeed sets the range as "not the keybnoard" ;p).
The InputText:onTapTextBox thing is commented further down, though.
|
A picture with a thousand words should be worth a thousand words (but not really, as its text won't be findable via Search): No issue if closed and reopened via the nav_bar keyboard button. |
|
Some issue with Text editor and just taping twice on the toggle keyboard nav_bar button: Keyboard (original I guess) shown, but it hides the nav_bar&co buttons (which must still be at bottom), + inputtext height sizing/redrawing issue, showing some old bits of the previous painting:
Just typing something (with VK or my PC keyboard) draws the nav bar buttons in that space back. But only once: hit that KB-toggle button again twice, same situation as on the screenshot, but typing does not help this time. Some other drawing issue when I typed Esc multiple time to close KOReader :): Somehow, no such issue at all when in Book style tweak (some (the?) other user of the full screen InputDialog): |
Yeah, I was wondering whether the SDL stuff was going to take it in stride, guess not ;p. |
Hmm, I tested on a mostly blank file, but, ditto, was hoping it would Just Work there ;p. |
Okay, that's sneakier than that, you need to have scrolled for it to screw up. (The good news is it's simply a missing refresh, not a missing redraw). |
And, also, don't scroll the document on toggle, restore the *current* position, not tha last init's.
|
Fixed the refresh/scrolling thing, but there was an (existing) weird behavior in that a toggle would restore out of date caret positions (i.e., the last init's, not the current), which means you'd essentially jump around the document when toggling. I fixed that, but I'm vaguely wondering if that was somehow on purpose? (That feels weird, because there's a lot of engineering going on in that codepath...). EDIT: Since I have a fresh emu build for the SDL thing, yeah, definitely existing behavior. |
No issues with a simple InputDialog either (e.g., Dic/Wik lookup). Looking at RBM, probably specific to EDIT: Forgot to apply the PR, lol. |
So, if I don't forget to apply the patch, yeah, that's reproducible there, which makes this easier to follow ;D. |
Ensure it's actually only on when a VK is shown, no matter how that VK gets shown/hidden.
Text editor (with or without keyboard shown on startup, doesn't matter), scroll via a swipe inside the text area (that may be important), toggle keyboard => will jump to the starting position. (I sort of mispoke when I mentioned the caret, I mostly mean the topmost visible line. That might match the line the caret is on, but depending on which way you scroll and which way you toggle, the actual caret might end up on the bottom line after the fix. Basically, what was visible before the toggle should still be after, instead of jumping back to the initial position).
I would assume so. IIRC, most of the calls inside InputText are for actual (physical) keyboard trigger (and possibly actual on-Kobo physical keyboard, not sure if SDL takes these codepaths).
The thought crossed my mind, and then I realized I have no idea why we even disable SDL text input at all. Binding conflicts is probably it? (Fun fact: you can still do some stuff when SDL text input is disabled, e.g., enter & return for sure, I assume directional arrows and nav stuff too, probably?). |
ReaderBookMark, but that was a red herring anyway, since I hadn't applied the changes at the time ;D. |
Meaning most stuff actually goes one or two layer deeper into InputDialog's InputText's ScrollTextWidget's own TextBoxWidget's scroll method. So AFAICT InputDialog's InputText has no clue about the updated positions, we have to ask it to query its own nested widgets, otherwise we're just reading back the stuff we instantiated it with. |
In which context? I thought we'd been working on adding it, not disabling it. |
long as there's an input dialog displayed
It's mainly for the sake of an implementation-specific detail. As far as InputDialog is concerned, we already have our own perfecelty valid top_line_num & charpos available. (It's a re-init, the fields won't have been cleared).
Basically, as it stands, the intent seemed to be to enabled SDL text input as long as a VirtualKeyboard is visible. This leads to the aforementioned quirks when you have an input field visible but a VK hidden. We could instead bracket those calls to make it so it's enabled as long as an InputDialog is visible, which would possibly be slightly less confusing, but would introduce another quirk: you'd be able to input text in a potentially unfocused field (likely the last one in focus). I haven't changed the behavior yet, but added a comment about it. |
Except things get gnarly when you start stacking multiple InputDialog, so, maybe scratch that thought ;p. |
What quirk? VK not visible = field not focused = why would you expect keyboard input to work. |
|
I meant the fact that navigation w/ the physical keyboard (especially enter & return) still works in that case ;). |
frontend/ui/widget/inputdialog.lua
Outdated
| @@ -635,7 +640,7 @@ function InputDialog:onClose() | |||
| self._top_line_num = self._input_widget.top_line_num | |||
| self._charpos = self._input_widget.charpos | |||
| if self.view_pos_callback then | |||
| -- Give back top line num and cursor position | |||
| -- Let the widget store the current top line num and cursor position | |||
There was a problem hiding this comment.
"the widget" in all the comments in this commit feels a bit confusing. For me, a "widget" is some UI small element (ie. InputDialog, InfoMessage). I think it would at least need "upper" or "parent": "upper widget".
And widgets don't usually handle/store settings, so "application", "using code", "upper code", something like that would sound better.
There was a problem hiding this comment.
Agreed, and doubly so because the widget is actually, well, this widget ;D.
Haven't had time to look by myself and being lazy :/ so...: why do they work ? :)
Yes indeed, "scroll via swipe" was important :) it's indeed handled in lower widgets and not fed back to upper InputText. |
Those are implemented very differently (i.e., identical to Kindle with hw kb). If you meant from a user perspective, sure, that's all missing functionality, but from a code perspective I don't know of any quirks unless they were introduced recently. |
Because they're standard keycodes in our mappings, I assume. i.e.,
I vaguely recall needing them when testing physical keyboards, possibly because the very thing might trigger an immediate scroll, requiring up-to-date coordinates on the spot? (It's been a while). But, indeed if that ends up going through the same close/init codepaths, they'd be redundant. Can't check right now, need to charge an OTG-capable device ;). Out of curiosity, how were you scrolling in your testing? |
Swiping over the textwidget with the mouse on the emulator. |
Well, it was charged ;). Verdict: I don't quite remember what they're here for, because at a quick glance I don't break anything by removing them, but, in any case, this does not take the InputDialog close/init codepaths (which makes sense, we're just scrolling), soooo, not going to rock the boat too much ;p. |
|
Quickly re-tested, on the emu and my Kobo: no issue noticed. I thought that, now that (I think?) you just hide/show a same keyboard instead of maybe recreating one each time you hide/show, that the keyboard layout we were in when hiding would be the same when shown again - but apparently not (which is fine by me :).
All ok with not rocking the boat :) (but this is just so not you :)) |
Yeah, that'd be a whole 'nother kettle of fish ;p. (And a bit of a fool's errand when the rest of the input widgets are always destroyed and recreated from scratch at runtime anyway).
;D. FWIW, I checked the blame, and I must have been thinking of some other small positioning related thingy when I was reminiscing about the OTG keyboard testing, because those are much, much older than that :s. |
Ought to be a no-op, since readonly means there's no VirtualKeyboard widget to toggle at all, but it's clearer this way, especially given how the keyboard_visible prop is decoumented.
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 1 of 2 files at r4, 2 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z)
Move as much of the state tracking as possible inside VirtualKeyboard itself.
InputDialog unfortunately needs an internal tracking of this state because it needs to know about it before the VK is shown, so we have to keep a bit of duplication in there, although we do try much harder to keep everything in sync (at least at function call edges), and to keep the damage contained to, essentially, the toggle button's handler.
(Followup to #10803 & #10850)
This change is