ReaderBookmark: show current page #9872

hius07 · 2022-12-05T07:44:38Z

(1) Show book current page in the list of bookmarks, for easier navigation.
(2) Boookmark list is opened showing the page with the book current page.
(A lot of duplicated code in the module has been removed)

Opening the bookmark list

Reading forward, all bookmarks are behind

Filtered

Closes #8047.

This change is

Frenzie · 2022-12-05T08:14:07Z

frontend/apps/reader/modules/readerbookmark.lua

+    local curr_page_string = self:getBookmarkPageString(curr_page_num)
+    local curr_page_item = {
+        page = curr_page_num,
+        text = DISPLAY_PREFIX["curr_page"] .. _("Book current page"),


Just current page should do the trick.

Suggested change

text = DISPLAY_PREFIX["curr_page"] .. _("Book current page"),

text = DISPLAY_PREFIX["curr_page"] .. _("Current page"),

We have a button to jump to the bookmark list page containing the book curent page.
The same string is used.

Is there a particular reason that one doesn't just say "current page"? :-)

Oh, yeah, that's extremely clunky wording (in both cases ;p).

i.e., I'd go with something like "On the current page" for the popup, and a plain "Current page" for the new stuff.

poire-z · 2022-12-05T10:03:27Z

frontend/apps/reader/modules/readerbookmark.lua

-    else
-        self.ui:handleEvent(Event:new("SetDogearVisibility", false))
-    end
+    local is_index = self:getDogearBookmarkIndex(pn_or_xp) and true or false


is_page_bookmarked ?

poire-z · 2022-12-05T10:07:22Z

frontend/apps/reader/modules/readerbookmark.lua

-                self.view.footer:onUpdateFooter(self.view.footer_visible)
-            end
-            return
+    local index = self:getBookmarkIndexBinary(item) or self:getBookmarkIndex(item)


The naming of the first method is a bit bad - we're not getting a bookmark index "binary" :)
getBookmarkIndexUsingBinarySearch(item) or self:getBookmarkIndexUsingFullScan(item)
or
getFastBookmarkIndex(item) or self:getFallbackBookmarkIndex(item) (so-so)
or something better than that :)

Btw, you told me before, but why do we need these 2 ways of getting a bookmark index?

why do we need these 2 ways of getting a bookmark index?

Non-binary search is used when changing highlight boundaries (pos0 and pos1).
We can avoid this by passing the old highlight to find the corresponding bookmark.

And as a fallback

koreader/frontend/apps/reader/modules/readerbookmark.lua

Lines 981 to 984 in 5696a7c

-- If we haven't found item, it may be because there are multiple

-- bookmarks on the same page, and the above binary search decided to

-- not search on one side of one it found on page, where item could be.

-- Fallback to do a full scan.

I did not investigate if there were any fails of binary search.

poire-z · 2022-12-05T11:18:57Z

A bit too much changes to review carefully, so trusting you on the refactoring.

But about the feature, adding a normal-size slot among bookmarks for

showing where the current page is, or how bookmarks relate to the current page
allowing jumping to current page ? does that happen, or is it just an inactive slot ?

I don't find it particulary pretty, especially on your screenshots with tall bookmark slots (may look a bit less ugly on my small one-line bookmarks):

it's a full slot, so having 3 bookmarks, I'll see 4 slots
it's a slot with small content, while other highlights around may have lots of text - not really pleasing, and jumping out while it's not the important stuff
the star icon is not a good choice: we use it to mark defaults, or to navigate bookmarks in the skim widget - here, it as no such powerful meaning

I guess it was the easier way to implement that, without touching to Menu, as it's just a regular slot.
But well, I'm not sure we'd want to change it that much because of one user feature request :/

Some other ideas, just for discussions, probably less easy to implement.

If they are inactive slots, don't add a slot, and just make the border separator larger ? (or show the separator when the separator setting is not enabled)
If keeping a full slot, have it styled very differently? dimed, or gray background, and/or centered/indented/italic text
wasn't the feature request to just open on the bookmark list's page with the current page ? Don't we already do that ? If not, we could just do that :)

