Re: Finished reviewing kris-async-branch

[Federico Mena Quintero <federico ximian com>]

Ahem, here is the list of comments.

  Federico

+ gtk+/tests/autotestfilechooser fails tests, and crashes.

+ With the VFS backend, .desktop links don't work.  Create a
  foo.desktop like this:

	[Desktop Entry]
	Encoding=UTF-8
	Version=1.0
	Type=Link
	URL=file:///home/federico/Documents
	Name=THIS IS A LINK

  In the file chooser, see that the folder in which you created the
  desktop file has a "THIS IS A LINK" item (this works).  But if you
  double-click on it, you don't get switched to
  /home/federico/Documents.

  This is especially bad for network browsing, since gnome-vfs
  internally spits a lot of .desktop files for SMB workgroups and
  other network stuff.

+ Did we think of something for how to prevent having a fucked install
  with a new GTK+ and an old libgnomeui (or vice-versa)?  Currently we
  install the VFS backend in $(libdir)/gtk-2.0/2.4.0/filesystems.  Can
  we put the new backends in $(libdir)/gtk-2.0/2.10.0/filesystems?
  This will ensure that mismatched versions won't look in the wrong
  place for the module.

+ gtkfilesystem.h

  + Do we leave GtkFileFolder::get_info()?

+ gtkfilesystem.c

  + The cache of rendered icons gets attached to the GtkIconTheme
    (gtkfilesystem.c:get_cached_icon()).  This means that the icons
    use memory all the time after a file chooser has been opened.  Can
    we have instead gtk_file_info_get_icon_name(), and put the cache
    in the caller?  We can have a cache-object that is shared by the
    file system model, the shortcuts pane, the path bar, etc.

  + Public functions need docs.  They are not really public, but we
    have particular semantics ("you must unref the handle in your
    callback") which need to be documented for when someone else
    has to modify the code.

+ gtkfilesystemunix.c

  + gtk_file_system_unix_get_folder() creates a GtkFileFolderUnix and
    queues an idle for load_folder().  It never removes this idle.  If
    the folder is unreffed/finalized before we hit the idle loop,
    we'll crash in load_folder().

  + Note the following:

	+static inline void
	+dispatch_get_info_callback (struct get_info_callback *info)
	+{
	+  (* info->callback) (info->handle, info->file_info, info->error, info->data);
	+
	+  if (info->file_info)
	+    gtk_file_info_free (info->file_info);
	+
	+  if (info->error)
	+    g_error_free (info->error);
	+
	+  g_object_unref (info->handle);
	+
	+  g_free (info);
	+}

	The caller callback must *not* free the file_info...

	+static inline void
	+dispatch_get_folder_callback (struct get_folder_callback *info)
	+{
	+  (* info->callback) (info->handle, info->folder, info->error, info->data);
	+
	+  if (info->error)
	+    g_error_free (info->error);
	+
	+  g_object_unref (info->handle);
	+
	+  g_free (info);
	+}

	... but the caller callback will need to unref() the folder.
	I'd prefer it if the caller always got data that it must
	free/unref itself.

  + In gtk_file_system_unix_get_info():

	+  /* Get info for "/" */
	+  if (!path)
	+    {
	+      info = file_info_for_root_with_error ("/", &error);
	+
	+      g_object_ref (handle);
	+      queue_get_info_callback (callback, handle, info, error, data);
	+
	+      return handle;
	+    }

        I hate to change my mind all the time, but now that get_info()
        gets a path rather than a folder *and* a subpath, can we make
        path==NULL be an error?  If the caller wants info about "/",
        it should pass exactly that path.

  + In gtk_file_system_unix_get_info():

	+   basename = g_path_get_basename (filename);
	+
	+   if (!stat_with_error (filename, &statbuf, &error))
	+     {
	+       g_free (basename);
	+
	+       g_object_ref (handle);
	+       queue_get_info_callback (callback, handle, NULL, error, data);
	+       return handle;
	+     }
	+
	+   if ((types & GTK_FILE_INFO_MIME_TYPE) != 0)
	+     mime_type = xdg_mime_get_mime_type_for_file (filename, &statbuf);
	+   else
	+     mime_type = NULL;
	+
	+   info = create_file_info (NULL, filename, basename, types, &statbuf,
	+                            mime_type);

        Create the basename right before calling create_file_info(),
        so that you don't have to free it prematurely if
        stat_with_error() fails.

  + In create_file_info():

	+  if (types & GTK_FILE_INFO_ICON)
	+    {
	+      IconType icon_type;
	+      char *icon_name;
	+      const char *mime_type;
	+
	+      icon_type = get_icon_type_from_path (folder_unix, filename, &mime_type);

	The passed folder_unix can be NULL since you call
	create_file_info() with a NULL folder in some places.
	Downstream, get_icon_type_from_path() returns a generic "file"
	icon if folder_unix==NULL.  Since you already have the stat
	info here (i.e. from create_file_info()), make
	get_icon_type_from_path() take that stat info if the folder is
	NULL.

  + In load_folder():

	+  if ((folder_unix->types & STAT_NEEDED_MASK) != 0)
	+    fill_in_stats (folder_unix);
	+
	+  if ((folder_unix->types & GTK_FILE_INFO_MIME_TYPE) != 0)
	+    fill_in_mime_type (folder_unix);

    GTK_FILE_INFO_ICON needs the stat and mime type; you should add
    GTK_FILE_INFO_ICON to STAT_NEEDED_MASK.

  + In gtk_file_system_unix_create_folder():

	   if (filename_is_root (filename))
	-    return TRUE; /* hmmm, but with no notification */
	+    {
	+      g_object_unref (handle);
	+      return NULL; /* hmmm, but with no notification */
	+    }

    We won't hit this code, I think; the mkdir("/") will return EEXIST
    or something.  Just remove it.  The old code was most likely bitrot.

  + In gtk_file_system_unix_create_folder():

	       folder_unix = g_hash_table_lookup (system_unix->folder_hash, parent);
	       if (folder_unix)
	 	{
	-	  GtkFileInfoType types;
	-	  GtkFilePath *parent_path;
	 	  GSList *paths;
	-	  GtkFileFolder *folder;
	+	  GtkFilePath *parent_path;
	+	  GtkFileFolderUnix *folder;

	 	  /* This is sort of a hack.  We re-get the folder, to ensure that the
	 	   * newly-created directory gets read into the folder's info hash table.
	 	   */

	-	  types = folder_unix->types;
	-
	 	  parent_path = gtk_file_path_new_dup (parent);
	-	  folder = gtk_file_system_get_folder (file_system, parent_path, types, NULL);
	+	  folder = g_hash_table_lookup (system_unix->folder_hash, parent_path);
	 	  gtk_file_path_free (parent_path);

	 	  if (folder)
	 	    {

	This is wrong; you look for the folder twice in the hash
	table, and the second time you do it by a GtkFilePath, while
	the hash table is indexed by string filenames.  I don't think
	you intended to do the second lookup, since you'll get the
	same value as the original folder_unix you already have.

  + In get_fallback_icon_name():

	+  return g_strdup (stock_name);
	+
	+}

	The callers already dup the name and free the returned value;
	don't dup it here.

  + In get_icon_name_for_directory():

	+    return g_strdup ("gnome-fs-directory");

	Likewise here, and in the other exit points of this function.

  + In get_special_icon_name():

	+  return g_strdup (name);
	 }

	Likewise here.

  + In get_is_hidden_for_file():

	You duplicate the g_file_get_contents() code from
	fill_in_hidden().  Can you factor that out from both
	functions?

+ gtkfilesystemgnomevfs.c

  + You have a lot of "SomeOperationData *info" for your async
    callbacks.  Since we already have GnomeVFSFileInfo and
    GtkFileInfo, can you rename those "info" variables to something
    like op_data?  It gets confusing otherwise :)

  + I don't like this:

	#define GTK_FILE_SYSTEM_GNOME_VFS_CALLBACK_TYPE "gtk-file-system-gnome-vfs-callback-type"
	#define GTK_FILE_SYSTEM_GNOME_VFS_CALLBACK_DATA #"gtk-file-system-gnome-vfs-callback-data"

	handle = gtk_file_system_handle_gnome_vfs_new (file_system);
	g_object_set_data (handle, GTK_FILE_SYSTEM_GNOME_VFS_CALLBACK_TYPE, ...);
	g_object_set_data (handle, GTK_FILE_SYSTEM_GNOME_VFS_CALLBACK_DATA, ...);

    Instead, can you add the corresponding fields to
    GtkFileSystemHandleGnomeVFS and not use g_object_set_data()?

  + In get_folder_complete_operation():

	+  if (info->parent_folder
	+      && !g_hash_table_lookup (info->parent_folder->children, folder_vfs->uri))
	+    {
	+      GSList *uris;
	+      FolderChild *child;
	+
	+      /* Make sure this child is in the parent folder */
	+      child = folder_child_new (folder_vfs->uri, info->file_info,
	+                                info->parent_folder->async_handle ? TRUE : FALSE);
	+
	+      g_hash_table_replace (info->parent_folder->children, child->uri, child);

	Since you just tested that the children hash table doesn't
	have the URI, could you use g_hash_table_insert()?  It makes
	it clear that nothing is getting replaced.

	+
	+      uris = g_slist_append (NULL, (char *) folder_vfs->uri);
	+      g_signal_emit_by_name (info->parent_folder, "files-added", uris);
	+      g_slist_free (uris);
	+    }

	"files-added" carries a list of GtkFilePath, not raw string
	URIs.  I know that gtk_file_path_new() will just copy the
	string.  For paranoia/correctness (and to save a list node
	allocation), could you do this instead:

		GSList uris;

		uris.next = NULL;
		uris.data = gtk_file_path_new_steal (folder_vfs->uri);
		g_signal_emit_by_name (info->parent_folder, "files-added", uris);

  + In gtk_file_system_gnome_vfs_get_folder(), you do this:

	1. See if the requested URI exists in your hash of loaded folders.
           If it does, queue an idle to return it.

	2. Otherwise, see if the (loaded) parent folder has info about
           the requested URI.

	3. Create a GtkFileFolderGnomeVFS right there even if the
           parent had no info about the requested URI.  You insert
           that folder_vfs in the global hash table of folders, even
           though you are not sure yet that the URI is a folder!  This
           is dangerous, because...

	4. ... later, in the get_folder_complete_operation() callback,
           you see if the URI is actually a folder or a .desktop link
           to one.  But if it is a .desktop file, you *remove* the
           originally-requested URI from the global hash table of
           folders, and then replace it with the resolved link URI.
           This is wrong.  Also, if it turns out that the
           originally-requested URI is not a .desktop file and not a
           folder, you also remove the URI from the global hash table
           of folders.

    You can't create the folder_vfs structure in
    gtk_file_system_gnome_vfs_get_folder() and put it in the global
    hash table of URI->FolderVFS, since you do not know yet if the URI
    is a folder.  You should create the folder_vfs until the callback
    where gnome-vfs has actually told you the type of the URI.

    Also, for a "file:///foo/bar.desktop" that links to
    "blah:///blah/blah", you have

	FolderVFS.children for "file:///foo"	|   file_system.folders
	-----------------------------------------------------------------------
		file:///foo/baz.txt		|   file:///foo
		file:///foo/bar.desktop		|   blah:///blah/blah

    That is, the global file_system.folders has *only* what you
    *really* know that is a folder.  You never replace URIs with
    resolved links or anything.

  + Similarly, in get_folder_complete_operation():

	+  else if (info->file_info->type != GNOME_VFS_FILE_TYPE_DIRECTORY)
	+    {
	+      g_set_error (&error,
	+		   GTK_FILE_SYSTEM_ERROR,
	+		   GTK_FILE_SYSTEM_ERROR_NOT_FOLDER,
	+		   _("%s is not a folder"),
	+		   info->uri);
	+
	+      g_hash_table_remove (folder_vfs->system->folders, folder_vfs->uri);
	+
	+      (* info->callback) (GTK_FILE_SYSTEM_HANDLE (info->handle),
	+			  NULL, error, info->callback_data);

	Don't put the folder_vfs in the global system->folders hash if
	you don't know yet that it is a folder, and don't remove it
	from the hash table later.

  + In gtk_file_system_gnome_vfs_get_info();

	+  if (!path)
	+    {
	+      /* NULL path */
	+      /* FIXME: report error via callback for this too? */
	       return NULL;
	     }

	path won't be NULL, because gtk_file_system_get_info() checks
	for it --- you don't need this check at all.

  + In gtk_file_system_gnome_vfs_get_info():

	+  vfs_info = gnome_vfs_file_info_new ();

	Stale code?  It's leaked and not used.

	+  uris = g_list_append (uris, gnome_vfs_uri_new (uri));

	Save a list node allocation by doing

		GList uris;

		uris.prev = uris.next = NULL;
		uris.data = gnome_vfs_uri_new (...);
		gnome_vfs_async_get_file_info (..., uris, ...);

  + In get_file_info_callback():

	  if (vfs_handle != info->handle->vfs_handle)
	    {
	      gdk_threads_leave ();
	      return;
	    }

	Shouldn't that be g_assert (vfs_handle ==
	info->handle->vfs_handle) instead?  You have such an assertion
	in get_folder_file_info_callback(), for example.

  + In cancel_operation_callback():

	+      case CALLBACK_GET_FOLDER:
	 	{
	          ...
	+	  struct GetFolderData *data = user_data;
	+
	+	  g_hash_table_remove (data->folder_vfs->system->folders, data->folder_vfs->uri);
	

	Similar to the above; don't insert the URI into the
	system->folders hash table until you are sure that it is a
	folder.  Then, you don't need to remove it from the hash here.

  + In cancel_operation_callback():

	+      case CALLBACK_CREATE_FOLDER:
	+	{
	+	  struct CreateFolderData *data = user_data;
	+
	+	  (* data->callback) (handle, data->path, NULL, data->callback_data);

	Why do you pass data->path to the user callback?  All the
	others, when canceled, get NULL for the requested path.

  + In gtk_file_system_gnome_vfs_volume_mount():

	   else if (GNOME_IS_VFS_VOLUME (volume))
	-    return TRUE; /* It is already mounted */
	+    return NULL; /* It is already mounted */
	
	Why not just call the user callback in an idle, saying "sure,
	it was mounted"?

  + In folder_purge_and_unmark():

	+  if (!folder_vfs->children)
	+    return;
	+

	Why do you need this?  The only caller already guards against that.

+ gtkfilesystemmodel.h

+ gtkfilesystemmodel.c

  + In _gtk_file_system_model_new():

	+  handle = gtk_file_system_get_folder (file_system, root_path, types,
	+				       got_root_folder_cb,
	+				       g_object_ref (model));
	+  model->pending_handles = g_slist_append (model->pending_handles, handle);

	gtk_file_system_get_folder() can return NULL.  In this case,
	you need to free the GtkFileSystemModel you just created, set
	the GError to "could not obtain the parent folder", and return NULL 
	as well.

  + In got_root_folder_cb():

	+  if (!g_slist_find (model->pending_handles, handle))
	+    {
	+      g_object_unref (model);
	+      return;
	+    }
	+
	+  model->pending_handles = g_slist_remove (model->pending_handles, handle);

	You walk the list twice.

  + In got_root_folder_cb():

	+  if (gtk_file_folder_is_finished_loading (model->root_folder))
	+    {
	+      queue_finished_loading (model); /* done in an idle because we are being created */

	Since got_root_folder_cb() is run in an idle anyway, don't
	delay the upstream notification further --- emit your own
	finished-loading signal here.	

	+      gtk_file_folder_list_children (model->root_folder, &roots, NULL);
	+    }
	+  else
	+    g_signal_connect_object (model->root_folder, "finished-loading",
	+			     G_CALLBACK (root_folder_finished_loading_cb), model, 0);

	This breaks in the case where

		1. a folder is not done loading

		2. the model requests that folder

		3. gets back a folder that is not done loading, so it
                   won't get "files-added" for the files that the
                   folder had already loaded

		4. thus the model ends up with missing children.

	You need to always call gtk_file_folder_list_children(), even
	if the folder you get is not done loading --- that will give
	you the children which the folder already had.

  + In got_root_folder_cb():

	Don't you need to emit "files-added" on your own, as you no
	longer load the initial list of files in the constructor?

  + In _gtk_file_system_model_path_do():
	+  if (!node)
	+    {
	+      /* Shouldn't really happen */
	+      gtk_file_paths_free (paths);
	+      return;

	It can really happen if the node is not visible.  Just remove
	the comment :)

  + In _gtk_file_system_model_path_do():

	+          handle = gtk_file_system_get_folder (model->file_system,
	+					       paths->data, model->types,
	+					       ref_path_cb, info);
	+	  model->pending_handles = g_slist_remove (model->pending_handles, handle);
	+        }

	You mean g_slist_append() here.

  + In get_children_get_folder_cb():

	+  if (cancelled || !folder)
	+    /* error, no folder, remove dummy child? */
	+    goto out;

	Yup, remove the dummy child.  It's unfortunate that this
	doesn't get tested at all right now.  I'll have to cook a test
	suite for when we replace
	GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER by an actual folder tree
	with expandable/collapsible nodes.

  + In get_children_get_folder_cb():

	Similar to got_root_folder_cb() - don't you need to emit
	"files-added" on your own here?

- gtkpathbar.h

	Looks good.

- gtkpathbar.c

	Looks good.

- gtkfilechooserentry.h

	Looks good.

- gtkfilechooserentry.c

	Looks good.

- gtkfilechooserembed.h

	Looks good.

- gtkfilechooserembed.c

	Looks good.

- gtkfilechooserdialog.c

  + In response_cb():

	if (!priv->response_requested && !_gtk_file_chooser_embed_should_respond (GTK_FILE_CHOOSER_EMBED (priv->widget)))
	  g_signal_stop_emission_by_name (dialog, "response");

	You need to reset priv->response_requested to FALSE after
	this, for apps that re-use the same file chooser dialog.

	The variable *probably* needs to be reset in a connect_after
	handler.  I'm not sure what happens if you click on the "Open"
	button many times before the async layer wakes up, for example.

- gtkfilechooserbutton.c

	Looks good, though I didn't look over it as carefully as for
	the other files.  I really want to have a semi-low-level
	GtkPlacesModel that we can share between GtkFileChooserButton
	and the places pane in GtkFileChooserDefault.  I also want a
	GtkPlaces *widget* that we can share directly between Nautilus
	and GtkFileChooserDefault.

- gtkfilechooserdefault.c

  + In shortcuts_reload_icons_get_info_cb():

	+  if (!g_slist_find (data->impl->reload_icon_handles, handle))
	+    goto out;

    You leak the handle.

  + In shortcuts_reload_icons():

	      info = g_new0 (struct ReloadIconsData, 1);
	      info->impl = g_object_ref (impl);
	      tree_path = gtk_tree_model_get_path (GTK_TREE_MODEL (impl->shortcuts_model), &iter);
	      info->row_ref = gtk_tree_row_reference_new (GTK_TREE_MODEL (impl->shortcuts_model), tree_path);
	      gtk_tree_path_free (tree_path);

    At the beginning of that function, you cancel all the pending
    handles.  But you never unref() the impl, not even in the
    callback.  Also, doesn't this in the callback

	  if (!g_slist_find (data->impl->reload_icon_handles, handle))
	    goto out;

    mean that you never really free the ReloadIconsData associated
    with handles?  The list no longer contains the old handles, or
    much worse, it has aliased pointers to new handles.

  + In shortcuts_remove_rows():

	+      shortcuts_free_row_data (impl, &iter);
	+      gtk_list_store_remove (impl->shortcuts_model, &iter);

    shortucts_free_row_data() will cancel the handle.  But what
    happens to the row_reference once the callback looks for it?  [Am I
    on crack?]

  + In get_file_info_finished():

	I'm thinking of not doing any validation at all for bookmarks,
	and just to let any valid path be put in there.  This will
	help with some things:

		- Bookmark files.  This will change shortly with
                  Emmanuele's RecentFilesAndBookmarks stuff, but
                  whatever.

		- Be friendlier to being disconnected from the
                  network.

		- Bookmarks to removable media?

	Other than that, looks good but it scares me that bookmarks
	got so complex :)

  + file_list_drag_data_received_get_info_cb() and
    file_list_drag_data_received_cb() have some duplication of code
    for the case of multiple selection.  Can we put that in a common
    function?

+ General comments:

	+ I don't like the fact that from the caller's viewpoint,
	  memory management of handles is different depending on
	  whether you canceled the operation or not.  If you didn't
	  cancel the operation, you must unref the handle in your
	  callback.  If you canceled the operation, your callback must
	  not unref the handle (i.e. gtkfilesystemgnomevfs unrefs the
	  handle after calling it).

	+ I've started writing a GtkFileSystemTest object, which is
	  a file system backend to be used for automatic tests.  It
	  will do things like spit tons of notifications, and let the
	  test suite control which "fake files" appear in it --- then
	  the test suite can ensure that the higher-level widgets have
	  the right data at the right times.

	+ The code in gtkfilechooserdefault has gotten way too
	  complex.  Please please please write a good document on the
	  internals so that future hackers will have a chance of
	  understanding it.