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

Dictionary: fix Fuzzy search setting appearance #10721

Merged
merged 4 commits into from Jul 24, 2023

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Jul 22, 2023

"Enable fuzzy search" checkbox is applicable to the current document only.
Do not show the checkbox when Dictionary Settings are called from the file browser.

Another way is to keep the checkbox and just change the item text, but in that case we'll lose the explanation dialog (it will remain on long-pressing only).


This change is Reviewable

@poire-z
Copy link
Contributor

poire-z commented Jul 22, 2023

Do not show the checkbox when Dictionary Settings are called from the file browser.

Feels a little odd.

Another way is to keep the checkbox and just change the item text

Feels better.
Enable fuzzy search by default - dunno if we already have other items spelled that way ("...by default").

but in that case we'll lose the explanation dialog (it will remain on long-pressing only).

I don't understand that. Isn't self:toggleFuzzyDefault() called on tap showing that explanation?

@hius07
Copy link
Member Author

hius07 commented Jul 22, 2023

Do you mean to keep the checkbox and show the dialog?
Usually the checkbox in menu is toggled just with a tap.

@hius07
Copy link
Member Author

hius07 commented Jul 22, 2023

dunno if we already have other items spelled that way ("...by default")

Would you like to enable or disable fuzzy search by default?

@poire-z
Copy link
Contributor

poire-z commented Jul 22, 2023

Do you mean to keep the checkbox and show the dialog?

Yes. At least, the checkbox is a visual indicator of the current state.

Usually the checkbox in menu is toggled just with a tap.

Ok, but still, it would feel more natural.

Btw, does "by default" means "on new books" (that is, is the value at opening time stored in that book docsettings, and changing the default won't have any effect on that book)?
Or does doing a lookup from filebrowser use that "default" setting - in which case "by default" is better. Or may be it doesn't need a "by default", is there is a popup window explaining/confirming what will happen ?

Would you like to enable or disable fuzzy search by default?

OK in a confirmbox, but may be none in the menu items?

@hius07
Copy link
Member Author

hius07 commented Jul 22, 2023

the checkbox is a visual indicator of the current state.

So maybe a usual "star" indicator is better.
It is shown in the buttons of the confirmation dialog.

@poire-z
Copy link
Contributor

poire-z commented Jul 22, 2023

Why not - but my other questions aimed at knowing if this setting in FileBrowser (even if I guess, people rarely go there when not in a book, either for tweaking that setting for FB lookups, for for setting it as default for future books) applies to lookups done in FileBrowser - in which case, the checkbox ("applies to now") feels as worthwhile as a star ("set as default, but could be unchecked for the current situation").
(Personally not fond of having too many stars :) but we don't visit that menu often, so I don't mind.)

@hius07
Copy link
Member Author

hius07 commented Jul 22, 2023

Looks like a bug: global disable_fuzzy_search is read in onReadSettings only.
If we lookup from the file browser before opening a doc, fuzzy search is always enabled.

@hius07
Copy link
Member Author

hius07 commented Jul 22, 2023

Updates:
-star indicator added
-we can enable/disable fuzzy search for the current file browser session
-setting is saved to doc_settings if the checkbox was toggled by a user only

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.

setting is saved to doc_settings if the checkbox was toggled by a user only

I understand that after reading the code as : it is saved in doc_setting ONLY once the user has toggled it once. As long as the user has not toggled it, it will use the default setting, even if that default is changed later.
Which is alright by me.
(A bit convoluted all this, but I've been there in the past with our various book/global/tap/long-press boolean states.... Not obvious to make it all clear in the UI.)

checked_func = function()
return self.disable_fuzzy_search ~= true
return not (self.ui.doc_settings and self.disable_fuzzy_search or self.disable_fuzzy_search_fm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't that leak ? if self.disable_fuzzy_search is false, you would use self.disable_fuzzy_search_fm ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry

@@ -416,10 +428,11 @@ function ReaderDictionary:onLookupWord(word, is_sane, boxes, highlight, link, tw
logger.dbg("dict stripped word:", word)

self.highlight = highlight
local disable_fuzzy_search = self.ui.doc_settings and self.disable_fuzzy_search or self.disable_fuzzy_search_fm
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here?
That is, if we toggled it to true then false (fuzzy search enabled), we would use the global setting?

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.

OK for me codewise.
Fine UI-wise, but other people thoughs welcome.

@hius07
Copy link
Member Author

hius07 commented Jul 23, 2023

Side question about the "star" indicator.
We now have 10 menu entries with an option of showing " ★".
Maybe it's time to add this to

function Menu.getMenuText(item)

Kind of text_star_func along with text_func?

@poire-z
Copy link
Contributor

poire-z commented Jul 23, 2023

Why not, in principle.
But we have a few of them where we don't only display a start (ie. styletweak and \u{F144}" when indispatcher, font and the fallback symbol), so this text_star_func might not often be used, and we'll get the star duplicated where we can't use text_star_func.
We also sometime concatenate icons, with some variable number of spaces before - depending also on the icon look. So, it could/should be a text_suffix_-symbols_func that would handle all of that :) but we'd probably be frustrated of it not always doing what we want, and we would add parameters to make it behave, and it could become a monster :)

@Frenzie
Copy link
Member

Frenzie commented Jul 23, 2023

Fine UI-wise, but other people thoughs welcome.

Other people's thoughts are always screenshots please. ;-P

@poire-z
Copy link
Contributor

poire-z commented Jul 23, 2023

Fine UI-wise, but other people thoughs welcome.

Other people's thoughts are always screenshots please. ;-P

Other lazy people thoughts... :)
Screenshots won't help much - I should have written UX instead of UI. I'm afraid it needs you to read the discussion and the code :)

@hius07
Copy link
Member Author

hius07 commented Jul 23, 2023

The changes to the current design are just "star" indicator when fuzzy search in enabled globally by default.

The checkbox "Enable fuzzy search" in file browser now works differently (previously it didn'w work at all).
Now it allows to enable/disable dict fuzzy serach in the current file browser session.

Per document setting is saved if a user checked/unchecked it himself only (previously the global setting was saved to doc_settings).

@Frenzie
Copy link
Member

Frenzie commented Jul 23, 2023

Yes, that sounds fine to me.

@hius07 hius07 merged commit 47dae6c into koreader:master Jul 24, 2023
3 checks passed
@hius07 hius07 deleted the dict-fuzzy-setting branch July 24, 2023 06:25
@hius07 hius07 added this to the 2023.07 milestone Jul 24, 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