hius07 · 2022-12-05T12:02:35Z

Current behaviour of the bookmark list:
-the bookmark list is opened at the first page
-bookmarks located in the book current page are in bold
-if there are no bookmarks in the book current page nothing is in bold
-there is a button in the popup dialog window "Current book page": on pressing it we jump to the page of the bookmark list with bookmarks located in the book current page. The button is disabled when there are no bookmarks in the book current page.

Reasons of changes:
-opening the bookmark list at the first page is useful if bookmarks are sorted by time - we can see the latest bookmark. We do not have such type of sorting (yet).
-so opening the bookmarks list at the page containing the bookmarks of the book current page looks more logical and useful.

What if there are no bookmarks in the book current page? An anchor (Current page item) explains where we are and why we see this page of the bookmark list.

I find this item rather useful, helping to navigate through the bookmarks and pages.
It is in bold because all bookmarks of the book current page are in bold.
Yes, the star sign is for default values. What is the "default value" for a book page? Current page.

The "Book current page" button in the popup dialog is always enabled now (as we always have an item in the book current page).
On pressing the "Current page" item we just go the book current page.

poire-z · 2022-12-05T12:26:20Z

Reasons of changes:
-opening the bookmark list at the first page is useful if bookmarks are sorted by time - we can see the latest bookmark. We do not have such type of sorting (yet).
-so opening the bookmarks list at the page containing the bookmarks of the book current page looks more logical and useful.

What if there are no bookmarks in the book current page? An anchor (Current page item) explains where we are and why we see this page of the bookmark list.
I find this item rather useful, helping to navigate through the bookmarks and pages.

I agree with all these "reasons for change". Nothing against that.
It's just the aesthetics that I "disagree" with - meaning I think we can do prettier.

And I agree that if sorted by page number, we should open on the current page. Reaching the start or end is just a matter of hitting the << or >> bottom chevrons buttons.

It is in bold because all bookmarks of the book current page are in bold.
Yes, the star sign is for default values. What is the "default value" for a book page? Current page.
The "Book current page" button in the popup dialog is always enabled now (as we always have an item in the book current page).

These are technicalities, because of the simple way you went at it, adding a normal slot and not touching Menu.
By themselves, I find these choices not really defendable :)

On pressing the "Current page" item we just go the book current page.

I think you're talking about the button in the popup dialog.
If you were saying that hitting the slot "Book current page", leaving bookmarks to show the current book page - well, that's the same as just exiting bookmarks :) So, this feature would not be needed.

Anyway, I think a border separator would work just as well as a cue about where your bookmarks are situated vs. the current page - keeping the bookmarks on the current page in bold.

opening the bookmark list at the first page is useful if bookmarks are sorted by time

I guess that when/if bookmarks are not sorted by page order (or reverse page order), there is no current page slot/border to show, as they may be many such "current" page in this unordered list :)

poire-z · 2022-12-05T13:00:42Z

I guess it was the easier way to implement that, without touching to Menu, as it's just a regular slot.
But well, I'm not sure we'd want to change it that much because of one user feature request :/

By it, I meant changing "the visual aspect of the bookmark list".
I'd be ok if we would add to Menu some generic callbacks, like emphasis_item_top_border_func, emphasis_item_bottom_border_func, open_on_first_item_matching_func...

hius07 · 2022-12-05T15:38:38Z

