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

Plugin manager #4159

Merged
merged 9 commits into from Aug 17, 2018

Conversation

Projects
None yet
5 participants
@robert00s
Contributor

robert00s commented Aug 14, 2018

Reference: #2544 and #4054
This PR gives you the option of enabling/disabling any plugin via the GUI. This is a continuation of the work @poire-z in #4054.
Screenshots:
test - koreader_015
test - koreader_016

@KenMaltby

This comment has been minimized.

KenMaltby commented Aug 14, 2018

"Kobo light"? is that for the warmth controls that some of the new Kobo devices have? The "Comfort lighting"? Some might think they can use their reader as a "flashlight" like with a Smartphone or that you can disable the regular front light, this way.

This could be a good place for some inline manual function descriptions, for the plugins.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Aug 14, 2018

@KenMaltby: Right. Should be: Frontlight gesture controller

@Frenzie

It's a bit of a silly comment perhaps, but shouldn't that be full_name?

local lfs = require("libs/libkoreader-lfs")
local logger = require("logger")
local _ = require("gettext")

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

Just because it's a low-level module which didn't previously require() much (lfs & logger only), and you don't know at which point it may be imported (well, it's used late, but still), it'd be safer to move the require()s for UIManager, InfoMessage and _ in the single callback where they are used. (So that when something require("pluginloader"), it doesn't go yet importing UIManager & the widget stuff.)

sandboxPluginEventHandlers(plugin_module)
table.insert(self.plugins, plugin_module)
if (plugins_disabled and plugins_disabled[entry:sub(1, -10)]) then
sandboxPluginEventHandlers(plugin_module)

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

Not sure we need this sandboxPluginEventHandlers() for disabled plugins (?)

element.fullname = plugin.fullname or plugin.name
element.name = plugin.name
element.enable = false
if element.name ~= "storagestat" then

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

May be we could put "storagestat" in a module local

local PLUGINS_OBSOLETE = {
    "storagestat"=true,
}

so it's no more harcoded there and above (where you added it 1 or 2 weeks ago).

This comment has been minimized.

@robert00s

robert00s Aug 14, 2018

Contributor

Another module or where to put PLUGINS_OBSOLETE?

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

In this module, just under local DEFAULT_PLUGIN_PATH = "plugins".

local DEFAULT_PLUGIN_PATH = "plugins"
local OBSOLETE_PLUGINS = {
    "storagestat" = true,
}

This comment has been minimized.

@robert00s

robert00s Aug 14, 2018

Contributor

Thanks!

callback = function()
local plugins_disabled = G_reader_settings:readSetting("plugins_disabled") or {}
plugins_disabled[plugin.name] = plugin.enable
plugin.enable = not plugin.enable

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

That's an unclear shortcut :) to store the former value before the not , because we want true in the settings when it's becoming false.
But we don't want false in the settings when it's becoming true, we'd prefer nil, so it's not stored in the setting and we only have the list of disabled plugins in it (with all =true)
So, you may do that after the not, with a clearer if plugin.enable then store nil (=erase) else store true end

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 14, 2018

This could be a good place for some inline manual function descriptions, for the plugins.

Indeed, plugins could all have a help_text = _([[...]]) additional attribute alongside the name and fullname ones, that could be shown (as in #3980) by a hold_callback in the menu item each registers, as well as in the menu items this PR builds in genPluginManagerSubItem().

(I'm still not sure what zsync, kosync, evernote and calibrecompanion do :)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Aug 14, 2018

kosync is some kind of progress sync, zsync some kind of local filesharing I believe, Evernote is some kind of notes sync and calibre companion does whatever calibre companion does, but I forget what that is.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Aug 14, 2018

@KenMaltby

This comment has been minimized.

KenMaltby commented Aug 14, 2018

Evernote and Calibre Companion function with or in a related manner to the commercial software, with the same names.

Zsync lets you "publish" the book you are reading and then be able to "subscribe to" it on other devices running KOReader.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Aug 14, 2018

This could be a good place for some inline manual function descriptions, for the plugins.

Can you give me short description for all plugins. Please :)

autofrontlight
batterystat 
evernote
keepalive
newsdownloader
send2ebook
storagestat
texteditor
autosuspend
calibrecompanion
goodreads
kobolight
perceptionexpander
SSH
systemstat
timesync
backgroundrunner
coverbrowser
hello
kosync
readtimer
statistics 
terminal
zsync
else
plugins_disabled[plugin.name] = nil
end
plugin.enable = not plugin.enable

This comment has been minimized.

@poire-z

poire-z Aug 14, 2018

Contributor

I'd still like to see it done after the plugin.enable = not plugin.enable, so it uses the new value instead of the old one:

               plugin.enable = not plugin.enable
               if plugin.enable then
                   plugins_disabled[plugin.name] = nil
               else
                   plugins_disabled[plugin.name] = true
               end
@Frenzie

This comment has been minimized.

Member

Frenzie commented Aug 14, 2018

Here are the first few. Off to feed the cats.

autofrontlight

Automatically turns the frontlight on and off once brightness in the environment reaches a certain level. Only supported on Kindle (?).

batterystat

Collects and displays battery statistics.

evernote

Exports hightlights and notes to the Evernote cloud.

keepalive

Keeps the device awake to prevent automatic Wi-Fi disconnects.

texteditor

A basic text editor for making small changes to plain text files.

@KenMaltby

This comment has been minimized.

KenMaltby commented Aug 14, 2018

Something like this:
Zsync lets you "publish" the book you are reading and then be able to "subscribe to" it on other devices running KOReader. See: https://github.com/koreader/koreader/wiki/Zsync-transport

Perhaps replacing the URL with the link under "related WiKi" If we had a local html copy of the WiKi on our device, it would be the preferred linked source.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 15, 2018

newsdownloader

Retrieves RSS and Atom news entries and saves them as HTML files.

storagestat

Obsolete :)

