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

Gesture manager (initial) #4240

Merged
merged 10 commits into from Sep 29, 2018

Conversation

Projects
None yet
5 participants
@robert00s
Contributor

robert00s commented Sep 22, 2018

This PR creates a gesture manager for now for 3 gestures and a limited number of actions.
I will try to expand the manager in the future.
Manager supports separate gestures for the file manager and the reader. Gestures from the manager have a higher priority than built-in gestures.

Settings available in Gear -> Device -> Gesture manager

a - koreader_173
a - koreader_174
a - koreader_175
kontratyp - koreader_177

Close: #1008

robert00s added some commits Aug 17, 2018

@KenMaltby

This comment has been minimized.

KenMaltby commented Sep 22, 2018

Minor question, how is "Disable" being used? Is it an option to disable that touch area? What is being disabled in each menu?

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 22, 2018

@KenMaltby
Disable is default for all touch area (Gestures as they are now and nothing changes).
If you want to switch to night mode after touching right bottom corner of the screen, you choose the Night mode in submenu Tap right bottom corner. If you want to cancel this action, you choose Disable.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 22, 2018

This gestures work only in FileManager and Reader mode. They don't work when another widget is opened (e.g. table of content, history - widgets have own set of predefined gesture).

@Frenzie

This comment has been minimized.

Member

Frenzie commented Sep 22, 2018

Most interesting. :-)

@KenMaltby

This comment has been minimized.

KenMaltby commented Sep 22, 2018

It looks like there would be room for the check box line to be "No Manager Option", checked by default. Perhaps "Radio Buttons" would work better?

OT;
I would think their could be those users who would like to be able to selectively "Disable" the "built-in" gestures, that get in their way.

@AlanSP1

This comment has been minimized.

AlanSP1 commented Sep 23, 2018

Is there somewhere info on all gestures? I think those in wiki are not up to date? Or I'm mistaken?

@Frenzie

This comment has been minimized.

Member

Frenzie commented Sep 23, 2018

There's the swipe to open/close menu and @poire-z just added swipe down to close some popups.

Is there anything else that's been added?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Sep 24, 2018

Is there anything else that's been added?

There is also pinch/spread in reader to change font size, #2768.

Is there somewhere info on all gestures?

I guess we're just mentionning the gestures that work in reader/filemanager, because there are many other that are widget specifics (Dictionary, TextViewer, TextEditor, ImageViewer, MovableContainer have their own set of gestures, that I think are already mentionned in their proper wiki page).

@poire-z

This comment has been minimized.

Contributor

poire-z commented Sep 24, 2018

It looks like there would be room for the check box line to be "No Manager Option", checked by default. Perhaps "Radio Buttons" would work better?

May be each action menu could just be organised differently, and avoid the word Disable:

Default action
--------------
No action
--------------
All the other new actions

eg, for Short diagonal swipe:

Full screen refresh (default)
--------------
Nothing
--------------
Folder up
Toggle nightmode
Reading progress
@@ -698,6 +698,7 @@ function Menu:init()
callback = function() self:onReturn() end,
bordersize = 0,
show_parent = self,
readonly = self.return_arrow_propagation,

This comment has been minimized.

@poire-z

poire-z Sep 24, 2018

Contributor

So, this return button will not work anymore? I don't see it being re-enabled.
I see it being used in the Statistics navigation. Anywhere else?

This comment has been minimized.

@robert00s

robert00s Sep 24, 2018

Contributor

Yes, It's also used in OPDS browser.

}
end
function ReaderGesture:subMenuAction(ges, action)

This comment has been minimized.

@poire-z

poire-z Sep 24, 2018

Contributor

This function name seems wrong, and should probably be renamed to ReaderGesture:setupGesture(ges, action)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Sep 24, 2018

Looks quite unintrusive to the other code, which is nice :)
(Can't comment about the touchzone/dependancy graph code, as I never really looked at it, so I trust you on this.)

@AlanSP1

This comment has been minimized.

AlanSP1 commented Sep 24, 2018

OK, I updated wiki gestures page with gestures you mentioned and I know of. If there's some more gestures, please mention them.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 25, 2018

  • new menu:
    pragnienie - koreader_035
  • refactoring code
  • add default gesture
  • add toggle frontlight gesture
  • toggle frontlight gesture move from kobolight plugin to gesture manager
  • prevent crash KO after choose reading progress and disable plugin
@@ -1187,7 +1188,9 @@ function Menu:onSwipe(arg, ges_ev)
elseif ges_ev.direction == "north" then
-- no use for now
do end -- luacheck: ignore 541
else -- diagonal swipe
elseif not G_reader_settings:readSetting("gesture_fm")["short_diagonal_swipe"] then

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

May be this won't crash (with "trying to index a nil value") because the order of things happening (on the first startup after one installs a nightly with this merged) is right.
But it looks safer to do it in 2 tests:

else -- diagonal swipe
    if G_reader_settings:readSetting("gesture_fm") and G_reader_settings:readSetting("gesture_fm")["short_diagonal_swipe"] then
        -- managed by gesture manager
        do end -- luacheck: ignore 541
    else
        -- trigger full refresh
        UIManager:setDirty(nil, "full")
    end
end

This comment has been minimized.

@robert00s

robert00s Sep 25, 2018

Contributor

Good idea. Thanks :)

