Re: [PATCH] Add tree functionality to list view

[Jürg Billeter <j bitron ch>]

On Die, 2005-06-14 at 18:04 +0200, Alexander Larsson wrote:
> Here are some more detailed review comments:

Thanks for your quick response.

> [some small leaks and other issues]

Will fix those.

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

Will have to check. May well be that it's a relic from a previous test.

>  	/* 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.

The way I understand the documentation[1], it's not possible to emit row
order changes in multiple subtrees at once or did I miss something?

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

Yes, but as I look at it, it may be cleaner if I just iterate over
GtkTreeIter objects and call fm_list_model_remove for all of them,
including dummy nodes, right?

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

Yeah, is probably more what people would expect, although it'd feel
somehow better if there was no need for dummy nodes at all, i.e. better
gtk+ support for partial loaded tree models.

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

With "(Empty)" it's possible to abandon the idle approach but there
might be some flicker between "Loading..." and "(Empty)" before the
dummy node gets replaced by a real child.

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

Wouldn't be that easy AFAICT as there might be a problem when dragging
an expanded directory into another expanded directory. That's the reason
I've added the remove code to add_file. Although it may make sense to
additionally remove subdirectories on row_collapsed, too, to release
resources. Or might the additional delay when collapsing and
re-expanding a large subdirectory be too big?

Will post an updated patch sometime this week.

Regards,

Jürg

[1] http://developer.gnome.org/doc/API/2.0/gtk/GtkTreeModel.html#gtk-tree-model-rows-reordered
-- 
Jürg Billeter <j bitron ch>