Skip to content

Conversation

@jack9603301
Copy link
Contributor

@jack9603301 jack9603301 commented Jul 5, 2024

@jack9603301 jack9603301 force-pushed the feature/plugin-RFC branch from 5286322 to 650f7c0 Compare July 5, 2024 09:40
@mmahmoudian mmahmoudian requested a review from veracioux July 5, 2024 09:53
@jack9603301 jack9603301 force-pushed the feature/plugin-RFC branch from 0c095b4 to e9e13dc Compare July 5, 2024 10:14
@jack9603301
Copy link
Contributor Author

This framework will introduce a new dependency: yaml-cpp

@jack9603301 jack9603301 force-pushed the feature/plugin-RFC branch from e9e13dc to 0bf17e0 Compare July 5, 2024 10:32
This is to counter PR checks, because it seems that the PR's qt environment does not support some specific parameters of this function - the latest version of qt5 compiles fine on Linux

Signed-off-by: Ouyang Chunhui <[email protected]>
@jack9603301 jack9603301 force-pushed the feature/plugin-RFC branch from 0bf17e0 to 66327f9 Compare July 5, 2024 10:34
@jack9603301 jack9603301 requested a review from mmahmoudian July 5, 2024 11:03
Since the plugin manager is an experimental feature, the compilation flag is turned off by default.

Signed-off-by: Ouyang Chunhui <[email protected]>
@mmahmoudian mmahmoudian requested a review from panpuchkov July 11, 2024 10:58
@jack9603301
Copy link
Contributor Author

@panpuchkov Can you take a look at this PR?

@panpuchkov
Copy link
Contributor

If we go with a plugin system, why do we need to have multiple #ifdef USE_PRINTER_SUPPORT, it looks weird.
If we don't want to have it as a built-in option and want to have a build option to include it or not, this is the case where it should be as a plugin.

@jack9603301
Copy link
Contributor Author

If we go with a plugin system, why do we need to have multiple #ifdef USE_PRINTER_SUPPORT, it looks weird.
If we don't want to have it as a built-in option and want to have a build option to include it or not, this is the case where it should be as a plugin.

In fact, this release has two features, one is plug-in support, and the other is a small patch that supports flameshot to send images directly to the printer.

You will find that these two are independent, and you are free to start either one.

@watzon
Copy link

watzon commented Jan 13, 2025

I feel like a reason this hasn't moved is because the PR is doing 2 things. It would probably be better to implement them one at a time.

@borgmanJeremy
Copy link
Collaborator

I agree that this needs to split the plugin from the printer support. So we can talk about both of those separately. I also can't understand how this implementation actually implements plugins. Where is the spec for what goes in the yaml file? How does a user implement a plugin?

@mmahmoudian
Copy link
Member

@jack9603301 would you please separate these two parts (i.e., plugin system, and printer support) and keep in this PR the plugin system part, and put the printer in another PR so that we get the ball rolling.

@jack9603301
Copy link
Contributor Author

okay, i will

@jack9603301
Copy link
Contributor Author

I agree that this needs to split the plugin from the printer support. So we can talk about both of those separately. I also can't understand how this implementation actually implements plugins. Where is the spec for what goes in the yaml file? How does a user implement a plugin?

https://github.com/flameshot-org/plugins/blob/fa6ed42b829a1e01e4a661501c33caa272a3d282/Watermark/plugin.yaml.example

Here is the first plug-in implemented - the yaml configuration and code of the watermark plug-in

@borgmanJeremy
Copy link
Collaborator

Thanks for clarifying. I looked through that plugin and this code again and had the following thoughts:

1.) The general idea behind this seems solid. IE build a library per plugin, each plugin gets a yaml file to configure its own properties. I like all that.

2.) The plugin directory should be configurable at run time through the UI so the user can tell the application where to load plugins from.

3.) I think the way this works now is each plugin is essential part of the "save pipeline" so each effect from each plugin will be added the image before saving. I would rather have a "plugin button" that pops up all loaded plugins, and then the user can select which plugin to apply.

3a.) Those plugins can either be "final actions" like saveWithWatermark in which case they will apply whatever formatting to the image and then terminate. another example would be save to PDF or print, both of those can be final action plugins

3b.) Or they can be "image actions" like if someone wants to implement a custom arrow. In that case the plugin should return a layer with the custom arrow or whatever.

@jack9603301
Copy link
Contributor Author

Thanks for clarifying. I looked through that plugin and this code again and had the following thoughts:

1.) The general idea behind this seems solid. IE build a library per plugin, each plugin gets a yaml file to configure its own properties. I like all that.

2.) The plugin directory should be configurable at run time through the UI so the user can tell the application where to load plugins from.

3.) I think the way this works now is each plugin is essential part of the "save pipeline" so each effect from each plugin will be added the image before saving. I would rather have a "plugin button" that pops up all loaded plugins, and then the user can select which plugin to apply.

3a.) Those plugins can either be "final actions" like saveWithWatermark in which case they will apply whatever formatting to the image and then terminate. another example would be save to PDF or print, both of those can be final action plugins

3b.) Or they can be "image actions" like if someone wants to implement a custom arrow. In that case the plugin should return a layer with the custom arrow or whatever.

Good idea, this is an initial implementation of an immature plugin system, are you willing to work with us to improve it?

A complete plugin system may require a lot of APIs and integrate with flameshot in a perfect way.

@mmahmoudian
Copy link
Member

@jack9603301

A complete plugin system may require a lot of APIs and integrate with flameshot in a perfect way.

I think we can start from some parts and expose ports to plugins eventually.

I think what @borgmanJeremy suggested is a very nice separation between the two types of plugins, and we can achieve a lot by:

  1. accepting layers from plugins and displaying them (Jeremy's 3b)
  2. Sending the screenshot to plugins as final actions (3a)

For now i believe the Print and PDF export are doing the 3a. So that part is almost done. When that is merged, we can move to implement 3b.

@jack9603301
Copy link
Contributor Author

@jack9603301

A complete plugin system may require a lot of APIs and integrate with flameshot in a perfect way.

I think we can start from some parts and expose ports to plugins eventually.

I think what @borgmanJeremy suggested is a very nice separation between the two types of plugins, and we can achieve a lot by:

  1. accepting layers from plugins and displaying them (Jeremy's 3b)
  2. Sending the screenshot to plugins as final actions (3a)

For now i believe the Print and PDF export are doing the 3a. So that part is almost done. When that is merged, we can move to implement 3b.

Yes, the key is that 3b also has its second item, which requires modifying the core code to execute the plug-in logic more deeply. We may also need to add some general plug-in logic and interfaces so that the plug-in manager can distinguish the types of different plug-ins, and then intelligently execute various logics based on specific types and interface definitions - because plug-ins will become more and more complex. This is essentially another implementation of Windows COM component technology. The role of the plug-in manager means that its logic will become more and more complex.

My ability is limited, and I can't spend all my time here, so I need the help of developers and other senior developers.

@borgmanJeremy
Copy link
Collaborator

Yeah I can help. Give me a few days to draw it out in more detail.

@jack9603301
Copy link
Contributor Author

Yeah I can help. Give me a few days to draw it out in more detail.

sure, thanks you!

@mmahmoudian
Copy link
Member

@jack9603301

Yes, it is a starting point, but there is still a lot of implementation to be done

Yes, but as you said, this is one of the largest additions to this project, so we can release it as experimental and have it disabled by default. This way we can gradually implement different parts.

I think what we need now is a clear plan. The RFC needs to be revised to reflect the discussions here and in the original RFC, in order to show us the path. Also, let's wait for @borgmanJeremy opinions and if he has sketched something.

I truly like to see this plugin system because it will give us more opportunity to focus on screenshot rather than imgur issues. It would be super extra great if we can support Python and Lua to lower the barrier to entry for users.

@jack9603301
Copy link
Contributor Author

Yes, but as you said, this is one of the largest additions to this project, so we can release it as experimental and have it disabled by default. This way we can gradually implement different parts.

Yes, I agree

I think what we need now is a clear plan. The RFC needs to be revised to reflect the discussions here and in the original RFC, in order to show us the path. Also, let's wait for @borgmanJeremy opinions and if he has sketched something.

Yes, let's wait.

@tessus
Copy link

tessus commented May 19, 2025

@mmahmoudian

I assume you are referring to endpoint plugins?

Yes, you were talking about augementation and final actions. But then you had 2 more categories for the final actions: middle and endpoint.

Also, should we disable the endpoint plugins if the user has already specified a final action in CLI? For example flameshot gui --clipboard.

IMO it is a problem to define a plugin as an endpoint or middlepoint plugin. The term final action is also not entirely correct. I would just put them in 2 categories:

  • augementation (layer)
  • action

What kind of action it is is defined by the user. e.g. the watermark plugin could be a middle point or endpoint.

So, if a user clicks on the watermark icon (which will be presumeably available similar as the other icons around the screenshot area), the watermark action is started right away. And afer the action is done, a user still could click other actions icons like save or upload.
If the action is added to the sidebar pipeline, it will be executed in the order it was set when clicking on a Go button to process the pipeline.

I haven't looked at the cli what --clipboard does. Does it mean the user can annotate the screenshot or does it mean that as soon as an area is selected it is put in the clipboard?
If I had designed flameshot it would only mean that after the flameshot screenshot window/overlay is closed, the image is added to the clipboard. In such a case, nothing changes either. A user can still apply a bunch of actions before closing that overlay window.
But as you said, this can be a separate discussion.

As for unique names. This can become a complex topic, but nonetheless is rather important. In other projects, plugin names might not be unique, but their id are. e.g. in Joplin, plugin ids follow the convention com.example.plugin, must be at least 16 characters long, without whitespace characters, and unique.
It doesn't really matter what you do, but you still have to follow some sort of convention. e.g. from your cli example, the "names" seem to be lowercase (or case insensitive), without spaces, and cannot contain a ,. Does this also mean it's the same name as the shared object file? There are certainly a few things to consider... ;-)

@jack9603301
Copy link
Contributor Author

I haven't looked at the cli what --clipboard does. Does it mean the user can annotate the screenshot or does it mean that as soon as an area is selected it is put in the clipboard?

--clipboard Copy the image to the clipboard

If I remember correctly

As for unique names. This can become a complex topic, but nonetheless is rather important. In other projects, plugin names might not be unique, but their id are. e.g. in Joplin, plugin ids follow the convention com.example.plugin, must be at least 16 characters long, without whitespace characters, and unique.
It doesn't really matter what you do, but you still have to follow some sort of convention. e.g. from your cli example, the "names" seem to be lowercase (or case insensitive), without spaces, and cannot contain a ,. Does this also mean it's the same name as the shared object file? There are certainly a few things to consider... ;-)

