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

Add --pin, --upload, --accept-on-select to CLI #1970

Merged
merged 17 commits into from
Oct 20, 2021

Conversation

veracioux
Copy link
Contributor

@veracioux veracioux commented Oct 12, 2021

TODO:

  • Open file dialog when --accept-on-select is the only option to flameshot gui
  • --print-geometry not working

Closes #526.
Closes #1956.
Closes #1964.
Closes #1920.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@veracioux veracioux marked this pull request as draft October 12, 2021 23:44
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@veracioux veracioux marked this pull request as ready for review October 15, 2021 08:06
@veracioux
Copy link
Contributor Author

@mmahmoudian Once you are able, test this and I'll merge.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@veracioux
Copy link
Contributor Author

@mmahmoudian I forgot to tell you this. You can test everything by running the tests/action_options.sh script. All the instructions should be in the script. While running it, make sure to check both the CLI messages and the notifications from the script and from flameshot.

Please don't bother to make any readability improvements, since I plan to change it and automate it one day.

@borgmanJeremy borgmanJeremy self-requested a review October 17, 2021 14:38
@mmahmoudian
Copy link
Member

mmahmoudian commented Oct 18, 2021

I love it. Works very nicely. Thank you.

While testing and exploring the horizons this PR (42fa9e0) opens, I realized:

  1. The following will never push anything to stdout. As the matter of fact, it will get stuck in Flameshot process and it will no end:
flameshot gui --accept-on-select --raw --delay 5
  1. when using the following, a strange thing happens. The image gets uploaded, URL ends up in the clipboard (I was testing the race to see which one will fill the clipboard and it turns out the URL overwrites), but the "Latest uploads" is empty!
flameshot gui --accept-on-select --upload --clipboard

The following is the URL that now I cannot delete form imgur:
https://i.imgur.com/0v0c9t1.png

I think we should not limit the user by making the --clipboard and --upload mutually exclusive, but we should perhaps warn the user as if they don't have clipboard manager (like me), they will lose their screenshot from their clipboard.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@veracioux
Copy link
Contributor Author

@mmahmoudian

  1. Fixed the --delay problem.

  2. When I try

flameshot gui --accept-on-select --upload --clipboard

the "Latest uploads" entry is created just fine. I tried to upload in all the variants I could think of (various option combinations, from gui without CLI options) and it still works. Please try it again - maybe it was a temporary connection problem or something.

You are right about the copy/upload clipboard conflict. But it only arises if the config option copyAndCloseAfterUpload is set. This option, if true, copies the URL to the clipboard and closes the upload progress widget. If false, the widget turns into a dialog with some actions you can do with the uploaded screenshot.

I made it so that:

  • If copyAndCloseAfterUpload is false, there is no conflict and everything behaves as it was before
  • If true, then... Well, see for yourself. It's easier than to explain it.

I believe this way there is no need for a warning. If you agree with this decision, I'll merge this.

NOTE: The failing builds are due to a server error. Will try to rerun later.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@veracioux
Copy link
Contributor Author

@mmahmoudian Make sure to try the latest commit, I forgot to push it earlier.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
@mmahmoudian
Copy link
Member

Fixed the --delay problem.

Works flawlessly 👍

Please try it again - maybe it was a temporary connection problem or something.

I'm getting this error constantly and have tried this on your branch and also my own fork and also master branch of the main repo, which means I cannot test this:

image

You are right about the copy/upload clipboard conflict. But it only arises if the config option copyAndCloseAfterUpload is set.

I don't have it set in my config. the following is the [General] section of my config:

[General]
contrastOpacity=188
disabledTrayIcon=false
drawColor=#ff00ff
drawFontSize=8
drawThickness=3
savePath=/home/mehrad/Pictures
savePathFixed=false
setSaveAsFileExtension=.png
showStartupLaunchMessage=true
startupLaunch=true
uiColor=#ff7300
undoLimit=100
uploadHistoryMax=0
useJpgForClipboard=false

@mmahmoudian mmahmoudian removed their assignment Oct 20, 2021
@veracioux
Copy link
Contributor Author

@mmahmoudian The copyAndCloseAfterUpload is true by default and therefore not included in the config file by default.

In any case, I'll explain the change in short terms. When both --clipboard and --upload are specified, the image is always saved to the clipboard. A dialog opens that allows you to copy the URL if you want to. If you agree with this behavior, I'm going to merge this.

I think the upload issue is either specific to you or a bug that goes beyond the scope of this PR. I suggest you open an issue for that.

@veracioux
Copy link
Contributor Author

@borgmanJeremy Are you still reviewing or?

@mmahmoudian
Copy link
Member

The copyAndCloseAfterUpload is true by default and therefore not included in the config file by default.

This is even getting confusing for me 😆 I guess we should either up our game in documenting the config options, or include them all in the config file along with their default value.

When both --clipboard and --upload are specified, the image is always saved to the clipboard. A dialog opens that allows you to copy the URL if you want to.

Makes total sense. Let's merge this.

I think the upload issue is either specific to you or a bug that goes beyond the scope of this PR

I don't think it is a bug. I thing as our users are growing, there are less and less available quota and the higher chance that we get error from Imgur. For example I literally just tried and it worked just fine: https://i.imgur.com/I835ijQ.png
Basically this is an indication that we should start prioritizing the custom uploads and custom imgur API keys.

@veracioux
Copy link
Contributor Author

I guess we should either up our game in documenting the config options

Definitely. As far as this option is concerned, I'll document it in another PR that I have currently open.

or include them all in the config file along with their default value.

I'm against doing this for multiple reasons:

  • Makes it harder to track which options you configured explicitly vs which ones are default options
  • If we decide to use a system-wide config file or include a --config option, this would override all the options. This would break a fallback mechanism that we could otherwise use
  • If we mess up an option's default value during its initial design, it's easier to change that default in later versions. If we include all options, then that option won't pick up the new default and it will stay broken until the user submits an issue and we dissect their config option. Example: the setSaveAsFileExtension option used to be PNG image (.png) (or something like that) by default and new files were saved as 2021-10-20PNG image (.png). Then we changed it. Not sure if flameshot used to write all the options before, but if it did, then everyone who downloads a new version of flameshot will have to change this by hand.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>
Copy link
Contributor

@borgmanJeremy borgmanJeremy left a comment

Choose a reason for hiding this comment

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

Looks Good.

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