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

[Non-touch] Add a menu item to toggle a bookmark on the current page #4262

Merged
merged 3 commits into from Oct 16, 2018

Conversation

Projects
None yet
3 participants
@onde2rock
Contributor

onde2rock commented Oct 9, 2018

close #4260

Add a menu item to toggle a bookmark on the current page
Only appear on non-touch device
@@ -55,6 +55,14 @@ function ReaderBookmark:addToMainMenu(menu_items)
self:onShowBookmark()
end,
}
if not Device:isTouchDevice() then
menu_items.add_bookmarks = {
text = _("Toggle bookmarks"),

This comment has been minimized.

@poire-z

poire-z Oct 10, 2018

Contributor

I guess it should be singular Toggle bookmark (or Toggle page bookmark ?)
And for consistency, name the menu item index similar to its text, eg: menu_items.toggle_bookmark

You could even add a function (untested):

function ReaderBookmark:isCurrentPageBookmarked()
    local pn_or_xp
    if self.ui.document.info.has_pages then
        pn_or_xp = self.view.state.page
    else
        pn_or_xp = self.ui.document:getXPointer()
    end
    return self:getDogearBookmarkIndex(pn_or_xp) and true or false
end

and use it as text_func = function() return self:isCurrentPageBookmarked() and "Remove bookmark for current page" or "Bookmark current page"

This comment has been minimized.

@onde2rock

onde2rock Oct 10, 2018

Contributor

Yes, that's better.

onde2rock added some commits Oct 10, 2018

@@ -55,6 +55,14 @@ function ReaderBookmark:addToMainMenu(menu_items)
self:onShowBookmark()
end,
}
if not Device:isTouchDevice() then

This comment has been minimized.

@Frenzie

Frenzie Oct 10, 2018

Member

Should we hide this on touch devices? Pinging some touch users to see what they think:

@KenMaltby @ersidnd @cramoisi

This comment has been minimized.

@poire-z

poire-z Oct 16, 2018

Contributor

Merging this (we can still show it later if there's feedback towards that).

@poire-z poire-z merged commit 53e7a0b into koreader:master Oct 16, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@onde2rock onde2rock deleted the onde2rock:no-touch-bookmarks branch Oct 16, 2018

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