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

FEATURE-1291: Delete File and close Image Editor #334

Open
wants to merge 5 commits into
base: release/1.3
Choose a base branch
from

Conversation

lupinitylabs
Copy link

This PR adds functionality as requested in https://greenshot.atlassian.net/browse/FEATURE-1291 (and by a friend of mine).

The Issue

A friend described the underlying issue as follows:

He uses the tool with the following destination settings checked:

  • Save directly (using preferred file output setting)
  • Open in image editor

Every time a screenshot is created, the screenshot is saved to disk and the editor is opened.

He uses the tool mainly for documentation and annotation, saves some of the screenshots, and discards others. However, even when not saving changes, the initial screenshot stays in the destination path with no way of easily deleting it, so the directory clutters up very quickly.

The requested solution was to have an easy way to quickly delete an image from disk and close the editor.

What it does

The PR adds a new "discard" button to the Image Editor ToolStrip, that asks the user if they want to delete the screenshot on disk and close the editor.

The same function is called if the user presses the DELETE key. It felt natural to choose this key - I understand that it is also used to remove the currently selected alteration applied to a screenshot, and that's why the action always requires the user to confirm a dialog so that they don't accidently delete the whole screenshot they are working on.

Another key could be chosen, but we felt that the delete key would be the most intuitive way.

Errors when deleting the File are handled in a try..catch block, but just ignored. The correct handling of errors was verified by exclusively locking the screenshot before discarding.

Further Implications

A new button and a new dialog text and title were added and provided in en_US and de_DE, but other languages are missing. I suppose Hacktoberfest is a good time to try to get those translated ;-)

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2021

CLA assistant check
All committers have signed the CLA.

@lupinitylabs lupinitylabs changed the title FEATURE-1291: Image Editor - delete file and close FEATURE-1291: Delete File and close Image Editor Oct 5, 2021
@Lakritzator
Copy link
Member

I love the way you came from his workflow to a solution.
This is cool, but it has some implications:

  1. is getting the translations. We currently do not have a good process to get that on the way, and we are trying to release 1.3 so it has to wait.
  2. the Greenshot editor is an editor, taking the delete key to delete the whole file (with or without asking is irrelevant) would change the way people are used to work. This friends workflow is as described, other make a capture and before they do something they delete the mouse cursor with the delete key. So we need another key for this.

As soon as I have some time I will do a code review, this was in between while waiting for someone.

Best wishes,
Robin

@lupinitylabs
Copy link
Author

Thanks Robin. Yes, I thought the translations might pose a problem, and also that the delete key is up for discussion. I don't know the project and the philosophy behind it, so I thought I'd just start with a working implementation, fleshing out the details with you later. So just let me know which changes you'd prefer once you have the time, and I'll change the PR accordingly. 😄

@lupinitylabs
Copy link
Author

Robin, just wanted to let you know you can also feel free to close this if you think it's not the right way to go.
I know how hard it is for maintainers to say no sometimes 😁 Just saying don't feel forced to leave this dangling. 👍

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.

None yet

3 participants