Custom book covers

[hius07]

Support for user's custom book covers.
Cover file cover.ext is located in the book sdr folder.
Can be added/removed manually or via the Book Information page.

No support for downloading covers from the web so not closing #3677.

---

This change is 

[poire-z]

(In #10311 (comment), I missed not having a reference to the candidate used - so I would not mind if it were added here, something like new.candidate_used = candidate_path or with a better name, candidate_read, candidate_source, new.source_file...?)

[poire-z]

Also, not really fond of having, for each DocSettings read, 7 additional checks/stat() for a file existence for each of the cover extension.
How many DocSettings can be read in our usual operations ? With CoverBrowser, I guess 12 will be read if there are 12 items per page - which would each currently do 3 stat() for the 3 possible DocSettings locations - so now each page would requre 10x12 stat() instead of 3x12.
Dunno how bothering this may be in the grand scheme of things. @NiLuJe ?

[NiLuJe]

It's... certainly not ideal. (Not terrible, but, not ideal).

I'd, at the very least, limit it to png/jpg, very, very eventually svg.

And, ideally, make it entirely opt-in so nobody actually has to suffer though the costs, as it's an extremely niche feature, and we shouldn't suffer for its sake ;).

[poire-z]

I'm not really a customer for that feature :), so my comments:

• not really happy with the many syscalls/stat() this will trigger
• will this work with scripts (.sh, .py) ? I don't use any, but that's the only reasonable usage I may see, associate proper icons to script files for easier quicker tap on them
• no issue with the thing you wondered about having a sidecar dir for the cover, but no docsetting: will the book be considered as opened or not?

My main (my) usability issue I immediately noticed when seeing the screenshots are the "Cover image: tap to display", that I use quite often, being too near the new (totally useless to me) "Custom cover image:..." and the risk to tap on it (and the more useless items you add to that Book information KVP, the more items-per-page I'll have to increase, the smaller the font size and item will be, the more risk of wrong tap :/).

Dunno if there would be a better UI, not bothering Book information display with Book information editing.
A top left menu popup icon ?
Long-press on Cover image: to access this custom cover image stuff ? but then, it's not really advertised (which would be fine by me :) and people won't know about it.
But if long-press on Cover image:, I would expect long-press on Title: to also allow me to update the title and other metadata (but it then gets more complicated as we would need to save this updated title into a docsetting, marking the document as opened). Or they would need to be saved in an independant sidecar_dir/metadata.custom alongside a sidecar_dir/cover.jpg.

[NiLuJe]

FWIW, I'd KISS and scrap the "set custom cover" UI bits altogether ;).

[Frenzie]

If this is to keep it limited, it's probably better to keep it truly limited to png, jpg and svg.

Otherwise, see:

koreader/frontend/document/pdfdocument.lua

Lines 384 to 404 in af45ec7

	--- Picture types ---
	registry:addProvider("gif", "image/gif", self, 90)
	-- MS HD Photo == JPEG XR
	registry:addProvider("hdp", "image/vnd.ms-photo", self, 90)
	registry:addProvider("j2k", "image/jp2", self, 90)
	registry:addProvider("jp2", "image/jp2", self, 90)
	registry:addProvider("jpeg", "image/jpeg", self, 90)
	registry:addProvider("jpg", "image/jpeg", self, 90)
	-- JPEG XR
	registry:addProvider("jxr", "image/jxr", self, 90)
	registry:addProvider("pam", "image/x-portable-arbitrarymap", self, 90)
	registry:addProvider("pbm", "image/x‑portable‑bitmap", self, 90)
	registry:addProvider("pgm", "image/x‑portable‑bitmap", self, 90)
	registry:addProvider("png", "image/png", self, 90)
	registry:addProvider("pnm", "image/x‑portable‑bitmap", self, 90)
	registry:addProvider("ppm", "image/x‑portable‑bitmap", self, 90)
	registry:addProvider("svg", "image/svg+xml", self, 80)
	registry:addProvider("tif", "image/tiff", self, 90)
	registry:addProvider("tiff", "image/tiff", self, 90)
	-- Windows Media Photo == JPEG XR
	registry:addProvider("wdp", "image/vnd.ms-photo", self, 90)

[hius07]

(1) UI
I think that UI for adding custom covers is needed.
At first, it is not an easy task to manually create the path to sdr folder for not-yet-opened books.
Secondly, a user would be able to make a screenshot of a page or a picture in the book and use it as a custom cover without connecting to the PC.

(2) Too many syscalls.
Actually the cover file extension doesn't matter (except svg).
So we can choose any png/jpg/gif/tiff file and rename it to cover.png when setting the custom cover.
2 syscalls instead of 7.

[Frenzie]

Agreed on the screenshot/page thing.

So we can choose any png/jpg/gif/tiff file and rename it to cover.png when setting the custom cover.

Both as a user and as a maintainer that's a bit dissatisfying. If we take the approach that the extension is ignored it should just be cover with no deception. :-)

[poire-z]

Both as a user and as a maintainer that's a bit dissatisfying. If we take the approach that the extension is ignored it should just be cover with no deception. :-)

I would not mind the deception :) It should at least be the format in which we would save any screenshot, so probable cover.png. Users could then rename any ISBN21321312.jpg to cover.png, if we are able to load it and auto detect/handle it as a JPEG.
It should have an extension if we want to be able to click on to watch it when inspecting folders over USB - or for the PC to show its thumbnail in Explorer.
I also think we don't need to handle SVG, if it saves one syscall.

[hius07]

It should have an extension if we want to be able to click on to watch it when inspecting folders over USB - or for the PC to show its thumbnail in Explorer.

Image viewers that I know (in Windows) shows image files with incorrect extensions well, just showing a warning about the extension.

[hius07]

if we are able to load it and auto detect/handle it as a JPEG.

For all non-svg files

koreader/frontend/ui/widget/imagewidget.lua

Line 183 in af45ec7

self._bb = RenderImage:renderImageFile(self.file, false, width, height)

and then the header is checked

koreader/frontend/ui/renderimage.lua

Lines 51 to 54 in af45ec7

	-- Guess if it is a GIF or a WebP image: the dedicated methods are able to handle
	-- animated GIF or WebP images, which MuPDF don't handle.
	local buffer = ffi.cast("unsigned char*", data)
	local header = ffi.string(buffer, math.min(4, size))

and finally

koreader/frontend/ui/renderimage.lua

Line 78 in af45ec7

return self:renderImageDataWithMupdf(data, size, width, height)

[Frenzie]

I also think we don't need to handle SVG, if it saves one syscall.

Wouldn't the approach I sketched be much more efficient? Just get the list of files in sidecar, go through the list without doing anything until you hit the first cover.supported-by-imageviewer-or-mupdf, the end.

[poire-z]

lfs.dir() would also give files in some random order, which would be even more non-deterministic and different from one sdr directories to another.
(And I guess it complicates a lot of other DocSettings related things if we were to use it to also pick metadata.xyz.lua.)

[Frenzie]

The order can be whatever you want, my point is you don't need to do lfs.attributes on everything one by one.

(And I guess it complicates a lot of other DocSettings related things if we were to use it to also pick metadata.xyz.lua.)

I've always thought that'd be better the way I sketched too tbh. ;-)

[NiLuJe]

Do you propose to disable using that icons as custom covers?

Well, yeah. I can't see any reason why anybody would want to anyway?

[poire-z]

Do you propose to disable using that icons as custom covers?

Well, yeah. I can't see any reason why anybody would want to anyway?

Because our icons are nice, and one might want to reuse them to associate a cover to ie. some shell script ?
Also, if we support SVG, we may get SVG from elsewhere that are like our icons.
Doesn't providing alpha=false/true help ?
And what happens with transparent GIF/PNG ?

[Frenzie]

Yeah, I'm rather of the opposite opinion, that the icons are the prime candidate for covers in an exclusively on-device situation. ;-)

[hius07]

Doesn't providing alpha=false/true help ?

No it doesn't

koreader/frontend/ui/widget/imagewidget.lua

Lines 197 to 201 in 60849ae

	-- Now, if that was *also* one of our icons, we haven't explicitly requested to keep the alpha channel intact,
	-- and it actually has an alpha channel, compose it against a background-colored BB now, and cache *that*.
	-- This helps us avoid repeating alpha-blending steps down the line,
	-- and also ensures icon highlights/unhighlights behave sensibly.
	if self.is_icon and not self.alpha then

[NiLuJe]

Then the whole caching aspect of is_icon needs to be rethought in there ;).

