Skip to content
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

History: search #11084

Merged
merged 7 commits into from Nov 9, 2023
Merged

History: search #11084

merged 7 commits into from Nov 9, 2023

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Nov 6, 2023

Search in history in filenames and books metadata (without opening files).

Available in the History
1

and by dispatcher in FM and Reader.
2


This change is Reviewable

Comment on lines 231 to 232
if match_table then
self.filter_text = match_table.filter_text
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole thing was a bit hard to review because of the choice of these variable names.
You choose self.filter_text because we already have a self.filter (not fond of "filter" when reading it, but ok, might be just my French influence).
Or may be its just "text" that reads usually as something we will display, not that we will search. You use "keywords" elsewhere, may be filter_keywords would sound better and consistent (even if keywords sounds like its multiple keywords and not a single string).
You pass them in a match_table (again, no real need to put "table" in its name:)), that you could have named filter_table or filtering_info.

Copy link
Member Author

Choose a reason for hiding this comment

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

This new search inherits from old

local keywords = self.search_value
if keywords ~= "*" then -- one * to show all files

and
local match_table = {
search_str = search_str,
bookmark = check_button_bookmark.checked,
highlight = check_button_highlight.checked,
note = check_button_note.checked,
case_sensitive = check_button_case.checked,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We can polish them all for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you feel (you're the code owner of all this :) and if it doesn't bug anybody else, fine with me.)

Copy link
Member

Choose a reason for hiding this comment

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

I haven't had a chance to read it yet. You could always name it filter_str(ing) if I understand the complaint correctly?

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.

Your search_string and search_info read well to me, thanks.

@hius07 hius07 merged commit d0d3cf7 into koreader:master Nov 9, 2023
2 of 3 checks passed
@hius07 hius07 deleted the history-search branch November 9, 2023 05:35
@hius07 hius07 added this to the 2023.11 milestone Nov 9, 2023
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.

None yet

3 participants