Created attachment 109344 [details] [review]
"small" patch

Gosh, took much longer than expected, in a nutshell this patch does:

* Nuke GtkFilePath
* get rid of the Unix and Win32 implementations
* Make GtkFileSystem a private object, which mostly provides help with async functions and makes dealing with volumes quite generic.
* Use GtkMountOperation to mount volumes.
* Use the patch in #522084 to support GIcon

Review gtkfilechooser.c:

diff --git a/gtk/gtkfilechooser.c b/gtk/gtkfilechooser.c
index cd513a5..fa003eb 100644
--- a/gtk/gtkfilechooser.c
+++ b/gtk/gtkfilechooser.c
@@ -454,7 +454,7 @@ gtk_file_chooser_get_filename (GtkFileChooser *chooser)
 
   if (file)
     {
-      result = g_file_get_basename (file);
+      result = g_file_get_path (file);
       g_object_unref (file);
     }
 
@@ -954,7 +954,7 @@ gtk_file_chooser_set_current_folder_uri (GtkFileChooser *cho
   g_return_val_if_fail (GTK_IS_FILE_CHOOSER (chooser), FALSE);
   g_return_val_if_fail (uri != NULL, FALSE);
 
-  file = g_file_new_for_path (uri);
+  file = g_file_new_for_uri (uri);
   result = _gtk_file_chooser_set_current_folder_file (chooser, file, NULL);
   g_object_unref (file);

Created attachment 111510 [details] [review]
Updated diff, with Jan's fixes

I tested the patch alone and then applied Jan's fixes. As new as I am to GFile, the fixes do what I expect and the file chooser appears to work correctly from my testing.

Attached is a diff including the fixes.

Created attachment 111513 [details] [review]
Rebase the patch to trunk and fix some conflicts.

Created attachment 111533 [details] [review]
Fixed some bugs

Updated:

In gtk_file_chooser_button_drag_data_received in the TEXT_PLAIN case it should be
g_file_parse_name (text)
instead of
g_file_new_for_uri (text)

In test_if_path_is_visible it should be
if (local_only && !g_file_is_native (file))
    return FALSE;
instead of
if (local_only && g_file_is_native (file))
    return FALSE;

In gtk_file_system_volume_get_display_name it should be 
return g_strdup (_(root_volume_token));
instead of
return _(root_volume_token);

In shortcut_find_position (gtkfilechooserdefault.c) it should be
if (base_file)
  g_object_unref (base_file);
instead of
g_object_unref (base_file);

TODO:
* It is not possible to select one of the Bookmarks in the GtkFileChooserButton combo box in GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER mode

Created attachment 111536 [details] [review]
Replace _gtk_file_chooser_label_for_uri with _gtk_file_chooser_label_for_file

(In reply to comment #5)
> Updated:
> 
> In gtk_file_chooser_button_drag_data_received in the TEXT_PLAIN case it should
> be
> g_file_parse_name (text)
> instead of
> g_file_new_for_uri (text)

I think that's more a feature than a bugfix, IMHO it could be considered after the big chunk is already in. Besides, I always thought that using URIs to represent files was quite a standard for drag and drop.

The rest looks quite nice, it looks like we're leaking the display names somewhere if it didn't crash with a non-copied _(root_volume_token), I'll have a look at these.

Oh, and it looks like we could also rename test_if_path_is_visible() to test_if_file_is_visible() as well.

Patch looks good to me, even though it was beyond me to fully review a patch with these stats: 
 19 files changed, 3608 insertions(+), 9530 deletions(-)

What the patch misses is a big fat README.in entry warning about the fact that we've removed the semi-private GtkFileSystem interface. 

I went ahead and removed the xdgmime uses that still remain after applying this patch. So you can further improve the removed-line count of this patch by nuking xdgmime/ too.

I'm in favor of committing this patch as is, and deal with possible remaining issues afterwards.

Thanks Matthias!

I've just committed the patch with a couple of leaks plugged in r20342.

(In reply to comment #8)
> Patch looks good to me, even though it was beyond me to fully review a patch
> with these stats: 
>  19 files changed, 3608 insertions(+), 9530 deletions(-)

Yeah... sorry about that, I couldn't find any sane way to provide incremental patches, keep things building, etc...

> 
> What the patch misses is a big fat README.in entry warning about the fact that
> we've removed the semi-private GtkFileSystem interface. 

I've added this blurb to the release notes:

* The GtkFileSystem semi-private interface has been removed.
  The GTK+ filechooser implementation now uses GIO directly, which has
  rendered external filesystem implementations unnecessary. Consequently,
  the GtkFileSystem interface is no longer available, nor the filechooser
  will load any GtkFileSystem implementation.

Hope it's enough.

> 
> I went ahead and removed the xdgmime uses that still remain after applying this
> patch. So you can further improve the removed-line count of this patch by
> nuking xdgmime/ too.

Right, it's nice we can get rid of this :), committed in r20343.

> 
> I'm in favor of committing this patch as is, and deal with possible remaining
> issues afterwards.
> 

Agreed, here's the ChangeLog entry:

2008-06-10  Carlos Garnacho  <carlos@imendio.com>

        Bug 520874 - Should use gio directly.

        * gtk/gtkfilesystem.[ch]: Turn into a private object, which mostly
        provides helper functions for asynchronous calls, folder abstraction
        and uniform handling of volumes/drives/mounts.

        * gtk/gtkfilesystemwin32.[ch]:
        * gtk/gtkfilesystemunix.[ch]: Removed, these are no longer required.

        * gtk/gtkfilechooser.c:
        * gtk/gtkfilechooserbutton.c:
        * gtk/gtkfilechooserdefault.c:
        * gtk/gtkfilechooserentry.[ch]:
        * gtk/gtkfilechooserprivate.h:
        * gtk/gtkfilechooserutils.c:
        * gtk/gtkfilesystemmodel.[ch]:
        * gtk/gtkpathbar.[ch]: Use GIO internally. Adapt to GtkFileSystem API.
        Do not load filesystem implementation modules.

        * gtk/Makefile.am:
        * gtk/gtk.symbols: the gtkfilesystem.h private header isn't installed
        anymore, nor the unix/win32 implementations.

        * README.in: Add blurb about these changes.

For information, this broke libgnomeui/file-chrooser which still requires gtk/gtkfilesystem.h

I filed that as bug 538025.