Skip to content

Touchmenu: fix menu refreshing on check#13996

Merged
hius07 merged 27 commits into
koreader:masterfrom
hius07:touchmenu-on-check
Jul 1, 2025
Merged

Touchmenu: fix menu refreshing on check#13996
hius07 merged 27 commits into
koreader:masterfrom
hius07:touchmenu-on-check

Conversation

@hius07

@hius07 hius07 commented Jun 27, 2025

Copy link
Copy Markdown
Member

By default, on tapping an item with a checkmark the Touchmenu:
(1) keeps the menu open
(2) highlights the item
(3) unhighlights the item
(4) fires the callback
(5) updates itself to show the changes.

If the callback updates the menu, (5) is not needed.
If the callback closes the menu, (3) and (5) are not needed.

This PR allows to avoid menu update duplicating and to avoid calling unneeded unhighlighting.


This change is Reviewable

@poire-z

poire-z commented Jun 27, 2025

Copy link
Copy Markdown
Contributor

I have a hard time reading/understanding these names (that can be true or false):
on_check_updated_in_callback
on_check_closed_in_callback
I don't know how to read them :) making sense of what they do while reading these names. I can't even suggest better names :)
Not sure it is something with English grammar/wording, or me just being stuck or used to thing ending with _callback being actually a callback/function (we have on_close_ges = {...}, on_tap_close_callback = function(arg, ges, footnote_height) , on_success_callback(entered_preset_name).)

If we can't find better names, I think they deserve some comment in the TouchMenuItem declaration.

@hius07

hius07 commented Jun 27, 2025

Copy link
Copy Markdown
Member Author

"on check" is a prefix, this is about items with checkmarks only, "on check" means "on tap".
Then, the menu is updated in the callback, or the menu is closed in the callback.

@hius07

hius07 commented Jun 27, 2025

Copy link
Copy Markdown
Member Author

This flag is placed right before the callback function, to be clearer that it is about the following callback.

@poire-z

poire-z commented Jun 27, 2025

Copy link
Copy Markdown
Contributor
    on_check_updated_in_callback = nil, -- set to true for item with checkmark if its callback updates menu
    on_check_closed_in_callback = nil, -- set to true for item with checkmark if its callback closes menu

I'm still bothered by the on_....._callback, where everywhere we would expect it to be a function.
What about ?:

callback_updates_menu = true,
callback_closes_menu = true,

@hius07

hius07 commented Jun 27, 2025

Copy link
Copy Markdown
Member Author

on_check came from the former name no_refresh_on_check.
I think the names should show that the flags are applicable only for items with a checkmark.

@poire-z

poire-z commented Jun 27, 2025

Copy link
Copy Markdown
Contributor
check_callback_updates_menu = true,
check_callback_closes_menu = true,
checkmark_callback_updates_menu = true,
checkmark_callback_closes_menu = true,
callback_updates_menu_on_check = true,
callback_closes_menu_on_check = true,

Starting the things with on_ or ending them with _callback is what I'd like to avoid.

@hius07

hius07 commented Jun 27, 2025

Copy link
Copy Markdown
Member Author

Okay

@hius07 hius07 merged commit 29bbc84 into koreader:master Jul 1, 2025
4 checks passed
@hius07 hius07 deleted the touchmenu-on-check branch July 1, 2025 18:44
@hius07 hius07 added this to the 2025.06 milestone Jul 1, 2025
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
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.

2 participants