Skip to content

CoverBrowser: fix getting cached info#10346

Merged
hius07 merged 5 commits into
koreader:masterfrom
hius07:fix-list-mosaic-cache
Apr 26, 2023
Merged

CoverBrowser: fix getting cached info#10346
hius07 merged 5 commits into
koreader:masterfrom
hius07:fix-list-mosaic-cache

Conversation

@hius07

@hius07 hius07 commented Apr 22, 2023

Copy link
Copy Markdown
Member

Some values of the cache array can be nil.


This change is Reviewable

Comment on lines +375 to +378
pages, percent_finished, status, has_highlight = unpack(self.menu.cover_info_cache[self.filepath])
pages = self.menu.cover_info_cache[self.filepath][1]
percent_finished = self.menu.cover_info_cache[self.filepath][2]
status = self.menu.cover_info_cache[self.filepath][3]
has_highlight = self.menu.cover_info_cache[self.filepath][4]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May be to avoid 4 similar lookups:

local cached_info = self.menu.cover_info_cache[self.filepath]
pages             = cached_info[1]
percent_finished  = cached_info[2]
...

(Also, I have a vague memory @NiLuJe may have had some other (shorter?) solution about intermediate nil values - does that ring a bell ?)

@NiLuJe NiLuJe Apr 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That would be table.pack + unpack.

As long as the original table has been packed via table.pack, it can contain intermediate nils and unpack will handle it.

(NOTE: That's because we patch LuaJIT to enable Lua 5.2 semantics for table.pack, for folks trying to reproduce at home with a bog-standard LuaJIT build).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

c.f., koreader/koreader-base#1535 for more details

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at CoverMenu:updateCache, that would be perfectly applicable there ;).

@hius07

hius07 commented Apr 22, 2023

Copy link
Copy Markdown
Member Author

Another way is to assign false instead of nil when creating the cache array in CoverMenu:updateCache().

@poire-z

poire-z commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

So, I'm fine with using false when we build/add to it if it causes no issue.
(Rather than using table.pack() as it feels it would change the way we build it :)

@NiLuJe

NiLuJe commented Apr 22, 2023

Copy link
Copy Markdown
Member
diff --git a/plugins/coverbrowser.koplugin/covermenu.lua b/plugins/coverbrowser.koplugin/covermenu.lua
index db069c4e0..534d1c859 100644
--- a/plugins/coverbrowser.koplugin/covermenu.lua
+++ b/plugins/coverbrowser.koplugin/covermenu.lua
@@ -61,7 +61,7 @@ function CoverMenu:updateCache(file, status, do_create, pages)
         status = summary and summary.status
         local highlight = doc_settings:readSetting("highlight")
         local has_highlight = highlight and next(highlight) and true
-        self.cover_info_cache[file] = {pages, percent_finished, status, has_highlight}
+        self.cover_info_cache[file] = table.pack(pages, percent_finished, status, has_highlight)
     else
         if self.cover_info_cache and self.cover_info_cache[file] then
             if status then

Feels pretty simple to me ;o).

@poire-z

poire-z commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

Feels pretty simple to me ;o).

Ok :) (I didn't look :/)

@poire-z poire-z mentioned this pull request Apr 22, 2023
@hius07

hius07 commented Apr 23, 2023

Copy link
Copy Markdown
Member Author
self.cover_info_cache[file] = table.pack(pages, percent_finished, status, has_highlight)

Doesn't work. It produces the table:

{
  [3] = "abandoned",
  n = 4
} --[[table: 0x47cdc9d0]]

But then

dummy, percent_finished, status = table.unpack(self.menu.cover_info_cache[self.filepath])

returns status == nil.

@NiLuJe

NiLuJe commented Apr 23, 2023

Copy link
Copy Markdown
Member

What's the actual input in that test?

@hius07

hius07 commented Apr 23, 2023

Copy link
Copy Markdown
Member Author

nil, nil, "abandoned", nil

@NiLuJe

NiLuJe commented Apr 23, 2023

Copy link
Copy Markdown
Member

Huh. I'd forgotten to test this with more esoteric nil placements, and that appears to confuse the len syntactic sugar #...

unpack(self.menu.cover_info_cache[self.filepath], 1, self.menu.cover_info_cache[self.filepath].n) will work, though...

@hius07

hius07 commented Apr 26, 2023

Copy link
Copy Markdown
Member Author

May be this small fix can appear in 2023.04, together with #10350.

@NiLuJe NiLuJe added this to the 2023.04 milestone Apr 26, 2023
@poire-z

poire-z commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

Go ahead and merge if you feel it's no risk.

@hius07 hius07 merged commit fd8cb14 into koreader:master Apr 26, 2023
@hius07 hius07 deleted the fix-list-mosaic-cache branch April 26, 2023 07:19
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
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.

3 participants