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

[Alexander Larsson <alexl redhat com>]

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.

However, I've gotten enough complaints about this, and don't personally
have a better proposal, so I guess it should go in.

There are some problems with the patch though:

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.

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
@@ -48,6 +48,7 @@
 #include <libnautilus-private/nautilus-global-preferences.h>
 #include <libnautilus-private/nautilus-program-choosing.h>
 #include <libnautilus-private/nautilus-tree-view-drag-dest.h>
+#include <src/nautilus-window-private.h>
 
Can't include this.

 #define NAUTILUS_PREFERENCES_TREE_VIEW_EXPANSION_STATE "tree-sidebar-panel/expansion_state"
 
@@ -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
 
 typedef struct {
@@ -71,6 +74,12 @@
 BONOBO_CLASS_BOILERPLATE (NautilusTreeView, nautilus_tree_view,
 			  NautilusView, NAUTILUS_TYPE_VIEW)
 
+static NautilusWindow *
+nautilus_tree_view_get_window (GtkWidget *view)
+{
+	return NAUTILUS_WINDOW (gtk_widget_get_ancestor (view, NAUTILUS_TYPE_WINDOW));
+}
+

Kill this.

 /*
  *   The expansion state storage is pretty broken
  * conceptually we have a gconf key, but we can't
@@ -276,7 +285,10 @@
 		}
 		   
 	} else if (uri != NULL) {	
-		nautilus_view_open_location_in_this_window (NAUTILUS_VIEW (view), uri);
+		if (strcmp (uri, view->details->selection_location) != 0) {
+			view->details->selection_location = g_strdup (uri);
+			nautilus_view_open_location_in_this_window (NAUTILUS_VIEW (view), uri);
+		}
 	}
 
 	g_free (uri);
@@ -503,6 +515,104 @@
 	update_filtering_from_preferences (NAUTILUS_TREE_VIEW (callback_data));
 }
 
+static gboolean
+show_iter_for_file (NautilusTreeView *view, NautilusFile *file, GtkTreeIter *iter)
+{
+	GtkTreeModel *model;
+	NautilusFile *parentFile;
+	GtkTreeIter parentIter;
+	NautilusFile *tmpFile;
+	gboolean valid;
+	GtkTreePath *path;
+	GtkTreePath *sort_path;
+
+	model = GTK_TREE_MODEL (view->details->child_model);
+
+	parentFile = nautilus_file_get_parent (file);

Parentfile is leaked.

+	if (parentFile == NULL) {
+		return gtk_tree_model_get_iter_first (model, iter);
+	}
+	if (!show_iter_for_file (view, parentFile, &parentIter)) {
+		return FALSE;
+	}
+
+	if (gtk_tree_model_iter_has_child (model, &parentIter)) {
+		for (valid = gtk_tree_model_iter_nth_child (model, iter, &parentIter, 0);
+	             valid;
+        	     valid = gtk_tree_model_iter_next (model, iter)) {
+			tmpFile = nautilus_tree_model_iter_get_file (NAUTILUS_TREE_MODEL (model), iter);

tmpFile is leaked

+			if (tmpFile != NULL &&
+        	            strcmp(nautilus_file_get_uri (tmpFile),
+                                   nautilus_file_get_uri (file)) == 0) {

Missing space after strcmp
both uris are leaked

+				return TRUE;
+			}
+		}
+	}
+	
+	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?

+	return FALSE;
+}
+
+static gboolean
+updating_selection_idle_callback (gpointer callback_data)
+{
+	NautilusTreeView *view;
+	NautilusFile *file;
+	GtkTreeIter iter;
+	GtkTreePath *path;
+	GtkTreePath *sort_path;
+	
+	view = NAUTILUS_TREE_VIEW (callback_data);
+
+	file = nautilus_file_get (view->details->selection_location);

file is leaked

+	if (!nautilus_file_is_directory (file)) {
+		file = nautilus_file_get_parent (file);

file is leaked again

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

+	}
+	path = gtk_tree_model_get_path (GTK_TREE_MODEL (view->details->child_model), &iter); 

path is leaked

+	sort_path = gtk_tree_model_sort_convert_child_path_to_path
+		(view->details->sort_model, path);
+	gtk_tree_view_scroll_to_cell (view->details->tree_widget, sort_path, NULL, FALSE, 0, 0);
+	gtk_tree_view_set_cursor (view->details->tree_widget, sort_path, NULL, FALSE); 
+	gtk_tree_path_free (sort_path);
+
+	return FALSE;
+}
+
+static void
+history_changed_callback (NautilusTreeView       *view,
+			  const Nautilus_History *history,
+			  gpointer                callback_data)
+{
+	g_assert (view == callback_data);
+
+	NautilusWindow *window;
+	window = nautilus_tree_view_get_window (view->details->scrolled_window);
+	if (window == NULL) {
+		return;
+	}
+
+	if (window->details->location == NULL) {
+		return;
+	}
+	if (view->details->selection_location != NULL &&
+            strcmp (view->details->selection_location, window->details->location) == 0) {
+		return;
+	}
+	if (view->details->selection_location != NULL) {
+		g_free (view->details->selection_location);
+	}
+	view->details->selection_location = g_strdup (window->details->location);
+
+	g_idle_add (updating_selection_idle_callback, view);
+}
+
 static void
 nautilus_tree_view_instance_init (NautilusTreeView *view)
 {
@@ -526,6 +636,12 @@
 
 	nautilus_view_construct_from_bonobo_control (NAUTILUS_VIEW (view), control);
 
+	view->details->selection_location = NULL;
+	nautilus_view_set_listener_mask (NAUTILUS_VIEW (view),
+					 NAUTILUS_VIEW_LISTEN_HISTORY);
+	g_signal_connect_object (view, "history_changed",
+                                 G_CALLBACK (history_changed_callback), view, 0);
+ 
 	eel_preferences_add_callback (NAUTILUS_PREFERENCES_SHOW_HIDDEN_FILES,
 				      filtering_changed_callback, view);
 	eel_preferences_add_callback (NAUTILUS_PREFERENCES_SHOW_BACKUP_FILES,



=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a suicidal native American stage actor with a robot buddy named Sparky. 
She's a blind antique-collecting soap star fleeing from a Satanic cult. They 
fight crime!