Re: Another chunk of review of kris-async-branch

[Kristian Rietveld <kris imendio com>]

Hey Federico,

On Fri, Apr 21, 2006 at 10:39:36AM -0500, Federico Mena Quintero wrote:
> + 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.

Fixed, also in all other places.

>   + 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.

No, it doesn't.  Since the handle is not in our list of pending handles
anymore, we will branch to out.  There the ReloadIconsData structure
will be freed.

>   + 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?]

If the node a row reference points at is deleted, reference->path is set
to NULL.  Once the callback gets the path from the reference, it will
check whether it is NULL.  If it really is NULL, we know the node doesn't
exist anymore and will clean up and bail out.

>   + 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 :)

It took a while to get that right :)  I guess we can drop the validation
stage later on, maybe even after we merged.


thanks,

-kris.