self:onShowOnOff()
return true
end

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

toggle frontlight gesture move from kobolight plugin to gesture manager

That seems a bit unfortunate to move the code that should logically be there, to some nearly unrelated outside module.
No way to keep it there, and have this plugin possibly plugged some stuff in ReaderGesture, just like you're used to do that in the Statistics plugin?
Or by having a event handler KoboLight:onToggleFrontLight() and ReaderGesture would just send or broadcast that event.

This comment has been minimized.

@robert00s

robert00s Sep 25, 2018

Contributor

I want to migrate all kobolight (Frontlight gesture controller) to readerfrontlight + readergesture and remove completely this plugin. This is first step (tap), next will be swipe left and right side of the screen and all logic will be moved to readerfrontlight.
I want to do that on another PR when new gestures will be added.

{_("Bookmarks"), "bookmarks", not self.is_docless},
{_("Folder up"), "folder_up", self.is_docless},
{_("Forward 10 pages"), "page_update_up10", not self.is_docless},
{_("Full screen refresh"), "full_refresh", true},

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

Nicely done, to have a single list of actions.

I see you ordered these alphabetically and not logically, but that english alphabetical order will be shuffled by translations in other languages :)
So, best to keep them logically ordered (with or without separator, as the separation in category is a bit suggestive :):
Page navigation: Back 10 page , Forward 10 pages, Folder up
Book navigation (in the order they are in the menu): Table of Content , Bookmarks , Reading Progress
Misc: Toggle Frontlight , Night mode , Full screen refresh

This comment has been minimized.

@robert00s

robert00s Sep 25, 2018

Contributor

Agree. Logical sorting is better :)

{_("Full screen refresh"), "full_refresh", true},
{_("Night mode"), "night_mode", true},
{_("Reading progress"), "reading_progress", ReaderGesture.getReaderProgress ~= nil},
{_("Table of context"), "toc", not self.is_docless},

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

Table of content (not context)

-- add default action to the top of the submenu
for _, entry in pairs(menu) do
if entry[2] == default then
local menu_entry_default = string.format("%s (default)", entry[1])

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

That (default) wording and position in the menu item string might be different in other translations, so I guess it should use some T(_("%1 (default)"), entry[1])

screen_zone = zone,
handler = function(gest)
if distance == "short" and gest.distance > Screen:scaleBySize(300) then return end
if direction and not directionContain(direction, gest.direction) then return end

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

You could avoid having this function called and its loop (not that expensive, but still) on each gesture check by just having above:
direction = {northeast = true, northwest = true, southeast = true, southwest = true}
instead of
direction = {'northeast', 'northwest', 'southeast', 'southwest'}
and just use here:
if direction and not direction[gest.direction] then return end
(or have a local function that build the first one from the second one, but that would be called only once in setupGesture, instead of at each time handler is called)

if return_value then
-- a long diagonal swipe may also be used for taking a screenshot,
-- so let it propagate
-- no implemented for now

This comment has been minimized.

@poire-z

poire-z Sep 25, 2018

Contributor

no implemented for now
Is it, or is it still not?
I guess you could use if return_value ~= nil then return return_value end
and above:

    elseif action == "full_refresh" then
        UIManager:setDirty(nil, "full")
        return_value = false

edit: or just set at start if return_value == nil then return_value = true end and change it back to false when needed, and no test needed at end.

This comment has been minimized.

@robert00s

robert00s Sep 25, 2018

Contributor

I had another conception a few days ago. This is no more needed so I will remove return_value :)

robert00s added some commits Sep 25, 2018

@AlanSP1

This comment has been minimized.

AlanSP1 commented Sep 25, 2018

Not sure if small typo is corrected, it is "table of content" not "context".

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 26, 2018

@AlanSP1
It was corrected yet :)

