Custom metadata #10861

hius07 · 2023-08-30T05:07:40Z

Long-press on a property in the Book information window to customize it.
custom_props and original doc_props are saved to custom_metadata.lua in sdr folder.

Maybe it's better to reorder the items, to put non-prop "Pages" to the end, closer to other numbers.

This change is

hius07 · 2023-08-30T05:14:28Z

TODO:
(1) Exporter plugin
(2) Reset button in the "long-press on a file" dialog: show 3 checkboxes for resetting separately document settings, custom cover, custom metadata.

poire-z

(Just some early comments, have to run, will review more later today.)

poire-z · 2023-08-30T06:24:23Z

frontend/docsettings.lua

@@ -16,6 +16,7 @@ local DocSettings = LuaSettings:extend{}

 local HISTORY_DIR = DataStorage:getHistoryDir()
 local DOCSETTINGS_DIR = DataStorage:getDocSettingsDir()
+local custom_metadata_filename = "/custom_metadata.lua"


This reads odd, as meaning that the file with be in the root folder /custom_metadata.lua.
I guess you want to avoid an additional concatenation .. "/" .. custom_metadata_filename, but still...

to avoid an additional concatenation

Yes, that's the only purpose.

Still "reads" odd :)

poire-z · 2023-08-30T07:07:40Z

frontend/docsettings.lua

-function DocSettings:removeSidecarDir(doc_path, sidecar_dir)
-    if sidecar_dir == self:getSidecarDir(doc_path, "doc") then
+function DocSettings.removeSidecarDir(doc_path, sidecar_dir)
+    if sidecar_dir == DocSettings:getSidecarDir(doc_path, "doc") then


There and below (and maybe elsewhere).
DocSettings:getSidecarDir with a : reads odd (and feels like a typo, and have me wondering what will happen if the DocSettings module table is used as the object self.)
It should be either DocSettings.getSidecarDir() with a . when it's a pure function, and no self object is needed.
Or self:getSidecarDir() with a : (as it was), if it looks better as a method and having an object self, even if self is not used.

I see that there, defining a function DocSettings.removeSidecarDir(), you need to use the method DocSettings:getSidecarDir() (probably because it's often used as a method), so it's indeed complicated.
Dunno how to make that clearer. Should if be made as a method DocSettings:removeSidecarDir() because it uses other methods ?
Not sure what's the best/cleaner. @Frenzie @NiLuJe ?
(Or may be it's just me on an early morning seeing evil where there is not :/)

I've started here and would like to continue transforming methods that do not require self, to functions.
It is a code optimization, and is clearer to read.
In DocSettings only flush, purge and getCoverFile require self.
EDIT: and new flushCustomMetadata.

you need to use the method DocSettings:getSidecarDir() (probably because it's often used as a method)

Yes, I tried to avoid massive refactoring not related to custom metadata.

and is clearer to read.

Disagree, for pretty much the reasons @poire-z pointed out.

I'd be fine with it if the whole module was pure, but this isn't the case here.

The only way I would see that viable is to fork those off to a dedicated LuaSettingsUtil module, but, again, I very strongly would suggest not bothering with that, especially if it involves a lot of code churn in existing code.

To my mind using self the way it was is clearer too (in fact I'm not sure I didn't write it in the first place 😅).

frontend/docsettings.lua

poire-z · 2023-08-31T07:26:42Z

frontend/apps/filemanager/filemanagerbookinfo.lua

    original_props = original_props or {}

    local props = {}
-    for _i, prop_key in ipairs(BookInfo.props) do
+    for _, prop_key in ipairs(BookInfo.props) do
        props[prop_key] = custom_props[prop_key] or original_props[prop_key]
    end


Still feeling setting display_title should be the job of this customizeProps() ?
If it really is the place where we "extend" the plain book metadata (with pages, display_title and custom metadata), may be the its name should be changed to extendProps() ?

poire-z · 2023-08-31T07:30:53Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+    -- but custom metadata exists, it has a copy of original doc_props
+    if not book_props then
+        local custom_metadata_file = DocSettings:getCustomMetadataFile(file)


(I have not yet met the custom_metadata code while reading) but should custom_metadata really contain a copy of the orignal book_props, even if not modified ?
I would think it should contain only key/value of modified props.

edit: ok, seems you save both original doc_props and custom ones (see my other comment below).

poire-z · 2023-08-31T08:17:11Z

frontend/apps/filemanager/filemanagerbookinfo.lua

-        -- Description may (often in EPUB, but not always) or may not (rarely
-        -- in PDF) be HTML.
-        description = util.htmlToPlainTextIfHtml(description)


(Was a bit sad that you removed my wildlife observation notes. But it is duplicated elsewhere, so not yet lost :)

Sorry for that, but there is another one:

koreader/frontend/apps/filemanager/filemanagerbookinfo.lua

Line 113 in 9b00a8f

-- Description may (often in EPUB, but not always) or may not (rarely in PDF) be HTML

poire-z · 2023-08-31T08:23:37Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+function BookInfo:showCustomEditDialog(file, book_props, metadata_updated_caller_callback, prop_key)
+    local input_dialog
+    input_dialog = InputDialog:new{
+        title = _("Edit book property: ") .. self.prop_text[prop_key]:sub(1, -2),


_("Title:"), when translated in French (haven't checked) may be _("Titre :") with a space or thin-space before the :.
I guess no issue if there's a trailing space in the text of the title you make for that widget, but dunno about other languages and how they would translate these (and if we should include the : in the translatable strings or not (and have the code append it, but then French won't get that space).
@Frenzie : thoughts?

Looks to me like it should be Edit book property: %1 instead of trying to figure out how to concatenate things, but iff template isn't appropriate for some reason the space should be concatenated without bothering the translator with it, as in Edit book property: (colon at the end, no space at the end).

My comment was more about your %1, from https://github.com/koreader/koreader/pull/10861/files#diff-10106ca66f5be0b0a2554fc344220689d85fa930768116d5650e655c558f465bR32
the list of the keys of the Book info KVPage, which includes their :, and is reused there to make the title "Edit this property", without the there uneeded : .

Ahh, there's a good chance that's being too clever yeah, although your French example is probably unproblematic (a tiny bit of extra space). But if Chinese uses some utf8 full width colon for example does it still work out...

Fixed (replace ":" to "").

poire-z · 2023-08-31T08:25:47Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+        if original_prop and prop_key == "description" then
+            original_prop = util.htmlToPlainTextIfHtml(original_prop)


So, if editing Description, and it was HTML, we'll start editing the TEXT version of it?
I guess that's ok and the most sane, to avoid getting bad/unbalanced HTML in the custom Description.

No, currently BookInfo:showCustomEditDialog() takes unstripped html description.
Should be fixed.

I see that we do not use original HTML'ed description anywhere.
Maybe clean it at the source, in Document:getProps()?

I see that we do not use original HTML'ed description anywhere.
Maybe clean it at the source, in Document:getProps()?

My preference in general is to save the orignal stuff as it is, original :)
If we later fix/tweak the cleaning code, it will apply immediately, without the need to refresh book metadata.
And maybe one day we'll want to display it as HTML (I think there was an issue about it - the resulting text being bad, that we solved another way).

But then I don't know how this plays with "custom description" :/ I guess the custom description should be plain text - dunno if where the htmlToPlainText stuff happens is too late to know about that.

poire-z · 2023-08-31T08:29:29Z

frontend/apps/filemanager/filemanagerbookinfo.lua

+            original_prop = self.custom_doc_settings:readSetting("doc_props")[prop_key]
+            custom_prop = self.custom_doc_settings:readSetting("custom_props")[prop_key]


There are both the original doc_props and the customized (only those modified?) in the custom_metadata.lua ?
Can you paste some sample of this custom_metadata.lua content?

-- we can read Lua syntax here! return { ["custom_props"] = { ["authors"] = "Dariusz Terefenko", ["series"] = "Jazz", ["series_index"] = 3, }, ["doc_props"] = { ["authors"] = "Terefenko, Dariusz;", ["language"] = "En", ["pages"] = 824, ["title"] = "Jazz Theory", }, }

poire-z · 2023-08-31T08:32:10Z

Maybe it's better to reorder the items, to put non-prop "Pages" to the end, closer to other numbers.

Yes, might looks better.
(Dunno about people so used to find "Cover image: tap to display" at the bottom, but it's not something we use everyday.)

poire-z · 2023-08-31T10:29:57Z

frontend/apps/filemanager/filemanagerbookinfo.lua

@@ -270,7 +269,7 @@ function BookInfo.getDocProps(file, book_props, no_open_document, no_customize)
        end
    end

-    return no_customize and (book_props or {}) or BookInfo.customizeProps(book_props, file)
+    return no_customize and (book_props or {}) or BookInfo.extendProps(book_props, file)


Guess no_customize could be renamed to not_extended or plain or raw or something else like that.

poire-z · 2023-08-31T11:18:28Z

remove unused no_customize

Good :)
What happened to the place that needed that ?

hius07 · 2023-08-31T12:09:11Z

What happened to the place that needed that ?

Eventually, it never existed.

poire-z · 2023-09-01T15:44:24Z

Some issue with this and the Reading statistics.
If I've read a book and swap its author and title, the statistics are gone, and new ones are created. If I read a few pages and fix the author or title again, a third book appear in the statistics and my Daily view.
Dunno which metadata affects other modules, and which metadata distinguish a book in the Statistics (authors and title probably, language and series I don't know).
May be it's worth to broadcast an event to inform other modules of some metadata changes, with previous and new values, so they can react on their side as needed.
For the statistics plugin, I don't know what should be done (don't really want to find out :/)

Allows me to fix an old pet-peeve of mine re: the terrible variable names used in there ;o).

Frenzie · 2023-09-01T16:32:28Z

I thought the stats were based on some kind of md5 and/or file location, but I guess that's the sync?

poire-z · 2023-09-01T16:39:18Z

Looks like it is the 3 items (title, authors, md5) that are used to find the book ID (ReaderStatistics:getIdBookDB()).
Not using the filepath makes it easy for us when moving files around, not having to update anything :)

Frenzie · 2023-09-01T16:45:50Z

Yes, agreed on not using the path. Using the title and authors seems a bit redundant given the md5.

poire-z · 2023-09-01T16:54:16Z

Using the title and authors seems a bit redundant given the md5.

Indeed :)
I remember of one request for having multi-users KOReader (so one and his partner can read the same book on a shared single device and have different reading progress, highlights...) that I suggested it was just simpler to copy/duplicate the book :) But then, with (title, authors, md5) and no filepath, the stats would me merged anyway.
But now that we can customize metadata, that solution may work again: rename book title to "Moby Dick (John)" and "Moby Dick (Jane)" for the copy, and it's fine.

Allows me to fix an old pet-peeve of mine re: the terrible variable names used in there ;o).

poire-z · 2023-09-10T08:05:19Z

Looks like that in customization, I can't "empty" a field (to remove trash, ie. from keywords, if we later have "browse by metadata/keyword").
Not sure it's a real use case and if we should care about that - just mentionning it.
(One can always set it to a single space to have the appearance it's empty.)

hius07 · 2023-09-10T08:45:45Z

I thought about that when coded the input dialog.
We cannot make a custom field nil.
We can make it false but it would complicate all the checks.
So I decided that a single space would work.

hius07 · 2023-09-10T08:47:37Z

It also cannot be "" because there are some checks that replace it with "N/A".

hius07 added 5 commits August 30, 2023 08:00

docsettings: custom metadata

997b663

readerview: getCurrentPageLineWordCounts()

bfb1e99

filemanagerbookinfo: custom metadata

2877e1c

readerui: customize props

bb485e9

filemanagerfilesearcher: getDocProps

6543206

poire-z reviewed Aug 30, 2023

View reviewed changes

hius07 added 2 commits August 30, 2023 20:07

docsettings: methods, not functions

92a690d

filemanagerbookinfo: methods, not functions

c9c6e3f

NiLuJe reviewed Aug 30, 2023

View reviewed changes

frontend/docsettings.lua Outdated Show resolved Hide resolved

hius07 added 2 commits August 30, 2023 20:37

docsettings: do not pass self.data.doc_path as arg

7ee630a

filemanagerbookinfo: update footer only if the title changed

9b00a8f

poire-z reviewed Aug 31, 2023

View reviewed changes

hius07 added 7 commits August 31, 2023 12:51

docsettings: no slash in "custom_metadata.lua"

ac6a888

filemanagerbookinfo: ":", extendProps, order

94cd6f5

readerui: extendProps

2db9b2f

readertypography: extendProps

f8ba547

screensaver: extendProps

6f71922

bookinfomanager: extendProps

bfb7757

filemanagerbookinfo: fix local

0cd0cef

poire-z reviewed Aug 31, 2023

View reviewed changes

filemanagerbookinfo: remove unused no_customize

33605eb

poire-z approved these changes Aug 31, 2023

View reviewed changes

hius07 merged commit ed2ea68 into koreader:master Sep 1, 2023

hius07 deleted the custom-metadata branch September 1, 2023 05:07

hius07 added this to the 2023.09 milestone Sep 1, 2023

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 1, 2023

DocSettings: Minor cleanup following koreader#10861

75c0a32

Allows me to fix an old pet-peeve of mine re: the terrible variable names used in there ;o).

hius07 mentioned this pull request Sep 2, 2023

Custom metadata: event #10869

Merged

NiLuJe added a commit that referenced this pull request Sep 6, 2023

DocSettings: Minor cleanup following #10861

ba5c7df

Allows me to fix an old pet-peeve of mine re: the terrible variable names used in there ;o).

poire-z mentioned this pull request Sep 12, 2023

Custom metadata: update Statistics db on metadata change #10894

Merged

poire-z mentioned this pull request Jan 4, 2024

The statistics of old read books have been reset #11318

Open

		if original_prop and prop_key == "description" then
		original_prop = util.htmlToPlainTextIfHtml(original_prop)

		original_prop = self.custom_doc_settings:readSetting("doc_props")[prop_key]
		custom_prop = self.custom_doc_settings:readSetting("custom_props")[prop_key]

Uh oh!

Custom metadata #10861

Custom metadata #10861

Uh oh!

Conversation

hius07 commented Aug 30, 2023 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hius07 commented Aug 30, 2023

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hius07 Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Aug 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Aug 31, 2023

Uh oh!

hius07 commented Aug 31, 2023

Uh oh!

poire-z commented Sep 1, 2023

Uh oh!

hius07 commented Aug 30, 2023 •

edited by Frenzie

Loading

hius07 Aug 30, 2023 •

edited

Loading

NiLuJe Aug 30, 2023 •

edited

Loading

poire-z Aug 31, 2023 •

edited

Loading