GNOME Bugzilla – Bug 562579
[Patch] Remove error dialog when directory does not exist
Last modified: 2009-06-12 19:14:58 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:
Created attachment 123614 [details] [review] Patch against filechooserdefault.c
Looks like this leaks the error when it doesn't show the error dialog.
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?
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 ?
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.
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.
> 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...
I meant to use "if (g_error_matches ..." instead of "if (!g_error_matches..."
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...
I'd prefer if the comment explained _why_ we ignore those errors. Please commit with some improved comment. To both branches, if you like.
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...
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.
Thanks for your attention! I'll try to find other little things to patch in GTK.
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.
Created attachment 129660 [details] [review] Sixth version, applying against trunk
Someone to review this trivial improvement patch?
*** Bug 341852 has been marked as a duplicate of this bug. ***
*** Bug 485766 has been marked as a duplicate of this bug. ***
Committed as 67632a578bf8bfd9ba89238fd75985e7a1fa1340.