Re: Patches improving tree view (multi-root and auto-follow)

[Jürg Billeter <j bitron ch>]

On Wed, 2003-06-11 at 11:08, Alexander Larsson wrote:
> On Sun, 2003-06-08 at 16:07, Jürg Billeter wrote:
> > As a relating patch I've fixed the tree view so it automatically follows
> > the folder changes in the list view pane.
> > See http://bugzilla.gnome.org/show_bug.cgi?id=82884
> 
> Lets take these one at a time, and start with this one.
> I'm still not 100% sure the optimal behaviour is to always follow the
> main view. That can cause problems if you're using the tree as drop
> target, and have to change the directory of the main view.
That's true but...

> However, I've gotten enough complaints about this, and don't personally
> have a better proposal, so I guess it should go in.
...that shows that this is the expected behaviour.

> There are some problems with the patch though:
Regarding the leaks: Sorry that I've written such a faulty patch but I'm
not used to glib behaviour and thought that allocated memory is managed
(the reason is perhaps that I've tried Gtk#...). I'll try to fix these
bugs.

> First of all you can't just include a private header like that and start
> poking in the main application datatypes. The view/application interface
> is very well specified, and the only thing a view can use is the
> interfaces in libnautilus and (if the view is shipping with nautilus)
> the interfaces in libnautilus-private.
> 
> In this case the way you handle location changes isn't right. You should
> instead catch the "load_location" signal on your view. It will be
> emitted whenever a location is loaded in the window.
Didn't know that. IIRC I had some problems that I couldn't catch the
load_location signal but I'll retry the whole thing without private
headers.

> Code details:
> 
> diff -uNr nautilus-2.3.3/components/tree/nautilus-tree-view.c nautilus-2.3.3-tree/components/tree/nautilus-tree-view.c
> --- nautilus-2.3.3/components/tree/nautilus-tree-view.c	2003-04-02 12:36:11.000000000 +0200
> +++ nautilus-2.3.3-tree/components/tree/nautilus-tree-view.c	2003-06-06 21:29:04.000000000 +0200
> @@ -61,6 +62,8 @@
>  	GHashTable   *expanded_uris;
>  
>  	NautilusTreeViewDragDest *drag_dest;
> +
> +	Nautilus_URI selection_location;
>  };
> 
> Please store the internal type as a normal string instead of a corba string. 
> Also, its not freed on view finalization
Ok.

> +			if (tmpFile != NULL &&
> +        	            strcmp(nautilus_file_get_uri (tmpFile),
> +                                   nautilus_file_get_uri (file)) == 0) {
> 
> Missing space after strcmp
Ok.

> +	path = gtk_tree_model_get_path (model, &parentIter);
> 
> path is leaked
> 
> +	sort_path = gtk_tree_model_sort_convert_child_path_to_path
> +		(view->details->sort_model, path);
> 
> sort_path is leaked
> 
> +	gtk_tree_view_expand_row (view->details->tree_widget, sort_path, FALSE);
> +
> 
> I must say i don't quite understand this later part. If the specified 
> file somehow didn't exist in the tree we show it anyway? When does this
> happen?

I really should have commented that...will probably do that. The
explanation: The for loop checks if the file exists already in the tree
as a direct child to parentIter. If it does not exist, the reason is
that the parent is not yet expanded. So we will expand the parent so the
child shall show up. But as the expansion works asynchronously we need
to wait a bit (so we pause a moment until the
updating_selection_idle_callback is called again). I hope it's clear now
even in my non-native English. Else please ask again.

> +static gboolean
> +updating_selection_idle_callback (gpointer callback_data)
> +{
> [...]
> +	if (!show_iter_for_file (view, file, &iter)) {
> +		return TRUE;
> 
> This creates a loop if we weren't able to show the file. Is this 
> expected to happen? If so, when? And what guarantees the termination
> of this loop?
We assume that the only reason we can't see the file is that the parent
has not yet expanded, so we'll try the next time. See also explanation
above.
There is one other reason I've discovered a bit after this patch: The
file isn't in the same root (typing in applications:/// will not result
in loop termination). I've fixed this in my multiroot patch but forget
to include it here, too. Will fix that.

I'll try to update the patch ASAP, probably tomorrow. What about the
multiroot patch? Shall I update it to reflect the changes in this patch
or will you comment on it so I can make all changes at once. Would be
nice if I at least knew what your opinion of the idea, my multiroot
patch implements, is.

Thx for your comments

Regards,

Jürg