Skip to content

Next/Previous Bookmark Gestures#5968

Merged
Frenzie merged 5 commits intokoreader:masterfrom
yparitcher:bookmark_gesture
Mar 16, 2020
Merged

Next/Previous Bookmark Gestures#5968
Frenzie merged 5 commits intokoreader:masterfrom
yparitcher:bookmark_gesture

Conversation

@yparitcher
Copy link
Copy Markdown
Member

@yparitcher yparitcher commented Mar 15, 2020 •
edited by Frenzie
Loading

Fixes #5965

This change is Reviewable

@Frenzie Frenzie added this to the 2020.04 milestone Mar 15, 2020
Frenzie
Frenzie approved these changes Mar 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine in principle.

poire-z
poire-z reviewed Mar 15, 2020
View reviewed changes
@yparitcher
Copy link
Copy Markdown
Member Author

yparitcher commented Mar 15, 2020 •
edited
Loading

I think something like Document:getCurrentPage() would probably make the most sense.

CreDocument already implements this.
the other Mupdf based ones don't, as the pages are managed in ReaderPaging by the frontend. what would be the best way to get the paging.current_page value in Document:?

@poire-z
Copy link
Copy Markdown
Contributor

poire-z commented Mar 15, 2020 

    if self.document.info.has_pages then
        curr_page = self.ui.paging.current_page
    else
        curr_page = self.document:getCurrentPage()
    end

what would be the best way to get the paging.current_page value in Document:?

If you're asking, may be there's none obvious :) and that's why it's the way it is.
May be forcing it to be a method of Document is not the most logical way (haven't checked), even if it looks like it would in the snippets you mentionned.

(I'd say: don't bother "fixing" that for such a little PR :)

@yparitcher
Copy link
Copy Markdown
Member Author

yparitcher commented Mar 15, 2020 •
edited
Loading

I think i managed it. Let me know if this looks good or i should abandon the whole mess

I made it a UI function.

poire-z
poire-z reviewed Mar 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@poire-z poire-z left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the way you did it, just using self.ui as the common parent.
(Don't forget to remove any currentpage stuff from ReaderGesture :)

poire-z
poire-z reviewed Mar 15, 2020
View reviewed changes
frontend/apps/reader/readerui.lua
self:showReader(new_file)
end

function ReaderUI:getCurrentPage()
Copy link
Copy Markdown
Contributor

@poire-z poire-z Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was about to suggest to put it as a more appropriate place (that at the end), with some other alike functions... but there are none :)
So, indeed, it's not the module where we expect such functions... so let it be there.
May be just add a comment -- little convenient helper
(Or move if into ReaderView, where it would be more at home?)

poire-z
poire-z reviewed Mar 15, 2020
View reviewed changes
Frenzie
Frenzie reviewed Mar 16, 2020
View reviewed changes
Frenzie
Frenzie reviewed Mar 16, 2020
View reviewed changes
Frenzie
Frenzie reviewed Mar 16, 2020
View reviewed changes
Frenzie
Frenzie approved these changes Mar 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poire-z Any remaining comments?

poire-z
poire-z approved these changes Mar 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Frenzie Frenzie merged commit 08359ee into koreader:master Mar 16, 2020
@yparitcher yparitcher deleted the bookmark_gesture branch March 16, 2020 15:52
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@yparitcher @mwoz123
[UX] Next/Previous Bookmark Gestures (koreader#5968)
d64f372 
Fixes koreader#5965
@poire-z poire-z mentioned this pull request May 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Frenzie Frenzie Frenzie approved these changes

@poire-z poire-z poire-z approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

2020.04

Development

Successfully merging this pull request may close these issues.

Gestures for jumping to next or previous bookmark?

3 participants

@yparitcher @poire-z @Frenzie