Reset settings/cover/metadata separately #10866

hius07 · 2023-09-01T05:11:08Z

This change is

Frenzie · 2023-09-01T09:14:46Z

frontend/apps/filemanager/filemanagerutil.lua

                    caller_callback()
                end,
            }
+            check_button_settings = CheckButton:new{
+                text = _("document settings, progress, bookmarks, highlights, notes"),
+                checked = sidecar_file and true or false,


But sidecar_file is already true or false? :-) (Since it's hasSidecarFile())

koreader/frontend/docsettings.lua

Lines 93 to 97 in ed2ea68

--- Returns path of `metadata.lua` file if it exists, or nil.

-- @string doc_path path to the document (e.g., `/foo/bar.pdf`)

-- @bool no_legacy set to true to skip check of the legacy history file

-- @treturn string

function DocSettings:hasSidecarFile(doc_path, no_legacy)

Er, that function name should be changed to get ASAP if that's the case imho.

But anyway, at the very least I'd do has_sidecar_file = hasSidecarFile() and true or false rather than doing it over and over where it's used so that the workaround is contained.

IIRC, there's precedent for this because of that very inconsistency in name vs. function (which has come up multiple times before, so, for once, I'd bite the bullet and fix all the things the right way there ;p).

(Might be just a matter of renaming the existing DocSettings:hasSidecarFile to DocSettings:getExistingSidecarFilepath (or a better sounding name), and just create a wrapper DocSettings:hasSidecarFile(......) return self:getExistingSidecarFilepath(......) and true or false - and not change all the things, just the ones that expect the filepath ?)

Depending on the current callers, yeah, a wrapper (one way or the other) is not a bad idea (and tail calls are basically free in LuaJIT, so it's pretty harmless).

IIRC, there's precedent for this because of that very inconsistency in name vs. function (which has come up multiple times before, so, for once, I'd bite the bullet and fix all the things the right way there ;p).

The pattern under discussion is unique to this PR (and should not exist), except it seems to have unfortunately been used with this very same function before since it became misnamed.

$ rg "has[A-Z][a-z]*$[a-zA-Z]*$ and" plugins/gestures.koplugin/defaults.lua 9: tap_left_bottom_corner = Device:hasFrontlight() and {toggle_frontlight = true,} or nil, 14: one_finger_swipe_left_edge_down = Device:hasFrontlight() and {decrease_frontlight = 0,} or nil, 15: one_finger_swipe_left_edge_up = Device:hasFrontlight() and {increase_frontlight = 0,} or nil, 61: two_finger_swipe_south = Device:hasFrontlight() and {decrease_frontlight = 0,} or nil, 62: two_finger_swipe_north = Device:hasFrontlight() and {increase_frontlight = 0,} or nil, 76: tap_left_bottom_corner = Device:hasFrontlight() and {toggle_frontlight = true,} or nil, 81: one_finger_swipe_left_edge_down = Device:hasFrontlight() and {decrease_frontlight = 0,} or nil, 82: one_finger_swipe_left_edge_up = Device:hasFrontlight() and {increase_frontlight = 0,} or nil, 128: two_finger_swipe_south = Device:hasFrontlight() and {decrease_frontlight = 0,} or nil, 129: two_finger_swipe_north = Device:hasFrontlight() and {increase_frontlight = 0,} or nil, frontend/apps/reader/readerui.lua 579: if not DocumentRegistry:hasProvider(file) and provider == nil then frontend/device/generic/device.lua 149: if self:hasKeys() and self.input and self.input.event_map then frontend/dispatcher.lua 80: toggle_key_repeat = {category="none", event="ToggleKeyRepeat", title=_("Toggle key repeat"), device=true, condition=Device:hasKeys() and Device:canKeyRepeat(), separator=true},

poire-z · 2023-09-01T09:07:42Z

frontend/docsettings.lua