local menu = {
--{_("Menu element), "action", enable_element},
{_("Nothing"), "nothing", true },
{_("Backward 10 pages"), "page_update_down10", not self.is_docless},

This comment has been minimized.

@Frenzie

Frenzie Sep 29, 2018

Member

Just Back 10 pages.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Sep 29, 2018

@robert00s Please merge when ready, unless @poire-z has any further comments. :-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Sep 29, 2018

No further comment, but I don't know if it's still work in progress ? (I haven't re-read it lately, but it seemed we were losing the full diagonal swipe to take screenshots.)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Sep 29, 2018

Ah, I didn't read it in detail because you already did.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 29, 2018

All work is done. Long diagonal swipe still take screenshots.

@Frenzie Frenzie merged commit dc5a479 into koreader:master Sep 29, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
function ReaderGesture:buildMenu(ges, default)
local gesture_manager = G_reader_settings:readSetting(self.ges_mode)
local menu = {
--{_("Menu element), "action", enable_element},

This comment has been minimized.

@Frenzie

Frenzie Sep 30, 2018

Member

This commented line is breaking the regex for translation because it contains a syntax error. But it can probably just be removed completely.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 1, 2018

2 small issues:
Now that ReaderGesture handles diagonal swipe, the original code is not used, so we may miss the additional stuff it does. For the reader, it additionally updated the footer:

else
-- update footer (time & battery)
self.view.footer:updateFooter()
-- trigger full refresh
UIManager:setDirty(nil, "full")
end

We can quickly get it back by adding to readerui.lua:

    if Device:isTouchDevice() then
        -- gesture manager
        self:registerModule("gesture", ReaderGesture:new {
            document = self.document,
+           view = self.view,
            ui = self,
        })
    end

and to readergesture.lua:

    elseif action == "full_refresh" then
+       if self.view then
+           -- update footer (time & battery)
+           self.view.footer:updateFooter()
+       end
        UIManager:setDirty(nil, "full")

But I don't know, may be it would be more correct to just not handle the action when it's the default action, and to just let the event propagate so it's handled by the original code (and we're not confused when we add some stuff to readerrolling and see it not happening because it's actually readergesture that is now handling the gesture).

Also (not investigated, was just testing), when setting bottom left to toggle night mode, night mode is not immediately set: you need to change page or trigger full refresh to see it.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Oct 1, 2018

@poire-z Thanks.

But I don't know, may be it would be more correct to just not handle the action when it's the default action, and to just let the event propagate so it's handled by the original code (and we're not confused when we add some stuff to readerrolling and see it not happening because it's actually readergesture that is now handling the gesture).

I think that your code is better than handle refresh by the original code. This would be (maybe) better for digonal swipe but not for tap (left or right corner). Without your patch when we select full refresh by tapping in corner we don't have update footer.

Also (not investigated, was just testing), when setting bottom left to toggle night mode, night mode is not immediately set: you need to change page or trigger full refresh to see it.

I check that later and prepare patch if needed.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Dec 2, 2018

Mhhhh, strange I just noticed this so long after this PR (so, may be it's not this PR fault but some later commit side effect, but I can't see which), but small diagonal refresh seems to not work anymore in "Menu" instances that are overlays over reader of filemanager: History, TOC, Bookmarks...
It still works in File Browser (althought the "short" distance limitation makes me miss some :)

Possibly because of this:

else -- diagonal swipe
if G_reader_settings:readSetting("gesture_fm") and G_reader_settings:readSetting("gesture_fm")["short_diagonal_swipe"] then
-- managed by gesture manager
do end -- luacheck: ignore 541
else
-- trigger full refresh
UIManager:setDirty(nil, "full")
end
end

Not sure if (or why it is not) that event is/should be propagated up the main widget (reader or filemanager), where it would/should reach the readergesture widget and be manager by it...

A bit confused. (Can someone else confirms it doesn't work?)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Dec 2, 2018

Seems like using a hack like this in the above code solves the issue:

    else -- diagonal swipe
        if self.show_parent and self.show_parent.tapPlus and -- it is FileManager
            G_reader_settings:readSetting("gesture_fm") and G_reader_settings:readSetting("gesture_fm")["short_diagonal_swipe"] then
            -- managed by gesture manager
            do end -- luacheck: ignore 541
        else
            -- trigger full refresh
            UIManager:setDirty(nil, "full")
        end
    end

We should probably make that more explicit by just adding an attribute in the FileChooser created by FileManager: i_am_filemanager = true :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Dec 5, 2018

(diagonal refresh fix added in #4378)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment