CoverBrowser: adjustable mosaic grid #11232

hius07 · 2023-12-16T09:12:52Z

This change is

Frenzie · 2023-12-16T11:50:17Z

plugins/coverbrowser.koplugin/main.lua

+                text_func = function()
+                    local nb_cols_portrait = tonumber(BookInfoManager:getSetting("nb_cols_portrait") or 3)
+                    local nb_rows_portrait = tonumber(BookInfoManager:getSetting("nb_rows_portrait") or 3)
+                    return T(_("Items per page in mosaic portrait mode: %1 x %2"), nb_cols_portrait, nb_rows_portrait)


Possibly × instead of plain x?

Frenzie

lgtm, @poire-z okay with the new settings?

poire-z · 2023-12-16T12:35:36Z

frontend/apps/filemanager/filemanagermenu.lua

-                                default_value = default_perpage,
+                        callback = function(touchmenu_instance)
+                            local current_value = FileChooser.perpage
+                            local default_value = FileChooser.items_per_page_default


(Wondered if you forgot to add changes to filechooser - but I see items_per_page_default is defined in Menu, which FileChooser is a subclass, so it will be found. Just saying just in case.)

poire-z · 2023-12-16T12:37:09Z

plugins/coverbrowser.koplugin/main.lua

-    table.insert (menu_items.filebrowser_settings.sub_item_table, 4, {
+    table.insert (menu_items.filebrowser_settings.sub_item_table, 5, {


Screenshot of the upper menu where we see this reordering please?

The order was wrong after adding "Show finished books".

poire-z · 2023-12-16T12:40:54Z

plugins/coverbrowser.koplugin/main.lua

+                        left_text = _("Columns"),
+                        left_value = nb_cols_portrait,
+                        left_min = 2,
+                        left_max = 5,


I argued against going up to 30 :) but may be 5 is too little ? I guess one could be happy with 8 or 10 on large devices ? There's still some limit with the font size and reability of text covers - I don't know what limit it would bring.

Waiting for @mergen3107 with big screen limits.

I guess it's mostly a matter of taste in the end, but I probably wouldn't go higher than 6 or 8?

On larger screens (monitors/TVs) it could make sense, but on the flip side when more than that makes sense you might also want 4 pages at once or something.

poire-z · 2023-12-16T12:53:14Z

There's possibly still the issue I mentionned at #11186 (comment):

Plus, if you think it's needed, the need to keep track of the geometry in the "M" tags, so thumbnails get refreshed to the final size when you tweak Cols and Rows. (I don't think I cared about the fact that the size change when one switch from portrait to landscape.)

All the things involving the variables cover_sizetag and cover_specs - which is quite clumsy as it was a bit painful to implement for Detailed list mode so covers are refreshed when we change items per page.
And the fact that I didn't really care about toggling portrait <> landscape which would also change it for Detailed list mode. And it feels it's more important with Mosaic where thumbnails are bigger, so any upscaling of cached smaller thumbnail would get the lack of image quality more easily noticed.

We could let that to the user so he has to "Refresh cached book info & refresh all", but I somehow like that things happen as needed as you browse.

hius07 · 2023-12-16T15:39:20Z

Re-extracting covers on every change of the grid dimensions takes time and resources.
As idea, in Cover mode we can always save to db "M" thumbnail the image scaled to the portrait 3x3 grid size (not depending on actual grid dimensions). And this cached image is up/down scaled to the actual grid.
Upscale to max size (in 2x2 grid) would not be visible.

Frenzie · 2023-12-16T15:42:39Z

I don't know, does it make sense to optimize for the edge case when you'll probably only change the setting a few times if ever?

poire-z · 2023-12-16T15:59:30Z

As idea, in Cover mode we can always save to db "M" thumbnail the image scaled to the portrait 3x3 grid size (not depending on actual grid dimensions).

I don't think we have the real size it would take with 3x3, when we are in 5x4.
Also, I wouldn't like to have non-adjusted thumbnails (whether they end up upscaled or downscaled), when I am using 5x4.

Upscale to max size (in 2x2 grid) would not be visible.

We also don't need larger than needed thumbnails in the db (it takes space, and cpu decoding), and cpu up/down-scaling that could slow down the display (maybe not noticable when 3x3, but probably more if 8x8).

Re-extracting covers on every change of the grid dimensions takes time and resources.

That's what we do in Detailed list.
Also, we indeed don't expect users to change this that much - and when they are playing with it, they can wait a bit and get perfect results (instead of having ugly stuff and thinking: ok, 8x8 is ugly, or damned, 3x3 was nicer before, how did I get there).

May be a ConfirmBox when we leave the DoubleSpinWidget, asking the user if he wants to get thumbnails refreshed for the new size ? May be we would not need to change all the sizetag related code (until we get a better idea on how to handle this), if we were just marking such covers as "dirty" with something like:
update bookinfo set cover_sizetag='x' where cover_sizetag='M' ?

mergen3107 · 2023-12-16T17:21:13Z

Thank you @hius07 for this new settings!
I tested by patching the diff.

Can you please allow max limit for landscpe rows to 5?
I can set my preferred 5x4 in portrait, but I can't set 4x5 in landscape.

Thanks!

mergen3107 · 2023-12-16T17:26:20Z

Waiting for @mergen3107 with big screen limits.

I can't find these settings in the settings.reader.lua, do you think we should put them there?

Meanwhile, let me try some extremes by setting grids in the code...

mergen3107 · 2023-12-16T17:41:44Z

My verdict:
Max limits for portrait (cols x rows) = 8 x 6.
Max limits for landscape = 6 x 8.
Although, even with these extremes, if book cover is ignored, text might overflow over the book's cover borders.
(I'll provide screenshots soon).

Anything denser is merely usable, I think.

hius07 · 2023-12-16T17:43:02Z

I can't find these settings in the settings.reader.lua

They are in settings/bookinfo_cache.sqlite3.

mergen3107 · 2023-12-16T17:44:42Z

Portrait (8 x 6):

Lanscape (6 x 8):

mergen3107 · 2023-12-16T17:45:28Z

They are in settings/bookinfo_cache.sqlite3.

Understood!
In this case, I don't think it is necessary to expose them to settings.reader.lua.

mergen3107 · 2023-12-16T17:49:43Z

For recalculating the image to proper size depending on the settings - I would agree, but only if there was an option to extract all History/Favorites icons during the global cache extraction (from Plus menu in FM).

Last time I asked about this the problem was that History/Favorites can containt various books.

But why can't we take the list from history.lua, and add cache extraction for these books' mosaic thumbnails?

Then, at the moment when global cache extraction is happenning, the mosaic thumbnails would be extracted once for all books in History/Favorites. Then, any new books' mosaic thumbnail would be extracted once History/Favorites is open (the way it is done now).

I didn't see any worring performance hit even at 10x10 in my recent tests.

mergen3107 · 2023-12-16T17:56:04Z

Although, even with these extremes, if book cover is ignored, text might overflow over the book's cover borders.

Additionally to that:

for these books (where only cover is ignored), only metadata's title can overflow above the book's cover frame and filename's box.
Also ignoring book's metadata kinda make this problem go away (in 8 x 6 of 8 x 6). But in principle, if filaname is very long, or I set 10 x 10, filename overflow the cover frame anyway.

I don't think this is a problem (for me), just my observations. (I don't know how hard it would be to calculate how much space book cover frame can afford, and the calculate where to truncate the filename with ellipsis...)

poire-z · 2023-12-16T17:56:38Z

Looks like we have some little computation bug with the position of the dogear sometimes:

(I kind of like it, as it feels like some external object put to protect the book corners :) but there's some inconsistencies. Probably a missing math.floor/ceil.)

Then, at the moment when global cache extraction is happenning, the mosaic thumbnails would be extracted once for all books in History/Favorites.

Because if you are in Filebrowser in detailed list, we don't know the size of thumbnails in History mosaic mode. We need to have it displayed to know the size of its thumbnails. (We could somehow save these size somewhere, but it didn't feel needed at the time I did all that.)

mergen3107 · 2023-12-16T18:01:55Z

Probably a missing math.floor/ceil

I am soon gonna be an active proponent of Math.round(num) :D (c.f. #11225 (comment))

mergen3107 · 2023-12-16T18:03:49Z

Because if you are in Filebrowser in detailed list, we don't know the size of thumbnails in History mosaic mode.

So, then this calculation of sizes can be done upon clicking Apply in the in the grid spin widget?
The same calculation that happens anyway when go into History/Favorites (or any other mosaic mode list)

mergen3107 · 2023-12-16T19:31:06Z

@hius07 , I tested it quickly, feel free to add this here if you want: #11234 (comment)

(I don't know if what I did is optimal, so please give feedback)

NiLuJe · 2023-12-17T00:18:01Z

I'd fully expect to recompute new thumbnails when changing the grid size, FWIW.

(And possibly go so far as trashing the now irrelevant ones, but, eeeeh, ;D).

NiLuJe · 2023-12-21T21:00:28Z

We often do not pass the default values to widget:new.
And there are many Geom:new without x and y.

True, but we should ;p. (/s)

Basically, it creates a table with two elements, but many Geom methods will actually write to the full set of four, which would incur a realloc to resize the table.
Since that's fairly easy to avoid by simply creating the table with all four, I tend to do that if I spot any missing around code I'm working on ;).

Since Geoms are fairly small and ubiquitous, there's a fair chance something will attempt to write to all four along the way, and it's easy & quick to do it right ;).

I'm not advocating for a full pass on the codebase, mind you, just not gratuitously breaking it when it's being done right ;p.

NiLuJe · 2023-12-21T21:01:44Z

We do not trash the cached image (that is the original cover, and it would be upscaled by ImageWidget).

In that case, what's being kept is whatever the very first extraction generated, then, right?

hius07 · 2023-12-22T03:52:27Z

In that case, what's being kept is whatever the very first extraction generated, then, right?

Yes, and it will be kept until we build a grid with small enough cells.

hius07 · 2023-12-22T11:31:15Z

The latest commits reduces writing to sql db while grid tunng, by keeping and maintaining grid dimensions in the menu instance.

hius07

Reviewable status: 1 of 7 files reviewed, 14 unresolved discussions (waiting on @Frenzie, @mergen3107, @NiLuJe, and @poire-z)

frontend/ui/widget/keyvaluepage.lua line 75 at r4 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

Please restore the missing fields, it's very much on purpose to avoid a potential table resize.

(c.f., 3743229, but I'm sure there are others, I might have done a pass on the codebase a while ago).

Done.

frontend/ui/widget/keyvaluepage.lua line 308 at r4 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

Ditto.

Done.

plugins/coverbrowser.koplugin/listmenu.lua line 274 at r4 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

I'd probably update that comment to say thumbnail instead of cover, to avoid confusion with the real, full-size cover.

Done.

plugins/coverbrowser.koplugin/main.lua line 173 at r1 (raw file):

Previously, Frenzie (Frans de Jonge) wrote…

Possibly × instead of plain x?

Done.

plugins/coverbrowser.koplugin/main.lua line 174 at r4 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

I'd possibly move the type coercion directly into BookInfoManager:loadSettings (e.g., self.settings[key] = tonumber(values[i]) or values[i]).

Done.

plugins/coverbrowser.koplugin/main.lua line 178 at r4 (raw file):

Previously, Frenzie (Frans de Jonge) wrote…

Agreed

Done.

plugins/coverbrowser.koplugin/mosaicmenu.lua line 864 at r3 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

Gah, thought this came from a LuaSettings, but, nope, it's straight from the db...
    -- To keep track of CoverBrowser settings
    CREATE TABLE IF NOT EXISTS config (
        key TEXT PRIMARY KEY,
        value TEXT
    );
Hence the strings...

Done.

plugins/coverbrowser.koplugin/mosaicmenu.lua line 545 at r4 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

Same comment as in ListMenu about cover -> thumbnail

Done.

hius07 · 2023-12-22T15:38:45Z

Not bad. After refreshing the cache for certain grid scale_factor equals 1 exactly. No scaling.

koreader/plugins/coverbrowser.koplugin/mosaicmenu.lua

Lines 593 to 598 in 934d214

    
           -- Let ImageWidget do the scaling and give us a bb that fit 
        
           local _, _, scale_factor = BookInfoManager.getCachedCoverSize(bookinfo.cover_w, bookinfo.cover_h, max_img_w, max_img_h) 
        
           local image= ImageWidget:new{ 
        
               image = bookinfo.cover_bb, 
        
               scale_factor = scale_factor, 
        
           }

NiLuJe · 2023-12-22T17:48:13Z

plugins/coverbrowser.koplugin/bookinfomanager.lua

-            self.settings[key] = values[i]
+            local value = values[i]
+            if value then
+                value = tonumber(value) or value -- convert cdata<int64_t> to lua number 


Technically it's going to be a string, because the SQL table has a TEXT type, but eh, that's the spirit ;p.

NiLuJe

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Frenzie, @hius07, @mergen3107, and @poire-z)

plugins/coverbrowser.koplugin/bookinfomanager.lua line 276 at r7 (raw file):

            if value then
                value = tonumber(value) or value -- convert cdata<int64_t> to lua number
            end

tonumber(nil) is perfectly safe, so you can simplify the whole inner loop to self.settings[key] = tonumber(values[i]) or values[i] (I wouldn't worry about duplicating the indexing either, it's insignificant, and liable to be faster than the extra store/load anyway).

(Plus the nit about the comment mentioned on GH ;)).

Code quote:

            local value = values[i]
            if value then
                value = tonumber(value) or value -- convert cdata<int64_t> to lua number
            end

NiLuJe

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Frenzie, @hius07, @mergen3107, and @poire-z)

poire-z

Looks fine.
But may be keep that for after v2023.12 (if there is a v2023.12), so we get another full 1 or 2 months for @NiLuJe other ideas (or to tame them) and we get to trash only dev-build users's bookinfo caches multiple time?

poire-z · 2023-12-23T22:42:06Z

plugins/coverbrowser.koplugin/bookinfomanager.lua

@@ -39,7 +39,7 @@ local BOOKINFO_DB_SCHEMA = [[
        cover_fetched       TEXT,     -- NULL / 'Y' = we tried to fetch a cover (but we may not have gotten one)
        has_meta            TEXT,     -- NULL / 'Y' = has metadata (title, authors...)
        has_cover           TEXT,     -- NULL / 'Y' = has cover image (cover_*)
-        cover_sizetag       TEXT,     -- 'M' (Medium, MosaicMenuItem) / 's' (small, ListMenuItem)
+        cover_sizetag       TEXT,     -- '1072x1448' (example, is the original cover image width and height)


If we stay with one thumbnail and downscaling, no need of new columns at all.

I understand, but reusing a column named "cover sizetag" to store the original cover image width and height concatenated as a string, even if it is good for the planet :), is not the most clear and explicite DB design and naming :)
So, if this ends up being good and stable, one day, on the next DB schema change, we'll probably change it to have 2 columns of numbers for source cover image w & h. No need to change it for now.

