Opened 3 years ago

Closed 3 years ago

#12945 closed enhancement (fixed)

[Screenshot] Display error messages when saving fails

Reported by: gbl08ma Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: Applications Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

In the process of fixing #10928, I noticed that the screenshot tool doesn't show any errors when saving the image fails (because of insufficient permissions, or if the destination for some reason is not a directory, or any other storage error).

On error, the window just stayed open, instead of closing. This can be very confusing for the users who might not remember that the normal behavior is to close on save, and thus may think the file saved sucessfully when it didn't.

This patch fixes the problem by adding error alerts to the saving function.

Attachments (2)

0001-Screenshot-better-error-handling-on-save.patch (2.6 KB ) - added by gbl08ma 3 years ago.
Squashed-commit-of-the-following.patch (3.2 KB ) - added by gbl08ma 3 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 by gbl08ma, 3 years ago

Has a Patch: set

comment:2 by pulkomandy, 3 years ago

Thanks for the patch. Some improvements would be nice:

  • The code for showing the alert is a bit repetitive. It would be nice to write it only once.
  • In one case, the string passed to B_TRANSLATE is different from the others. I would use the same string (with the error details in a %s), so people working on localization only have to translate this part once.
  • The "Error" BAlert title should be translated. It is not visible in the window itself with the default decorator, but it is visible in Deskbar, and could be visible with other decorators as well. A more descriptive title ("Failed to save screenshot", for example), would be nice.
  • Instead of returning B_ERROR, you could return the exact error code, which you have stored in the status_t variable.

comment:3 by gbl08ma, 3 years ago

I finally had some time to work on this. The patch I attached replaces the previous one, I hope it answers all your points.

The fact that I worked directly on the master branch back when I began working on this has come to bite me in the back, and it was a major pain to get a patch that contained both commits and nothing more. I think I'll just reset my master to origin/master and start fresh (and hopefully not forget to create branches for each thing I work on, next time).

comment:4 by pulkomandy, 3 years ago

Resolution: fixed
Status: newclosed

Applied in hrev50892, thanks!

To extract work from a branch with more commits, I think the simplest path would be:

git checkout origin/master
git checkout -b screenshot_fixes # or whatever new branch name (optional, only if you want to keep the branch)
git cherry-pick sha1_of_commits # replace with SHA1 hash of commits you want to pick, of course
git format-patch origin/master # get the patch
git checkout master # get back to your "normal" branch
Note: See TracTickets for help on using tickets.