+    if data_to_purge.doc_settings or data_to_purge.custom_cover_file or data_to_purge.custom_metadata_file then
+        if lfs.attributes(self.doc_sidecar_dir, "mode") == "directory" then
+            os.remove(self.doc_sidecar_dir) -- keep parent folders
+        end
+        if lfs.attributes(self.dir_sidecar_dir, "mode") == "directory" then
+            util.removePath(self.dir_sidecar_dir) -- remove empty parent folders


May be a comment to mention that these will silently do nothing if the directory(ies) is not empty? (If that's the case.)

poire-z · 2023-09-01T09:08:43Z

frontend/docsettings.lua

+function DocSettings:purge(sidecar_to_keep, data_to_purge)
+    local custom_cover_file, custom_metadata_file
+    if sidecar_to_keep == nil then


Remind me when/for what this sidecar_to_keep is used?

koreader/frontend/docsettings.lua

Line 289 in ed2ea68

self:purge(sidecar_file) -- remove old candidates and empty sidecar folders

In :flush, to keep the actual sidecar file only.

poire-z · 2023-09-01T09:11:00Z

frontend/apps/filemanager/filemanagerutil.lua

+    local custom_cover_file = DocSettings:findCoverFile(file)
+    local custom_metadata_file = DocSettings:getCustomMetadataFile(file)


Just wondering: any reason for these 2 methods (somehow similar in purpose) are named differently?
Why not findCoverFile and findCustomMedatadaFile - or getCoverFile and getCustomMedataFile ?

We use

koreader/frontend/docsettings.lua

Line 429 in ed2ea68

function DocSettings:getCoverFile(reset_cache)

when we have opened doc_settings (to use the cache).

poire-z · 2023-09-01T09:14:17Z

frontend/apps/filemanager/filemanagerutil.lua

+                        custom_cover_file    = check_button_cover.checked and custom_cover_file,
+                        custom_metadata_file = check_button_metadata.checked and custom_metadata_file,


There is not .old (backup) of these, right? So we can pass directly the files we have found, right?
But :purge() will find them again if we pass no arg, right?
(Feels a bit odd that 2 places do the same work, but ok.)

Right.
We must do this work to enable/disable the ConfirmBox checkbuttons, prior to purge.

Ok, but we could just pass custom_cover_file=true and custom_metadata_file=true (booleans, like the 3rd one doc_settings = true is, for consistency), and let :purge() do again its normal work of finding them (again, yes, but this may not need that optimisation).

I think we should avoid double scanning the drive to find custom cover and metadata files again.

OK, not insisting :) as all this is quite localized and used only by these 2 modules.
(For a cleaner API, we would accept custom_cover_file=true (so all 3 keys can be booleans), and when it is a boolean, find the filepath - but when it is a string, assume it's the filepath that has already been searched and found, and use that as an optimization.)

poire-z · 2023-09-01T09:16:00Z

frontend/docsettings.lua

+        if lfs.attributes(self.dir_sidecar_dir, "mode") == "directory" then
+            util.removePath(self.dir_sidecar_dir) -- remove empty parent folders
+        end
+    end


So, no more return value from this :purge() - or its user filemanagerutil.purgeSettings(file). Sure there was no other places that made use of it?

Yes, reset was the only user of the return value, to update the coverbrowser cache.

hius07 · 2023-09-01T10:35:20Z

Actually there is some (historical) mess with the names:
getSidecarDir - returns the generated name of the dir, always truthy
getSidecarFile - returns the generated name of the file, always truthy
hasSidecarFile - returns the path to the actually existing file, or nil
findCoverFile - returns the path to the actually existing file, or nil
getCoverFile - returns the path form the cache if not empty, otherwise calls findCoverFile
getCustomMetadataFile - returns the path to the actually existing file, or nil

The worst is hasSidecarFile, but it has about a dozen entries all around to fix.

And our get is ambiguous: generate or check for existence.

poire-z · 2023-09-01T10:46:18Z

(^ no real problem for me, as long as it's documented in the docstrings/comments.)

Frenzie · 2023-09-01T12:06:09Z

hasSidecarFile is a problem, the rest is not.

And our get is ambiguous: generate or check for existence.

It slipped by me, but this change was bad. ;-)

https://github.com/koreader/koreader/pull/10329/files#diff-e8930dfe3beade539965223b8d896677cae3de5ea9f57717fe02a02a1a579930

Frenzie · 2023-09-02T06:45:22Z

Thanks for the changes! I think that about does it?

poire-z · 2023-09-02T15:47:54Z

frontend/apps/filemanager/filemanagerutil.lua

+            }
+            confirmbox:addWidget(check_button_cover)
+            check_button_metadata = CheckButton:new{
+                text = _("custom book properties"),


We handlle book metadata as book_props internally.
Dunno if we should use "book properties" or "book metadata" in the UI. "book property" feels more vague than metadata to me.
(Also in the previous PR allowing editing, we show "Book property: title" and "Edit book property: Title".)

Another "Metadata" is explained here

koreader/frontend/ui/elements/common_settings_menu_table.lua

Line 547 in a767ad4

Book view settings, reading progress, highlights, bookmarks and notes (collectively known as metadata) are stored in a separate folder named <book-filename>.sdr (".sdr" meaning "sidecar").

Amazon says "metadata fields":
https://kdp.amazon.com/en_US/help/topic/G201097560

I guess we felt the need to mention "metadata" in "(collectively known as metadata)" because our docsettings ("docsettings.epub.lua" would have been a quite good name) file is named metadata.epub.lua, as a way to indicate to the user who has met them in some file browser that this file is what we are talking about.
Too late to rename all these metadata.epub.lua :) (and external tools expect them named liked that) but I would have called that doc settings or doc properties.

And I would, as Amazon, name the book metadata fields... book metadata :)

The simpler would be to remove "(collectively known as metadata)" from that string. Do we mention "metadata" elsewhere?
Or find another user facing name for these title/authors/language/keywords...
Dunno.
Needs more input.

I vaguely recall some discussion about that string, but not why "collectively known as metadata" is in there.

NiLuJe · 2023-09-05T04:59:09Z

frontend/apps/filemanager/filemanagerutil.lua

            local confirmbox = ConfirmBox:new{
                text = T(_("Reset this document?") .. "\n\n%1\n\n" ..
-                    _("Document progress, settings, bookmarks, highlights, notes and custom cover image will be permanently lost."),
+                         _("Resetted information will be permanently lost."),


Late to the party, but: Reset, set is an irregular verb (set/set/set ;)).

Which possibly makes this turn of phrase slightly clunky, so I'd drop it entirely, e.g., Information will be permanently lost.

I believed so as well but googled and got
https://en.wiktionary.org/wiki/resetted
https://www.wordsense.eu/resetted/

Note that over on https://www.wordsense.eu/reset/#English it explicitly says reset as the simple past. The other one is broadcasting that some people use resetted (and broadcasted), which is one of those it's technically wrong but often clearer kind of things.

But all that as an aside. Information will be permanently lost or Information will be permanently lost after resetting sounds better anyway.

Fixed in #10869.

I believed so as well but googled and got https://en.wiktionary.org/wiki/resetted https://www.wordsense.eu/resetted/

Huh. That's another meaning entirely, in a very specific field (and a chiefly local one at that), but a fun quirk I wasn't aware of nonetheless ;).

hius07 added 2 commits September 1, 2023 08:09

docsettings: purge settings/cover/metadata separately

687b4e0

filemanagerutil: reset settings/cover/metadata separately

c4cce93

Frenzie added this to the 2023.09 milestone Sep 1, 2023

Frenzie reviewed Sep 1, 2023

View reviewed changes

poire-z reviewed Sep 1, 2023

View reviewed changes

docsettings: comment

0784126

filemanagerutil: has_sidecar_file

918e024

poire-z mentioned this pull request Sep 1, 2023

DocSettings: Minor cleanup following #10861 #10868

Merged

hius07 added 4 commits September 2, 2023 09:38

docsettings: fix hasSidecarFile

e002097

filemanagerutil: hasSidecarFile

1d5b202

documentregistry: hasSidecarFile

3cfdd4d

filechooser: hasSidecarFile

4b6d9b4

poire-z approved these changes Sep 2, 2023