Yup, I'll keep that in mind, as I may end up tweaking that to some extent, depending on how it all shakes up ;).

poire-z · 2023-12-23T22:47:27Z

plugins/coverbrowser.koplugin/bookinfomanager.lua

@@ -270,7 +270,7 @@ function BookInfoManager:loadSettings(db_conn)
        local keys = res[1]
        local values = res[2]
        for i, key in ipairs(keys) do
-            self.settings[key] = values[i]
+            self.settings[key] = tonumber(values[i]) or values[i] -- TEXT db field


I'd possibly move the type coercion directly into BookInfoManager:loadSettings (e.g., self.settings[key] = tonumber(values[i]) or values[i]).

I guess I'm fine with that, because we are in a little limited context of code that save & load stuff thru here, and it's possible most actually save & load numbers.

But generically, I find this change worse. Previously, only code saving numbers had to know they would get strings and need to do the conversion themselves. With this, any code saving strings may now get numbers for strings that looked like a number, and would have to be sure to coerce them back to be strings.

Lua is generally pretty lenient about string/number automatic conversions, especially number -> string ones, so I wouldn't expect any surprises.

(I did indeed plan to double-check what the hell we store in there, though ;)).

poire-z · 2023-12-23T22:52:36Z

plugins/coverbrowser.koplugin/bookinfomanager.lua

+    local max_img_h = cover_specs.max_cover_h
+    if img_w > max_img_w or img_h > max_img_h then -- original image bigger than placeholder
+        local new_cover_w, new_cover_h = BookInfoManager.getCachedCoverSize(img_w, img_h, max_img_w, max_img_h)
+        if new_cover_w > bookinfo.cover_w or new_cover_h > bookinfo.cover_h then return true end -- bigger thumbnail needed


(Would be easier to read with "return true" as usual on a new indented line.)

Trusting you both on all the maths :)

poire-z · 2023-12-23T22:56:18Z

plugins/coverbrowser.koplugin/main.lua

-        table.insert(sub_item_table, {
+        sub_item_table[i] = {


Nothing against it, but just curious: why this change ? :)
(I personally find indexing less flexisble than insert(), ie. if you were to want to skip one of the item in some cases.)

One day I met

koreader/plugins/exporter.koplugin/main.lua

Line 241 in e7ee900

submenu[#submenu + 1] = v:getMenuTable()

and made a simple performance test of
(1) t[i] = a
(2) table.insert(t, a)
(3) t[#t + 1] = a

(1) is the fastest, (3) is the slowest.

Yup, unlikely to matter outside of extremely hot codepaths though, but it's mostly harmless here ;).

Yes, for UI stuff, we don't really need any optimisation of this kind.
When we just fill/append stuff in an array, table.insert() is just obvious - no need to read around to be sure that [i] is always the right value.

Frenzie · 2023-12-24T08:55:39Z

Looks fine. But may be keep that for after v2023.12 (if there is a v2023.12), so we get another full 1 or 2 months for @NiLuJe other ideas (or to tame them) and we get to trash only dev-build users's bookinfo caches multiple time?

I should theoretically have a couple of hours later today so if that pans out I'll distill out the more important commits. :)

I won't have much upload to speak of though.

NiLuJe · 2023-12-24T16:11:40Z

I should theoretically have a couple of hours later today so if that pans out I'll distill out the more important commits. :)

I won't have much upload to speak of though.

Welp, it's Azure time anyway ;D.

Frenzie · 2023-12-24T22:24:30Z

Oh, I only meant slower than I have at home, no data limit issues.

hius07 · 2024-01-03T08:18:55Z

If we have a couple of weeks until the January release, this can be safely merged I believe.

hius07 added 7 commits December 16, 2023 11:00

keyvaluepage: function getDefaultItemsPerPage()

0ed7d9f

menu: self.font_size

948f361

filemanagermenu: show curent values in menu

ec90f5a

coverbrowser: mosaic mode grid settings

2dfe9d8

mosaicmenu: adjustable grid

91520a6

Update main.lua

94d4802

main: grid spinwidget presicion

10d257c

Frenzie reviewed Dec 16, 2023

View reviewed changes

Frenzie approved these changes Dec 16, 2023

View reviewed changes

poire-z reviewed Dec 16, 2023

View reviewed changes

coverbrowser: nice × sign

96a1bd2

mergen3107 mentioned this pull request Dec 16, 2023

Tweak number of columns and rows in Mosaic mode to fit more content #11186

Closed

mergen3107 mentioned this pull request Dec 16, 2023

Max number of items according to physical screen size/dpi #11234

Closed

Merge branch 'koreader:master' into mosaic-grid-settings

9dae804

hius07 added 5 commits December 22, 2023 13:18

bookinfomanager: convert sql string to number

5c6c4a9

coverbrowser: keep grid dimensions in menu instance

b0564ce

mosaicmenu: take grid dimensions from menu instance

776c39f

listmenu: take grid dimensions from menu instance

815f4c4

bookinfomanager: trailing space

934d214

hius07 commented Dec 22, 2023

View reviewed changes

NiLuJe reviewed Dec 22, 2023

View reviewed changes

NiLuJe requested changes Dec 22, 2023

View reviewed changes

hius07 added 2 commits December 23, 2023 09:57

bookinfomanager: simple tonumber

591273e

coverbrowser main: on grid change, update FC base class too

4efb894

NiLuJe requested review from Frenzie and poire-z December 23, 2023 15:55

NiLuJe approved these changes Dec 23, 2023

View reviewed changes

poire-z approved these changes Dec 23, 2023

View reviewed changes

bookinfomanager: style nit

c7d2339

hius07 added this to the 2024.01 milestone Dec 24, 2023

hius07 merged commit ea9ef67 into koreader:master Jan 12, 2024
3 of 4 checks passed

hius07 deleted the mosaic-grid-settings branch January 12, 2024 06:14

hius07 mentioned this pull request Jan 15, 2024

Mosaic: Spurious? thumbnails regeneration #11351

Open

poire-z mentioned this pull request Mar 6, 2024

covers in History not shown #11521

Closed

		table.insert (menu_items.filebrowser_settings.sub_item_table, 4, {
		table.insert (menu_items.filebrowser_settings.sub_item_table, 5, {

CoverBrowser: adjustable mosaic grid #11232

CoverBrowser: adjustable mosaic grid #11232

Conversation

hius07 commented Dec 16, 2023 • edited

Choose a reason for hiding this comment

Frenzie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poire-z commented Dec 16, 2023

hius07 commented Dec 16, 2023 • edited

Frenzie commented Dec 16, 2023

poire-z commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023 • edited

hius07 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

poire-z commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

mergen3107 commented Dec 16, 2023

NiLuJe commented Dec 17, 2023

NiLuJe commented Dec 21, 2023 • edited

NiLuJe commented Dec 21, 2023

hius07 commented Dec 22, 2023

hius07 commented Dec 22, 2023 • edited

hius07 left a comment

Choose a reason for hiding this comment

hius07 commented Dec 22, 2023

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

poire-z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Dec 24, 2023

NiLuJe commented Dec 24, 2023

Frenzie commented Dec 24, 2023

hius07 commented Jan 3, 2024

hius07 commented Dec 16, 2023 •

edited

hius07 commented Dec 16, 2023 •

edited

mergen3107 commented Dec 16, 2023 •

edited

NiLuJe commented Dec 21, 2023 •

edited

hius07 commented Dec 22, 2023 •

edited