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

Various changes to sorting #4067

Merged
merged 2 commits into from Aug 7, 2018

Conversation

Projects
None yet
6 participants
@alethiophile
Contributor

alethiophile commented Jul 10, 2018

I'm using this for several changes I'd like to make to the file sorting code, some of which will require consultation.

  • When sorting by atime/"last read", don't force entries without a sidecar file to the end of the list.

    Doing so technically fits the "when last read" criterion, but it also has the effect of shuffling newly added files to the end. I think that in practice, a newly added file is equally of interest to the user as a just-opened one, and so both should be at the beginning. All that's required here is to remove the special case for sidecar files; when a file is added, its atime is set to the mtime, so the sort key works automatically. This is implemented in the commit present now.

  • Fix or remove the "date added" entry

    Currently, the "sort by date added" setting has the effect of sorting by ctime (change time). This is almost certainly incorrect; ctime is updated whenever the file data or metadata changes, and so in this instance should be basically equivalent to mtime (which is already present under the "date modified" header as a different setting). I can't think of a good use for a ctime sort setting. The alternative would be to implement a real sort by date added, which would require adding some external metadata (since there's no way to get creation time from the inode) and would in most cases be identical to "date modified" anyway. (The exception here is when books are changed/updated after they're added; however, in such cases it usually seems that an updated book should also be at the front of the list.)

    I'm somewhat inclined to just remove that setting rather than try to add nontrivial and questionably useful new functionality to make it work as described. The current status quo is a straightforward conflict between description and code; I think something definitely needs to change.

  • Make title actually sort by title

    Currently, "sort by title" actually sorts by the underlying filename, which is only occasionally equivalent to the title in the metadata, and isn't visible in the file manager (when using the cover-browser plugin) at all. It seems this should definitely sort by the metadata title, at least when cover-browser is active. However, the code that knows about the metadata browser display is all inside cover-browser; it may be this change should be implemented there, via some monkey-patch magic like it already uses, to keep the sort order and the displayed title from having a mismatch.

These issues are definitely open to discussion as to how they should be resolved; I'd appreciate any commentary on my proposed fixes.

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented Jul 10, 2018

for your last point, what about sortie by series / number (if added in title) ?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jul 10, 2018

Make title actually sort by title

Well, best to rename it sort by filename, as we want to keep that (it's been the default sort option, there are people that use Classic mode only and no coverbrowser, and people may use the filename with some number prefix to sort things as they like it).

And then add a sort by title if really needed and implemented (CoverBrowser could plug a sort method into FileChooser - but there's the thing that when entering a directory, not all files may yet be indexed - they are while you browse).

@alethiophile

This comment has been minimized.

Contributor

alethiophile commented Jul 11, 2018

Well, best to rename it sort by filename, as we want to keep that (it's been the default sort option, there are people that use Classic mode only and no coverbrowser, and people may use the filename with some number prefix to sort things as they like it).

This seems right, and trivially doable.

And then add a sort by title if really needed and implemented (CoverBrowser could plug a sort method into FileChooser - but there's the thing that when entering a directory, not all files may yet be indexed - they are while you browse).

Handling the metadata indexing may be a pain.

My instinct is to make it return filename if not yet indexed and then title once the indexing finishes, like the display handles it. But this has the issue where as the indexing finishes, the entries may jump around in the listing, rather than just changing display. Also, the display only needs to index the entries visible on the current page, whereas the sort function needs to know about every file in the directory.

Any comments on the added/ctime mode? Is this valuable enough to write the real functionality, or should it just be removed?

Rename 'sort by title' to 'sort by filename'
This is in accordance with the actual function of the code, which uses filename
rather than title as specified in file metadata.
@alethiophile

This comment has been minimized.

Contributor

alethiophile commented Jul 11, 2018

Did the title -> filename rename. They're localized strings, so this will have to interact with the l10n in some way, but I don't know enough about that subsystem to handle that now.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jul 11, 2018

It "interacts" automatically. ;-)

The strings are extracted by a Python script and uploaded to Transifex every night. So 48 hours after a PR is merged the first translations will be included in a nightly.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jul 11, 2018

Any comments on the added/ctime mode? Is this valuable enough to write the real functionality, or should it just be removed?

I don't sort by anything that filename very often, so I have no opinion. pinging @robert00s who added these.
One thing about sort by last read: if atime is not reliable, one other thing is: the creation time of the sidecar file (metadata.epub.lua): the old one is remaned too .old and a new one is created when it is saved, so when closing a document. I don't think the current code uses that fact.
(Or some c/m/time of the sidecar dir, which must be updated when we create a new file - or erase it with Purge .sdr)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented Jul 11, 2018

I think someone took care of atime recently by adding a touch, so at least for stuff opened by KOReader itself, we're good. Don't know when it's done though (on open or on close?).

@alethiophile

This comment has been minimized.

Contributor

alethiophile commented Jul 12, 2018

@NiLuJe: I wrote the patch with touching atime on open; it happens in the history code, at the same time the book is added to the history file. (Not actually sure when that's called, but I'd guess on open.)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jul 25, 2018

Currently, "sort by title" actually sorts by the underlying filename

You can definitely rename it sort by filename if you prefer, but I consider seemingly banal behaviors like this one of KOReader's biggest strengths. (It's been addressed since but I feel rather strongly about this point. My previous remark was a quickie from my phone so I stuck to the most pertinent thing back then.)

As far as I'm concerned, virtually all metadata is most likely useless junk. For example, this is the latest ebook I purchased in the mode I don't really use:

screenshot_2018-07-25_21-05-53

Actually the really surprising thing is that most metadata in my screenshot is actually not too bad…

Currently, the "sort by date added" setting has the effect of sorting by ctime (change time). This is almost certainly incorrect;

Note that a fairly important consideration here is also something like "how does FAT32/VFAT behave on Kobo/Kindle/etc." I'm mentioning it not because it changes anything — it probably doesn't even support ctime at all — but it's something that should probably be double checked before making any changes. :-)

I see no real need for ctime support myself, but of course it's one of those cases where if someone is willing to put in the effort, there's little to object to.

@alethiophile

This comment has been minimized.

Contributor

alethiophile commented Jul 26, 2018

As far as I'm concerned, virtually all metadata is most likely useless junk. For example, this is the latest ebook I purchased in the mode I don't really use:

I definitely agree the 'sort by filename' mode should remain. This patch is only renaming it.

I see no real need for ctime support myself, but of course it's one of those cases where if someone is willing to put in the effort, there's little to object to.

Two separate issues. One: I can't think of any situation where ctime is in any way different from mtime here. (Per Unix filesystem semantics, this would only be when the metadata of the ebook file has changed, but the data hasn't. This isn't a case I can ever imagine really coming up, particularly in this context where all files are root-owned and permissions are irrelevant. Which is leaving aside whatever it actually means on a FAT FS; I'll have to experiment with that.)

mtime is of course already one of the options, and should remain. But unless ctime actually has an application separate from it, there's no extra benefit to having it, and extra menu options aren't costless. On those grounds, it seems removing it is preferable.

The second issue is just that it's currently labeled as "creation time". This is just inaccurate; we don't actually have a creation time sort mode, and writing one would be mildly difficult and questionably worthwhile. So if the ctime code is to remain, it at least needs to be relabeled.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jul 26, 2018

For example, this is the latest ebook I purchased in the mode I don't really use

For people that use it, and get occasional crappy metadata (10% of my EPUB have such crap as your Untitled): long-press on the item > Ignore metadata and you'll get the filename instead.

I guess if there were to be some Sort by metadata methods, they'll be more hurt by crappy metadata that this simple display (think some book have as authors Firstname Lastname, others for that same author Lastname, Firstname)....
And if one is already taking care of editing and tidying metadata, he has already tidy'ed the filenames and put its prefered sort method in the naming. (OK, then, these Sort by metadata could be used as an occasional alternative when searching a specific book when Soft by filename is not enough - but we have Find a file - but adding these Sort by metadata is much work for only occasional use.)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented Jul 26, 2018

FWIW, that's, like, 137% of my Calibre workflow: cleaning up crap metadata and unifying everything ;D.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jul 26, 2018

FWIW, that's, like, 137% of my Calibre workflow: cleaning up crap metadata and unifying everything ;D.

And I don't want such a thing as a "calibre workflow". I just name my files sensibly at save time. :-P

Two separate issues. One: I can't think of any situation where ctime is in any way different from mtime here. (Per Unix filesystem semantics, this would only be when the metadata of the ebook file has changed, but the data hasn't. This isn't a case I can ever imagine really coming up, particularly in this context where all files are root-owned and permissions are irrelevant. Which is leaving aside whatever it actually means on a FAT FS; I'll have to experiment with that.)

You can set Windows files to be read-only or hidden, which is useful even without ownership. Whether FAT sets an equivalent to mtime or even whether mtime is terribly useful on a daily basis is another matter.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 6, 2018

@robert00s : any opinion on this?

@robert00s

This comment has been minimized.

Contributor

robert00s commented Aug 7, 2018

First: I agree we should rename sort by title to sort by filename. It's good idea.
Second: I don't use sort by date (added/modified) so I cannot comment that but remove slidecar checking from access looks ok.
Personally, I'm using sort by percentage.
I think we can add experimental ctime sort (as separate PR) and looks how it works.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 7, 2018

remove slidecar checking from access looks ok.

Thanks, that's what I wanted to hear :)
Merging, as it's already long and old, and to allow merging #4140 and removing all traces of "by title".
(I never sort, but I'm OK with removing "By date added"/ctime)

@poire-z poire-z merged commit 637abfd into koreader:master Aug 7, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment