GNOME Bugzilla – Bug 615376
if saving an image fails it's done silently
Last modified: 2011-07-17 10:08:58 UTC
Just tried saving an image in a folder I don't have the necessary rights for and I was not notified that in fact nothing was saved. Even the Save menu item was disabled as normal. Just the SaveConfirmationDialog popping up again at exit made me suspicious.
*** Bug 617604 has been marked as a duplicate of this bug. ***
*** Bug 633076 has been marked as a duplicate of this bug. ***
I have been started to cook a patch for this bug, but I have a question. What should EOG do when you don't have rights permission? a) EOG should disable SAVE command if you don't have write permission like gedit do. b) EOG should show one message into error message area as do when you don't have read permissions like eog do. IMHO I think that a) it is a better solution. What do you think?
(In reply to comment #3) > I have been started to cook a patch for this bug, but I have a question. What > should EOG do when you don't have rights permission? > > a) EOG should disable SAVE command if you don't have write permission like > gedit do. > > b) EOG should show one message into error message area as do when you don't > have read permissions like eog do. > > IMHO I think that a) it is a better solution. What do you think? I do think a) is better solution for "save" command, but you also have to considering "save as" command, which save as to a place that don't have rights permission. A warning dialog should be add here just as like gedit do.
Well, I would find it useful if I could immediately choose a different location (aka the Save As dialog pops up). Possibly a message bar could be shown first explaining what's wrong and allow something like Retrying, Cancelling and Relocating. Maybe some usability experts can help.
How does this relate to https://bugzilla.gnome.org/show_bug.cgi?id=320688 ? Does the supplied patch in bz320688 resolve this?
(In reply to comment #6) > How does this relate to https://bugzilla.gnome.org/show_bug.cgi?id=320688 ? > Does the supplied patch in bz320688 resolve this? Bug 320688 is pretty much a question for a good reaction if the target file is readonly. This bug here is that if you save an image with eog it will apparently always finish successfully even if something bad happened (and that's not only a readonly target file). So, it can happen that you rotated your images, clicked save everytime and once you open your image's folder in nautilus have to notice that nothing has changed.
(In reply to comment #7) > (In reply to comment #6) > > How does this relate to https://bugzilla.gnome.org/show_bug.cgi?id=320688 ? > > Does the supplied patch in bz320688 resolve this? > > Bug 320688 is pretty much a question for a good reaction if the target file is > readonly. This bug here is that if you save an image with eog it will > apparently always finish successfully even if something bad happened (and > that's not only a readonly target file). So, it can happen that you rotated > your images, clicked save everytime and once you open your image's folder in > nautilus have to notice that nothing has changed. Cool. I think the patch I wrote for Bug 320688 does provide the functionality you mentioned in the last sentence. The patch makes eog behave like gimp when trying to save to a path that is write protected. The only issue with the patch in 320688 would be if g_access succeeds, but the write fails. I am not sure what would cause that though. File system out of space? some I/O error? I think. I would not mind taking a look at the code to put some checks in for write errors.
Created attachment 191202 [details] [review] Patch to fix this bug This is my patch to fix this bug. - Refactored EogErrorMessageArea to include new buttons like CANCEL, RELOAD and SAVEAS. - Added new custom responses to EogErrorMessageArea for new buttons. - Refactored EogWindow to handle new EogErrorMessageArea responses. - Refactored EogImage to include write permissions verification in SAVE and SAVE AS processes.
Review of attachment 191202 [details] [review]: The general idea is pretty nice. :) It doesn't cover the save-on-exit-case yet however, but I guess we'll need a different UI for that anyway. ::: src/eog-image.c @@ +1755,3 @@ + /* fail if there is not write rights to save */ + if (!eog_image_can_be_saved (img)) { + g_set_error (error, EOG_IMAGE_ERROR, I don't think this is correct here. This checks if the source is writable, but it should rather check if the destination is. Save As shouldn't have any problems with read-only sources. @@ +1772,3 @@ return FALSE; } + Whitespace/TAB ::: src/eog-window.c @@ +1288,3 @@ + eog_window_cmd_save_as (action_save_as, window); + break; + } Trailing whitespace ;)
Created attachment 192085 [details] [review] Review of the last patch (In reply to comment #10) > The general idea is pretty nice. :) Thanks, Felix. > It doesn't cover the save-on-exit-case yet however, but I guess we'll need a > different UI for that anyway. Sorry, I forget this case. In this new patch is corrected. > I don't think this is correct here. > This checks if the source is writable, but it should rather check if the > destination is. > Save As shouldn't have any problems with read-only sources. I think that in SAVE AS case is necessary to evaluate the file destination: a) if the file exists, its write permission b) if the file don't exists, its parent directory write permission. > Trailing whitespace ;) Sorry, I have learned how to remove whitespaces recently.
(In reply to comment #11) > > It doesn't cover the save-on-exit-case yet however, but I guess we'll need a > > different UI for that anyway. > > Sorry, I forget this case. In this new patch is corrected. Oh, okay! I didn't actually mean to talk you into doing it. It was more a mental note for me. But, cool! :) > > I don't think this is correct here. > > This checks if the source is writable, but it should rather check if the > > destination is. > > Save As shouldn't have any problems with read-only sources. > > I think that in SAVE AS case is necessary to evaluate the file destination: > a) if the file exists, its write permission > b) if the file don't exists, its parent directory write permission. Agreed. Thanks a lot for this one, Javier. Works pretty nice so far. :) commit 3da565496c3e9aa623d6287fa1c1aea5158b3f61 Author: Javier Sánchez <> Date: Fri Jul 15 13:09:39 2011 +0200 Fixed Bug 615376 - if saving an image fails it's done silently. - Refactored EogErrorMessageArea to include new buttons like CANCEL, RELOAD and SAVEAS. - Added new custom responses to EogErrorMessageArea for new buttons. - Refactored EogCloseConfirmationDialog to include new buttons like CLOSE, CANCEL, SAVE and SAVE AS. - Added new custom responses to EogCloseConfirmationDialog for new buttons. - Refactored EogWindow to handle new EogErrorMessageArea and EogCloseConfirmationDialog responses. - Refactored EogImage to include write permission verification in SAVE and SAVE AS processes. - Added a new method to EogImage API called eog-image-is-file-writable to determine is when the user has write permission on the image file. https://bugzilla.gnome.org/show_bug.cgi?id=615376 This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.