Skip to content

File search, FileChooser and others #10994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Oct 12, 2023
Merged

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Oct 11, 2023

All is cross-bound, hence the combined PR.

(1) File search
File search now is available in the file browser only.
Standard file dialog pop-up buttons added. Closes #10991.

1

(2) File manager
Unapplicable pop-up buttons in the file dialog are hidden (previously they were disabled).

Supported file
2

Unsupported file
3

(3) FileChooser
"Show" settings (finished books, hidden files, unsupported files) are centralized in FileChooser.
Optimization: do not init the settings every time when FM is launched.

(4) Dispatcher
"File search" is moved from General to File browser section.
"Fulltext search" is moved from General to Reader section.
Tidy: lines of code are ordered in accordance with the action menu order, comments fixed/added.

(5) History
Finished books timestamp is dimmed if "Freeze last read date of finished books" is enabled.


This change is Reviewable

@Frenzie Frenzie added this to the 2023.10 milestone Oct 11, 2023

--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in the context of the comment I just left this confuses me greatly. The newline provides the clarifying separation, what need is there for a visual distraction. :-P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dashes because
-sections are divided with empty lines
-look like separators

"sort_mixed",
"----------------------------",
"start_with",

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, lgtm, though the impact is a little hard to gauge in places.

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a lot to carefully read the whole thing.
But looks ok, and trusting you :)

(I thought the Dispatcher reorganisation was something I mentionned before at #10370 - but was happy to see it was taken care of in #10387, so not sure :) may be not completely, haven't reread these.)

Copy link
Member

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Frenzie and @hius07)


frontend/dispatcher.lua line 57 at r1 (raw file):

Previously, hius07 wrote…

Dashes because
-sections are divided with empty lines
-look like separators

"sort_mixed",
"----------------------------",
"start_with",

I sort of like the idea, but I might go with four dashes to make it clearer that it's not just a leftover empty comment oir something. (Also, ---- means a HR in MarkDown, right, so it's not entirely random?).


frontend/dispatcher.lua line 127 at r1 (raw file):

    file_search = {category="none", event="ShowFileSearch", title=_("File search"), filemanager=true, separator=true},
    --
    -- go_to

Those two I'm mildly confused by, though. Was the intent to match the actual Menu layout, but those two aren't applicable to Dispatcher?


frontend/ui/widget/filechooser.lua line 460 at r1 (raw file):

    local setting = not self[mode]
    self[mode] = setting
    FileChooser[mode] = setting

I'm not necessarily a fan of having this both in the instance and the class.

I'd much prefer it be one or the other.

(no preference as to which, so go with whatever is more practical, likely the class).

(Note that, if it's nil in the instance, looking up the instance will go to the class anyway).

@hius07
Copy link
Member Author

hius07 commented Oct 11, 2023

Was the intent to match the actual Menu layout, but those two aren't applicable to Dispatcher?

That was the intention.
go_to and back appears in two sections in the menu, but are coded once:

go_to = {category="none", event="ShowGotoDialog", title=_("Go to page"), filemanager=true, reader=true},

@hius07
Copy link
Member Author

hius07 commented Oct 11, 2023

Note that, if it's nil in the instance, looking up the instance will go to the class anyway

And what is the benefit if of setting self[mode]=nil?
Looking up farther, from the instance to the class?

@NiLuJe
Copy link
Member

NiLuJe commented Oct 11, 2023

And what is the benefit if of setting self[mode]=nil?
Looking up farther, from the instance to the class?

Not necessarily what I was going for ;).

I just meant that if a field isn't set in an instance, looking it up in said instance will resolve it in the class instead. (With the intent being that having the actual storage in the class means you don't necessarily have to hunt down every instance access to make them explicit class access instead... As long as nothing ever sets it in the instance, that is. Although, FWIW, I do prefer when class members are always explicitly accessed as such, and not via self ;)).

@hius07
Copy link
Member Author

hius07 commented Oct 11, 2023

Oh, yes, this line can just be removed, thanks.

@hius07 hius07 merged commit e577c79 into koreader:master Oct 12, 2023
@hius07 hius07 deleted the filechooser-etc branch October 12, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Allow standard file browser operations on file browser search results
4 participants