Created attachment 115730 [details] [review]
patch

Just hit this bug yesterday too, good work!

Awesome, that is just what I was missing ^_^

Might be nice to break out the addition of local-only to the entry as a separate patch.

+          impl->update_current_folder_cancellable =
+            _gtk_file_system_mount_enclosing_volume (impl->file_system, [...]

This had me confused. The filesystem methods really need documentation about the lifecycle handling of the returned cancellable objects.

+  if (chooser_entry->local_only &&
+      !g_file_is_native (chooser_entry->current_folder_file))
+    return;

I haven't tried this patch, so I don't know how this behaves, but I wonder if it would be a good idea to pop up one of those completion tooltips explaining that this file chooser can only do local files. Otherwise users might be confused if some file choosers complete remote files in the entry and some don't.

*** Bug 139261 has been marked as a duplicate of this bug. ***

(In reply to comment #4)
> Might be nice to break out the addition of local-only to the entry as a
> separate patch.
> 
> +          impl->update_current_folder_cancellable =
> +            _gtk_file_system_mount_enclosing_volume (impl->file_system, [...]
> 
> This had me confused. The filesystem methods really need documentation about
> the lifecycle handling of the returned cancellable objects.

Basically, since the callbacks are ensured to be called, you could just not keep track of the cancellable, and just unref it in the callback.

So the async filesystem methods return the GCancellable so you can cancel these operations (and the callback will unref it). Basically I'm reusing the cancellable pointer in impl because both the get_folder() and mount_enclosing_volume() will run sequentially (if needed) and given the case you want the overall operation to be cancelled at any point.

> 
> +  if (chooser_entry->local_only &&
> +      !g_file_is_native (chooser_entry->current_folder_file))
> +    return;
> 
> I haven't tried this patch, so I don't know how this behaves, but I wonder if
> it would be a good idea to pop up one of those completion tooltips explaining
> that this file chooser can only do local files. Otherwise users might be
> confused if some file choosers complete remote files in the entry and some
> don't.

Yeah, sounds worthwhile, I'll work on splitting the local-only changes and get this done.

Nice work, Garnacho :)

Matthias is right; please split the patch into three patches:

1. Mount the volume if unmounted.

2. Disconnect from the folder in gtkfilechooserentry.c, call discard_completion_store(), cancel the cancellables.  For the first part, there are three places in the code that do disconnect()/unref()/nullify - please factor that out into a discard_current_folder() function.

3. Add support for local_only; see the following.

When you do explicit completion by hitting Tab, the code goes like this:

start_explicit_completion()
  refresh_current_folder_and_file_part()
    reload_current_folder()
      start_loading_current_folder()
        _gtk_file_system_get_folder(), and your code to check for local_only

When refresh_current_folder_and_file_part() returns, start_explicit_completion() has this check:

  if (!chooser_entry->current_folder_file)
    {
      /* Here, no folder path means we couldn't parse what the user typed. */

which is of course pretty ad-hoc :)

The *other* place that calls refresh_current_folder_and_file_part() and has a more or less similar check is in start_autocompletion().

So, the main problem is how to propagate the information from start_loading_current_folder() up to those two callers --- "I refused to start loading the current_folder_file because it's not a local file".  You may want to add a new flag or something, so that you can distinguish that case from the normal "user typed something bogus".

Once you have that information, you can simply call pop_up_completion_feedback() with a suitable message.

Created attachment 116249 [details] [review]
[PATCH] Fix gtk_file_system_parse() to accept full URIs.

 gtk/gtkfilesystem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Created attachment 116250 [details] [review]
[PATCH] Mount the enclosing volume if the folder we're switching to is not mounted.

 gtk/gtkfilechooserdefault.c |   47 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

Created attachment 116251 [details] [review]
[PATCH] Discard correctly the current folder.

 gtk/gtkfilechooserentry.c |   53 +++++++++++++++++++++-----------------------
 1 files changed, 25 insertions(+), 28 deletions(-)

Created attachment 116252 [details] [review]
[PATCH] Add local_only to GtkFileChooserEntry.

 gtk/gtkfilechooserdefault.c |    3 +++
 gtk/gtkfilechooserentry.c   |   30 ++++++++++++++++++++++++++++++
 gtk/gtkfilechooserentry.h   |    4 ++++
 3 files changed, 37 insertions(+), 0 deletions(-)

+
+          g_object_unref (cancellable);
+          toplevel = gtk_widget_get_toplevel (GTK_WIDGET (impl));
+
+          mount_operation = gtk_mount_operation_new (GTK_WINDOW (toplevel));
+
+          impl->update_current_folder_cancellable =
+            _gtk_file_system_mount_enclosing_volume (impl->file_system, data->file,
+                                                     mount_operation,
+                                                     update_current_folder_mount_enclosing_volume_cb,
+                                                     data);

It seems somewhat misdesigned that you free the passed-in cancellable here,
only to get a new one back from the next operation. Really, _gtk_file_system_mount_enclosing_volume and similar operations should (optionally) take an existing cancellable.


          set_busy_cursor (impl, TRUE);

Where does that get undone ? Should update_current_folder_mount_enclosing_volume_cb set it back ?


+      beep (chooser_entry);

That beep function seems pretty pointless. Better to just call
gtk_widget_error_bell directly, methinks. But thats independent from this change.


+      pop_up_completion_feedback (chooser_entry, _("Only local paths can be selected"));

Might be better to talk about 'local files' here ? Or maybe not, since then you have to look at the action to figure out if you are selecting files or directories. Anyway, don't forget to announce this string addition to gnome-i18n@gnome.org when committing.

I've now played a bit with the patch, and there are a number of areas where it doesn't behave as I would expect.

First, with local-only set to TRUE, I enter ftp://ftp. At this point, I start getting a completion popup, but it lists completions for the local root directory. As I continue to type, the completion window hides and comes back for each character I type, very annoying. When I'm done typing ftp://ftp.gtk.org, I hit Tab to get completions. Instead, I get a tooltip that says "Invalid path". Ok, I forget the /, so lets add that and try Tab again. After typing /, I get all the local / completions popped up again. Typing Tab now finally gives me 
"Only local paths can be selected". Ok, but if I now type a b, I get local completions for /b.

Now, set local-only to FALSE, and repeat the experiment. All the annoyances repeat until I have entered ftp://ftp.gtk.org. At this point, I get adventurous and hit Enter. The uri I typed vanishes, but no directory change takes place :-(
Ok, enter it all again and add a /. Hitting Tab now gives me a tooltip saying "Completing..." for a while, and then...no completions. Probably because it wasn't mounted yet, so hit Enter. Password dialog comes up, and allows me to mount it, and indeed, the directory is changed. At this point, the entry is emptied again, so I wonder how to continue typing now... trying p, followed by Tab doesn't give me any completions, I would have expected it to complete to pub. So I continue to type it manually, up to pub/ and hit Tab again, still no completions. Adding a b, I suddenly do get completions for the two directories in ftp://ftp.gtk.org/pub/ that are starting with b.

Backing up one last time, by changing to the the local root. At this point, ftp.gtk.org is mounted, so I type ftp:// and type Tab to see if I get a completion - nope.


Overall, I would say that we haven't quite mastered how completions and mounting are supposed to interact...

Created attachment 122952 [details] [review]
[PATCH] Mount the enclosing volume if the folder we're switching to is not mounted.

(In reply to comment #9)
> Created an attachment (id=116250) [edit]
> [PATCH] Mount the enclosing volume if the folder we're switching to is not
> mounted.
> 
>  gtk/gtkfilechooserdefault.c |   47 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 47 insertions(+), 0 deletions(-)
> 

Attaching updated patch, preventing endless loop when error happens (incl. login dialog cancellation). Doing unref of some objects and setting set_busy_cursor (impl, FALSE) looks unfortunate to me to happen in the callback, however change_folder_and_display_error() doesn't take any errors into account.

Anyway, the patch works fine and correctly spawns mount process during browsing in the right tree view.

+  impl->update_current_folder_cancellable = NULL;
+
+  if (cancelled)
+    goto out;
+
+  if (error)
+    {
+      error_changing_folder_dialog (data->impl, data->file, g_error_copy (error));
+      impl->update_current_folder_cancellable = NULL;
+      impl->reload_state = RELOAD_EMPTY;
+      set_busy_cursor (impl, FALSE);
+      goto out;
+    }

Seems to me that update_current_folder_cancellable is getting set to NULL twice 
in the error case. 

What I wonder about here is: Who is resetting the busy cursor if the operation was cancelled or successful ?


+          impl->update_current_folder_cancellable =
+            _gtk_file_system_mount_enclosing_volume (impl->file_system, data->file,
+                                                     mount_operation,
+                                                     update_current_folder_mount_enclosing_volume_cb,
+                                                     data);
+          set_busy_cursor (impl, TRUE);


I know it probably can't happen that the mount operation completes synchronously, but it would still look cleaner to me to set the busy cursor before starting the mount op. 


Other than that, I'm in favour of committing this.

2008-12-13  Matthias Clasen  <mclasen@redhat.com>

        * gtk/gtkfilechooserdefault.c (update_current_folder_get_info_cb):
        Mount the enclosing volume if the folder we're switching to is not
        mounted. Patch by Tomas Bzatek, based on work by Carlos Garnacho

Created attachment 126899 [details] [review]
Updated patchset based on Garnacho's patches

I just committed this patchset; it is based on Garnacho's patches.

2009-01-20  Federico Mena Quintero  <federico@novell.com>

	http://bugzilla.gnome.org/show_bug.cgi?id=545980 -
	GtkFileChooserEntry should handle URIs

	* gtk/gtkfilesystem.c (_gtk_file_system_parse): Detect URI schemes
	and parse the full URI.
	(has_uri_scheme): New function, stolen from the old
	gtkfilesystemgnomevfs.c.

	Patch by Carlos Garnacho <carlos@imendio.com>:

	* gtk/gtkfilechooserentry.c (discard_current_folder): New
	function, factored out for when we need to get rid of the
	current_folder.
	(gtk_file_chooser_entry_dispose): Use discard_current_folder().
	(finished_loading_cb): Fix prototype.
	(load_directory_get_folder_callback): Discard the completion
	store, as well as clearing the completion feedback, if we find an
	error while loading the folder.  Also, use
	discard_current_folder().
	(reload_current_folder): Use discard_current_folder().

	Patch by Carlos Garnacho <carlos@imendio.com> - add a local_only
	property to GtkFileChooserEntry:

	* gtk/gtkfilechooserentry.c (struct _GtkFileChooserEntry): Add a
	local_only field.
	(_gtk_file_chooser_entry_init): Default to local_only being true.
	(start_explicit_completion): Don't allow completion of non-native
	files if local_only is turned on.
	(start_loading_current_folder): Don't start loading non-native
	folders if local_only is turned on.
	(_gtk_file_chooser_entry_set_local_only): New function.
	(_gtk_file_chooser_entry_get_local_only): New function.

	* gtk/gtkfilechooserentry.h (_gtk_file_chooser_entry_set_local_only,
	_gtk_file_chooser_entry_get_local_only): New prototypes.

	* gtk/gtkfilechooserdefault.c (set_local_only): Set the local_only
	property on the entry.

	Fix completion so it doesn't pop up for every character in a URI
	hostname:

	* gtk/gtkfilechooser.h (GtkFileChooserError): Add a
	GTK_FILE_CHOOSER_ERROR_INCOMPLETE_HOSTNAME.

	* gtk/gtkfilesystem.c (_gtk_file_system_parse): Return an
	"incomplete hostname" error if the user has not typed a full
	hostname yet in an URI.

	* gtk/gtkfilechooserentry.c (append_common_prefix): If we get an
	incomplete hostname, just don't pop up an error, since that is a
	transient state and the user doesn't need to be notified about it.
	(refresh_current_folder_and_file_part): Don't revert to showing
	the base folder if we have an incomplete hostname.
	(reload_current_folder): Handle the passed folder being NULL, even
	if we must force a reload.  Also, reload the folder if we didn't
	have a cancellable for it (i.e. we hadn't started to load it
	before).

*** Bug 569506 has been marked as a duplicate of this bug. ***

*** Bug 559626 has been marked as a duplicate of this bug. ***