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

TouchMenu: Search menu to search the menu #9876

Merged
merged 12 commits into from
Dec 6, 2022
Merged

Conversation

Frenzie
Copy link
Member

@Frenzie Frenzie commented Dec 6, 2022

Fixes #9800.

This is #9823.


This change is Reviewable

Comment on lines 87 to 107
local image_widget
if self.show_icon then
--- @todo remove self.image support, only used in filemanagersearch
-- this requires self.image's lifecycle to be managed by ImageWidget
-- instead of caller, which is easy to introduce bugs
if self.image then
image_widget = ImageWidget:new{
image = self.image,
width = self.image_width,
height = self.image_height,
alpha = self.alpha ~= nil and self.alpha or false, -- default to false
}
else
image_widget = IconWidget:new{
icon = self.icon,
alpha = self.alpha == nil and true or self.alpha, -- default to true
}
end
else
image_widget = WidgetContainer:new()
end
Copy link
Member Author

@Frenzie Frenzie Dec 6, 2022

Choose a reason for hiding this comment

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

@poire-z I didn't follow this PR closely (feel free to take over instead if you're more up to date with it). What's this extra image widget stuff about?

Copy link
Contributor

Choose a reason for hiding this comment

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

cf #9823 (comment)
This confirmbox change should not be picked.
Where this ConfirmBox is used, it should either be switched to use a ButtonDialog (wider, no icon) as I suggested - or keep the ConfirmBox and use as the ConfirmBox's icon the icon of that top menu (as @hius07 suggested at #9823 (comment) if I got that right).

@Frenzie
Copy link
Member Author

Frenzie commented Dec 6, 2022

Here's a screenshot of the latest changes. I'll go ahead and merge it since it was otherwise already reviewed.
image

@Frenzie Frenzie added this to the 2022.12 milestone Dec 6, 2022
@Frenzie Frenzie added the UX label Dec 6, 2022
Comment on lines 34 to 38
text = _("Filebrowser"),
},
setting = {
icon = "appbar.settings",
text = _("Settings"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you have set the icon in the ConfirmBox, do we need to still have these (translated) text here.
And do we also need it in the ConfirmBox (this text would be shown nowhere else, so it relates to nothing, except giving a name to the icon on its left in the ConfirmBox.

Copy link
Member Author

@Frenzie Frenzie Dec 6, 2022

Choose a reason for hiding this comment

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

You're probably right, though there may upsides to having an official public name. (Then again, it's only the document and formatting menu that are a bit harder to talk about.)

I'll remove it.

@poire-z
Copy link
Contributor

poire-z commented Dec 6, 2022

There's also these uneeded always-false if not self.ui cf #9823 (comment).
And I haven't really reviewed the TouchMenu code, I just tested that it works.
But as it is quite located in its own section (except for these self.search_layout, which are populated even if we don't use the feature), and that it's not essential, you can go ahead merging it - it will be easier to look at it and fix/clean it if needed.

Frenzie and others added 2 commits December 6, 2022 21:32
Co-authored-by: poire-z <poire-z@users.noreply.github.com>
Comment on lines 1021 to 1019
if val.sub_item_table_func and extensive_search then
local sub_item_table = val.sub_item_table_func()
local perpage = ""
if sub_item_table.max_per_page then
perpage = "[" .. sub_item_table.max_per_page .."]"
end
recurse(sub_item_table, path .. ".sub_item_table_func" .. perpage, text, icon, depth)
elseif val.sub_item_table then
if val.sub_item_table then
Copy link
Contributor

Choose a reason for hiding this comment

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

After a quick look, it seems that with extensive_search=false, it wasn't (and isn't) searching inside sub_item_table_func ?
(one example of menu item inside such a func: About font-family fonts or Ignore publisher font names when font-family is set)
If that's the case, put the above back with just if val.sub_item_table_func then ?

@Frenzie Frenzie merged commit e58a12b into koreader:master Dec 6, 2022
@Frenzie Frenzie deleted the searchMenu branch December 6, 2022 21:02
@Frenzie
Copy link
Member Author

Frenzie commented Dec 6, 2022

There's also these uneeded always-false if not self.ui

Oh drat, I overlooked this comment. But indeed my main intent was to finish up what I saw as a final few minor remarks. :-)

@poire-z
Copy link
Contributor

poire-z commented Dec 6, 2022

Just git pull'ed, and a few more minor remarks:
A search for "font-family" returns no result, while a search for "family" does.
Also some refresh glitch at some point on the emulator:
image

@Frenzie
Copy link
Member Author

Frenzie commented Dec 6, 2022

- is a special character in patterns, so that seems to be expected or an oversight. font%-family will work… of course no one knows that :-)

I haven't seen that glitch.

@Frenzie
Copy link
Member Author

Frenzie commented Dec 6, 2022

More worrisome is that you can get this crash trying to animate a font result.

  1. Search for font%-family
  2. Click on one of them with animate enabled, for instance Sans Serif
./luajit: frontend/ui/widget/touchmenu.lua:1080: attempt to index a nil value
stack traceback:
	frontend/ui/widget/touchmenu.lua:1080: in function '_get_widget'
	frontend/ui/widget/touchmenu.lua:1205: in function 'action'
	frontend/ui/uimanager.lua:967: in function '_checkTasks'
	frontend/ui/uimanager.lua:1408: in function 'handleInput'
	frontend/ui/uimanager.lua:1531: in function 'run'
	./reader.lua:335: in main chunk
	[C]: at 0x561a3c01a79d

@poire-z
Copy link
Contributor

poire-z commented Dec 6, 2022

I guess regex search should go, and it should be plain search, given the target audience.
Also, with the sub_item_func, looking for "text" gives me a Menu with 51 pages, with many items decuplated.
Some errata needed from the editors of The Unfinished Works of the Dev formerly known as @zwim :)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Dec 7, 2022
@Frenzie
Copy link
Member Author

Frenzie commented Dec 7, 2022

with many items decuplated

Duplicated or just with the same name?

(That is, proper bug or more of a presentation issue?)

@poire-z
Copy link
Contributor

poire-z commented Dec 7, 2022

ok, looks like it's not a same menu duplicated. I was searching for "text", and I got many times some font name like "STIX Two Text". But also "Fulltext search", "Text editor: open last file". Each probably 100 times. Or search for "Noto", and you get all our Noto fonts many times.
Why ? Because all these happen in many submenus:

  • font submenu : we get the same font list in all the "Set font for family name 'serif'". We also get it for the Gestures action > Reflowable document > Set font.
  • Text editor, fulltext search: we must also get it many times because it's a gesture action, so available in each gesture submenu...

So, we would probably need to flag some submenus so they are not traversed by the search.

And that's what I feared with these kind of PRs. Feature totallly uneeded, but it looked localized and fun, so ok, not giving it much thoughts - and it ends up bleeding its crap elsewhere and becomes a curse: now, each time we'll add some dynamic submenu, we'll have to remember about this crappy menu search (and bug new contributors with "hey, you don't know that, but you have to think about that menu search that nobody uses; yes, it's ridiculous, but it's our heritage - you should comply...")

@Frenzie
Copy link
Member Author

Frenzie commented Dec 7, 2022

We could revert it. It's cool, but I don't know how useful it is. Simply put, I might know ToC entries per page exists somewhere and I may not immediately be able to find it, but I also don't know that it's called ToC entries per page. As such I may well find it more quickly by going down a few menus than trying various text strings. To work around that you'd need concepts like keywords attached to menu entries, which would indeed become rather silly.

On the flip side, forgetting about the full menu for a second, something using this principle could potentially be quite useful in gestures or fonts.

For example, say I want to see which gesture is attached to "open previous document" (ignoring the exact UI to trigger and display the results):

Frenzie added a commit to Frenzie/koreader that referenced this pull request Dec 7, 2022
@poire-z
Copy link
Contributor

poire-z commented Dec 7, 2022

To work around that you'd need concepts like keywords attached to menu entries, which would indeed become rather silly.

Well, our hierarchical and thematic keywords are actually our menu tree :)
(If you know there must be something related to TOC entries per page, you'd first find TOC, look around, "how, there's a setting>" in the same "separator section", let's go there, "yes, obvious!" :)

using this principle could potentially be quite useful in gestures or fonts.
For example, say I want to see which gesture is attached to "open previous document"

Being able to output/read a "gesture (event to action) and reverse gesture (action to event) sheet" could be nice. It could just be another submenu, readonly maybe - or a generated EPUB :)
The multiswipe submenu is usually enough for me to get an overall view of what is set (or what I have set), it's 4 pages, easy to navigate to find the one you look for (and/or think about how to reorganize your gestures). It's a bit less practical for the others gestures than multiswipe, as you have to enter each submenu (one finger swipe, hold corner...)
But I don't think a "text search" would be really useful, and the need to figure some text for that thing (Folder shortcuts ? I would type "favorite directories" and find nothing :)
(I thought about suggesting this "menu search" should also search for the text in the "help_text" - but then, you'd get results that look like they are not an answer to your search.)

I'm not opposed to the "menu search feature", as the animation makes it look fun - and I wasn't doing the work.
But it needs some more brain and work to solve the many little issues we noticed above.

And I'm a bit upset that, after I initially warned it might be complicated, that after some amount of work (the most fun part), and because we were hinting at the little complexities at the end (that have to be solved, I'm quite proud we take the time to review/test, and notice things, and don't merge stuff with noticed issues not fixed), zwim decides to abandon all that because it was too much at one moment (even if there was no user, not even himself, needy of the features in his 2 or 3 opened PRs), and there was no hurry). And that I/we would be forced into taking on that work on our shoulders (at some time I haven't decided, and on a topic I have little interest on).
So, even if 90% of the work is there, I'm totally happy with reverting it.
(But I also think I could manage a few days of being upset, and possibly finding solutions to these little problems, and not feel the guilt of killing a 90%-made PR that could be useful to 2 users in the next decade, because of emotions :)

@Frenzie
Copy link
Member Author

Frenzie commented Dec 7, 2022

I'd feel somewhat bad about having half the translations already done and then reverting it, but luckily it's just a few small strings.

I did slightly overestimate how done it was based on first playing around with it, since I failed to approach it as a tester.

@poire-z
Copy link
Contributor

poire-z commented Dec 10, 2022

I may make the invisible TrapWidget visible, with its small bottom left text, while the animation is ongoing - just because I was somehow spending a few seconds at the end of the animation to notice it has actually ended :)
image

Then I wonder if the [x] animation checkbox is made too early:
image

and may be it should be a user choice later, with possibly another button, so when looking at the result, the user can decide to go there directly, or to get the animation up to there:

image
(do we need the first 2 lines here?)

This wording "Walk me there / Walking you there" sounds nice in this context, but it's a bit different style of wording from anywhere else. Thoughts ?

Also, I'll invert current tap/hold action: tap will show the ConfirmBox above - so the user learns the path to there and this might be just enough for him to go there manually - and long-press will open the menu directly (probably not animated, to be quick).

@Frenzie
Copy link
Member Author

Frenzie commented Dec 10, 2022

Then I wonder if the [x] animation checkbox is made too early:

Agreed.

This wording "Walk me there / Walking you there" sounds nice in this context, but it's a bit different style of wording from anywhere else. Thoughts ?

"Show me"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Search menu entry
3 participants