Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Gesture manager: add action - show dictionary/Wikipedia #4699

Merged
merged 1 commit into from Mar 2, 2019

Conversation

Projects
None yet
3 participants
@Frenzie
Copy link
Member

Frenzie commented Mar 1, 2019

References #4687.

@Frenzie Frenzie added the UX label Mar 1, 2019

@Frenzie Frenzie added this to the 2019.03 milestone Mar 1, 2019

@Frenzie Frenzie requested a review from poire-z Mar 1, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Looks OK, but I tested it (the lookups, not the gestures) and strangely, the keyboard is a lot less responsive with this patch than without...
I can't really see why, so may be it's something with my emulator (but patch -p1 and patch -R -p1 repeatedly show the same differences...)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 1, 2019

You mean the onscreen keyboard?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Yes, our on-screen keyboard.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 1, 2019

I'll give it a try on my H2O this weekend. I don't think I notice anything. Is there a particular way to demonstrate the problem?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 1, 2019

Put the mouse cursor on a virtual keyboard key, and hit your mouse left click like mad
And ... have "Disable double tap" unchecked :) (I had unchecked earlier today to check some of your stuff I guess).
So, no problem when double tap is disabled (10 mad clicks = 10 letters). But there's a difference when it is not disabled (10 mad clicks = 1 or 2 letters)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

