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

RFC for upload plugins #955

Closed
wants to merge 1 commit into from
Closed

RFC for upload plugins #955

wants to merge 1 commit into from

Conversation

borgmanJeremy
Copy link
Contributor

No description provided.

@borgmanJeremy
Copy link
Contributor Author

Closes #45
Closes #50
Closes #159
Closes #188
Closes #348
Closes #377
Closes #458
Closes #547
Closes #608
Closes #777

@hosiet
Copy link
Member

hosiet commented Sep 22, 2020

Some direct thoughts after reading:

"One python script for one upload service" is indeed a fascinating idea. However, we all know that the reality is always more complicated. If flameshot were only to support Linux platform and use interpreter/interpreter library from host system, I will support such idea. However, considering that we are also doing Windows (and other generic Linux packages), it would be more problematic.

  • We not only need to embed a full python(3) interpreter: we must embed the whole Python standard library as well to support functions like network connection. Distributing flameshot with Qt5 is already making the installer program large, now think about adding a whole Python...
  • What would happen if the user-written plugin needs some 3rdparty python library (like requests)?

The most conservative and well-proven plan is surely using so/dll plugin with dlopen(). If we worry about ABI compatibility, we just define pure C interface functions.


My first thought on plugin system with script is to use Lua, which is designed to be embedded scripting language in the first place. However, it is expected that most users are not very familiar with it. The applicability on network communication is also doubtful.

@borgmanJeremy
Copy link
Contributor Author

@hosiet

* We not only need to embed a full python(3) interpreter: we must embed the whole Python standard library as well to support functions like network connection. Distributing flameshot with Qt5 is already making the installer program large, now think about adding a whole Python...

* What would happen if the user-written plugin needs some 3rdparty python library (like `requests`)?

Actually as flameshot we do not need to distribute python or 3rd party add-ons. On both linux and windows the python interpreter needs to be found in the users path. For external libraries the user/script author is responsible for that. Flameshot will simply look in standard places: https://docs.python.org/3/c-api/intro.html#embedding-python

For instance on windows a user could install the 3rd party libraries with Conda or however they normally get packages.

I do see your point that for Windows users initial installation will be troublesome as they will need to navigate to a website and download python manually which is a hassle. This is a very valid critique I want to spend more time on to see what other windows programs do to make this easier for users.

The most conservative and well-proven plan is surely using so/dll plugin with dlopen(). If we worry about ABI compatibility, we just define pure C interface functions.

Yeah I think if it does not work out with python or I get significant feedback that python is the wrong way to go, I will do the standard dlopen() with a C interface.

My first thought on plugin system with script is to use Lua, which is designed to be embedded scripting language in the first place. However, it is expected that most users are not very familiar with it. The applicability on network communication is also doubtful.

Yes I considered Lua since it is specifically meant for embedding, but the idea that users could very easily adjust thier upload scripts made me choose python over it.

@Jieiku
Copy link

Jieiku commented Sep 22, 2020

If this means I might be able to upload to my nextcloud with flameshot, then I am very excited to try this out! Let me know if you need any help testing. I use flameshot on both Linux and Windows and do not mind installing and configuring stuff.

Currently I take a picture with flameshot
open web browser and navigate to my clouds picture folder
upload the picture to nextcloud
copy the share link in nextcloud so that I can paste it to friend/associate

being able to just take a picture with flameshot and hit the upload to nextcloud button and have the share link copied to my clipboard would be killer!

@mmahmoudian
Copy link
Member

First things first, disclaimer: I'm no expert as I've never wrote/implemented a plugin system. So the following is just my understanding from the plugin developer's point of view.

This is a nice step forward but for sure it is a relatively hard one as it brings a whole new set is issues with it.

Now to some of the points of the RFC and @hosiet's points:

  • I think the plugin should return more than a binary. We might want it to return signed integers, and dedicate positive integers to progress percentage, and negative integers to error codes (as there are so many ways this can fail and user should be able to correctly report the error when filling up the bug report.

  • It was not clear in the text if we are shipping all the plugins with Flameshot or there will be a plugin loaded that user can load it into the Flameshot later. An alternative way it to do something similar to omyzsh (have a folder and every plugin goes there manually), or alternatively vim-plug (as reads a section in config file and automatically fetch the plugin from github and place it in specified folder).

  • Regarding the language (Python, Lua, ...) I agree that the majority of people know python and that would be a nice choice compared to Lua or C++, as it would attract more community engagement. Also if I'm running a script on my computer I prefer to read it first and know it doesn't do nasty stuff (despite of having sandboxing). so an interpreting language imho is better than a compiled one.

@seamus-45
Copy link
Contributor

seamus-45 commented Sep 22, 2020

This is a very valid critique I want to spend more time on to see what other windows programs do to make this easier for users.

I saw couple of installers, which ask user to install extra software for extended functionality, like Python interpreter.

Yeah I think if it does not work out with python or I get significant feedback that python is the wrong way to go, I will do the standard dlopen() with a C interface.

What about ShareX-like approach? Their custom uploaders looks very well. Might be not such flexible as Lua or Python could be, but easy to use for end user.

@borgmanJeremy
Copy link
Contributor Author

First things first, disclaimer: I'm no expert as I've never wrote/implemented a plugin system. So the following is just my understanding from the plugin developer's point of view.

Thanks @mmahmoudian, let me answer some of the questions.

This is a nice step forward but for sure it is a relatively hard one as it brings a whole new set is issues with it.

Now to some of the points of the RFC and @hosiet's points:

* I think the plugin should return more than a binary. We might want it to return signed integers, and dedicate positive integers to progress percentage, and negative integers to error codes (as there are so many ways this can fail and user should be able to correctly report the error when filling up the bug report.

Sure, we can certainly return more. We can either do error codes or an error string.

I don't think its worth the complexity of an upload progress bar. This would require constant communication between the plugin thread and the main GUI thread. The file's are also relatively small so it should not take too long.

* It was not clear in the text if we are shipping all the plugins with Flameshot or there will be a plugin loaded that user can load it into the Flameshot later. An alternative way it to do something similar to omyzsh (have a folder and every plugin goes there manually), or alternatively vim-plug (as reads a section in config file and automatically fetch the plugin from github and place it in specified folder).

My intent is to provide sample plugins in the actual flameshot installation. Then users can manually drop extra plugins into this folder similar to omyzsh. We could also keep a community-plugin repo as an easy place to find reviewed plugins.

My main goal with this architecture is to allow flexibility for upload services but not have a large maintenance burden in the c++ code. I fear if we implement this as a core feature in c++ we will spend too much time updating this and adding new features.

* Regarding the language (Python, Lua, ...) I agree that the majority of people know python and that would be a nice choice compared to Lua or C++, as it would attract more community engagement. Also if I'm running a script on my computer I prefer to read it first and know it doesn't do nasty stuff (despite of having sandboxing). so an interpreting language imho is better than a compiled one.

Yes, and the other main thing with sandboxing in a scripting language is a bad plugin will not crash the entire application. I do not want bad community plugins to result in a large number of issues reported against flameshot.

@holazt
Copy link
Collaborator

holazt commented Sep 22, 2020

First of all Core + Plugins is designed to be a great idea. But I think no matter how we design a plugin system, we have to make sure that Core is robust, extensible, and stable. Plug-ins are just extensions of its functionality.

@Martin-Eckleben
Copy link
Collaborator

Martin-Eckleben commented Sep 22, 2020

Hmm
First thing coming to my mind is: this sounds like every low level user is gonna write his own plugins for every API.
In reality I think only a very, very small amount of users will write custom plugins for their own niche API's.

We will probably step by step "collect" Plugins contributors have written for the big API's and bundle the biggest and reference smaller ones?
Not developing them but kind of "manage" the biggest.

I'm pretty sure that's what everybody already talks about?

As for C++ vs Python I'm torn back and forth.

Urging all low level users to install Python is heavy - thinking about non tech people I know that feels like a really bad idea.
If so we should at least make the Python install process as easy as possible like @seamus-45 suggested in his comment.

C++ for the 0,001% of people writing a custom upload plugin is not a no-go I think.
With a good guide and examples (as most plugins will do standard web requests) definitely not unreasonable considering that major APIs will be already connected I think.

The ShareX approach mentioned by @seamus-45 which is simply put "providing config for webcalls" sounds actually best to me?
Everything is becoming web protocols - especially connecting web APIs :)
(probably I like this most as I'm a webdev and don't see any API being important or standard that is not HTTPS :D )

@borgmanJeremy borgmanJeremy linked an issue Sep 23, 2020 that may be closed by this pull request
@mmahmoudian mmahmoudian linked an issue Sep 23, 2020 that may be closed by this pull request
@erickskrauch
Copy link

@Martin-Eckleben, don't forget about sftp :)

I think that the API in C++ will be better than the Python dependency, because on Windows it seems to be something alien. With the C++ API a plugin can be written in any programming language by the Swig (even in PHP, lol).

@l29ah
Copy link

l29ah commented Nov 4, 2020

However this is not easy for many end users

I don't see how C++ or Python API makes it inherently easier.

and not cross platform.

Can you name a platform supported by flameshot but lacking std{in,out} or argv[]?

@borgmanJeremy
Copy link
Contributor Author

The intention of the latest implementation is to allow users to load a plug-in that will integrate with the GUI so they need not interface with an API at all

@sergix44
Copy link

sergix44 commented Nov 4, 2020

I still think that a declarative approach like ShareX does is the easiest for the end user like I've mentioned before, a python API is how Screencloud for example support external uploaders, but I found annoying start to develop a plugin for basically do a post request.

Would be great to have some kind of configuration file like ShareX: I'm the creator of XBackBone, that's basically a backend for screenshot/sharing tools. For example, to support sharex it's a simple json file (https://github.com/ShareX/CustomUploaders/blob/master/file.io.sxcu) that contains where to send the request, with params, etc and how to interpret the response (getting the url). This, for example, is how XBackBone supports those clients: https://xbackbone.app/clients.html

You can use the script in combination with the "Open with.." of flameshot, but I don't think is a robust system.

@msva
Copy link

msva commented Nov 5, 2020

You can use the script in combination with the "Open with.." of flameshot, but I don't think is a robust system.

That's why it would be brillint, if flamrshot would have something like that:
a) either a config somewhere inside ~/.config (XDG_CONFIG_DIR), where user can specify stdint-out-uploaders like:

[uploaders]
UploaderName1=command one
UploaderName2=command --two -with some_args

b) or list the contents of ~/.local/share/flameshot/uploaders

and load the list in "upload with" menu, executing specified command as upload command when user selects the one.

Maybe, this "upload with" menu would be a plugin itself, but it would be ideal if it will be in flameshot out-of-the-box

@trentwiles
Copy link

Would love something like this as there are very few decent screenshot programs for linux and this is one of the few good ones.

@rajeshisnepali
Copy link

image
Option to map shortcut with key_up & key_down key 😞

@hosiet
Copy link
Member

hosiet commented Feb 15, 2021

@rajeshisnepali your comment seems completely offtopic and unrelated with this pull request. If you have any suggestions, please open a separate issue.

@skorokithakis
Copy link

I agree with @l29ah, all you need is the ability to call a program and read its stdout and stderr, as I described in #1463.

@mmahmoudian
Copy link
Member

@borgmanJeremy I think #178 also can be addressed by this RFC as it fits the general theme as far as I understand it.

@pitkes22
Copy link

pitkes22 commented Dec 1, 2021

Why was this closed?

@borgmanJeremy
Copy link
Contributor Author

I think GitHub discussion s will work better. I'll have a formal proposal in a day or two. We've been discussing it on matrix and slack.

@dgw
Copy link
Contributor

dgw commented Dec 1, 2021

I'll have a formal proposal in a day or two.

Look forward to seeing it, even though I'm not using flameshot on my current system. Drop a link/x-ref here when the thread is up, would you? Cheers! 🙏

@wolandtel
Copy link

As a temporary workaround could you add service url to settings? I need to upload images to non-public cloud. I just could implement imgur api.

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