Skip to content

Conversation

@hius07
Copy link
Member

@hius07 hius07 commented Jun 13, 2024

1

2


This change is Reviewable

@Frenzie Frenzie added this to the 2024.06 milestone Jun 13, 2024
Comment on lines +284 to +285
page = _("page number, reverse"),
date = _("date, reverse"),
Copy link
Member

Choose a reason for hiding this comment

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

Ascending/descending are often used, but I find reverse clearer. ;-)

@hius07
Copy link
Member Author

hius07 commented Jun 13, 2024

Excuse me, I am deliberately breaking the backward compatibility: new users and users without bookmarks_items_reverse_sorting in the settings will get non-reverse order.

Copy link
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.

Trusting you on the code.

@poire-z
Copy link
Contributor

poire-z commented Jun 14, 2024

Excuse me, I am deliberately breaking the backward compatibility: new users and users without bookmarks_items_reverse_sorting in the settings will get non-reverse order.

Why ? For technical reasons, or because you think it is better non-reverse ?
(Not often using bookmarks, but it feels better to have them in reverse with the newest at top - rather than in order and jumping on the last page, and finding them either at top, in the middle, at bottom. No strong opinion though.)

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

Not technical reasons.
(1) If bm are sorted by page number, the bm list is opened on the page containing the bookmark in the current book page (or the closest one). So if we read forward and make bookmarks - we always see the most recent bookmark on opening the bm list. (Just to remind: page numbers of bookmarks after the current page are dimmed). And it is convenient to see the page numbers ascending like in the ToC.
(2) If bm are sorted by date (new mode), the non-reverse ordering means the date descending, i.e. the first bookmark in the list is the most recent one. And in this mode bm list is opened in the first page:

self:updateBookmarkList(nil, self.sorting_mode == "page" and curr_page_index_filtered or 1)

Summarizing, I believe that non-reverse mode should be the default one, and the setting for it is saved as nil.

@poire-z
Copy link
Contributor

poire-z commented Jun 14, 2024

Fine with your explanations.

(2) If bm are sorted by date (new mode), the non-reverse ordering means the date descending, i.e. the first bookmark in the list is the most recent one. And in this mode bm list is opened in the first page:

This feels a bit odd. that the [ ] reverse would have quite opposite effects whether by page / by date. For someone that reads naturally from book start to end, I would expect these 2 options would behave quite the same.

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

File browser sort by date shows the most recent first.

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

@ryanwwest, what is the non-reverse order for sorting bookmarks by date: ascending or descending?
Ref. FR #11822

@ryanwwest
Copy link
Contributor

@hius07 I think descending would be first page of results top result has the highest number (most recent), then gets further away. Ascending would then be oldest entries first. Though not sure if I understood.

I notice that some bookmarks options are found in the Bookmarks list page in the hamburger menu item (filtering) while others are in Bookmark icon -> Settings -> Bookmarks. I was thinking that sort would make more sense to go with the Filter content since the typical online workflow with searching things is to have filter and sort options nested together. But I also see that 'Sort by largest page number' is already not with the filter items. I wonder if it would be better to move the sort options to the filter menu?

And at a bigger level, potentially unify all the Bookmarks page list settings in one place but that's probably for another time and debate.

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

So, "Sort by date" shows the oldest bookmark first in the list, and "Sort by date, reverse" shows the most recent bookmark first, right? (Like @poire-z says)
Do you want to see the bookmark date in the list? Where? I'd like to show the page number in any sorting mode. Adding the datetime to the page number would eat the room for bookmark text.

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

I'm also going to add searching by datetime in 2024-04-06 22:48:28 format (can be partial). Is it useful?

@ryanwwest
Copy link
Contributor

ryanwwest commented Jun 14, 2024

Thanks for this, @hius07 .

If you're asking my opinion (I haven't tested the PR), while I think most recent bookmarks first would be helpful, I get @poire-z 's point that maybe bookmark order should be default (which shouldn't say 'reversed') match earliest to latest like when you read a book left to right, earliest to latest. I think people can just change the order as long as it's easy for them to find sort setting.

As for date visibility in Bookmarks list, yes that's for (2) of #11822. Could it go under the page number on the right, so it's always visible and in the same place even with long note/highlight? I'm not sure how much space that needs.

I think that's the best time format. When you say search, is this search restricted to bookmarks page only or more general?

@ryanwwest
Copy link
Contributor

Maybe for timestamp location in bookmark list, if it's right-aligned below the page number the date could be on top line, time on bottom line, so it take up less horizontal space.

@hius07
Copy link
Member Author

hius07 commented Jun 14, 2024

Our right part (tab) of the list is single-line only, sorry.

@jonnyl2
Copy link
Contributor

jonnyl2 commented Jun 14, 2024

[hius07] If bm are sorted by date (new mode), the non-reverse ordering means the date descending, i.e. the first bookmark in the list is the most recent one. And in this mode bm list is opened in the first page:

[poire-z] This feels a bit odd. that the [ ] reverse would have quite opposite effects whether by page / by date. For someone that reads naturally from book start to end, I would expect these 2 options would behave quite the same.

I think there was a misunderstanding here? They do behave the same as @hius07 described: non-reverse order means the latest bookmark OR the highest page number comes first. And in my opinion that is the most desirable behavior as well.

To summarize: Non-reverse by date/time: 22:00; 21:33; 20:25... | by page: 800; 677; 533...
Reverse: 20:25; 21:33; 22:00 | 533; 677; 800

@hius07
Copy link
Member Author

hius07 commented Jun 15, 2024

Changes:
(1) consistent "page" and "date" sorting modes: non-reverse mode means page increasing or date from old to new.
(2) in "date" sorting mode append the datetime in the beginning of the item text.

@hius07 hius07 merged commit fb88e8d into koreader:master Jun 16, 2024
@hius07 hius07 deleted the bm-sort-by-date branch June 16, 2024 11:24
Commodore64user pushed a commit to Commodore64user/KOReader_fork that referenced this pull request Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants