Re: [PATCH] Add tree functionality to list view

[Alexander Larsson <alexl redhat com>]

On Tue, 2005-06-14 at 11:22 +0200, Juerg Billeter wrote:
> Hi
> 
> I've created a draft implementation of a tree view[1], as I thought it's
> easier to decide whether it's useful if there is a working
> implementation to play with.
> 
> I've added auto-expand for dnd which works with the sidebar tree view,
> too. This patch may solve some of the problems some users have with
> spatial nautilus.
> 
> There are probably still some rough edges needing cleanup but it doesn't
> work too bad so far. Besides that, expansion state saving and loading
> have yet to be added for spatial windows.
> 
> Any comments?

Here are some more detailed review comments:

 @@ -386,10 +412,27 @@ drag_motion_callback (GtkWidget *widget,
 	
 	action = get_drop_action (dest, context, drop_path);
 	
+	gtk_tree_view_get_drag_dest_row (GTK_TREE_VIEW (widget), &old_drop_path,
+					 NULL);

leaks old_drop_path

+	gtk_tree_view_get_drag_dest_row (tree_view, &drop_path, NULL);
+	gtk_tree_view_expand_row (tree_view, drop_path, FALSE);

leaks drop_path

+	result = nautilus_directory_contains_file (view->details->model, file);
+
+	for (node = view->details->sub_directory_list; !result && node != NULL;
+	     node = node->next) {
+		directory = NAUTILUS_DIRECTORY (node->data);
+		result |= nautilus_directory_contains_file (directory, file);
+	}

short-circuit return instead of looping over all items

+void
+fm_directory_view_add_sub_directory (FMDirectoryView  *view,
+				     NautilusDirectory*directory)

s/sub_directory/subdirectory/ in general

+		view->details->sub_directory_list = g_list_append (
+				view->details->sub_directory_list, directory);

prepend is faster

@@ -6881,19 +6979,21 @@ fm_directory_view_notify_selection_chang
 		if (eel_g_list_exactly_one_item (selection)) {
 			file = NAUTILUS_FILE (selection->data);
 			
-			if (nautilus_file_needs_slow_mime_type (file)) {
-				nautilus_file_call_when_ready
-					(file,
-					 NAUTILUS_FILE_ATTRIBUTE_SLOW_MIME_TYPE,
-					 NULL,
+			if (file != NULL) {


Is this really needed now, with the check in fm_list_view_get_selection_foreach_func?
Its bad if the file==NULL rows leak out outside FMListView.


fm-list-model.c:

+	struct _FileEntry *parent;

use a typedef here

+	if (parent == NULL) {
+		file_entry = NULL;
+		files = model->details->files;
+	} else {

no need to assign file_entry here


 	/* Let the world know about our new order */
-	path = gtk_tree_path_new ();
 
 	g_assert (new_order != NULL);
 	gtk_tree_model_rows_reordered (GTK_TREE_MODEL (model),
 				       path, NULL, new_order);
 
-	gtk_tree_path_free (path);

This calls gtk_tree_model_rows_reordered multiple times when we recurse.
Should only call it once in the end.
 

+			if (g_sequence_get_length (files) == 1) {
+				GSequencePtr dummy_ptr = g_sequence_get_ptr_at_pos (files, 0);
+				FileEntry *dummy_entry = g_sequence_ptr_get_data (dummy_ptr);
+				if (dummy_entry->file == NULL) {
+					/* replace the dummy loading entry */
+					iter.stamp = model->details->stamp;
+					iter.user_data = dummy_ptr;

this iter is just overwritten below, and not used

 static void
 fm_list_model_remove (FMListModel *model, GtkTreeIter *iter)

+			if (child_file_entry->file != NULL) {
+				fm_list_model_remove_file (model, child_file_entry->file);
+			} else {
+				g_sequence_remove (child_ptr);
+			}

Don't you need to call gtk_tree_model_row_deleted for the dummy node?
And update the stamp.

+	if (file_entry != NULL) {
+		if (g_sequence_get_length (file_entry->files) == 0) {
+			gtk_tree_path_up (path);
+			parent_iter.stamp = model->details->stamp;
+			parent_iter.user_data = file_entry->ptr;
+			gtk_tree_model_row_has_child_toggled (GTK_TREE_MODEL (model),
+							      path, &parent_iter);
+		}
+	}

Maybe this should add back a dummy node instead.
And have the "(Empty)" text for it. It avoids the flashing with the expander comming
and going.

+void
+fm_list_model_sub_directory_done_loading (FMListModel *model, NautilusDirectory *directory)
+{
+	model->details->sub_dirs_done = g_list_append (model->details->sub_dirs_done, directory);
+	
+	/* We want lower priority than the idle that handles displaying pending
+	files to avoid collapsing a tree node just because the files haven't
+	been added yet. */
+	g_idle_add_full (G_PRIORITY_DEFAULT_IDLE + 20,
+			 sub_directory_done_loading_idle_callback, model, NULL);
+}

ref the model and the directory during the idle?

Also, i don't think this is the right way. We'll have to do the "(Empty)" approach
of the treeview to avoid strangeness with the expanders.

 fm_list_view_add_file (FMDirectoryView *view, NautilusFile *file)
 {
-	fm_list_model_add_file (FM_LIST_VIEW (view)->details->model, file);
+	FMListModel *model;
+	
+	model = FM_LIST_VIEW (view)->details->model;
+	if (!fm_list_model_add_file (model, file)) {
+		fm_directory_view_remove_sub_directory (view,
+					nautilus_directory_get_for_file (file));
+		fm_list_model_remove_file (model, file);
+		fm_list_model_add_file (model, file);
+	}
 }

A better approach would be to remove subdirectories on row_collapsed.
That way we don't use resources for expanded and later collapsed subdirs.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a war-weary misogynist gangster She's a time-travelling out-of-work 
lawyer living homeless in New York's sewers. They fight crime!