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

Reader style tweaks: register in Dispatcher manually #9816

Merged
merged 7 commits into from
Nov 22, 2022

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Nov 20, 2022

Style tweaks can be applied with a gesture or added to a profile.

01

02

03

04


This change is Reviewable

@poire-z
Copy link
Contributor

poire-z commented Nov 20, 2022

You will solve https://www.mobileread.com/forums/showthread.php?p=4274863#post4274863 :) (to which I would have replied "yes, wrong tool for the job", but ok, no other tool available, so the one you just made will have to do).

@hius07
Copy link
Member Author

hius07 commented Nov 20, 2022

Also some discussion in #8590 (comment).

Comment on lines 128 to 130
text = self.is_tweak_registered and _("Don't show in action list") or _("Show in action list"),
callback = function()
self.toggle_tweak_registered_callback()
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording "tweak registered" in this pure styletweaks context is a bit confusing - I would mention "dispatcher" in the variables/settings/methods, ie. is_dispatcher_registered.

Copy link
Member Author

Choose a reason for hiding this comment

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

It corresponds with self.registered_tweaks and style_tweaks_registered G_setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I saw that. But even style_tweaks_registered is a bit anonymous when just viewing the settings. If style_tweaks_dispatcher_registered is too long, just seeing "dispatcher" is better than "registered" (into what?). style_tweaks_dispatcher ? style_tweaks_in_dispatcher ? so-so :/
registered sounds too alike enabled when just studying this readerstyletweak.lua module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like style_tweaks_in_dispatcher, is_tweak_in_dispatcher, self.tweaks_in_dispatcher would read well.
If you really feel like sticking with "..._registered", please add some comment where this thing is created/read/saved, -- registered in dispatcher, so we don't have to follow it all the way to where it is neighboured by dispatcherRegisterStyleTweak() to know it is actually in Dispatcher that it is registered :)

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'm not stuck, just have no good idea. Let it be "in_dispatcher".

{category="none", event="ToggleStyleTweak", arg=tweak_id, title=T(_("Toggle style tweak: %1"), tweak_title), rolling=true})
end

local function dispatcherRemoveStyleTweak(tweak_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording only again: do we use Register/Remove elsewhere ? I would expect Register/Unregister.

Copy link
Member Author

Choose a reason for hiding this comment

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

local function dispatcherRegisterProfile(name)
Dispatcher:registerAction("profile_exec_"..name,
{category="none", event="ProfileExecute", arg=name, title=T(_("Profile %1"), name), general=true})
end
local function dispatcherRemoveProfile(name)
Dispatcher:removeAction("profile_exec_"..name)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't notice that at the time (or I was less interested by the profile plugin than by this styletweak module :).
I'd prefer to read all (these 2 only then?) as register/unregister - but up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

self.ui.doc_settings:saveSetting("book_style_tweak", self.book_style_tweak)
self.ui.doc_settings:saveSetting("book_style_tweak_enabled", self.book_style_tweak_enabled)
self.ui.doc_settings:saveSetting("book_style_tweak_last_edit_pos", self.book_style_tweak_last_edit_pos)
end

local function dispatcherRegisterStyleTweak(tweak_id, tweak_title)
Dispatcher:registerAction("style_tweak_"..tweak_id,
{category="none", event="ToggleStyleTweak", arg=tweak_id, title=T(_("Toggle style tweak: %1"), tweak_title), rolling=true})
Copy link
Contributor

Choose a reason for hiding this comment

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

Dunno if that's possible (or if all user-added settings and styletweaks are just non-contextualized stuff for Dispatcher), but seeing your screenshot, a separator between previous and all styletweaks would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Frenzie Frenzie added this to the 2022.12 milestone Nov 21, 2022
@Frenzie Frenzie modified the milestones: 2022.12, 2022.11 Nov 22, 2022
@Frenzie Frenzie merged commit 5b889a0 into koreader:master Nov 22, 2022
@hius07 hius07 deleted the register-style-tweaks branch November 22, 2022 12:49
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