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 341852 - FileChooserButton should not show error when pre-set to a nonexistent folder
FileChooserButton should not show error when pre-set to a nonexistent folder
Status: RESOLVED DUPLICATE of bug 562579
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
unspecified
Other Linux
: High major
: Small fix
Assigned To: gtk-bugs
Federico Mena Quintero
: 348412 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-05-15 12:59 UTC by Christian Persch
Modified: 2009-06-12 16:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
diff patch for bug 341852 (501 bytes, patch)
2008-08-16 05:34 UTC, Jorge Kalmbach
none Details | Review
patch for bug 341852 (555 bytes, patch)
2008-08-16 18:07 UTC, Jorge Kalmbach
none Details | Review
proposal patch for bug 341852 (600 bytes, patch)
2008-08-18 14:23 UTC, Jorge Kalmbach
none Details | Review
proposal patch for bug 341852 (550 bytes, patch)
2008-08-18 21:11 UTC, Jorge Kalmbach
none Details | Review
proposal patch for bug 341852 (657 bytes, patch)
2008-08-19 04:02 UTC, Jorge Kalmbach
reviewed Details | Review

Description Christian Persch 2006-05-15 12:59:49 UTC
Steps to reproduce:
0) Set a nonexisting directory in the gconf key /apps/epiphany/directories/downloads_folder
1) Start epiphany, Edit->Preferences

Actual result:
The prefs dialogue opens and an error dialogue opens on top of it, saying:

The folder contents could not be displayed
Error getting information for '/home/test/Desktop/Downloads': No such file or directory


Expected result:
The prefs dialogue opens, but the filechooserbutton does NOT show an error dialogue (maybe showing "invalid directory" or sth like that in the button).

gtk+ HEAD.
Comment 1 Christian Persch 2006-07-23 12:46:13 UTC
*** Bug 348412 has been marked as a duplicate of this bug. ***
Comment 2 Federico Mena Quintero 2007-01-25 20:16:14 UTC
Retitling for clarity.
Comment 3 Jorge Kalmbach 2008-08-16 05:34:44 UTC
Created attachment 116721 [details] [review]
diff patch for bug 341852

I added a extra check on gtk_file_chooser_set_current_folder to verify that the filename is actually a valid folder.
Comment 4 Jorge Kalmbach 2008-08-16 05:36:05 UTC
Here the diff patch:

Index: gtkfilechooser.c
===================================================================
--- gtkfilechooser.c	(revision 21112)
+++ gtkfilechooser.c	(working copy)
@@ -647,6 +647,7 @@
   
   g_return_val_if_fail (GTK_IS_FILE_CHOOSER (chooser), FALSE);
   g_return_val_if_fail (filename != NULL, FALSE);
+  g_return_val_if_fail (g_file_test(filename, G_FILE_TEST_IS_DIR), FALSE);
 
   file = g_file_new_for_path (filename);
   result = gtk_file_chooser_set_current_folder_file (chooser, file, NULL);
Comment 5 Diego Escalante Urrelo (not reading bugmail) 2008-08-16 05:46:58 UTC
Hey Jorge!

Using the patch, what happens when you open a non existant directory? Does the file chooser sets the home dir as the default or something? What's the behaviour?

Comment 6 Jorge Kalmbach 2008-08-16 06:00:29 UTC
Diego:

When you try to set a non existant directory the filechooser will set it to the current working dir. This is the default behavior of the gtkfilechooser.

Thanks for your answer.
Comment 7 Diego Escalante Urrelo (not reading bugmail) 2008-08-16 06:07:34 UTC
Ah, well it is really simple, looks ok to me. 
What do you say Christian?
Comment 8 Christian Dywan 2008-08-16 11:01:04 UTC
Aren't _set_current_folder_uri and_set_current_folder_file hit by the same problem? Then those should be fixed as well.
Comment 9 Jorge Kalmbach 2008-08-16 18:07:03 UTC
Created attachment 116754 [details] [review]
patch for bug 341852 

You're right Christian, thanks.

gtk_file_chooser_set_current_folder and gtk_file_chooser_set_current_folder_uri, both call gtk_file_chooser_set_current_folder_file to set the current folder. I move the G_FILE_TEST_IS_DIR test to the last method, that way I cover the 3 methods for setting the current folder.

The default behavior still is set the current working dir if the directory non exists.

This patch replace my first patch, and resolve the issues addressed by Christian. 

Index: gtkfilechooser.c
===================================================================
--- gtkfilechooser.c	(revision 21136)
+++ gtkfilechooser.c	(working copy)
@@ -1021,6 +1021,7 @@
   g_return_val_if_fail (GTK_IS_FILE_CHOOSER (chooser), FALSE);
   g_return_val_if_fail (G_IS_FILE (file), FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
+  g_return_val_if_fail (g_file_test (g_file_get_uri (file), G_FILE_TEST_IS_DIR), FALSE);
 
   return GTK_FILE_CHOOSER_GET_IFACE (chooser)->set_current_folder (chooser, file, error);
 }
Comment 10 Christian Dywan 2008-08-17 10:16:49 UTC
(In reply to comment #9)
> Created an attachment (id=116754) [edit]
> patch for bug 341852 
>
> Index: gtkfilechooser.c
> ===================================================================
> --- gtkfilechooser.c    (revision 21136)
> +++ gtkfilechooser.c    (working copy)
> @@ -1021,6 +1021,7 @@
>    g_return_val_if_fail (GTK_IS_FILE_CHOOSER (chooser), FALSE);
>    g_return_val_if_fail (G_IS_FILE (file), FALSE);
>    g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
> +  g_return_val_if_fail (g_file_test (g_file_get_uri (file),
> G_FILE_TEST_IS_DIR), FALSE);
> 
>    return GTK_FILE_CHOOSER_GET_IFACE (chooser)->set_current_folder (chooser,
> file, error);
>  }

Unfortunately that won't work. A GFile can by nature contain URIs that are not local and g_file_test can't handle those. Despite the name, g_file_test predates GFile and was not designed to work with it.

Since g_file_query_exists can potentially stall very long if the file is sitting on a dead slow server I don't think we can use that. But from looking at the code I didn't see what the appropriate solution would be, and I'm not really familiar with that code.

I suppose gtk_file_chooser_set_current_folder could still do the easy file test for what it's worth.
Comment 11 Jorge Kalmbach 2008-08-18 14:23:51 UTC
Created attachment 116865 [details] [review]
proposal patch for bug 341852

We could do the following on the gtk_file_chooser_set_current_folder_file:

+  if (g_file_is_native (file))
+  {
+    g_return_val_if_fail (g_file_test (g_file_get_uri (file), G_FILE_TEST_IS_DIR), FALSE);
+  }

That will check if the folder exists only when the GFile is native. This will solve the 50% of the problem and will catch the calls to set_current_folder and set_current_folder_uri.

From GFile documentation: 
"A native file s one expressed in the platform-native filename format, e.g. "C:\Windows" or "/usr/bin/". This does not mean the file is local, as it might be on a locally mounted remote filesystem."

