Skip to content

QuickMenu: add long-press on profile #10671

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

Merged
merged 14 commits into from
Jul 28, 2023
Merged

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Jul 10, 2023

Fixes #10668 (comment).

Long-press on a profile entry in a QuickMenu calls a new QM with the content of the profile and "Execute all" button.


This change is Reviewable

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.

@yparitcher : ok for you?

@yparitcher
Copy link
Member

@hius07

I thought to do it slightly differently in yparitcher@4d0578d
This will leave the signature of Dispatcher:execute the way it is without adding any new arguments.

What are your thoughts? (I dont intend to rewrite all you dispatcher PRs, i just like keeping the function signature clean :)

@hius07
Copy link
Member Author

hius07 commented Jul 12, 2023

Can you please clarify what are the benefits?
The code is more complicated, with two tableDeepCopy calls.

@poire-z
Copy link
Contributor

poire-z commented Jul 22, 2023

@yparitcher : any followup discussion ?

@yparitcher
Copy link
Member

Sorry for the delay,

I thought it would be better not to complicate Dispatcher:execute() with quickmenu details, even if it makes the quickmenu code a little more complicated.

If people think otherwise then that is ok also.

@hius07
Copy link
Member Author

hius07 commented Jul 23, 2023

To summarize: the purpose is to "execute" a profile overriding its show_as_quickmenu setting, i.e. to forcibly run a profile or to show it as a QM.
(1) The initial idea is to add the third argument to Dispatcher:execute(), with options true - show a QM, false - run, nil - honor the profile's show_as_quickmenu setting. (The most of calls will be from other gestures/actions with nil)
(2) Proposed alternative way is to hide the flag into the profile itself. To avoid pollution of the original profile settings, tableDeepCopy is called twice to create temporary profiles.

@poire-z
Copy link
Contributor

poire-z commented Jul 23, 2023

I agree with trying to avoid tableDeepCopy.

i just like keeping the function signature clean :)

function Dispatcher:execute(settings, gesture, show_as_quickmenu)
If we change it, and want to keep the function signature not too ugly, may be pass a table, so it's generic enough if we later want to add other behaviour tweaks ?

function Dispatcher:execute(settings, gesture, exec_tweaks)
exec_tweaks = { show_as_quickmenu = false }
(exec_tweaks or overrides or params or a better name by better english talking people :)

@Frenzie
Copy link
Member

Frenzie commented Jul 23, 2023

Something like overrides seems clearer unless I misunderstand the intent.

@hius07
Copy link
Member Author

hius07 commented Jul 26, 2023

function Dispatcher:execute(settings, gesture, exec_tweaks)
The full gesture table is not used, we need a gesture position for QM anchoring only, so we can hide the flag anchor_quickmenu = ges.pos into the exec_tweaks table.

@hius07
Copy link
Member Author

hius07 commented Jul 26, 2023

Update: dispatcher execution is managed via the container (table) exec_props.
Currently has two properties:
qm_show: forcibly show QM / run
qm_anchor: quick menu anchoring

@hius07
Copy link
Member Author

hius07 commented Jul 26, 2023

Not good, I've missed some events get gesture as an argument.

@hius07
Copy link
Member Author

hius07 commented Jul 26, 2023

So, 3 arguments to Dispatcher:execute.

font_size = 22,
callback = function()
UIManager:close(quickmenu)
Dispatcher:execute(settings, { qm_show = false })
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a nil or false "gesture" as the 2nd argument ?
(I guess it would look odd if we put "gesture" inside the exec_props table ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

(I guess it would look odd if we put "gesture" inside the exec_props table ?

I think it'll simplify a little bit.
@yparitcher can you please review.

@yparitcher
Copy link
Member

LGTM

You can merge the gesture argument if you want, but you don't have to.

@hius07
Copy link
Member Author

hius07 commented Jul 27, 2023

Shouldn't we use elseif when checking the category in Dispatcher:execute()?

@poire-z
Copy link
Contributor

poire-z commented Jul 27, 2023

On line now 1084? I Looks like we could.

@hius07
Copy link
Member Author

hius07 commented Jul 27, 2023

And further, there are two more checks.

Comment on lines +1053 to +1054
if ((exec_props == nil or exec_props.qm_show == nil) and settings.settings and settings.settings.show_as_quickmenu)
or (exec_props and exec_props.qm_show) then
Copy link
Member

@yparitcher yparitcher Jul 27, 2023

Choose a reason for hiding this comment

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

This conditional is a bit long and confusing to read, bur not sure of any way that is better

@hius07 hius07 merged commit b36ccc7 into koreader:master Jul 28, 2023
@hius07 hius07 deleted the QuickMenu-long-press branch July 28, 2023 15:10
@hius07 hius07 added this to the 2023.07 milestone Jul 28, 2023
@mergen3107
Copy link
Contributor

Thank you very much @hius07 !
This PR works great on -44!

@mergen3107
Copy link
Contributor

Only thing missing is that Font name change doesn’t have notification, but I think this is because usually fonts are select in the top menu, while they are all visible :D

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.

FR: Profiles - make an option to "Tap to dismiss notifications"
5 participants