[PATCH] Refrain from automatic selection and trip expansion in the divelist

Dirk Hohndel dirk at hohndel.org
Thu Sep 20 14:24:05 PDT 2012


"Lubomir I. Ivanov" <neolit123 at gmail.com> writes:

> From: "Lubomir I. Ivanov" <neolit123 at gmail.com>
>
> Divelist fill:
> When we first build the list, leave it to the user to first interact with it
> - expand trips, select dives. An example scenario is that when
> the topmost trip is automatically opened and the first dive inside selected,
> while the trip itself contains a lot of dives, the user may be looking for
> a dive inside the bottommost trip, therefore has to scroll down. This is
> in the lines of user preference logic and potentially there could be
> application settings for it.

I disagree with this change. I don't want subsurface to open with just a
divelist and no dive displayed. That seems simply unattractive to me.
And displaying the last dive in the list seems a reasonable starting
point.

> Delete:
> When deleting a dive, similar to some GUI file managers, leave no dives
> selected afterwards. Sill expand the last trip which we can obtain from
> the actual callback's GtkTreePath.

Sorry, I disagree with this one as well. If I have multiple dives
selected and delete one of them, the others should stay selected.

Worse, your patch gets "amount_selected" into an inconsistent state (it
will likely be too big) which can trigger all kinds of problems. For
example, if you deleted the very last dive in a data file and then try
to add a new dive, with your patch subsurface will crash as it tries to
look at the selected dive that it thinks still exists even though the
data structures have been freed.

So NACK on both patches from me.

/D

>
> Signed-off-by: Lubomir I. Ivanov <neolit123 at gmail.com>
> ---
>  divelist.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/divelist.c b/divelist.c
> index d0332c2..e84ccdf 100644
> --- a/divelist.c
> +++ b/divelist.c
> @@ -1252,12 +1252,6 @@ static void fill_dive_list(void)
>  	update_dive_list_units();
>  	if (gtk_tree_model_get_iter_first(MODEL(dive_list), &iter)) {
>  		GtkTreeSelection *selection;
> -
> -		/* select the last dive (and make sure it's an actual dive that is selected) */
> -		gtk_tree_model_get(MODEL(dive_list), &iter, DIVE_INDEX, &selected_dive, -1);
> -		first_leaf(MODEL(dive_list), &iter, &selected_dive);
> -		selection = gtk_tree_view_get_selection(GTK_TREE_VIEW(dive_list.tree_view));
> -		gtk_tree_selection_select_iter(selection, &iter);
>  	}
>  }
>  
> @@ -1855,6 +1849,7 @@ static void delete_dive_cb(GtkWidget *menuitem, GtkTreePath *path)
>  	gtk_tree_model_get_iter(MODEL(dive_list), &iter, path);
>  	gtk_tree_model_get(MODEL(dive_list), &iter, DIVE_INDEX, &idx, -1);
>  	dive = get_dive(idx);
> +
>  	if (dive->divetrip) {
>  		/* we could be displaying the list model, in which case we can't find out
>  		 * if this is part of a trip and the only dive in that trip from the model,
> @@ -1877,19 +1872,9 @@ static void delete_dive_cb(GtkWidget *menuitem, GtkTreePath *path)
>  		dive_table.dives[i] = dive_table.dives[i+1];
>  	dive_table.nr--;
>  	free(dive);
> +
>  	dive_list_update_dives();
> -	/* now make sure the same dives stay selected and if necessary their trips are expanded
> -	 * also make sure that amount_selected stays consistent */
> -	amount_selected = 0;
> -	for_each_dive(i, dive) {
> -		if (dive->selected) {
> -			GtkTreePath *path = get_path_from(dive);
> -			if (MODEL(dive_list) == TREEMODEL(dive_list))
> -				gtk_tree_view_expand_to_path(tree_view, path);
> -			gtk_tree_selection_select_path(selection, path);
> -			amount_selected++;
> -		}
> -	}
> +	gtk_tree_view_expand_to_path(tree_view, path);
>  	mark_divelist_changed(TRUE);
>  }
>  
> -- 
> 1.7.11.msysgit.0
>
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface

-- 
Dirk Hohndel
Intel Open Source Technology Center


More information about the subsurface mailing list