Skip to content

add provider module#12641

Merged
pazos merged 4 commits intokoreader:masterfrom
pazos:provider
Dec 18, 2024
Merged

add provider module#12641
pazos merged 4 commits intokoreader:masterfrom
pazos:provider

Conversation

@pazos
Copy link
Copy Markdown
Member

@pazos pazos commented Oct 14, 2024

Some background in #7279

  • Implements a Provider singleton that plugins can use to extend supported features

Thirdparty plugin developers can use:

local Provider = require("provider")
Provider:register("exporter", "my-service", my-implementation)

where my-implementation is a table that implements the interface for an exporter target.

  • Makes the exporter plugin compatible with thirdparty providers

  • Splits plugin loading into two steps: discovery and load. That way we get a list of all candidate plugins to load for the different paths and we sort those starting with "provider" to be load before the rest of them.


This change is Reviewable

@pazos pazos marked this pull request as draft October 14, 2024 18:41
@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Oct 14, 2024

Seems fine to me.

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Oct 16, 2024

pinging you, @poire-z: your comment in #7279 (comment) mentions "translator" as a potential usage.

How do you envision "extensible" interfaces that are part of core?. I mean the exporter plugin use their "extensions" as targets, the cloud storage/sync could use them as account types.

Does make sense to have generic settings as part of this Provider interface?

For instance: somebody uses google translate, installs provider-microsoft-translator.koplugin. The next time it requests a translation it should be prompted or the program needs to make some guess between available options.

The UI seems generic because it's always "select one among providers of this feature" so it could be something like UIManager:show(Provider:settings(feature)) wired up when there's more than one implementation for a core feature.

Anyhow this is all in theory. "translator" isn't a feature this PR is aware yet :)

@poire-z
Copy link
Copy Markdown
Contributor

poire-z commented Oct 16, 2024

mentions "translator" as a potential usage.
How do you envision "extensible" interfaces that are part of core?.

Sorry, I don't have the brain bandwidth at the moment to envision anything :)
(Moreover when this is not really an issue, and just you wishing to build a better world :) a wish that will soon vanish when the real world will strike again :))

For Translator, I think anybody could just plug their preferred service with just dropping a user-patch that would override some functions in translator.lua.
If we were to allow plugins for that, they could do the same: replace core functions.
As for having a UI and generic infrastructure to handle all that (listing available translator plugins, enabling (only one of them), or enabling multiple ones per language), that's a bit too much thinking needed.
(And user patches were created for that: decreasing the amount of core thinking :))

I understand this is quite a bit more needed for exporter plugins, as everybody has its own favorite service - and it's a bit more interacting with everything, so the need for some well thought core infrastructure.

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Oct 22, 2024

Sorry, I don't have the brain bandwidth at the moment to envision anything :)

My bad. I've shall call it "think", which everybody can do even on dial-up brain bandwidth :)

(Moreover when this is not really an issue, and just you wishing to build a better world :) a wish that will soon vanish when the real world will strike again :))

My little better world is almost built :) I just want a to do a little effort that would allow us to remove unsupported/unneeded/unwanted features.

Patches are great for features that users want but we don't want to support.
But for features that were there and are no longer there a patch is an overcommitment :)

I understand this is quite a bit more needed for exporter plugins, as everybody has its own favorite service - and it's a bit more interacting with everything, so the need for some well thought core infrastructure.

Exporters already inherit a callback for common exporter settings. Those exporters that have no specific settings use the callback directly. Exporters with more configuration override the method and implement their own thing.

But you're totally correct. Maybe I'm overthinking and none of the "core" things need to be done now :/

@pazos pazos force-pushed the provider branch 2 times, most recently from 75460df to 24556fa Compare October 31, 2024 17:06
@pazos
Copy link
Copy Markdown
Member Author

pazos commented Oct 31, 2024

Moved exporter koplugin to providers.
Added a plugin to showcase a provider-x.koplugin

@Frenzie Frenzie added this to the 2024.11 milestone Oct 31, 2024
local WidgetContainer = require("ui/widget/container/widgetcontainer")

-- this is a hack, remove me
package.path = "plugins/exporter.koplugin/?.lua;" .. package.path
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure how to tackle this.

Is it too bad to include it on our own package.path?

The other way is to move exporter.koplugin/base.lua to frontend/export.lua or something similar.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't it go at the end if done this way?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Shouldn't it go at the end if done this way?

😄 No idea.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mainly mean in the sense that with a path override you want to put things in front, but we're just adding here rather than overriding.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yay!

I ended up adding it in setupkoenv.lua.

Moving the base exporter itself to frontend isn't enough, as exporters might also use one of the templates (markdown, html). Moving everything to base sounds a bit crazy.

Conditionally add the path when the exporter plugin is enabled isn't as option. Providers are independent plugins that can be enabled on their own.

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Nov 4, 2024

Issues so far:

  • Loading FM and reader instances: A plugin might be loaded before providers.

It 's workarounded in the exporter plugin by replacing self.targets with a function that generates those targets on demand.

I'm wondering if it makes sense to load provider- plugins before the rest of them.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 5, 2024

I'm wondering if it makes sense to load provider- plugins before the rest of them.

Probably easier than coming up with a proper dependency resolver ;o).

@pazos pazos force-pushed the provider branch 2 times, most recently from 0dd2697 to 6ae781d Compare November 5, 2024 20:10
@pazos
Copy link
Copy Markdown
Member Author

pazos commented Nov 6, 2024

Probably easier than coming up with a proper dependency resolver ;o)

It turns it isn't as easier as I thought.

The loader iterates lookup paths, fetching each koplugin dir inside and trying to loading it.

For providers to take precedence over the rest of plugins we would need to split "plugin discovery" from "plugin load", generate a table of candidates to load, sort providers first, and finally load plugins.

TBH too much work for what we get :(. I think current behaviour isn't too bad (at least for exporter and cloud/sync).

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Nov 6, 2024

It turns it isn't as easier as I thought.

It turns out I was wrong :p

@pazos pazos force-pushed the provider branch 2 times, most recently from 5761b5c to e883e98 Compare November 6, 2024 17:28
@pazos pazos force-pushed the provider branch 4 times, most recently from f304768 to 3dd5c27 Compare November 27, 2024 16:25
@pazos
Copy link
Copy Markdown
Member Author

pazos commented Nov 27, 2024

Ready for review, updated OP

@pazos pazos marked this pull request as ready for review November 27, 2024 16:25
@pazos pazos force-pushed the provider branch 2 times, most recently from 964af99 to cea7762 Compare December 3, 2024 00:07
@pazos pazos changed the title RFC: basic provider/consumer infrastructure add provider module Dec 3, 2024
@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Dec 3, 2024

@pazos It looks tidy to me. The CI still has some minor whitespace complaints. Btw, did you see my message on Gitter?

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Dec 3, 2024

@pazos It looks tidy to me. The CI still has some minor whitespace complaints. Btw, did you see my message on Gitter?

Sorry. I replied now.

@hius07
Copy link
Copy Markdown
Member

hius07 commented Dec 5, 2024

if Provider:size("exporter") > 0 then
local tbl = Provider:getProvidersTable("exporter")
for k, v in pairs(tbl) do
t[k] = v
end
end

This chunk is expected to be duplicated in all generic plugins. Can be moved to Provider, receiving name and t as args.

@pazos
Copy link
Copy Markdown
Member Author

pazos commented Dec 11, 2024

Waiting a bit for potential reviewers. I'll plan to merge the friday unless somebody says something ;)

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Dec 11, 2024

It looks sane to me like before. Not much else to add. ^_^

@pazos pazos merged commit e503cc4 into koreader:master Dec 18, 2024
@pazos pazos deleted the provider branch December 18, 2024 18:40
pazos added a commit to koreader/contrib that referenced this pull request Dec 18, 2024
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.

5 participants