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

[feat, UX] Gesture manager: add option - open previous document #4641

Merged
merged 3 commits into from Feb 22, 2019

Conversation

Projects
None yet
2 participants
@Frenzie
Copy link
Member

Frenzie commented Feb 22, 2019

Fixes #4333.

@Frenzie Frenzie force-pushed the Frenzie:gesture-manager-previous-file branch from 3fbb163 to eb788c1 Feb 22, 2019

@@ -97,6 +97,7 @@ function ReaderGesture:buildMenu(ges, default)
{_("Reading progress"), "reading_progress", ReaderGesture.getReaderProgress ~= nil},
{_("Full screen refresh"), "full_refresh", true},
{_("Night mode"), "night_mode", true},
{_("Open previous document"), "open_previous_document", true},

This comment has been minimized.

@poire-z

poire-z Feb 22, 2019

Contributor

This list is getting quite crowded :)
I would move this Open previous document nearest to History (same kind of category for me).
It may need to be sorted. I'd see various categories (with separators if possible):

  • document page navigation
  • document info panels (toc, bookmarks, reading progress).
  • other documents list or switch (history, Open previous)
  • device stuff (night mode, suspend, frontlight...)
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 22, 2019

Dunno how hard it would be to show the action chosen (empty meaning "nothing") in the action menu list, as the concat of the 2 strings may be long.
May be some other symbol meaning "some action is assigned to this gesture" could be simpler to add, and would already be quite helpful?

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 22, 2019

My intent is to add default actions to at least most of the default gestures prior to release, so just "assigned" shouldn't be too useful. (Though still worth considering.)

@Frenzie Frenzie merged commit 9a92792 into koreader:master Feb 22, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:gesture-manager-previous-file branch Feb 22, 2019

@Frenzie Frenzie added this to the 2019.03 milestone Feb 22, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 24, 2019

Regarding the ordering of actions, and separators, I would go with something like:

    local menu = {
        {_("Nothing"), "nothing", true },

        {_("Back 10 pages"), "page_jmp_back_10", not self.is_docless},
        {_("Previous page"), "page_jmp_back_1", not self.is_docless},
        {_("Forward 10 pages"), "page_jmp_fwd_10", not self.is_docless},
        {_("Next page"), "page_jmp_fwd_1", not self.is_docless},
        {_("Back to previous location"), "previous_location", not self.is_docless, true},

        {_("Table of contents"), "toc", not self.is_docless},
        {_("Bookmarks"), "bookmarks", not self.is_docless},
        {_("Reading progress"), "reading_progress", ReaderGesture.getReaderProgress ~= nil, true},

        {_("History"), "history", true},
        {_("Open previous document"), "open_previous_document", true, true},

        {_("Full screen refresh"), "full_refresh", true},
        {_("Night mode"), "night_mode", true},
        {_("Suspend"), "suspend", true},
        {_("Toggle frontlight"), "toggle_frontlight", Device:hasFrontlight()},
        {_("Toggle accelerometer"), "toggle_gsensor", Device:canToggleGSensor()},
        {_("Toggle rotation"), "toggle_rotation", not self.is_docless, true},

        {_("Zoom to fit content width"), "zoom_contentwidth", not self.is_docless},
        {_("Zoom to fit content height"), "zoom_contentheight", not self.is_docless},
        {_("Zoom to fit page width"), "zoom_pagewidth", not self.is_docless},
        {_("Zoom to fit page height"), "zoom_pageheight", not self.is_docless},
        {_("Zoom to fit column"), "zoom_column", not self.is_docless},
        {_("Zoom to fit content"), "zoom_content", not self.is_docless},
        {_("Zoom to fit page"), "zoom_page", not self.is_docless, true},
        -- Trailing separator here :( as folder_up is only shown when in file manager

        {_("Folder up"), "folder_up", self.is_docless, true},
    }

I'm not sure it makes sense to have them in submenus by category - in that context, it's nicer to see all those that are available, with just separators.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 24, 2019

Sure, just stick it in a PR (as well as that skimming thing).

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 24, 2019

@poire-z Actually hold on, I'll refactor this part of the code slightly so the currently selected action can be shown, which would conflict with this reordering (which I'll do simultaneously).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 24, 2019

OK, I'm on it.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 24, 2019

Oh, i had it ready, and didn't see your post.
Feel free to pick stuff from poire-z@b4b1994 (including the G_reader_settings:readSetting("multiswipes_enabled") to be moved into :init())

edit: I changed a bit the order from what I suggested above. Tested as in my commit.

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.