BUG: multiple selections are lost when re-sorting
Dirk Hohndel
dirk at hohndel.org
Wed Feb 20 00:09:11 PST 2013
Dirk Hohndel <dirk at hohndel.org> writes:
> This used to work.
>
> - Start Subsurface
> - Select several dives inside one or more trip(s)
> - change sort column (dives are still selected)
> - change back to 'trip sort'
> - only the 'selected_dive' is still selected
>
> As it happens, that first successful change of sorf column (from trips
> to one of the others) appears to be the ONLY one where we keep the selection.
This is a fun one... and since Linus thinks it's timing dependent I
spent a LOT of time clicking back and forth, but I think I have it
figured out.
The whole things bisects to commit 7e7cbb0dce33f8527cb348e020f4800b0482f39e
Author: Linus Torvalds <torvalds at linux-foundation.org>
Date: Fri Feb 1 09:40:54 2013 +1100
When switching sort order, scroll the dive list to the current dive
And looking at the code that was changed there is this lovely call to
scroll_to_current that was added to update_column_and_order():
@@ -2551,6 +2577,7 @@ static void update_column_and_order(int colid)
gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(dive_list.model), colid, sortorder[colid]);
gtk_tree_view_column_set_sort_order(treecolumns[colid - 1], sortorder[colid]);
second_call = FALSE;
+ scroll_to_current(GTK_TREE_MODEL(dive_list.model));
}
Which ends up calling through scroll_to_this() to scroll_to_path() which
does this:
+static void scroll_to_path(GtkTreePath *path)
+{
+ gtk_tree_view_expand_to_path(GTK_TREE_VIEW(dive_list.tree_view), path);
+ gtk_tree_view_scroll_to_cell(GTK_TREE_VIEW(dive_list.tree_view), path, NULL, FALSE, 0, 0);
+ gtk_tree_view_set_cursor(GTK_TREE_VIEW(dive_list.tree_view), path, NULL, FALSE);
+}
And according to the gtk docs that last call is the problem:
"
void gtk_tree_view_set_cursor()
Sets the current keyboard focus to be at path, and selects it. [...]
"
Oops. So we need to add the other selected dives back to the selection
after doing that or they get lost. But wait. Why does it sometimes work?
Well, if you switch from Tree view to list view, the de-selection happens
in the old model. So that explains that.
But why does it work sometimes with the test dives? I think it's that
ordering thing of the calls to the callback. If we scroll_to_path BEFORE
we restore what was selected we are good. But if it takes us to long to
get here (and this is not "multi-thread race condition", this is "random
order of callbacks in gtk race condition") then we may already have
restored the selection and think that we are done...
So my fix for this is pretty brute force. I put a static variable around
the gtk_tree_view_set_cursor() call that tells our selection code that
we are in "set_cursor" and that it should ignore the de-selections that
are happening. And right after that call I re-create all the gtk
selections from our own data. You can actually see things get rendered
and then get re-selected. Kinda funky.
This fixes another thing that the commit above broke: shift-cursor-up
and -down work again to select multiple dives. I noticed that being
broken a few days ago but then forgot to follow up.
Finally, this introduces one oddity. When you start subsurface and the
last dive (top one displayed) is part of a trip, then both the dive and
the trip entry were marked by Gtk as selected. And actually even if you
changed sort columns and back to the tree view and had a dive selected
that was the top one in its trip, then the trip entry would be selected
as well.
Turns out that was caused by some rather odd logic in set_selected that
now became visible - set_selected would expand and select a trip if it's
first child was selected. I have no explanation why that ever might have
made sense... I replaced this with code that simply restores the
expanded and selected state from our data structures.
Now either I wrote some brilliant code that fixes two subtle bugs or I
just made a major mess out of something that was mostly working. Only
one way to find out...
PLEASE TEST.
Especially Linus, since I re-wrote "correct by design" code written by
you, I'm a bit more nervous than usual...
This has been pushed (based on "looks right, works for me"), but the
commit is below for your reference as well.
/D
commit b406dbe1c05fa618c3c1d1f23751ccf07629f0d3
Author: Dirk Hohndel <dirk at hohndel.org>
Date: Tue Feb 19 23:50:55 2013 -0800
Fix tracking of selected dives across sort column changes
With commit 800b0482f39e ("When switching sort order, scroll the dive
list to the current dive") we introduced an unwanted new behavior. When
changing sort columns we could lose all selections except for the main
"selected_dive". This was caused by the call to gtk_tree_view_set_cursor
on the selected_dive which unselected all other dives.
As a side-effect this also fixed another bug that was introduced by the
same change: shift-cursor-up and -down now works again to select multiple
dives with the keyboard.
But fixing that bug unearthed a different issue. Our code that restored
the selection state of the tree model oddly decided to mark a divetrip as
selected if its first child was selected. The bug above masked that by
immediately unselecting the trip again, but now that this was fixed the
problem was immediately obvious: we would start with both the first trip
and the first dive in that trip selected (well, since we are in reverse
order it's actually the chronologically last trip and last dive...).
Instead we now use the remembered state of the trip to determine whether
it should be expanded or selected.
Signed-off-by: Dirk Hohndel <dirk at hohndel.org>
diff --git a/divelist.c b/divelist.c
index 50df6e1..0951194 100644
--- a/divelist.c
+++ b/divelist.c
@@ -45,6 +45,9 @@ static struct DiveList dive_list;
dive_trip_t *dive_trip_list;
gboolean autogroup = FALSE;
+static gboolean in_set_cursor = FALSE;
+static gboolean set_selected(GtkTreeModel *model, GtkTreePath *path,
+ GtkTreeIter *iter, gpointer data);
/*
* The dive list has the dive data in both string format (for showing)
@@ -2206,11 +2209,21 @@ static gboolean button_press_cb(GtkWidget *treeview, GdkEventButton *event, gpoi
return FALSE;
}
+/* make sure 'path' is shown in the divelist widget; since set_cursor changes the
+ * selection to be only 'path' we need to let our selection handling callbacks know
+ * that we didn't really mean this */
static void scroll_to_path(GtkTreePath *path)
{
+ GtkTreeSelection *selection;
+
gtk_tree_view_expand_to_path(GTK_TREE_VIEW(dive_list.tree_view), path);
gtk_tree_view_scroll_to_cell(GTK_TREE_VIEW(dive_list.tree_view), path, NULL, FALSE, 0, 0);
+ in_set_cursor = TRUE;
gtk_tree_view_set_cursor(GTK_TREE_VIEW(dive_list.tree_view), path, NULL, FALSE);
+ in_set_cursor = FALSE;
+ selection = gtk_tree_view_get_selection(GTK_TREE_VIEW(dive_list.tree_view));
+ gtk_tree_model_foreach(MODEL(dive_list), set_selected, selection);
+
}
/* we need to have a temporary copy of the selected dives while
@@ -2234,18 +2247,21 @@ static gboolean set_selected(GtkTreeModel *model, GtkTreePath *path,
gtk_tree_model_get(model, iter, DIVE_INDEX, &idx, -1);
if (idx < 0) {
- GtkTreeIter child;
- if (gtk_tree_model_iter_children(model, &child, iter))
- gtk_tree_model_get(model, &child, DIVE_INDEX, &idx, -1);
- }
- dive = get_dive(idx);
- selected = dive && dive->selected;
- if (selected) {
- gtk_tree_view_expand_to_path(GTK_TREE_VIEW(dive_list.tree_view), path);
- gtk_tree_selection_select_path(selection, path);
+ /* this is a trip - restore its state */
+ dive_trip_t *trip = find_trip_by_idx(idx);
+ if (trip && trip->expanded)
+ gtk_tree_view_expand_to_path(GTK_TREE_VIEW(dive_list.tree_view), path);
+ if (trip && trip->selected)
+ gtk_tree_selection_select_path(selection, path);
+ } else {
+ dive = get_dive(idx);
+ selected = dive && dive->selected;
+ if (selected) {
+ gtk_tree_view_expand_to_path(GTK_TREE_VIEW(dive_list.tree_view), path);
+ gtk_tree_selection_select_path(selection, path);
+ }
}
return FALSE;
-
}
static gboolean scroll_to_this(GtkTreeModel *model, GtkTreePath *path,
@@ -2369,7 +2385,7 @@ static gboolean modify_selection_cb(GtkTreeSelection *selection, GtkTreeModel *m
int idx;
GtkTreeIter iter;
- if (!was_selected)
+ if (!was_selected || in_set_cursor)
return TRUE;
gtk_tree_model_get_iter(model, &iter, path);
gtk_tree_model_get(model, &iter, DIVE_INDEX, &idx, -1);
More information about the subsurface
mailing list