Minimize DocSettings:open() calls #11437

hius07 · 2024-02-04T08:59:43Z

Recently added features (status buttons in file popup dialogs, aux providers support, frozen history for finished books) caused extra DocSettings:open() calls. Opening a book from the file browser gives 4 calls, popup file dialog gives 3 calls.
With this PR the number of calls is reduced: 2 calls while opening a book (that is the required minimum), 1 call for the popup dialog in FM, Hist, Coll.

This change is

Frenzie · 2024-02-04T12:21:23Z

frontend/apps/filemanager/filemanager.lua

-    if not provider then -- check associated
-        local provider_key = DocumentRegistry:getAssociatedProviderKey(file)
-        provider = provider_key and DocumentRegistry:getProviderFromKey(provider_key)
+    if provider == nil then


This is for clarity?

If you mean nil, I think it's just a matter of taste, now I tend to use not for boolean values only. Not strong opinion.

poire-z

A bit heavy to review, so trusting you.

hius07 · 2024-02-05T09:06:26Z

A bit heavy to review

Two ideas:
(1) Opening a book: FileManager:openFile now gets the associated or registered provider and passes it to ReaderUI (previously FM checked for associated provider only, and then ReaderUI again got the provider to open the document).
(2) File popup dialogs in FM, Hist, Coll now get doc_settings at showup and pass it to the buttons that need it.

poire-z · 2024-02-05T09:27:35Z

frontend/apps/filemanager/filemanager.lua

                self.bookinfo = file_manager.coverbrowser and file_manager.coverbrowser:getBookInfo(file)
-                table.insert(buttons, filemanagerutil.genStatusButtonsRow(file, close_dialog_refresh_callback))
+                local doc_settings_or_file
+                if has_sidecar then
+                    doc_settings_or_file = DocSettings:open(file)
+                    if not self.bookinfo then
+                        local props = doc_settings_or_file:readSetting("doc_props")
+                        self.bookinfo = FileManagerBookInfo.extendProps(props, file)


I found it a bit odd to store in self (FileManager) some bookinfo (about a specific book), but looks like it comes from another recent PR #11404. So, nothing new.
But just asking: the bookinfo of the "last file long-pressed" (right?) is held in here as self.bookinfo until another one is long-pressed ? Or is it cleaned up at some point? No problem keeping a reference to it for a long time even if no longer of interest?

Actually, not in FM but in the file_chooser instance.
It is not cleaned if no other file is long-pressed.

At a very quick glance at that module, it can contain a self.document reference, so that is very very much an issue if this one is still live!

Which one?
self.bookinfo contains book props from the sql db.

I was looking at FileManagerBookInfo itself, which I assumed is what self.bookinfo points to. If it only contains props, I would suggest renaming that variable instead ;).

There's an existing convention with FM/RD modules where self.thing points to the active instance of the AppThing module, which explains my confusion ;).

(e.g., ReaderZooming -> self.zooming)

And...

koreader/frontend/apps/reader/readerui.lua

Lines 418 to 423 in 962477e

-- book info

self:registerModule("bookinfo", FileManagerBookInfo:new{

dialog = self.dialog,

document = self.document,

ui = self,

})

;)

I understand, you mean

koreader/frontend/apps/filemanager/filemanager.lua

Line 404 in 962477e

self:registerModule("bookinfo", FileManagerBookInfo:new{ ui = self })

Yep, exactly ;).

So, yeah, if that's not really that, I'd feel less confused with a better variable name then ;).

Maybe, self.cachedbookinfo, to avoid names doubling.

If it only contaiins book properties, having a prop in there sounds sensible to me, but it's your show ;).

poire-z · 2024-02-05T09:28:32Z

frontend/apps/filemanager/filemanagerhistory.lua

-    -- try to stay on current page
-    local select_number = nil


We no longer try ? :) Why not? Or is it magically ensured somewhere else?

We pass -1 in switchItemTable and stay on the page even after removing an item.
Have I missed anything?

No, I just didn't know (or forgot) about the effect of -1:

koreader/frontend/ui/widget/menu.lua

Lines 1143 to 1157 in 962477e

--[[

the itemnumber paramter determines menu page number after switching item table

1. itemnumber >= 0

the page number is calculated with items per page

2. itemnumber == nil

the page number is 1

3. itemnumber is negative number

the page number is not changed, used when item_table is appended with

new entries

alternatively, itemmatch may be provided as a {key = value} table,

and the page number will be the page containing the first item for

which item.key = value

--]]

function Menu:switchItemTable(new_title, new_item_table, itemnumber, itemmatch, new_subtitle)

So, if it works as stated, no problem :)

poire-z · 2024-02-05T09:34:00Z

Two ideas:

Thanks for the details (worth adding to the first post when you open a PR so the discovery is made easier :).
(I'm sure your ideas are good, and I sure can be lazy, so "trusting you" from me means "I can trust you and keep being lazy because I know I can trust you" :))

hius07 · 2024-02-06T09:10:04Z

Still hesitating with the name.
My self.bookinfo is a bridge between FM/Reader/Hist/Coll that use bookinfo for a module name and doc_props for book props, and CoverBrowser that uses bookinfo for cached book info (with additional fields like has_cover or ignore_cover that are absent in props).
So self.bookinfo can be

koreader/frontend/apps/filemanager/filemanager.lua

Line 276 in 1fe9084

    
           self.bookinfo = file_manager.coverbrowser and file_manager.coverbrowser:getBookInfo(file)

or

koreader/frontend/apps/filemanager/filemanager.lua

Lines 281 to 282 in 1fe9084

    
           local props = doc_settings_or_file:readSetting("doc_props") 
        
           self.bookinfo = FileManagerBookInfo.extendProps(props, file)

poire-z · 2024-03-19T23:09:29Z

Random hijack to mention some issue, not sure it's related to this PR, but maybe, or to some similar one.

File browser set to detailed list.
Open a book (I have "Start with last book")
Go to File browser (with the top menu icon)
Long-press on any book in this first shown File browser page: the dialog misses "Refresh cached book info" and "Ignore cover/metadata".
Now, just swipe to the next File browser page, and back to that initial page.
Long-press on any book in this same File browser page: the dialog now has "Refresh cached book info" and "Ignore cover/metadata".

hius07 added 7 commits February 4, 2024 10:47

documentregistry: enable aux in getProvider()

67e537e

readhistory: remove "history frozen" check

396c60c

readerui: add "history frozen" check

816681c

filemanagerutil: use doc_settings_or file

65b315b

filemanager: use doc_settings_or file

2e0b670

filemanagercollection: use doc_settings_or file

c28746a

filemanagerhistory: use doc_settings_or file

1fe9084

Frenzie reviewed Feb 4, 2024

View reviewed changes

Frenzie added this to the 2024.02 milestone Feb 4, 2024

poire-z approved these changes Feb 5, 2024

View reviewed changes

poire-z reviewed Feb 5, 2024

View reviewed changes

hius07 added 5 commits February 7, 2024 09:46

filemanager: naming

f6505c5

filemanagercollection: naming

580ba68

filemanagerhistory: naming

c74229a

filemanagerutil: naming

7a0e101

covermenu: naming

e83b570

hius07 merged commit b8090c6 into koreader:master Feb 7, 2024
3 checks passed

hius07 deleted the min-doc-settings branch February 7, 2024 08:35

hius07 mentioned this pull request Feb 8, 2024

FileManager: less lfs calls #11452

Merged

hius07 mentioned this pull request Mar 20, 2024

CoverBrowser: fix file popup dialog #11576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimize DocSettings:open() calls #11437

Minimize DocSettings:open() calls #11437

hius07 commented Feb 4, 2024 •

edited by Frenzie

Frenzie Feb 4, 2024

hius07 Feb 4, 2024

poire-z left a comment

hius07 commented Feb 5, 2024

poire-z Feb 5, 2024

hius07 Feb 5, 2024

NiLuJe Feb 5, 2024

hius07 Feb 5, 2024

NiLuJe Feb 5, 2024 •

edited

NiLuJe Feb 5, 2024 •

edited

hius07 Feb 5, 2024

NiLuJe Feb 5, 2024

hius07 Feb 5, 2024

NiLuJe Feb 5, 2024

poire-z Feb 5, 2024

hius07 Feb 5, 2024

poire-z Feb 5, 2024

poire-z commented Feb 5, 2024

hius07 commented Feb 6, 2024

poire-z commented Mar 19, 2024

	-- book info
	self:registerModule("bookinfo", FileManagerBookInfo:new{
	dialog = self.dialog,
	document = self.document,
	ui = self,
	})

	--[[
	the itemnumber paramter determines menu page number after switching item table
	1. itemnumber >= 0
	the page number is calculated with items per page
	2. itemnumber == nil
	the page number is 1
	3. itemnumber is negative number
	the page number is not changed, used when item_table is appended with
	new entries

	alternatively, itemmatch may be provided as a {key = value} table,
	and the page number will be the page containing the first item for
	which item.key = value
	--]]
	function Menu:switchItemTable(new_title, new_item_table, itemnumber, itemmatch, new_subtitle)

Minimize DocSettings:open() calls #11437

Minimize DocSettings:open() calls #11437

Conversation

hius07 commented Feb 4, 2024 • edited by Frenzie

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poire-z left a comment

Choose a reason for hiding this comment

hius07 commented Feb 5, 2024

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

NiLuJe Feb 5, 2024 • edited

Choose a reason for hiding this comment

NiLuJe Feb 5, 2024 • edited

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 Feb 5, 2024

hius07 commented Feb 6, 2024

poire-z commented Mar 19, 2024

hius07 commented Feb 4, 2024 •

edited by Frenzie

NiLuJe Feb 5, 2024 •

edited

NiLuJe Feb 5, 2024 •

edited