Well, a new requirement, we need a unified plugin management database? Maybe plugin.yaml under the plugin directory should be used to define plugin meta information, and dynamic configuration should be placed in another place?

@borgmanJeremy
Copy link
Collaborator

Here are some initial thoughts. As I was writing this I realized we need more prototyping to really understand how this will work in practice.

Plugins

A plugin .yaml file defines where to find the plugins, their icons, and the plugin type. This tells flameshot where to find the plugins, what icon to use to display, and what hover text to display.

Each plugin may have its own config, but management of that config is left to the plugin developer. I reccomend defaulting the search path for plugin configs to ~/.config/flameshot/plugins

---
plugins:
- name: zigzagarrow
  type: augmentation
  hover_text: An arrow with zig zags
  path: /path/to/plugin.so

- name: imguruploader
  type: final_action
  hover_text: Uploads the image to imgur
  path: /path/to/plugin.so

The flameshot UI will have a new "plugin button" as sketched with the "P" below

image

When a user clicks the plugin button, the existing menu will be replaced with menu icon's for all the plugins configured in plugins.yaml as shown below. There will be a back button that restores the main menu.

A divider can be used to differential augmentation vs final action plugins.

image

Plugin requirements

Plugins will be implemented in C++ and Qt. Final action plugins will be passed a const QPixmap&. It is required to return a boolen indicating success or failure of the final action.

An augmentation plugins will likely require a refactor of the existing tool framework. Conceptually it will be required to implement a process() function like every other tool, but it will take some thinking and prototyping to figure out how to do this.

Known issues

  1. Using C++ as an interface to plugins is probably a bad idea in the long term since it does not have a stable ABI. However, I think its the fastest way to start prototyping the plugin system. Once we have a good idea of how plugins really work we can move to a C api.

  2. Plugins need to be recompiled anytime flameshot is building with an updated Qt or compiler version. So if a user installs flameshot via a package manager but compiles thier own plugins, when they update via the package manager they may need to recompile the plugins if the Qt version is updated.

  3. There is currently no tie-in to the CLI for final action plugins. I think that should stay out of scope for round 1 of plugins

  4. There is no requirement to tie into the upload history widget. I think we can address that in future updates after we iron out how plugins work in practice.

@tessus
Copy link

tessus commented May 20, 2025

final_action

I really would like to stress again that final_action is the wrong term, especially if you want to run multiple actions in a pipeline. It makes no sense to run several "final" actions in sequence. ;-)

As explained here #3661 (comment), they should rather be called action. Unless I misunderstood the entire plugin framework and what it is supposed to do.

@jack9603301
Copy link
Contributor Author

@borgmanJeremy One question is: where is the plugin configuration stored? Maybe we should differentiate between metadata.yaml and dynamic configure?

@jack9603301
Copy link
Contributor Author

  1. Using C++ as an interface to plugins is probably a bad idea in the long term since it does not have a stable ABI. However, I think its the fastest way to start prototyping the plugin system. Once we have a good idea of how plugins really work we can move to a C api.

Seriously, I agree, the C API is more stable

C++ API is not necessarily unfeasible, perhaps we can refer to the implementation of COM

@borgmanJeremy
Copy link
Collaborator

@tessus "final action" is what I am proposing. It is the same as save to file, upload to imgur, copy to clipboard etc. All those actions terminate the CaptureWindow when activated.

I don't think we need to support a pipeline of those actions. If someone wants to save to file, and upload to imgur, their plugin can do both those actions. A watermark plugin should be an augmentation.

@jack9603301 The config for plugins can also go in .config/flamshot. Something like:

.
├── flameshot.ini
├── plugins
│   └── imguruploader
│       ├── config.yaml
│       ├── imguruploader.so
│       └── imguruploader.svg
└── plugins.yaml

@jack9603301
Copy link
Contributor Author

jack9603301 commented May 20, 2025

I don't think we need to support a pipeline of those actions. If someone wants to save to file, and upload to imgur, their plugin can do both those actions. A watermark plugin should be an augmentation.

@jack9603301 The config for plugins can also go in .config/flamshot. Something like:

.
├── flameshot.ini
├── plugins
│   └── imguruploader
│       ├── config.yaml
│       ├── imguruploader.so
│       └── imguruploader.svg
└── plugins.yaml

This seems like a good directory layout, or we could consider:

├── flameshot.ini
└── plugins
    ├── com
    │   └── pub1
    │       └── imguruploader
    │           ├── imguruploader.so
    │           ├── imguruploader.svg
    │           └── metadate.yaml
    └── runtime.dat

runtime.dat: A sqlite2 database for managing dynamic information of plugins
metadate.yaml: Plugin-specific static metadata configuration
com.pub1.imguruploader: Format based on Java package naming standards

"final action" is what I am proposing. It is the same as save to file, upload to imgur, copy to clipboard etc. All those actions terminate the CaptureWindow when activated.

The execution flow of multiple plug-ins may become a powerful feature, because when the customer's functions can be completed by executing existing plug-ins in sequence, there is no need for users to write duplicate plug-ins for their needs.

@borgmanJeremy
Copy link
Collaborator

I really don't want to complicate this with a sql database of any sort.

The java naming standard is fine with me for the plugin names.

I don't disagree on the pipeline, but let's not complicate this more than needed to start. This is already more work than I think is feasible haha.

@jack9603301
Copy link
Contributor Author

I really don't want to complicate this with a sql database of any sort.

The java naming standard is fine with me for the plugin names.

I don't disagree on the pipeline, but let's not complicate this more than needed to start. This is already more work than I think is feasible haha.

We can implement a simple version first, but the key is how to store the runtime configuration? If not using a sql database?

@borgmanJeremy
Copy link
Collaborator

What do you mean by runtime configuration? The order of the icons is defined by the order of the definition in the yaml file.

@jack9603301
Copy link
Contributor Author

What do you mean by runtime configuration? The order of the icons is defined by the order of the definition in the yaml file.

Each plugin may have its own configuration information, and different configurations may be required according to different needs, and even different configurations for each customer. Therefore, perhaps we need a more flexible way to manage these plugin configurations. If we plan to do an execution flow mechanism later, the same principle applies. In short, the flameshot plugin system needs to manage these user configurations.

@borgmanJeremy
Copy link
Collaborator

Each plugin gets its own folder in .config, each plugin manages its own configuration (I would recommend a yaml file)

@jack9603301
Copy link
Contributor Author

Each plugin gets its own folder in .config, each plugin manages its own configuration (I would recommend a yaml file)

flameshot write dynamic user configuration to medadate.yaml?

@mmahmoudian
Copy link
Member

mmahmoudian commented May 21, 2025

I agree with @borgmanJeremy that SQLite might be an overkill. Imho each plugin is responsible for their internal configuration. They can chose to store it in yaml or xml or sqlite. That's their choice. We can recommend the plugin devs to stick to yaml. As for metadata, I think yaml is very suitable.

The execution flow of multiple plug-ins may become a powerful feature

I second this. It would be a game changer and it would make the plugins less bloated, and they can stick to unix philosophy of doing one thing but doing it well.

I don't disagree on the pipeline, but let's not complicate this more than needed to start. This is already more work than I think is feasible haha.

I suggest not implementing it right-away in the first phase, but keep enough room in the logic of the code to be able to implement it in later phases.

I really would like to stress again that final_action is the wrong term, especially if you want to run multiple actions in a pipeline. It makes no sense to run several "final" actions in sequence. ;-)

I can see the point @tessus is making, but we already are using the term in the CLI and so far we didn't get any feedback about the naming as far as I can recall. Nonetheless, we can change the name to something better when we get one. For instance we can survey users on Fediverse via our Mastodon account and see which one they pick most.

P.s., thanks everyone for the enlightening discourse. 🤓

@jack9603301
Copy link
Contributor Author

I agree with @borgmanJeremy that SQLite might be an overkill. Imho each plugin is responsible for their internal configuration. They can chose to store it in yaml or xml or sqlite. That's their choice. We can recommend the plugin devs to stick to yaml. As for metadata, I think yaml is very suitable.

It seems like a good idea. We can provide APIs to load configuration information from plugins or write configurations. The specific behavior is done by the plugins themselves, but in order to achieve interoperability, this means that the flameshot core needs to expose some APIs for plugins to call to obtain operation permissions and interoperate!

@jack9603301 jack9603301 marked this pull request as draft May 21, 2025 18:50
@jack9603301
Copy link
Contributor Author

@borgmanJeremy @tessus any updates?

@tessus
Copy link

tessus commented May 25, 2025

@jack9603301 not from me. I have already expressed my ideas (and my dislike of the term final_action for something that is in fact not final. ;-)). I agree with anyone who wants to get started and then go from there. however, a few things should be clarified upfront so that future development doesn't require a complete rewrite. But I am sure the project devs have thought of that already.

@borgmanJeremy
Copy link
Collaborator

For me the idea of plugins is great. And then the implementation becomes extremely complex so I lose excitement. I'd like the implementation to be as simple as possible in core flameshot. So the idea of a permissions manager seems like too big of a scope.

I'd like to first focus on the "augmentation" plugins as those for better with (my personal) core ideals for what flameshot should be.

@jack9603301
Copy link
Contributor Author

For me the idea of plugins is great. And then the implementation becomes extremely complex so I lose excitement. I'd like the implementation to be as simple as possible in core flameshot. So the idea of a permissions manager seems like too big of a scope.

I'd like to first focus on the "augmentation" plugins as those for better with (my personal) core ideals for what flameshot should be.

Yes, we need some people to focus on implementing plugins, although this is very complex, complexity is determined by the characteristics of plugins

@mmahmoudian

@mmahmoudian
Copy link
Member

I'm not a very skillful C++ programmer, and I've never coded a plugin system, so my point of view is mainly from a user perspective.

I suggest starting from a core system that can accept augmentationa for now, but we leave some room for the "final actions" (or whatever name we at the end come up with that suite better). For this, I suggest the following:

  1. Let's discuss the plugin system as a whole so that we can outline the general design and use-cases
  2. Only select a small subset (e.g., adding an svg as augmentation) and focus on implementing that
  3. Break this into sub-tasks so that multiple devs can work on different parts and we distribute the load (e.g., validation of plugins and settings, the core plugin system that triggers the plugin, the plugin itself to generate the layer based on the config)

I believe @borgmanJeremy and @jack9603301 are both positive on the plugin system and are willing to work on implementation. @tessus are you interested in taking part in this or would you rather participate in the discussions? I would also tag @FelixJochems and @b0ch3nski in case they are also interested in this.

Considering my lack of technical skills that this requires, I would personally follow the steps and path that @borgmanJeremy defines and I will try to provide as much support as I can.

Perhaps if the tasks are well defined, more C++ devs can join this cause by providing patches and PRs.

@tessus
Copy link

tessus commented May 25, 2025

I have written plugin systems in C before, but not in C++. Additionally I haven't really written code professionally in a while, especially since I moved into people management years ago. Thus I'd rather stick to participating in the discussion.

@jack9603301
Copy link
Contributor Author

RFC: #3953

@jack9603301
Copy link
Contributor Author

I have written plugin systems in C before, but not in C++. Additionally I haven't really written code professionally in a while, especially since I moved into people management years ago. Thus I'd rather stick to participating in the discussion.

New implementation: Will be separated from Qt plugin
The C+ interface must be obtained from the C interface

@tessus
Copy link

tessus commented Jun 29, 2025

@mmahmoudian when it comes to custom uploaders, maybe adding support for custom uploader profiles like ShareX or ishare is easier than using plugins. E.g. one can define a custom uploader and mark it in the configuration screen to be used when clicking the upload icon. The response payload (and/or a field, if the response is a json payload) is then presented to the user with the option to copy it to the clipboard.
Since you already have the code for the upload to imgur, adding such custom uploader profiles might not be as complicated.

What do you think?

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.

7 participants