View reviewed changes

poire-z reviewed Sep 2, 2023

View reviewed changes

filemanagerutil: "properties" -> "metadata"

5fac3ed

poire-z approved these changes Sep 4, 2023

View reviewed changes

Frenzie approved these changes Sep 4, 2023

View reviewed changes

hius07 merged commit f46f341 into koreader:master Sep 5, 2023

hius07 deleted the reset-separately branch September 5, 2023 04:42

NiLuJe reviewed Sep 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset settings/cover/metadata separately #10866

Reset settings/cover/metadata separately #10866

hius07 commented Sep 1, 2023 •

edited by Frenzie

Loading

Frenzie Sep 1, 2023 •

edited

Loading

hius07 Sep 1, 2023

Frenzie Sep 1, 2023 •

edited

Loading

Frenzie Sep 1, 2023 •

edited

Loading

NiLuJe Sep 1, 2023 •

edited

Loading

poire-z Sep 1, 2023

NiLuJe Sep 1, 2023 •

edited

Loading

Frenzie Sep 1, 2023 •

edited

Loading

poire-z Sep 1, 2023

poire-z Sep 1, 2023

hius07 Sep 1, 2023

poire-z Sep 1, 2023

hius07 Sep 1, 2023

poire-z Sep 1, 2023

hius07 Sep 1, 2023

poire-z Sep 1, 2023

hius07 Sep 1, 2023

poire-z Sep 1, 2023

poire-z Sep 1, 2023

hius07 Sep 1, 2023

hius07 commented Sep 1, 2023 •

edited

Loading

poire-z commented Sep 1, 2023

Frenzie commented Sep 1, 2023 •

edited

Loading

Frenzie commented Sep 2, 2023

poire-z Sep 2, 2023

hius07 Sep 2, 2023

hius07 Sep 2, 2023

poire-z Sep 2, 2023

Frenzie Sep 2, 2023

NiLuJe Sep 5, 2023 •

edited

Loading

hius07 Sep 5, 2023

Frenzie Sep 5, 2023

hius07 Sep 5, 2023

NiLuJe Sep 5, 2023 •

edited

Loading

	--- Returns path of `metadata.lua` file if it exists, or nil.
	-- @string doc_path path to the document (e.g., `/foo/bar.pdf`)
	-- @bool no_legacy set to true to skip check of the legacy history file
	-- @treturn string
	function DocSettings:hasSidecarFile(doc_path, no_legacy)

		local custom_cover_file = DocSettings:findCoverFile(file)
		local custom_metadata_file = DocSettings:getCustomMetadataFile(file)

		custom_cover_file = check_button_cover.checked and custom_cover_file,
		custom_metadata_file = check_button_metadata.checked and custom_metadata_file,

Reset settings/cover/metadata separately #10866

Reset settings/cover/metadata separately #10866

Conversation

hius07 commented Sep 1, 2023 • edited by Frenzie Loading

Frenzie Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Frenzie Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

NiLuJe Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Frenzie Sep 1, 2023 • edited Loading

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

hius07 commented Sep 1, 2023 • edited Loading

poire-z commented Sep 1, 2023

Frenzie commented Sep 1, 2023 • edited Loading

Frenzie commented Sep 2, 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

NiLuJe Sep 5, 2023 • edited Loading

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 5, 2023 • edited Loading

Choose a reason for hiding this comment

hius07 commented Sep 1, 2023 •

edited by Frenzie

Loading

Frenzie Sep 1, 2023 •

edited

Loading

Frenzie Sep 1, 2023 •

edited

Loading

Frenzie Sep 1, 2023 •

edited

Loading

NiLuJe Sep 1, 2023 •

edited

Loading

NiLuJe Sep 1, 2023 •

edited

Loading

Frenzie Sep 1, 2023 •

edited

Loading

hius07 commented Sep 1, 2023 •

edited

Loading

Frenzie commented Sep 1, 2023 •

edited

Loading

NiLuJe Sep 5, 2023 •

edited

Loading

NiLuJe Sep 5, 2023 •

edited

Loading