New menu option and filemanager filter to hide finished books #7158 #10895

mytskine · 2023-09-12T15:52:34Z

Fixes issue #7158

The default behavior is to display the finished books (no change on upgrade). For consistency with the two similar options, it represented by a checkbox "Show hidden books" that is checked by default.

The implementation is straightforward, meaning that, when the option is unchecked, each file will require a call to filemanagerutil.getStatus that checks its status.

For clarity, the code uses the "finished books" expression because the condition is relevant to the book metadata, while the other settings are about file attributes.

This change is

NiLuJe · 2023-09-12T16:19:11Z

frontend/apps/filemanager/filemanager.lua

@@ -128,6 +128,7 @@ function FileManager:setupLayout()
        right_icon_hold_callback = false, -- propagate long-press to dispatcher
    }

+    local show_finished = G_reader_settings:hasNot("show_finished") or G_reader_settings:isTrue("show_finished")


~~Redundant, hasNot will never be true if isTrue is, so this can be reduced to simply isTrue.~~

Wait. Brainfart ;p. Kinda forgot that this needs to be enabled by default ;).

nilOrTrue is what you want ;).

I wanted that setting to be true by default, which isTrue() does not grant. I couldn't find a way to declare a default settings value. My experience is that a plain isTrue() will return false if the value is not set (nil), while hasNot() returns true.
If hasNot() or isTrue() isn't suitable there, what is the idiomatic way with koreader to check for nil or true?

Don't we have a helper function called nilOrTrue()? :-P

Yep, see my followup above ;). (GH's weird update notifications strike again, I guess ;)).

The second comment didn't show up in my browser yesterday, so sorry my reply was useless. Thanks for the advice.

hius07 · 2023-09-13T05:13:24Z

Filter FileChooser:show_file() is used in two more places.

(1) When moving sdr folder to a new location

koreader/frontend/apps/filemanager/filemanagermenu.lua

Line 944 in 3f677a7

and FileChooser:show_file(f) and DocSettings:hasSidecarFile(fullpath)

I think finished books should not be excluded here, so it is okay.

(2) In file search

koreader/frontend/apps/filemanager/filemanagerfilesearcher.lua

Line 171 in 3f677a7

and FileChooser:show_file(f) then

Dunno if you want to exclude finished book from the search results.

In any case the check for absent fullpath should be added in

koreader/frontend/ui/widget/filechooser.lua

Line 97 in 2e8d418

    
           if not self.show_finished and filemanagerutil.getStatus(fullpath) == "complete" then return false end

mytskine · 2023-09-13T12:16:15Z

@hius07 filemanagerutil.getStatus(nil) is valid, but you're right that it's better to avoid half a dozen nested function calls. It also makes this condition more similar to the previous line of code.

I should have mentioned that I have no experience with Lua expect for a quick try a few years ago. So a thorough code review is very useful! I felt uneasy with the untyped language. I'm not even confident in the tools I used, since my Lua LSP failed to list one of the references to FileChooser:show_file().

hugleo · 2023-09-15T01:33:00Z

You can use grep ;-)

cd koreader/frontend

