-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support configurable extra plugin lookup path #2693
Conversation
@@ -23,7 +23,6 @@ local order = { | |||
"highlight_options", | |||
"change_font", | |||
"hyphenation", | |||
"read_timer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to prefer this plugin to be placed in tools? IMO, it's more like a "reading experience".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem a touch out of place with the other rendering options. There should at least be a separator in between if it goes in the typeset menu. I didn't want to make things controversial by changing the menu around (and I didn't have time to think about it anyway) but it should be something more like
-- rendering stuff
"page_overlap",
"switch_zoom_mode",
"set_render_style",
--sep, I don't think highlight options go with anything else really
"highlight_options",
--sep, font-related (typeset) stuff
"change_font",
"floating_punctuation",
"hyphenation",
--sep, "reading experience" stuff
"read_timer"
--speed reading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, for consistent reason. It's placed under tools tab in filemanager. We should either keep it consistent between apps or only enable it in reader.
Secondly, that tab is named typeset, which means anything that controls rendering results. A timer doesn't fit into this category. With the new UX proposed by @baskerville and the new separator feature, we can combine navigation and typeset into one read menu, then it makes more sense to put timer there.
logger.warn("Error when loading", mainfile, plugin_module) | ||
elseif type(plugin_module.disabled) ~= "boolean" or not plugin_module.disabled then | ||
local lookup_path_list = { DEFAULT_PLUGIN_PATH } | ||
local extra_paths = G_reader_settings:readSetting("extra_plugin_paths") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we directly add DataStorage:getDataDir()/plugins/ to this list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't really expect many people to use this feature right now. We can always add a default path to the list if it turns out to be a popular feature. My preference is to not introduce this extra lookup overhead to all users for now.
To keep it consistent between reader and filemanager
…ories Extra plugin lookup paths can be set in global reader setting via key extra_plugin_paths. Value of the key can either be a string or an array of strings.
Looks good to me. |
This feature can be handy for platforms that does custom packaging like android, where plugins directory is not accessible from users and wiped every time the package is updated.