Sorry, I do not like the idea.
Mysterious thick separator (What's this? asks a user) insted of a self-explainatory consisten entry, additionally showing the current page number.

poire-z · 2022-12-05T16:03:07Z

Do I have 6 or 7 bookmarks ? There is one thing that looks like a bookmark (ie, as big, as bold as its possible neighbours), that is under the "Bookmarks" heading - that is not a bookmark.

Sorry, I do not like the idea. Mysterious thick separator (What's this? asks a user) insted of a self-explainatory consisten entry, additionally showing the current page number.

The user can ask many things (why these bookmarks are bold ?) and discover things himself (because they are on the current page).
(We are usually both on the "discoverability" side, rather than on the big-hint-in-your-face" side :)

If you really don't want a thick separator, I think you need to make that not-a-real-bookmark-slot really different (and as a part of that difference: if real bookmarks have a leading icon, this one should not have any).

poire-z · 2022-12-05T16:11:45Z

( Quick hardcoded hack to tweak linesize & line_color in Menu, just to show what I had in mind:
)

hius07 · 2022-12-05T16:33:59Z

Maybe dots?

poire-z · 2022-12-05T17:32:52Z

Maybe dots?

That makes it indeed a bit more different :) May be with some indentation (or blank icon), or centered with dots on both sides. And/or dimmed (if dimming is not used with bookmarks.)

But it still feels like a dev hack (which it is :)
Depending on how people use bookmarks, they would probably not need to see it. I would not (I mostly use bookmarks to temporarily highlight stuff I'll later look up on a computer - so I open bookmarks every other day to see if there is stuff, and I usually expect it to be empty. Now, I'll expect it to have this useless item :)
I also feel most people will have that "current page" be some noise, at the top or bottom of the bookmark list, as what they bookmarked is in the past content.
And when you browse bookmarks, seeing the bookmark list and taping to go to a page, when back, you should see your last jumped bookmark in bold (except may be if it is multipage, or if you read some pages around), so it's also not really needed.

It could be a setting, but well, I prefer solutions that don't need a setting and that work for most cases. And I guess a thickborder could be that solution: it would always be there, either at the top or at the bottom, and would be less bothering I think.

NiLuJe · 2022-12-05T18:04:27Z

I'd tend to be on the "minimalism" side of the argument, FWIW.

But the "keep it empty if it's empty" argument is easy to deal with: just, well, keep the widget empty if there's no bookmarks ;o).

NiLuJe · 2022-12-05T18:05:12Z

Random idea: dim the page numbers for stuff that is behind (e.g., lower than) the current page number?

That should only require minimal Menu trickery, and sort of achieves the same goal?

poire-z · 2022-12-06T07:28:31Z

Random idea: dim the page numbers for stuff that is behind (e.g., lower than) the current page number?

Feels like it would be enough (unless I failed to understand how important it is to see the current page among bookmarks) and the less intrusive - even more if we instead dim page numbers that are after, as when reading a book, the bookmarks you have made would be behind, so if these are kept non-dim, it's a no visible change for most people.

NiLuJe · 2022-12-06T18:20:36Z

(Keep in mind that I'm a very very light bookmark user, so I'm just throwing wild ideas at the wall here ;)).

hius07 · 2022-12-07T07:38:04Z

mandatory dim

poire-z · 2022-12-07T08:54:32Z

This looks ok to me, unintrusive enough, and giving the info (that you were actually on page 679 :)

poire-z

No other comment about the code.
Still the comments I made previously about some naming:

is_index => is_page_bookmarked (unless is_index somehow talks to you better)
getBookmarkIndexBinary() should really not end with just "Binary" - better with: "...BinarySearch", or "...WithBinarySearch" or "...UsingBinarySearch"

poire-z · 2022-12-08T16:04:38Z

Are you fine with the final result ? Does it answer enough #8047 ?
Or do you miss that additional "Current page" slot ? Did you end up sharing my concerns about it?

hius07 · 2022-12-08T22:44:28Z

I'm okay with the dimmed page number.

hius07 added 3 commits December 5, 2022 09:35

Update readerhighlight.lua

4ee7eac

readerbookmark: show current page

5e62128

Update readerbookmark.lua

5d9f953

Frenzie reviewed Dec 5, 2022

View reviewed changes

poire-z reviewed Dec 5, 2022

View reviewed changes

hius07 added 2 commits December 8, 2022 11:19

menu: mandatory dim

134dcbb

readerbookmark: mandatory dim

cc3ff12

poire-z reviewed Dec 8, 2022

View reviewed changes

getBookmarkIndexBinarySearch, getBookmarkIndexFullScan and others

580fbb0

Update readerbookmark.lua

8c60319

poire-z approved these changes Dec 8, 2022

View reviewed changes

Frenzie added this to the 2022.12 milestone Dec 9, 2022

Frenzie added the UX label Dec 9, 2022

Frenzie merged commit 05cd59e into koreader:master Dec 9, 2022

hius07 deleted the bookmarks-show-current-page branch December 9, 2022 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReaderBookmark: show current page #9872

ReaderBookmark: show current page #9872

hius07 commented Dec 5, 2022 •

edited by Frenzie

Frenzie Dec 5, 2022

hius07 Dec 5, 2022 •

edited

Frenzie Dec 5, 2022

NiLuJe Dec 5, 2022

NiLuJe Dec 5, 2022

poire-z Dec 5, 2022

poire-z Dec 5, 2022

hius07 Dec 5, 2022

poire-z commented Dec 5, 2022

hius07 commented Dec 5, 2022 •

edited

poire-z commented Dec 5, 2022

poire-z commented Dec 5, 2022 •

edited

hius07 commented Dec 5, 2022

poire-z commented Dec 5, 2022

poire-z commented Dec 5, 2022

hius07 commented Dec 5, 2022

poire-z commented Dec 5, 2022

NiLuJe commented Dec 5, 2022 •

edited

NiLuJe commented Dec 5, 2022 •

edited

poire-z commented Dec 6, 2022

NiLuJe commented Dec 6, 2022

hius07 commented Dec 7, 2022

poire-z commented Dec 7, 2022

poire-z left a comment

poire-z commented Dec 8, 2022

hius07 commented Dec 8, 2022

	text = DISPLAY_PREFIX["curr_page"] .. _("Book current page"),
	text = DISPLAY_PREFIX["curr_page"] .. _("Current page"),

	-- If we haven't found item, it may be because there are multiple
	-- bookmarks on the same page, and the above binary search decided to
	-- not search on one side of one it found on page, where item could be.
	-- Fallback to do a full scan.

ReaderBookmark: show current page #9872

ReaderBookmark: show current page #9872

Conversation

hius07 commented Dec 5, 2022 • edited by Frenzie

Frenzie Dec 5, 2022

Choose a reason for hiding this comment

hius07 Dec 5, 2022 • edited

Choose a reason for hiding this comment

Frenzie Dec 5, 2022

Choose a reason for hiding this comment

NiLuJe Dec 5, 2022

Choose a reason for hiding this comment

NiLuJe Dec 5, 2022

Choose a reason for hiding this comment

poire-z Dec 5, 2022

Choose a reason for hiding this comment

poire-z Dec 5, 2022

Choose a reason for hiding this comment

hius07 Dec 5, 2022

Choose a reason for hiding this comment

poire-z commented Dec 5, 2022

hius07 commented Dec 5, 2022 • edited

poire-z commented Dec 5, 2022

poire-z commented Dec 5, 2022 • edited

hius07 commented Dec 5, 2022

poire-z commented Dec 5, 2022

poire-z commented Dec 5, 2022

hius07 commented Dec 5, 2022

poire-z commented Dec 5, 2022

NiLuJe commented Dec 5, 2022 • edited

NiLuJe commented Dec 5, 2022 • edited

poire-z commented Dec 6, 2022

NiLuJe commented Dec 6, 2022

hius07 commented Dec 7, 2022

poire-z commented Dec 7, 2022

poire-z left a comment

Choose a reason for hiding this comment

poire-z commented Dec 8, 2022

hius07 commented Dec 8, 2022

hius07 commented Dec 5, 2022 •

edited by Frenzie

hius07 Dec 5, 2022 •

edited

hius07 commented Dec 5, 2022 •

edited

poire-z commented Dec 5, 2022 •

edited

NiLuJe commented Dec 5, 2022 •

edited

NiLuJe commented Dec 5, 2022 •

edited