(Or, better, just a way to pass is_alpha, as that's sorta the only thing that matters here?)

[NiLuJe]

What I meant to say in my previous answer (had to run, so it was a bit on the short side ;p) is that, currently, ImageWidget's handling of SVG is built exclusively for use with our icons.

It does a lot of weird shit that is is very very specific to that use-case, and should absolutely not be used for anything else, ever.

So, basically, we'd need a whole new way of handling SVGs in ImageViewer, with sensible alpha behavior, is what I meant ;).

(So either do that, or drop the SVG support until that's doable).

[NiLuJe]

Okay, re-reading ImageWidget, it's slightly less problematic than I initially remembered, still, not a fan of it, and you shouldn't need to.

Without that the KOReader mdlight icons are shown as a black box (they require alpha = true).

That would imply something is compositing against a black background somewhere? ImageWidget should get you a BB with an alpha channel if alpha is true.
Now, writing this, I remember saying elsewhere than we do not actually support covers with an alpha channel, so that somewhere might very well be fmbookinfo or the FM or whatever's drawing the covers (most likely because black happens to be the default bb color because of zero-initialization) ;p.

Doesn't providing alpha=false/true help ?

No it doesn't

(The excerpt linked after that is not entirely relevant, as it's there to allow us to actually keep the alpha channel of the icon in the very few cases we need it so as to be able to do alpha-blending at display time; otherwise, we composite over a white background and cache that composition).

TL;DR: I'm still not a fan of this, and it at the very least would require a comment to explain that this is because something something we don't support covers with an alpha channel, so we fudge it by compositing against a white background via this flag.

That does leave an actual branch that might be problematic in practice, though:

koreader/frontend/ui/widget/imagewidget.lua

Lines 574 to 584 in 60849ae

	-- In night mode, invert all rendered images, so the original is
	-- displayed when the whole screen is inverted by night mode.
	-- Except for our *black & white* icons: we do *NOT* want to invert them again:
	-- they should match the UI's text/backgound.
	--- @note: As for *color* icons, we really *ought* to invert them here,
	--- but we currently don't, as we don't really trickle down
	--- a way to discriminate them from the B&W ones.
	--- Currently, this is *only* the KOReader icon in Help, AFAIK.
	if Screen.night_mode and not self.is_icon then
	bb:invertRect(x, y, size.w, size.h)
	end

[hius07]

Another way is to get cover image as usual, i.e. to replace

koreader/frontend/apps/filemanager/filemanagerbookinfo.lua

Lines 292 to 302 in e38bb05

	if custom_cover then
	local ImageWidget = require("ui/widget/imagewidget")
	local img_widget = ImageWidget:new{
	file = custom_cover,
	file_do_cache = false,
	is_icon = util.getFileNameSuffix(custom_cover) == "svg",
	}
	cover_bb = img_widget:getImageCopy()
	img_widget:free()
	return cover_bb
	end

with

if custom_cover then
    local cover_doc = DocumentRegistry:openDocument(custom_cover)
    if cover_doc then
        cover_bb = cover_doc:getCoverPageImage()
        cover_doc:close()
        return cover_bb
    end
end

[Frenzie]

I was surprised not to see it that way in the first place. ^_^

[poire-z]

DocumentRegistry:openDocument(custom_cover)

I was not keen on using the possibly more expensive openDocument - as it would load crengine when you have to handle a SVG.
But if such SVGs have some texts or are complicated, crengine/LunaSVG may do a better job at rendering it than our ImageWidget/nanosvg.
So, well, if you want good support, you have to use DocumentRegistry:openDocument(custom_cover).

[hius07]

Updates based on the reviews.
Open custom cover as a document to get custom cover image.

[NiLuJe]

Looks okay at a (very) quick glance ;).

Reviewed 4 of 4 files at r20, all commit messages.
Reviewable status: all files reviewed, 36 unresolved discussions (waiting on @hius07 and @poire-z)

[poire-z]

os.rename! Unless crossing FS boundaries is actually a concern?

I think it might be a concern (if the books are on another partition than the one koreader is in, or moving books across partitions) ?

So, really not a concern ?
copyFile + os.remove feels safer.

[hius07]

A (future) design question.
Sometimes it is convenient to have a checkbox in the ConfirmBox (eg "Don't ask again").
The idea is to have addWidget() in ConfirmBox similar to that in InputDialog.
I am not sure if it looks nice (with the left icon):

[poire-z]

As long as it's not for "Don't ask again", fine with me :) (#9786 (comment), #9786 (comment)).
May be it can look better with just no separator ? Just a blank line in the text, or some vertical padding before the checkbox ?

[poire-z]

Not yet merged because of the conflicts? - or else?

[poire-z]

Not strictly related, but just witnessed that when looking at this feature:
In file picker (or pathchoose as it must be named), "Long-press to choose a file", the top left "home" icon is quite small, ~ twice smaller than the one we get in File browser.
Not using file picker often, so no idea since when it has become small - and if it is voluntary or not.

[hius07]

no idea since when it has become small

From its birth (#8646, default TitleBar size).

[poire-z]

Ok :) really not using that widget often :/ (Although I must have seen it when picking a file in TextEditor.)
But ok, I guess it might be a visual hint that what you see with that small icon is not your regular File browser :)