Skip to content
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

Filemanger showing wrong thumbnails, if images are changed. #6922

Closed
zwim opened this issue Nov 29, 2020 · 17 comments
Closed

Filemanger showing wrong thumbnails, if images are changed. #6922

zwim opened this issue Nov 29, 2020 · 17 comments

Comments

@zwim
Copy link
Contributor

zwim commented Nov 29, 2020

Does your feature request involve difficulty completing a task? Please describe.
The filemanager shows thumbnails of images (jgp, png).
If the file is changed, filemanager doesn't recognize this change. It shows the cached image.

Describe the solution you'd like
Filemanager should check at least if the creation date and the filesize of the cached image match the actual file. If not, the cache should be updated.

@zwim zwim changed the title FR: [a handful of words describing the FR] Filemanger showing wrong thumbnails, if images are changed. Nov 29, 2020
@pazos
Copy link
Member

pazos commented Nov 29, 2020

The filemanager shows thumbnails of images (jgp, png).

It is part of the coverbrowser plugin

If the file is changed, filemanager doesn't recognize this change. It shows the cached image.

That's a feature, not a bug. The user is expected to update the cache if the files changed.

Filemanager should check at least if the creation date and the filesize of the cached image match the actual file. If not, the cache should be updated.

That sounds like a bug to me.

A image worth more than 1000 words :)

Sin nombre

Better to update the wiki: https://github.com/koreader/koreader/wiki/Cover-browser to explain that.

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

Maybe I expressed the problem not the right way.
Start KOReader -> list the book directory in filemanager (cover images enabled as thumbnails)
Close KOReader
Connect the device with USB, change some images (but the name stays the same)
Open KOReader -> list the book directory

Then you see the old thumbnail, even if the file has a new date and a different size :-(

@pazos
Copy link
Member

pazos commented Nov 29, 2020

Then you see the old thumbnail, even if the file has a new date and a different size :-(

That's still a feature for me. Instead of having to check all the files all the time, you use an option once (including subdirs) to regenerate chache and prunning old files.

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

Hmmm, filemanager or coverbrowser are smart enough to realize, that a file is deleted during usage of KOReader (and grays that out). So something IS scanning the files.

I am not asking to scan all the time, but on startup of KOReader/filemanager. It has to be checked that the cache is consistent with the disk. I am also not asking for time-consuming hashes or so. IMHO date and size would be enough.

@NiLuJe
Copy link
Member

NiLuJe commented Nov 29, 2020

What @pazos said ;).

A stat is expensive, and walking a directory even more so, especially on FAT. I/O is to be cached and avoided at all costs ;).

@NiLuJe
Copy link
Member

NiLuJe commented Nov 29, 2020

As far as graying stuff out, History != FM ;).

IIRC, the FM does the least amount of I/O necessary to basically show what's there, and much of the actual data is then pulled from the sidecar.

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

OK, FM does minimal I/O, but FM doesn't show cached thumbnails. That's up to coverbrowser.

I don't have a good feeling, if a software (coverbrowser) doesn't check, if its cached data is valid.
I am with you, if it comes to minimize the usage of resources. But checking the filesize and creation date of the content of one directory isn't that time-consuming. I'm not asking to validate the whole cache, but just the files showed in Filemanger (with coverbrowser thumbnails on).

If you say this as a feature, then it is a feature!

@Frenzie
Copy link
Member

Frenzie commented Nov 29, 2020

You wouldn't check all the files all the time, it'd be the exact same thing as when you add one or more new files in a folder.

@poire-z
Copy link
Contributor

poire-z commented Nov 29, 2020

I don't have a good feeling, if a software (coverbrowser) doesn't check, if its cached data is valid.

Come on :)
I'd say it's neither a feature not a bug, just a didn't'think'of.
I could have easily made it do that check, if I had thought of this use case:

Connect the device with USB, change some images (but the name stays the same)

But I didn't :) We're an ebook reader, and it's very rare that one would get 2 different books with the same name that you would place in the same directory over a certain period of time...

I think the only real use case (that you're not telling) is your CoverImage plugin :) that generates always the same filename with different content. Did you notice this "bug" because of it?

Anyway, it might not be much work to have CoverBrowser do that. But it would need a new DB schema (that would trash users' current DB, and no need for any migration, it's a trashable db) to add a filedate and a filesize columns... But is it really worth it ? (The IO might be minimal, we might already be doing the stat to get the filesize.)

The other solution for the rare person that has some rare images that might change content with a same name is just to select Ignore cover and have its cover just display its filename :)

(Anti-use case: PDFs where we write highlights into: they would change size and date: they would then be refreshed very often.)

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

I think the only real use case (that you're not telling) is your CoverImage plugin :) that generates always the same filename with different content. Did you notice this "bug" because of it?

@poire-z: You are right. I did not think to mention the CoverImage, because I wanted to show up a general problem (feature?) with the thumbnails.
I stumbled over that strange behavior, when playing with jpg support. It almost drove me crazy, that some images were not shown during development. I was looking into turbojpg, created images, some thumbnails where shown, others not. And then I realized is has something to do with caching.

An no, it was not a bug report, but a feature request :)

Anyway, it might not be much work to have CoverBrowser do that. But it would need a new DB schema (that would trash users' current DB, and no need for any migration, it's a trashable db) to add a filedate and a filesize columns... But is it really worth it ? (The IO might be minimal, we might already be doing the stat to get the filesize.)

If you ask that way, I'd say: Don't change the DB schema.

The other solution for the rare person that has some rare images that might change content with a same name is just to select Ignore cover and have its cover just display its filename :)

(Anti-use case: PDFs where we write highlights into: they would change size and date: they would then be refreshed very often.)

OK, thank you very much for your explanations.

@zwim zwim closed this as completed Nov 29, 2020
@Frenzie
Copy link
Member

Frenzie commented Nov 29, 2020

(The IO might be minimal, we might already be doing the stat to get the filesize.)

We definitely are.


If you ask that way, I'd say: Don't change the DB schema.

I think it's better to add date & size to the DB.

@NiLuJe
Copy link
Member

NiLuJe commented Nov 29, 2020

If we start tracking stat metadata, I'd propose that, while we're there, we ditch the partial md5 madness and switch to a generated UUID to id files ;).

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

Am I missing something o.o?

@NiLuJe
Copy link
Member

NiLuJe commented Nov 29, 2020

(No, just piggy-backing on an older discussion about UUIDs vs. md5 in the SQL db ;p).

@zwim
Copy link
Contributor Author

zwim commented Nov 29, 2020

OK, I forget it ( and don't warm up cold coffee) ;))

@Frenzie
Copy link
Member

Frenzie commented Nov 29, 2020

There's no UUID/hash/whatever in the cover DB.

@NiLuJe
Copy link
Member

NiLuJe commented Nov 29, 2020

That's a good point, wrong DB ;p.

This one appears to index on path, essentially.

So, I don't remember in which context we were discussing UUIDs vs. md5, then :D. Oh, well.

poire-z referenced this issue in NiLuJe/koreader Dec 8, 2020
... before I start breaking it to avoid the amount of useless copies
and memory allocs in there... :D
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants