After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 562579 - [Patch] Remove error dialog when directory does not exist
[Patch] Remove error dialog when directory does not exist
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
2.14.x
Other All
: Normal minor
: ---
Assigned To: gtk-bugs
Federico Mena Quintero
: 341852 485766 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-11-28 15:38 UTC by Milan Bouchet-Valat
Modified: 2009-06-12 19:14 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
Patch against filechooserdefault.c (1.22 KB, patch)
2008-11-28 15:39 UTC, Milan Bouchet-Valat
needs-work Details | Review
Second version fixing leak (1.29 KB, patch)
2008-11-29 10:20 UTC, Milan Bouchet-Valat
none Details | Review
Third version, simpler and cleaner (706 bytes, patch)
2008-11-30 16:13 UTC, Milan Bouchet-Valat
none Details | Review
Fourth version, inverting the 'if' condition (705 bytes, patch)
2008-11-30 18:06 UTC, Milan Bouchet-Valat
accepted-commit_now Details | Review
Fifth version, improved comment (828 bytes, patch)
2008-12-02 21:40 UTC, Milan Bouchet-Valat
committed Details | Review
Sixth version, applying against trunk (1.47 KB, patch)
2009-02-27 15:41 UTC, Milan Bouchet-Valat
none Details | Review

Description Milan Bouchet-Valat 2008-11-28 15:38:47 UTC
Please describe the problem:
This is  a long-standing issue. Many applications remember last directory browsed and suggest the GtkFileChooser to start there. When this directory has been (re)moved, the user gets an error before he has done anything. This is really not good for the user experience, and GTK looks stupid here to every user.

Attached is a patch that generates a specific GError when we switch to a file/dir does not exist, and this allows to avoid displaying an error dialog when the only problem is this. I can't see any situation in which telling the user the folder does not exist will help him: normally the file chooser will only list existing ones, else they will disappear immediately.

I hope this fix does not break anything, have a look, it's only a few lines more in a single file.

Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Milan Bouchet-Valat 2008-11-28 15:39:59 UTC
Created attachment 123614 [details] [review]
Patch against filechooserdefault.c
Comment 2 Matthias Clasen 2008-11-29 04:51:04 UTC
Looks like this leaks the error when it doesn't show the error dialog.
Comment 3 Milan Bouchet-Valat 2008-11-29 10:20:39 UTC
Created attachment 123642 [details] [review]
Second version fixing leak

How stupid! Thanks for your quick feedback. Here's a new version that should fix that.

What do you think of the general idea, though?
Comment 4 Matthias Clasen 2008-11-29 18:39:58 UTC
 I think the general idea is ok. What is not ok is the g_file_test in the file chooser. Shouldn't we just go ahead and try to change to the folder, and get a suitable error back from the filesystem implementation ?
Comment 5 Milan Bouchet-Valat 2008-11-30 16:13:09 UTC
Created attachment 123694 [details] [review]
Third version, simpler and cleaner

I've dived into the calls involved by a folder change, and I think I've found the place where I can do the change cleanly: update_current_folder_get_info_cb(), which is a callback for _gtk_file_system_get_info.

This function already uses this kind of tweak: if a file does not exist, it should look for its parent before reporting errors. The problem is, g_file_get_parent() return null if the folder containing the wanted file has been removed, and thus we cannot go a level higher, which would have been the best thing to do. I don't know is there's a solution, but mine could be sufficient.
Comment 6 Matthias Clasen 2008-11-30 17:36:26 UTC
I'd turn around the sense of the if. 

And, hmm, I wonder if we can get this not found error in other situations, e.g. when trying to go up when the parent directory is not accessible. In that case, we do want an error dialog.
Comment 7 Milan Bouchet-Valat 2008-11-30 17:49:00 UTC
> I'd turn around the sense of the if.
Which 'if'? I' don't understand what you mean here.

About you second point, I think it cannot be an issue since in the case you describe, we would get anything but not G_IO_ERROR_NOT_FOUND. For example the error would be G_IO_ERROR_PERMISSION_DENIED, G_IO_ERROR_NOT_SUPPORTED, or G_IO_ERROR_FAILED...

For now, if you click again on the current folder in the path bar when you've removed it in the meantime, you get an error, and you're back to the parent. With the patch, you'd immediately be there, which is more logical. What could be another improvement would be updating the path bar when the folder is removed, but that's another (and independent) story...
Comment 8 Matthias Clasen 2008-11-30 18:00:58 UTC
I meant to use "if (g_error_matches ..." instead of "if (!g_error_matches..."
Comment 9 Milan Bouchet-Valat 2008-11-30 18:06:18 UTC
Created attachment 123704 [details] [review]
Fourth version, inverting the 'if' condition

Oh, sure. ;-) I was wondering which one was the best, considering logic and clarity...
Comment 10 Matthias Clasen 2008-12-02 18:09:27 UTC
I'd prefer if the comment explained _why_ we ignore those errors.

Please commit with some improved comment. To both branches, if you like.
Comment 11 Milan Bouchet-Valat 2008-12-02 21:40:15 UTC
Created attachment 123835 [details] [review]
Fifth version, improved comment

Here's the patch. But I cannot commit it myself, obviously, I'm not a GTK+ dev...
Comment 12 Matthias Clasen 2008-12-14 00:45:42 UTC
2008-12-13  Matthias Clasen  <mclasen@redhat.com>

        Bug 562579 – Remove error dialog when directory does not exist

        * gtk/gtkfilechooserdefault.c (update_current_folder_get_info_cb):
        Don't show an error dialog when changing to a non-existing folder,
        since this is ususally just an annoyance. 
Comment 13 Milan Bouchet-Valat 2008-12-14 14:54:27 UTC
Thanks for your attention! I'll try to find other little things to patch in GTK.
Comment 14 Milan Bouchet-Valat 2009-02-27 15:40:11 UTC
I'm realizing my patch only catches a part of the cases it should fix. Actually, this is only working with gtk_file_chooser_set_filename() when by chance the first-level parent dir is existing. Setting it to /usr/nonexistent/ works (thanks to a g_file_get_parent()), but not /usr/nonexistent/nonexistent. And gtk_file_chooser_set_folder() fails even with /usr/nonexistent. All this can be checked using testfilechooser in ./tests/.

Actually, the code is a little dirty in that function, and I missed the point that the error handling code is somewhat duplicated in the function. I must check errors once per level, but also at the end of the recursive process, for the original error. This really allows us to go up as far as we want until a dir exists or / is reached. This will fix bug 341852 BTW.

As I said, the patch is merely a duplication of the 10 lines used above. The first part is here to fix identation issues (a tab was hidden in my first patch). Not much to say.
Comment 15 Milan Bouchet-Valat 2009-02-27 15:41:41 UTC
Created attachment 129660 [details] [review]
Sixth version, applying against trunk
Comment 16 Milan Bouchet-Valat 2009-04-26 12:17:34 UTC
Someone to review this trivial improvement patch?
Comment 17 Milan Bouchet-Valat 2009-06-12 16:00:58 UTC
*** Bug 341852 has been marked as a duplicate of this bug. ***
Comment 18 Milan Bouchet-Valat 2009-06-12 16:43:49 UTC
*** Bug 485766 has been marked as a duplicate of this bug. ***
Comment 19 Federico Mena Quintero 2009-06-12 19:14:58 UTC
Committed as 67632a578bf8bfd9ba89238fd75985e7a1fa1340.