Custom metadata: fixes #10889

hius07 · 2023-09-11T06:19:55Z

Various fixes, including #10869 (comment) and #10869 (comment).

This change is

poire-z · 2023-09-11T06:42:47Z

Regarding display_title not up to date.
With this PR, if updating title for the opened book:

09/11/23-08:40:07 WARN  ReaderStatistics:onBookMetadataChanged {
  doc_props = {
    display_title = "cjkpunct43",    -- good
    pages = 2,
    title = "cjkpunct43"
  } --[[table: 0x7f13382a95a8]],
  filepath = "/space/kobo/koreader_stuff/test/cjkpunct.txt",
  metadata_key_updated = "title",
  metadata_value_new = "cjkpunct43",
  metadata_value_old = "cjkpunct42"
} --[[table: 0x7f1338275cd8]]

But if updating title from History when another book is opened:

09/11/23-08:41:41 WARN  ReaderStatistics:onBookMetadataChanged {
  doc_props = {
    display_title = "cjkpunct43",    -- should be 44 ?
    pages = 2,
    title = "cjkpunct44"
  } --[[table: 0x7f58c8133930]],
  filepath = "/space/kobo/koreader_stuff/test/cjkpunct.txt",
  metadata_key_updated = "title",
  metadata_value_new = "cjkpunct44",
  metadata_value_old = "cjkpunct43"
} --[[table: 0x7f58b0b107a8]]

poire-z · 2023-09-11T06:46:46Z

frontend/apps/filemanager/filemanagerbookinfo.lua

@@ -177,7 +179,7 @@ end

 -- Returns extended and customized metadata.
 function BookInfo.extendProps(original_props, filepath)
-    local custom_metadata_file = DocSettings:getCustomMetadataFile(filepath)
+    local custom_metadata_file = filepath and DocSettings:getCustomMetadataFile(filepath)


Is this just for safety ? Or are these cases when it is called without a filepath?

If we call without filepath, do not customize.
We do it in covermenu, where bookinfo is already customized.

If we call without filepath, do not customize.

So, if it is not a regular nil check for safety, this deserves a comment.

We do it in covermenu, where bookinfo is already customized.

Then, why does covermenu need to have it go thru :extendProps() ?

Then, why does covermenu need to have it go thru :extendProps() ?

To make a copy of bookinfo fileds that are needed in book_props.

Is it possible that the data in coverbrowser is obsolete and so can't be trusted?
ie. if customizing when in file browser in classic mode - then later switching to detailed list mode ?
(Not saying we should do something about that case - just wondering how trustworthy the book_props coming from SQL is. I guess I will have the same issue for statistics if they are disabled and re-enabled... Also, dunno if people use detailed/mosaic in history, and classic mode in File browser.)

When customizing, we refresh (delete) coverbrowser cache entry even in Classic mode.
So bookinfo is up to date.
An extreme case if to disable coverbrowser plugin, customize, then enable the plugin back.

poire-z · 2023-09-11T06:51:55Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+        if book_props.pages then
+            local original_props = custom_doc_settings:readSetting("doc_props")


Might need a comment. I asume this if book_props.pages is to know if it comes from the SQL bookinfo? Or else?
Also, why is it needed to save an updated doc_props.pages in the custom_metadata.lua ? Will it be updated if we later change font size - or not until we again change custom metadata?

If number of pages changed (eg font size changed), update a copy in custom_metadata file.
So, this update is done when customizing any field only.
We can update it every time when number of pages changes, but this will require some complications in DocSetting that I'd avoid.

Why do we need to save it?
It feels it shouldn't need to be in there, as it's not really metadata, and is not something that relates to what we have in custom_props.

return { ["custom_props"] = { ["title"] = "cjkpunct44", }, ["doc_props"] = { ["pages"] = 2, ["title"] = "cjkpunct", }, }

(If not saved, no question to ask about when to update it :)

It can be used in

koreader/frontend/apps/filemanager/filemanagerbookinfo.lua

Lines 224 to 231 in 86420ca

-- If still no book_props (book never opened or empty "stats"),

-- but custom metadata exists, it has a copy of original doc_props

if not book_props then

local custom_metadata_file = DocSettings:getCustomMetadataFile(file)

if custom_metadata_file then

book_props = DocSettings:openCustomMetadata(custom_metadata_file):readSetting("doc_props")

end

end

We can open a book, then reset doc_settings but keep custom metadata.
And have number of pages.

Well, ok if you feel it's valuable.
But in other situations (when we have a docsettings present), we are sure an updated pages will be used, and not this possibly obsolete/innacurate one?

Sure, doc_props are fetched from custom_metadata.lua if sdr file doesn't exist only.

poire-z · 2023-09-11T06:53:53Z

frontend/apps/filemanager/filemanagerbookinfo.lua

        if prop_key == "title" then -- generate if original is empty
-            self.ui.doc_props.display_title = prop_value or filemanagerutil.splitFileNameType(file)
+            book_props.display_title = prop_value or filemanagerutil.splitFileNameType(file)


(I assume we don't need to call :extendProps() because prop_value is already the customized title?)

poire-z · 2023-09-11T07:32:29Z

frontend/apps/filemanager/filemanagerbookinfo.lua

@@ -416,11 +416,13 @@ function BookInfo:setCustomMetadata(file, book_props, prop_key, prop_value)
    -- in memory
    prop_value = prop_value or custom_doc_settings:readSetting("doc_props")[prop_key] -- set custom or restore original
    book_props[prop_key] = prop_value
+    if prop_key == "title" then -- generate if original is empty


-- generate if original is empty

Shouldn't it be: if updated title is empty ?

We cannot have customized title empty.

So, -- generate when customization reset and original is empty ?

poire-z · 2023-09-11T07:57:14Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+            local original_props = custom_doc_settings:readSetting("doc_props")
+            if original_props.pages ~= book_props.pages then
+                original_props.pages = book_props.pages
+                custom_doc_settings:saveSetting("doc_props", original_props)
+            end


(Feels a bit verbose, and given that readSetting("doc_props") gives a reference to the same table (I guess it can't be nil and that original_props.pages won't crash), and that you're not replacing the table but just updating a value, we could just use instead of these 5 lines:

local original_props = custom_doc_settings:readSetting("doc_props") original_props.pages = book_props.pages

but as you prefer :)
)

Do you propose to not use saveSetting when a setting is a table? Always?
(Many of my "Correct access to doc_settings" PRs should be reverted)
Like

koreader/frontend/apps/reader/readerui.lua

Line 475 in cc82ead

self.doc_settings:saveSetting("summary", summary)

Do you propose to not use saveSetting when a setting is a table? Always?

Uh, no, not taking such a responsability :)
May be it is fine and more readable the way you do it (even if in effect, it assigns the same thing, ie a = a).
Pinging @NiLuJe for thoughts.

From what little I can see given than I haven't checked the context, you don't need the explicit saveSetting, as the reference hasn't changed, as @poire-z mentioned, so you can just update the field like what he proposed?

We all understand that. The question is which form we should favor ?
A: readSetting() and knowing it's a table and a shared reference, not needing :saveSetting() ?
B: readSetting() and saveSetting always so it's all explicite, whether it's a table (where it would be uneeded) or a number/string (where it is needed)

(Or no favoritism, and is it up to the writer ? :)

Definitely A, the API returns native objects for a reason ;).

The read+save dance is only necessary when references need to be broken.

(Barring extreme ambiguity or scoping concerns, i.e., as usual, common sense matters ;))

poire-z

All looks fine to me, tested while finalizing the statistics db update: no issue noticed with this PR (and all mine fixed and statistics update working).

(No matter what you decide to do with readSetting/saveSetting).

hius07 added 2 commits September 11, 2023 09:16

filemanagerbookinfo: fixes

7f5ad0a

covermenu: pass a copy of bookinfo to FilemanagerBookInfo

0a7ad6f

poire-z reviewed Sep 11, 2023

View reviewed changes

Update filemanagerbookinfo.lua

86420ca

poire-z reviewed Sep 11, 2023

View reviewed changes

hius07 added 2 commits September 11, 2023 11:06

filemanagerbookinfo: comments

78635bc

filemanagerbookinfo: fix update display_title

2a63dea

poire-z approved these changes Sep 11, 2023

View reviewed changes

filemanagerbookinfo: no saveSetting for a table

df5d138

hius07 merged commit 108d877 into koreader:master Sep 12, 2023

hius07 deleted the custom-metadata-fixes branch September 12, 2023 04:54

Frenzie added this to the 2023.09 milestone Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom metadata: fixes #10889

Custom metadata: fixes #10889

hius07 commented Sep 11, 2023 •

edited by Frenzie

Loading

poire-z commented Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

poire-z Sep 11, 2023

hius07 Sep 11, 2023

poire-z Sep 11, 2023

NiLuJe Sep 11, 2023 •

edited

Loading

poire-z Sep 11, 2023

NiLuJe Sep 11, 2023 •

edited

Loading

NiLuJe Sep 11, 2023

poire-z left a comment

		if book_props.pages then
		local original_props = custom_doc_settings:readSetting("doc_props")

	-- If still no book_props (book never opened or empty "stats"),
	-- but custom metadata exists, it has a copy of original doc_props
	if not book_props then
	local custom_metadata_file = DocSettings:getCustomMetadataFile(file)
	if custom_metadata_file then
	book_props = DocSettings:openCustomMetadata(custom_metadata_file):readSetting("doc_props")
	end
	end

Custom metadata: fixes #10889

Custom metadata: fixes #10889

Conversation

hius07 commented Sep 11, 2023 • edited by Frenzie Loading

poire-z commented Sep 11, 2023

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

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

NiLuJe Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Sep 11, 2023 • edited Loading

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 Sep 11, 2023 •

edited by Frenzie

Loading

NiLuJe Sep 11, 2023 •

edited

Loading

NiLuJe Sep 11, 2023 •

edited

Loading