Ah, so then it's probably not a performance thing (which would be bothersome) but simply a case of (un)intended double tap delay. I still can't reproduce it though, neither in file manager nor reader.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Most probably a property that can be set to a widget to have double tap disabled for this widget (even if it's not disabled globally) - that went missing (or was not set anymore) after your small refactoring.

frontend/apps/reader/modules/readerpaging.lua:    Input.disable_double_tap = false
frontend/apps/reader/modules/readerpaging.lua:    Input.disable_double_tap = true
frontend/ui/uimanager.lua:    if widget.disable_double_tap == false then
frontend/ui/uimanager.lua:        Input.disable_double_tap = false
frontend/ui/uimanager.lua:        Input.disable_double_tap = true
frontend/ui/uimanager.lua:    Input.disable_double_tap = true
frontend/ui/uimanager.lua:            if self._window_stack[i].widget.disable_double_tap == false then
frontend/ui/uimanager.lua:                Input.disable_double_tap = false
frontend/ui/widget/virtualkeyboard.lua:    disable_double_tap = true,
frontend/device/input.lua:    disable_double_tap = true,
@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

So then is it only in dictionary?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

With "Disable double tap" not disabled:

Without this PR / with this PR

Dictionnary lookup: OK / SLOW
Wikipedia lookup: SLOW / SLOW
Fullteaxt search: OK / OK
Reading statistics > Settings : OK / OK
Text editor: OK / OK
Rename bookmark: SLOW / SLOW

So, it's a more general issue when double tap is not disabled, that is taken care of in various places with keyboard input, but not all - and you broke only Dictionary lookup with this PR (so, once unbroken, that trick should be put into these other places).

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

I don't really understand why I'm failing to spot the problem but that makes sense. The dictionary lookup is something I actually changed (concretely, by copying it from Wikipedia) so it's a rewrite that (seemingly) functions the same.

Wikipedia was just a tiny reshuffling of the exact same code.

What's not clear to me is why this should be an issue, because the VirtualKeyboard uses disable_double_tap = true,.

As a quick workaround you could see if adding disable_double_tap = true, to the InputDialog widget resolves the problem.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

@poire-z Instead of using InputDialog directly we should be able to use the InputContainer's onInput event (just like in the menu, which apparently worked well with double tap). It'd make for slightly less code but I don't really see how it should make a difference in actual functionality. :-/

function InputContainer:onInput(input)
local InputDialog = require("ui/widget/inputdialog")
self.input_dialog = InputDialog:new{
title = input.title or "",
input = input.input,
input_hint = input.hint_func and input.hint_func() or input.hint or "",
input_type = input.type or "number",
buttons = input.buttons or {
{
{
text = input.cancel_text or _("Cancel"),
callback = function()
self:closeInputDialog()
end,
},
{
text = input.ok_text or _("OK"),
is_enter_default = true,
callback = function()
input.callback(self.input_dialog:getInputText())
self:closeInputDialog()
end,
},
},
},
}
UIManager:show(self.input_dialog)
self.input_dialog:onShowKeyboard()
end

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

As a quick workaround you could see if adding disable_double_tap = true, to the InputDialog widget resolves the problem.

Nope, this does not solve it:

local InputDialog = InputContainer:new{
+    disable_double_tap = true,
    is_always_active = true,
@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

Hm, curious. Anyway, shall we merge this as is, seeing how the problem is unrelated, or strip out the dictionary part (since Wikipedia is already borked)?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Go ahead. We'll have that into master to fix them all when we find the culprit.

@Frenzie Frenzie merged commit 57ce8dc into koreader:master Mar 2, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:action-dictwiki branch Mar 2, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Might be dependant of readerrollin being used.
There's this on widget show:

-- check if this widget disables double tap gesture
if widget.disable_double_tap == false then
Input.disable_double_tap = false
else
Input.disable_double_tap = true
end

and this on widget close:
-- make it disabled by default and check any widget that enables it
Input.disable_double_tap = true
-- then remove all references to that widget on stack and refresh
for i = #self._window_stack, 1, -1 do
if self._window_stack[i].widget == widget then
self._dirty[self._window_stack[i].widget] = nil
table.remove(self._window_stack, i)
dirty = true
else
-- If anything else on the stack was dithered, honor the hint
if self._window_stack[i].widget.dithered then
refreshdither = true
logger.dbg("Lower widget", self._window_stack[i].widget.name or self._window_stack[i].widget.id or tostring(self._window_stack[i].widget), "was dithered, honoring the dithering hint")
end
if self._window_stack[i].widget.disable_double_tap == false then
Input.disable_double_tap = false
end
end
end

So, any widget on the stack that didn't disable double tap will enable it, and all the others that did disable it are screwed (#2155).
So, readerui sets disable_double_tap = false (for readerrolling double tap to go to prev/next chapter), so I guess any input/virtualkeyboard that is stacked over it don't get double tap disabled as they requested.
(Why no issue when Input invoked from menu, no idea :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

This seems to solve the issue (but as it's uimanager wild territory, I won't go there alone :) so pinging @NiLuJe - or we might want to wait after release to fix that):

--- a/frontend/ui/uimanager.lua
+++ b/frontend/ui/uimanager.lua
@@ -318,8 +318,9 @@ function UIManager:close(widget, refreshtype, refreshregion, refreshdither)
     widget:handleEvent(Event:new("FlushSettings"))
     -- first send close event to widget
     widget:handleEvent(Event:new("CloseWidget"))
-    -- make it disabled by default and check any widget that enables it
+    -- make it disabled by default and check if any widget wants it disabled or enabled
     Input.disable_double_tap = true
+    local requested_disable_double_tap = nil
     -- then remove all references to that widget on stack and refresh
     for i = #self._window_stack, 1, -1 do
         if self._window_stack[i].widget == widget then
@@ -333,11 +334,15 @@ function UIManager:close(widget, refreshtype, refreshregion, refreshdither)
                 logger.dbg("Lower widget", self._window_stack[i].widget.name or self._window_stack[i].widget.id or tostring(self._window_stack[i].widget), "was dithered, honoring the dithering hint")
             end

-            if self._window_stack[i].widget.disable_double_tap == false then
-                Input.disable_double_tap = false
+            -- Set double tap to how the topest that specified how it wants it want it
+            if requested_disable_double_tap == nil and self._window_stack[i].widget.disable_double_tap ~= nil then
+                requested_disable_double_tap = self._window_stack[i].widget.disable_double_tap
             end
         end
     end
+    if requested_disable_double_tap ~= nil then
+        Input.disable_double_tap = requested_disable_double_tap
+    end
     if dirty and not widget.invisible then
         -- schedule remaining widgets to be painted
         for i = 1, #self._window_stack do
@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

I actually solved a similar issue with the screensaver a couple of years ago (except in that case properly resolving which widget was the topmost for drawing purposes). We'll need at least a few more days prior to release so this might just be on the safe enough side of risky.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

Besides some more finishing touches on the multiswipes (mainly defaults & some actions) #4708 is a prerequisite. I'd also like to have #4667 finished to ensure an optimal F-Droid first impression.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Another thing would be to disable double tap by default, or altogether.
It seems only used by readerolling to double tap in the 1/4 top left & right corners to go to prev/next chapters.
Otherwise, it will just make tap'ing feels slow to new users (that have not yet disabled it).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 2, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Mar 2, 2019

But double tap is disabled by default, isn't it?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 2, 2019

Oh, looks like it is :) sorry. And good for me to not have to look at that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.