In the last case, if the remote filesystem is already mounted it will no present any delay on the call. If the uri is something like "smb://machine/folder" g_file_is_native will return FALSE and the g_file_test will not be performed. This is not a complete fix, but will work for native folders.
Comment 12 Christian Dywan 2008-08-18 14:39:01 UTC
(In reply to comment #11)
> Created an attachment (id=116865) [edit]
> proposal patch for bug 341852
> [snip]
> In the last case, if the remote filesystem is already mounted it will no
> present any delay on the call. If the uri is something like
> "smb://machine/folder" g_file_is_native will return FALSE and the g_file_test
> will not be performed. This is not a complete fix, but will work for native
> folders.

For what I want, that seems good enough to catch many cases already. So I agree, independant from a solution for the 'unmounted remote' case, it makes sense to me.
Comment 13 Christian Persch 2008-08-18 16:29:10 UTC
I don't think a g_return_if_fail for the non-folder case makes sense. Assertions are for catching programming errors, but this is just a regular error (consider that between the programme checking whether the GFile refers to a folder and calling this function, the folder might have been removed and a regular file put into its place!).
Comment 14 Jorge Kalmbach 2008-08-18 17:18:02 UTC
But, we are catching a programming error. If the programmer don't verify that the folder he will set as the current folder is actually a folder, then we have to do it for him. 

The situation you describe is a race condition. If that happen, then we will see the classical error: "The folder contents can't be displayed". This patch will cover the most common cases, it will not cover special cases, is a partial solution (as I say before).
Comment 15 Christian Persch 2008-08-18 20:03:14 UTC
(In reply to comment #14)
> But, we are catching a programming error. If the programmer don't verify that
> the folder he will set as the current folder is actually a folder, then we have
> to do it for him. 

Yes, the code should verify it is a folder and return an error otherwise. But since the race condition can occur, you must not fail an assertion [which is a crash e.g. with fatal warnings]. So it is _not_ a programming error, just a runtime error.
Comment 16 Jorge Kalmbach 2008-08-18 21:11:11 UTC
Created attachment 116907 [details] [review]
proposal patch for bug 341852

Patch fixed to achieve the suggestions made by Christian Persch. 
change: assertion was removed.

Index: gtkfilechooser.c
===================================================================
--- gtkfilechooser.c	(revision 21136)
+++ gtkfilechooser.c	(working copy)
@@ -1022,6 +1022,14 @@
   g_return_val_if_fail (G_IS_FILE (file), FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
+  if (g_file_is_native (file))
+  {
+    if (!g_file_test (g_file_get_uri (file), G_FILE_TEST_IS_DIR))
+    {
+      return FALSE;
+    }
+  }
+
   return GTK_FILE_CHOOSER_GET_IFACE (chooser)->set_current_folder (chooser, file, error);
 }
 
Thanks to all for your comments.
Comment 17 Christian Dywan 2008-08-19 00:49:48 UTC
Are you sure that a *silent* return is a good idea? We could certainly just issue a normal g_warning. That's what for instance the icon theme code does in cases related to missing icons. It means that you can still see that something's wrong, but it's not a critical.
Comment 18 Jorge Kalmbach 2008-08-19 04:02:03 UTC
Created attachment 116921 [details] [review]
proposal patch for bug 341852

Added the g_warning.
g_file_get_uri was replaced for g_file_get_path because g_file_test was not handling well the URIs (it was returning FALSE on some valid paths). 

Index: gtk/gtkfilechooser.c
===================================================================
--- gtk/gtkfilechooser.c	(revision 21154)
+++ gtk/gtkfilechooser.c	(working copy)
@@ -1022,6 +1022,15 @@
   g_return_val_if_fail (G_IS_FILE (file), FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
+  if (g_file_is_native (file))
+  {
+    if (!g_file_test (g_file_get_path (file), G_FILE_TEST_IS_DIR))
+    {
+      g_warning("gtk_file_chooser_set_current_folder_file() called on invalid directory\n");
+      return FALSE;
+    }
+  }
+
   return GTK_FILE_CHOOSER_GET_IFACE (chooser)->set_current_folder (chooser, file, error);
 }
Comment 19 Emmanuele Bassi (:ebassi) 2008-08-19 04:26:06 UTC
(In reply to comment #18)
> Created an attachment (id=116921) [edit]
> proposal patch for bug 341852
> 
> Added the g_warning.
> g_file_get_uri was replaced for g_file_get_path because g_file_test was not
> handling well the URIs (it was returning FALSE on some valid paths). 

it should not surprise you: g_file_test() takes a path, not a URI:

  gboolean g_file_test (const gchar *filename,
                        GFileTest    test);

  @filename : 	 a filename to test in the GLib file name encoding

  http://library.gnome.org/devel/glib/stable/glib-File-Utilities.html#g-file-test

it's a bit of an unfortunate case: g_file_test() was created a long time before we had GFile or GIO. well, one thing to deprecate when we release GLib 3.x, I guess. :-)

> Index: gtk/gtkfilechooser.c
> ===================================================================
> --- gtk/gtkfilechooser.c        (revision 21154)
> +++ gtk/gtkfilechooser.c        (working copy)
> @@ -1022,6 +1022,15 @@
>    g_return_val_if_fail (G_IS_FILE (file), FALSE);
>    g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
> 
> +  if (g_file_is_native (file))
> +  {
> +    if (!g_file_test (g_file_get_path (file), G_FILE_TEST_IS_DIR))
> +    {
> +      g_warning("gtk_file_chooser_set_current_folder_file() called on invalid
> directory\n");
> +      return FALSE;
> +    }
> +  }

the coding style is wrong - you need to indent the curly braces of two spaces.

(In reply to comment #15)
> (In reply to comment #14)
> > But, we are catching a programming error. If the programmer don't verify that
> > the folder he will set as the current folder is actually a folder, then we have
> > to do it for him. 
> 
> Yes, the code should verify it is a folder and return an error otherwise. But
> since the race condition can occur, you must not fail an assertion [which is a
> crash e.g. with fatal warnings]. So it is _not_ a programming error, just a
> runtime error.
 
if it is a runtime error, and a recoverable one at that, it would be better to use the GError we are already passing the function. it's not really a programming error to do something and then check if it failed - it's the way this kind of API should be used:

  You should never use g_file_test() to test whether it is safe to perform an
  operation, because there is always the possibility of the condition changing
  before you actually perform the operation.  -- from g_file_test() docs

in this case, call set_current_folder_file() on a GFile and if it fails recover from that point.
Comment 20 Diego Escalante Urrelo (not reading bugmail) 2008-10-20 03:57:54 UTC
Jorge, any news?
Comment 21 Milan Bouchet-Valat 2009-02-27 15:40:15 UTC
A new patch I posted for bug 562579 (duplicate) should do the trick. It uses a fix that has been committed for a while but that was no complete. Extending its logic should be good.
Comment 22 Milan Bouchet-Valat 2009-06-12 16:00:58 UTC

*** This bug has been marked as a duplicate of 562579 ***