$ grep -ilr "FileChooser:show_file" ./* 2>/dev/null
./apps/filemanager/filemanagermenu.lua
./apps/filemanager/filemanagerfilesearcher.lua
./ui/widget/filechooser.lua

$ grep -ilr "nilOrTrue" ./* 2>/dev/null
./apps/filemanager/filemanager.lua
./apps/filemanager/filemanagerutil.lua
./apps/filemanager/filemanagermenu.lua
...

Or some more tips here: #10708

mytskine · 2023-09-15T06:49:26Z

I'd already read #10708 and thought that the tools were better than plain text search. I'll use "rg", e.g. rg --type lua -w FileChooser:show_file, which is better suited than grep in my opinion. But searching text means there will be misses and false positives, whereas a LSP should know the scopes of the variables, detect libraries loaded under any name, etc.

poire-z · 2023-10-03T06:59:06Z

frontend/ui/widget/filechooser.lua

    for _, pattern in ipairs(self.exclude_files) do
        if filename:match(pattern) then return false end
    end
-    return self.show_unsupported or self.file_filter == nil or self.file_filter(filename)
+    if not self.show_unsupported and self.file_filter ~= nil and not self.file_filter(filename) then return false end
+    if not self.show_finished and fullpath ~= nil and filemanagerutil.getStatus(fullpath) == "complete" then return false end


I think local FileChooser = Menu:extend{ might be missing:

show_hidden = false, -- set to true to show folders/files starting with "." + show_finished = true, -- show all books

In case it's used from something else than FileManager.

Yes, and more over.
If we start with, say, Last book, and call PathChooser, it doesn't follow our view settings, because they are read in FM:

koreader/frontend/apps/filemanager/filemanager.lua

Line 132 in 0ac258f

local show_unsupported = G_reader_settings:isTrue("show_unsupported")

I think all these 3 view flags must be initiated from G_reader_settings directly in local FileChooser = Menu:extend{.

If in :extend, it's set at startup time and won't be reflected if we change the setting until a restart.
Rather in FileChooser:init()

won't be reflected if we change the setting

We change self when changing a setting

koreader/frontend/ui/widget/filechooser.lua

Lines 458 to 459 in 0ac258f

function FileChooser:toggleUnsupportedFiles()

self.show_unsupported = not self.show_unsupported

Yes, but this will apply to the current self, not to future FileChooser instances.

Anyway, I was going to rewrite these three togglings a bit.
I think no need to have their parts in both FM and FileChooser, they can be combined in one place.

koreader/frontend/apps/filemanager/filemanager.lua

Lines 788 to 791 in 0ac258f

function FileManager:toggleUnsupportedFiles()

self.file_chooser:toggleUnsupportedFiles()

G_reader_settings:saveSetting("show_unsupported", self.file_chooser.show_unsupported)

end

koreader/frontend/ui/widget/filechooser.lua

Lines 458 to 461 in 0ac258f

function FileChooser:toggleUnsupportedFiles()

self.show_unsupported = not self.show_unsupported

self:refreshPath()

end

not to future FileChooser instances.

True, we have

koreader/frontend/ui/widget/pathchooser.lua

Line 39 in 0ac258f

self.show_hidden = G_reader_settings:isTrue("show_hidden")

And we do not need this in FM, because the setting can be changed within FM only.

Or not, changing the setting doesn't change the base class. I'll think of it.

You could change the setting in the class:

function FileChooser:toggleUnsupportedFiles() - self.show_unsupported = not self.show_unsupported + FileChooser.show_unsupported = not FileChooser.show_unsupported

poire-z · 2023-10-03T07:00:13Z

@hius07 : letting you review and merge this one if ok and if there's no pb with your ideas in #10962.

hius07 · 2023-10-04T10:04:40Z

CircleCI produces the error downloading zlib-1.2.13.tar.xz.
I cannot synchronize the branch from the web.

…r#7158 The default behavior is to display the finished books (no change on upgrade). For consistency with the two similar options, it represented by a checkbox "Show hidden books" that is checked by default. The implementation is straightforward, meaning that, when the option is unchecked, each file will require a call to `filemanagerutil.getStatus` that checks its status. For clarity, the code uses the "finished books" expression because the condition is relevant to the *book* metadata, while the other settings are about *file* attributes.

NiLuJe · 2023-10-04T15:05:53Z

I cannot synchronize the branch from the web.

Done ;).

NiLuJe reviewed Sep 12, 2023

View reviewed changes

mytskine force-pushed the issue-7158-hide-finished-books branch from 2e8d418 to 994a4df Compare September 13, 2023 12:09

hius07 approved these changes Sep 15, 2023

View reviewed changes

poire-z reviewed Oct 3, 2023

View reviewed changes

François Gannaz and others added 2 commits October 4, 2023 17:05

Update filechooser.lua

b9765be

NiLuJe force-pushed the issue-7158-hide-finished-books branch from 69b1e5b to b9765be Compare October 4, 2023 15:07

hius07 merged commit 1283028 into koreader:master Oct 4, 2023
3 checks passed

hius07 added this to the 2023.10 milestone Oct 4, 2023

hius07 mentioned this pull request Oct 4, 2023

FR: [Option to hidden finished books] #7158

Closed

hugleo mentioned this pull request Dec 25, 2023

Can't go to next file when 'show_finished' is disabled and the book is marked as finished #11266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New menu option and filemanager filter to hide finished books #7158 #10895

New menu option and filemanager filter to hide finished books #7158 #10895

mytskine commented Sep 12, 2023 •

edited by Frenzie

NiLuJe Sep 12, 2023 •

edited

NiLuJe Sep 12, 2023 •

edited

mytskine Sep 12, 2023

Frenzie Sep 12, 2023

NiLuJe Sep 12, 2023 •

edited

mytskine Sep 13, 2023

hius07 commented Sep 13, 2023

mytskine commented Sep 13, 2023

hugleo commented Sep 15, 2023

mytskine commented Sep 15, 2023

poire-z Oct 3, 2023

hius07 Oct 3, 2023 •

edited

poire-z Oct 3, 2023

hius07 Oct 3, 2023

poire-z Oct 3, 2023

hius07 Oct 3, 2023

hius07 Oct 3, 2023

hius07 Oct 3, 2023

hius07 Oct 3, 2023

poire-z Oct 3, 2023

poire-z commented Oct 3, 2023

hius07 commented Oct 4, 2023

NiLuJe commented Oct 4, 2023

	function FileChooser:toggleUnsupportedFiles()
	self.show_unsupported = not self.show_unsupported

	function FileManager:toggleUnsupportedFiles()
	self.file_chooser:toggleUnsupportedFiles()
	G_reader_settings:saveSetting("show_unsupported", self.file_chooser.show_unsupported)
	end

	function FileChooser:toggleUnsupportedFiles()
	self.show_unsupported = not self.show_unsupported
	self:refreshPath()
	end

New menu option and filemanager filter to hide finished books #7158 #10895

New menu option and filemanager filter to hide finished books #7158 #10895

Conversation

mytskine commented Sep 12, 2023 • edited by Frenzie

NiLuJe Sep 12, 2023 • edited

Choose a reason for hiding this comment

NiLuJe Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hius07 commented Sep 13, 2023

mytskine commented Sep 13, 2023

hugleo commented Sep 15, 2023

mytskine commented Sep 15, 2023

Choose a reason for hiding this comment

hius07 Oct 3, 2023 • 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

Choose a reason for hiding this comment

poire-z commented Oct 3, 2023

hius07 commented Oct 4, 2023

NiLuJe commented Oct 4, 2023

mytskine commented Sep 12, 2023 •

edited by Frenzie

NiLuJe Sep 12, 2023 •

edited

NiLuJe Sep 12, 2023 •

edited

NiLuJe Sep 12, 2023 •

edited

hius07 Oct 3, 2023 •

edited