Skip to content

Menu search fixes#13493

Merged
hius07 merged 5 commits intokoreader:masterfrom
hius07:menu-search
Apr 1, 2025
Merged

Menu search fixes#13493
hius07 merged 5 commits intokoreader:masterfrom
hius07:menu-search

Conversation

@hius07
Copy link
Copy Markdown
Member

@hius07 hius07 commented Mar 31, 2025

(1) Do not open the upper menu when Menu search dialog is called by a gesture/profile.
(2) In search results dialog mark disabled items with "medium shade".
(3) "Open/walk" doesn't highlight disabled items (text of disabled highlighted item is not visible).
(4) "Open/walk" stops on the first disabled menu item if it has subitems.

1

"Open/walk" will show:

2


This change is Reviewable

Comment thread frontend/ui/widget/touchmenu.lua Outdated
if type(v) == "table" and not v.ignored_by_menu_search then
local entry_text = v.text_func and v.text_func() or v.text
is_disabled = is_disabled or v.enabled == false or (v.enabled_func and v.enabled_func() == false)
local entry_displayed_text = is_disabled and "\u{2592} " .. entry_text or entry_text -- Medium Shade + Hair Space
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(If that space that looks like a space in github is really a hair space, I'd rather see it as \u{200A}.)
oh, also on next line, so may be there: all spaces here and above are Hair Space U+200A

Comment thread frontend/ui/widget/touchmenu.lua Outdated
local indent = text and ((" "):rep(math.min(depth-1, 6)) .. "→ ") or "→ " -- all spaces here are Hair Space U+200A
local walk_text = text and (text .. "\n" .. indent .. entry_text) or (indent .. entry_text)
is_disabled = is_disabled or v.enabled == false or (v.enabled_func and v.enabled_func() == false)
local entry_displayed_text = is_disabled and "▒ " .. entry_text or entry_text
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I preferred when you used "\u{2592} " ... - Medium Shade + Hair Space.
That's been our usage recently (may be not here with the arrows, but recently in the skimwidget rtl arrows).

Copy link
Copy Markdown
Member

@Frenzie Frenzie Mar 31, 2025

Choose a reason for hiding this comment

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

On average I don't really like the look of \u{2592} and such and I prefer to avoid it when reasonable, but the comment saying there's a hair space is quite important either way to avoid future mishaps.

Edit: but I don't particularly mind the \u{} thing when it's not strictly necessary either, we're talking a very minor preference. And the shade looks more weird than clear like an arrow. :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Answer please:
(1) ▒  or \u{2592}
(2) → or \u{2192}
(3) "Hair Space" comment in both lines or in the second line only ("here and above")

local entry_displayed_text = is_disabled and "▒ " .. entry_text or entry_text
local indent = text and ((""):rep(math.min(depth-1, 6)) .. "→ ") or "→ " -- all spaces here and above are Hair Space U+200A

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My preference is to use \u{} notation for anything not ASCII.
On github and in proper editors (ie. all but mine :)), they will look OK. (I use a latin1 terminal, so these look like crap, but it's my problem I know).
But the fact is that nobody will be able to type it, we all must be just copy & paste ▒ from somewhere else. And to find out what it is, I had to copy it into a google search to get its codepoint.
So, I prefer \u{} which 1) shows up in all kind of editors and in github and 2) document the codepoint.

For stuff that is translatable and/or is only regular ASCII and may have/need hairspaces, I'll be ok to just have a comment saying these spaces are hairspaces, to keep the longer string readable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(1) ▒  or \u{2592}

Probably the latter; the former looks a bit like a rendering bug. :-)

(2) → or \u{2192}

I much prefer →
But there's certainly no need to go changing any if they happen to be different, and if @poire-z prefers the latter I'm okay with that too. It might be worth putting the → in the comment alongside the name in that case.

(3) "Hair Space" comment in both lines or in the second line only ("here and above")

I'd just stick it on each line for the future.

@hius07 hius07 merged commit 25101d2 into koreader:master Apr 1, 2025
4 checks passed
@hius07 hius07 added this to the 2025.03 milestone Apr 1, 2025
@hius07 hius07 deleted the menu-search branch April 5, 2025 07:20
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.

3 participants