autosuspend

Suspends the device when no interaction for some time.

kobolight

Controls the fronlight with gestures on the left border of screen.

perceptionexpander

Improves your reading speed with the help of two vertical lines over the text.

SSH

Allows connecting and transfering files to the device over SSH.

systemstat

Shows system statistics.

timesync

Synchronizes the device time with NTP servers.

backgroundrunner

Service to other widgets: allows tasks to be run regularly in the background.

coverbrowser

Alternative display modes for file browser and history.

hello

How are you?

kosync

Synchronizes your reading progess to a server and across your devices.

readtimer

Shows an alarm after a specified amount of time.

statistics

Keeps and displays your reading statistics.

terminal

Executes simple commands and shows their output.

send2ebook

Receives articles sent with the Send2Ebook PC/Android application.

calibrecompanion

Keep company to calibre :)

goodreads

Allows browsing and searching Goodreads database of books: shows cover, description... (well, you know what it does better than me :)

@robert00s

This comment has been minimized.

Contributor

robert00s commented Aug 15, 2018

Thanks for descriptions :)

@@ -240,6 +240,7 @@ BackgroundRunner:_schedule()
local BackgroundRunnerWidget = WidgetContainer:new{
name = "backgroundrunner",
fullname = _("Background runner"),
description = _([[Service to other widgets: allows tasks to be run regularly in the background.]]),

This comment has been minimized.

@poire-z

poire-z Aug 15, 2018

Contributor

Oups, I meant: Service to other plugins. Sorry.

@@ -20,6 +20,8 @@ end
local SSH = WidgetContainer:new{
name = 'SSH',
fullname = _("SSH"),
description = _([[Allows connecting and transfering files to the device over SSH.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

That'd be transferring, but I'll suggest the plainer

Connect and transfer files to the device using SSH.

name = "AutoSuspend",
name = "autosuspend",
fullname = _("Auto suspend"),
description = _([[Suspends the device when no interaction for some time.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

Suspends the device after a period of inactivity?

(Merely grammatically speaking, suspends the device when no interaction has occurred for some time.)

@@ -238,6 +239,8 @@ BackgroundRunner:_schedule()
local BackgroundRunnerWidget = WidgetContainer:new{
name = "backgroundrunner",
fullname = _("Background runner"),
description = _([[Service to other plugins: allows tasks to be run regularly in the background.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

Service to other plugins: allows tasks to run regularly in the background.

@@ -23,6 +23,8 @@ require("ffi/zeromq_h")
--]]
local CalibreCompanion = InputContainer:new{
name = "calibrecompanion",
fullname = _("Calibre companion"),
description = _([[Send documents from Calibre library directly to device via Wifi connection]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

calibre (lowercase c)
Wi-Fi (I vote wifi but at some point we decided on Wi-Fi)

@@ -9,6 +9,9 @@ local _ = require("gettext")
local NetworkMgr = require("ui/network/manager")
local Goodreads = InputContainer:new {
name = "goodreads",
fullname = _("Goodreads"),
description = _([[Allows browsing and searching Goodreads database of books.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

the Goodreads database of books

name = 'Hello',
name = 'hello',
fullname = _("Hello"),
description = _([[This is debug plugin]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

This is a debugging plugin.

@@ -22,6 +22,8 @@ local swipe_touch_zone_ratio_warmth = { x = 7/8, y = 1/8, w = 1/8, h = 7/8, }
local KoboLight = WidgetContainer:new{
name = 'kobolight',
fullname = _("Frontlight gesture controller"),
description = _([[Controls the fronlight with gestures on the left border of screen.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

frontlight

@@ -20,6 +20,8 @@ end
local KOSync = InputContainer:new{
name = "kosync",
fullname = _("Progress sync"),
description = _([[Synchronizes your reading progess to a server and across your devices.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

across your KOReader devices?

@@ -19,6 +19,8 @@ local NetworkMgr = require("ui/network/manager")
local TimeSync = WidgetContainer:new{
name = "timesync",
fullname = _("Time sync"),
description = _([[Synchronizes the device time with NTP servers.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

Synchronizes the device with with NTP servers to within a few milliseconds of UTC?

I don't know, that might still sound a bit mystical.

This comment has been minimized.

@robert00s

robert00s Aug 16, 2018

Contributor

Synchronizes the time between the device and the time on the Internet. ?

@@ -15,6 +15,8 @@ int rmdir(const char *);
require("ffi/zeromq_h")
local ZSync = InputContainer:new{
name = "zsync",
fullname = _("Zsync"),
description = _([[Devices in the same Wifi network can transfer documents between each other directly.]]),

This comment has been minimized.

@Frenzie

Frenzie Aug 15, 2018

Member

Wi-Fi

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented Aug 16, 2018

"backgroundrunner" seems to me more a meta-plugin than a plugin. Is it wise to to allow it's deactivation ?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 16, 2018

"backgroundrunner" seems to me more a meta-plugin than a plugin. Is it wise to to allow it's deactivation ?

It's used only by the AutoFrontLight plugin on Kindle Voyage and Oasis, and by the newer Kobo with warmth light (KoboPowerD:calculateAutoWarmth).
On all other devices, it's used by nothing, but it wakes koreader from its "I'm just waiting for a touch event" (so the device's cpu from its sleep state) every 2 seconds.
So, it may be not wise because of the above dependancies, but it's useful to those who don't like stuff that's not necessary.
All other plugins do nothing when enabled if you don't use them.

So, I guess these auto front light will just not work if one disables this plugin. But that's the purpose of disabling plugins :)
We could add a few words about these dependancies in the description of backgroundrunner (or the autofrontlight one ? but what about Kobo with warmths?)

I rambled on BackgroundRunner in #3078 (comment) and #3608 (comment)

@KenMaltby

This comment has been minimized.

KenMaltby commented Aug 16, 2018

Sounds like something I would want to turn off. If I don't need it, having it do anything every 2 seconds is something I would like to prevent.

@poire-z poire-z merged commit 4428ecb into koreader:master Aug 17, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 18, 2018

Mhhhh, there's something wrong with this PR.
Moving the check for disabled plugin from this first line (where it was):

if mode == "directory" and entry:find(".+%.koplugin$")
and not (plugins_disabled and plugins_disabled[entry:sub(1, -10)]) then

to after this block, you are actually loading the plugin:
if mode == "directory" and entry:find(".+%.koplugin$") then
local mainfile = plugin_root.."/main.lua"
package.path = string.format("%s/?.lua;%s", plugin_root, package_path)
package.cpath = string.format("%s/lib/?.so;%s", plugin_root, package_cpath)
local ok, plugin_module = pcall(dofile, mainfile)

(You needed that to get its name and description.)

For example, background runner now runs its stuff even if disabled, because it does schedule itself in the body of the module, and not in a function:

BackgroundRunner:_schedule()
local BackgroundRunnerWidget = WidgetContainer:new{
name = "backgroundrunner",
fullname = _("Background runner"),
description = _([[Service to other plugins: allows tasks to run regularly in the background.]]),
runner = BackgroundRunner,
}

I really don't see how to easily fix that...
The most correct thing would be to keep not loading the plugins that are disabled, but you will lose access to their name & description.
Otherwise, we'd need to go thru each plugin to make sure no real impacting code is run (except variables and function definitions, and you need the :init() to be run to get the object and name and description... and you would still load all the stuff it require() at the top...)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Aug 18, 2018

Split it up into a main.lua and a metadata.lua or something along those lines?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Aug 18, 2018

Yep, that could work (_meta.lua or _spec.lua or _plugininfo.lua then, so it's at the top in ls output and we're not confusing anything with the books' metadata.lua)

@robert00s robert00s deleted the robert00s:plugin_manager branch Aug 18, 2018

@robert00s robert00s referenced this pull request Aug 21, 2018

Closed

Plugin management #2544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment