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 default file extension configuration #1995

Merged
merged 9 commits into from
Oct 25, 2021

Conversation

Lyqst
Copy link
Contributor

@Lyqst Lyqst commented Oct 21, 2021

Closes #1304
There was already a config option for this, called setSaveAsFileExtension, but it wasn't exposed in the configuration panel. I've changed it to defaultFileExtension so it has a clearer name, and added it to the general options tab in the configuration panel as a ComboBox.

@Lyqst Lyqst changed the title Add default file extension configuration Add default file extension configuration (#1304) Oct 21, 2021
@Lyqst Lyqst changed the title Add default file extension configuration (#1304) Add default file extension configuration Oct 21, 2021
@veracioux
Copy link
Contributor

veracioux commented Oct 21, 2021

@Lyqst Thank you for your contribution. I didn't test the feature in detail, but I have a few small criticisms:

  • When I restart flameshot, the spin box should load the extension from the config file. Currently it displays an empty extension every time.
  • I don't think you should change existing option names. This will break people's existing configs. At least until we implement a way for people to resolve configs between flameshot versions (I'm not aware of any ongoing discussions about this). I agree that setSaveAsFileExtension is a poorly chosen name.
  • I think the "Preferred save file extension:" text would look better if you put it inside the "Save Path" group (to make it indented).

@Lyqst
Copy link
Contributor Author

Lyqst commented Oct 21, 2021

@veracioux Thanks for your comments!

When I restart flameshot, the spin box should load the extension from the config file. Currently it displays an empty extension every time.

Not sure what you mean with spin box, but when I test these changes on my machine (Manjaro), it loads whatever option has been previously saved. Maybe I'm missing something here, please let me know.

I don't think you should change existing option names. This will break people's existing configs. At least until we implement a way for people to resolve configs between flameshot versions (I'm not aware of any ongoing discussions about this). I agree that setSaveAsFileExtension is a poorly chosen name.

I see your point, makes total sense to keep compatibility with existing configs. I'll change this back to the old name.
On a side note, the previous config was only working with extensions beginning with . (.png instead of png), so to keep full compatibility I'll append the . to each options in the ComboBox.

I think the "Preferred save file extension:" text would look better if you put it inside the "Save Path" group (to make it indented).

I agree, I'll make this change.

@veracioux
Copy link
Contributor

@Lyqst I meant to say combo box instead of spin box. Confused them because they look similar. And it does actually load the extension from the config. But my config contained .png instead of png so the combo box displayed it as an empty string.

While you are at it, would you mind adding a ValueHandler that forces the setSaveAsFileExtension's value to be a valid image mime-type (plus the . at the beginning)? (see src/utils/valuehandler.h) This way, the user will get a config error if the image format is unsupported.

@Lyqst
Copy link
Contributor Author

Lyqst commented Oct 25, 2021

Hi @veracioux, I've implemented the ValueHandler, with the added value that the config will now accept image formats with or without the ..

@veracioux
Copy link
Contributor

@Lyqst Thanks for the changes. I hope the ValueHandler was intuitive enough to implement. That class is a recent addition to flameshot.

There are a few problems though:

  • When I select an extension in the GUI, it is saved to the config without the ., but that is recognized by the value handler as an error
  • When I try to take a screenshot and save it, the file extension does not match the one I configured

@Lyqst
Copy link
Contributor Author

Lyqst commented Oct 25, 2021

@veracioux I think it was intuitive enough! Only thing I wasn't clear at first was to set the type of the handler in the recognizedGeneralOptions map, but figured it out after it a bit of digging.

When I select an extension in the GUI, it is saved to the config without the ., but that is recognized by the value handler as an error

It was an issue in the check function, it was comparing against the raw val, instead of the extension variable with the . removed (if it had one).

When I try to take a screenshot and save it, the file extension does not match the one I configured

Had to pass the format to the properScreenshotPath call so it could use it to generate the filename.

@veracioux
Copy link
Contributor

@Lyqst Working like a charm!

@veracioux I think it was intuitive enough! Only thing I wasn't clear at first was to set the type of the handler in the recognizedGeneralOptions map, but figured it out after it a bit of digging.

Thanks for the feedback. I hope this will be crystal clear to new contributors once we make a developer documentation.

If it's not too much of a bother, could you just place the "Preferred save file extension" along with its combo box into the "Save Path" group for better indentation? Just for aesthetic reasons.

Other than that, if you have nothing to add and if the checks pass, I can get this merged.

@Lyqst
Copy link
Contributor Author

Lyqst commented Oct 25, 2021

@veracioux done!

@veracioux
Copy link
Contributor

@Lyqst Perfect! Thank you very much for your contribution.

@veracioux veracioux merged commit 067f4a1 into flameshot-org:master Oct 25, 2021
@Lyqst Lyqst deleted the default-file-extension branch November 8, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request 1of2: preferred default jpg/png/gif extension
2 participants