GNOME Bugzilla – Bug 697645
Better range selection
Last modified: 2013-04-29 13:13:47 UTC
The range selection in gnome-documents is pretty weak atm. The shift-click range-selection is kinda broken wrt how it usually works (it picks the "nearest" selected item and ranges to it rather than the last non-range selected item. Also, there is no support for rubberband selection. This patchset is a RFC for how this could work better.
Created attachment 241064 [details] [review] Make multi range selection work right When you range select you want the range to start from the last single selection you made, not from some random selected item. For instance If you first select item 3-5 and then 9-7 you want 7,8,9 selected not (like now) 6,7. Instead of tracking just a boolean for the selection we keep track of an integer (which is backwards API/ABI compatible with a gboolean), where non-zero means selected. Any time we make an initial (i.e. non-range) selection (which may be the start of a range) we pick a new value for the selection, and this way we can find the last initial selection when doing a range selection. We keep the old code as a fallback if the user stiff uses boolean for the list model column, as that always converts values to 0 or 1.
Created attachment 241065 [details] [review] Support rubberband selection in selection mode
Created attachment 241066 [details] [review] Change SELECTED model row to int to allow new selection features
Review of attachment 241064 [details] [review]: Thanks for the patch. The behavior that your patch introduces is definitely better, and I think we should do something to this effect. I am not sure about the approach: why do you need to store the "selection generation" in the model? Can't you save the GtkTreePath of the initial selection item in GdMainView's private struct and use that instead? ::: libgd/gd-main-view.c @@ +485,3 @@ + self->priv->initial_selection_generation++; + if (self->priv->initial_selection_generation < 0) + self->priv->initial_selection_generation = INITIAL_SELECTION_GENERATION_START; How could self->priv->initial_selection_generation < 0 happen here? Overflow?
Review of attachment 241065 [details] [review]: I think being able to use rubberband selection while in selection mode would indeed be useful. Some comments on the patch: - it's missing the draw part of the rubberband - as far as I can see, this should be possible to implement even if we don't change the model column to an integer; just toggle the selected values for any item inside the rubberband range (that's also how it works e.g. in Nautilus). ::: libgd/gd-main-view.c @@ +382,3 @@ + gtk_tree_path_free (path); + iter = *first_element; + iter = *last_element; This looks like an optimization that could go in a separate patch.
Review of attachment 241066 [details] [review]: I'd rather find a way to avoid this, as explained in previous comments.
Review of attachment 241064 [details] [review]: For the same reason we store the selection in general in the model: The model can change over time, invalidating the tree paths. For instance, a new file may pop up at the first position, and now your tree path points to the item before the one you selected.
Review of attachment 241064 [details] [review]: ::: libgd/gd-main-view.c @@ +485,3 @@ + self->priv->initial_selection_generation++; + if (self->priv->initial_selection_generation < 0) + self->priv->initial_selection_generation = INITIAL_SELECTION_GENERATION_START; yes, overflow.
Review of attachment 241065 [details] [review]: Do we really need to draw the rubberband? The selection is visible in the checkboxes. toggling would work, i'm not sure that is expected behaviour in general though. Does other apps than nautilus do that? ::: libgd/gd-main-view.c @@ +382,3 @@ + gtk_tree_path_free (path); + iter = *first_element; + } Its not an optimization. Its required because we now call do_select_range() without knowing the order of first_element and last_element, making it break when you rubberband to the left.
Created attachment 241230 [details] [review] gd-main-view: Make selection_mode_do_select_range handle any order This makes the function work no matter what order the two iters are in the model, where previously it would break unless first_element was before last_element.
Created attachment 241231 [details] [review] Make multi range selection work right When you range select you want the range to start from the last single selection you made, not from some random selected item. For instance If you first select item 3-5 and then 9-7 you want 7,8,9 selected not (like now) 6,7. We keep the column id of the last item we initially selected (i.e. no range select). If this item disappears or something we fall back on the old code. We can also simplify the selection_mode_select_range code a bit now that selection_mode_do_select_range handles ranges in any order.
Review of attachment 241230 [details] [review]: Looks good, thanks!
Review of attachment 241231 [details] [review]: Looks good to me, thanks! ::: libgd/gd-main-view.c @@ +379,3 @@ + GD_MAIN_COLUMN_ID, &id, + -1); + if (id != NULL && strcmp (id, self->priv->last_selected_id) == 0) Can use g_strcmp0 here instead.
Attachment 241064 [details] pushed as b82e4c3 - Make multi range selection work right Attachment 241230 [details] pushed as 70f3b79 - gd-main-view: Make selection_mode_do_select_range handle any order Attachment 241231 [details] pushed as b82e4c3 - Make multi range selection work right
Created attachment 241302 [details] [review] gd-main-view: Add rubberband selection We support a rubberband-like drag-selection when in selection mode.
Created attachment 241316 [details] some drawing trouble
Created attachment 241317 [details] maybe the same ?
few observations on the rubberband selection (other than the drawing problem in those screenshots): - need to scroll during selection - it would feel much more natural if I could start the selection by click-and-drag, rather than manually switching to selection mode and then starting to drag some observations on range selection: - shift/control-click works as expected - shift/control-space or enter doesn't. Would be nice to make these work the same as their mouse counterparts
Created attachment 241326 [details] [review] gtk3: Support .rubberband in .content-view This adds a .content-view.view.rubberband selector for the rubberband. If this is not set then the ".view.rubberband" and ".content-view.view" both match at the same specificity, and the later wins due to it being listed after.
Sorry, i forgot this patch to gnome-themes-standard that is also needed. It should fix the first issue. I have not seen the second drawing issue though.
About the behaviour: Yes, scrolling is needed. non-selection-mode click-and-drag means initiation of DnD of the icon, so I don't think we can use that as is. However, we currently allow ctrl-click and right-click to mean select and switch to selection mode, so it makes sense to allow ctrl-drag and right-drag to initiate a drag selection even when not in selection mode. Another alternative is to allow drag to start if you click outside any icon and start dragging. However, this doesn't work in the list view, and is a bit fiddly, so I don't think its a good idea. ctrl-click is need in the traditional icon view because the default operation of click there is to unselect the current and select the clicked item. This is not all that important in the selection mode because it is much more focused on selecting multiple items by default (i.e. click does not unselect anything). So, in selection mode ctrl-click is not very important. The same is true for ctrl-space/enter. We do however support ctrl-click in non-selection-mode to mean select and switch to selection mode. We should add the same behaviour for ctrl-space/enter (plus make it do normal select if already in selection mode). shift-space/enter currently does nothing in selection mode, we should make it do the same thing as shift-click. Another thing that would be nice to have is a standard keyboard shortcut to switch to selection mode. Currently the only way is to keynav to the button and activate it, although with the above ctrl-space/enter on an item would also work. Maybe we could pick one of the F1-F12 keys for this?
Created attachment 241336 [details] [review] main-view: Support ctrl-drag and right-drag to switch to selection mode
Created attachment 241338 [details] [review] main-view: Support CTRL/SHIFT-space/enter to select items CTRL is ignored in selection mode, but means switch to selection mode and select in non-selection mode. SHIFT means range-select, like shift+click.
Works much better, thanks. One unrelated oddity that I notice in passing: In list view, you can select a row by hitting space or enter. But only if the focus is _not_ on the checkbox itself.
Review of attachment 241326 [details] [review]: This looks good to me.
Review of attachment 241302 [details] [review]: Thanks, this looks mostly good, with some (mostly stylistic) comments below. ::: libgd/gd-main-icon-view.c @@ +272,3 @@ + gtk_style_context_get (context, state, + "border-top-width", &border_width, + GTK_WIDGET_CLASS (gd_main_icon_view_parent_class)->draw (widget, cr); Doesn't really matter, but I'd rather you used gtk_style_context_get_border() ::: libgd/gd-main-list-view.c @@ +216,3 @@ + if (self->priv->rubberband_start) + { + GdkRectangle lines_rect; I don't think you need to save and restore the cairo context here; you only call gtk_render_* methods below and they all should save/restore the context already. @@ +258,3 @@ +gd_main_list_view_set_rubberband_range (GdMainViewGeneric *mv, + GtkTreePath *start, + gtk_tree_view_get_cell_area (GTK_TREE_VIEW (self), As this is identical between the list and icon views, I wonder if you could move it to GdMainViewGeneric - you could e.g. store the rubberband paths using g_object_set_data() and/or have a private method that returns them from the generic if you don't want to do g_object_get_data() internally from the subclass. ::: libgd/gd-main-view.c @@ +87,3 @@ + if (self->priv->rubberband_select_first_path) + gtk_tree_path_free (self->priv->rubberband_select_first_path); You can change all of these checks with calls to g_clear_pointer (also below and in the generic subclasses).
Review of attachment 241336 [details] [review]: Looks good with the following comment fixed ::: libgd/gd-main-view.c @@ +666,3 @@ + force_selection = + (event->button == 3) || + ((event->button == 1) && (event->state & GDK_CONTROL_MASK)); Can you please split this into a helper event_triggers_selection_mode() function? We also have a similar check in on_button_release_event().
Review of attachment 241338 [details] [review]: Looks good to me.
Attachment 241302 [details] pushed as 700f73a - gd-main-view: Add rubberband selection Attachment 241338 [details] pushed as f5388cb - main-view: Support CTRL/SHIFT-space/enter to select items
Actually, this is not fixed until libgd is revved in gnome-documents.
Comment on attachment 241326 [details] [review] gtk3: Support .rubberband in .content-view Attachment 241326 [details] pushed as 5adac13 - gtk3: Support .rubberband in .content-view
Created attachment 241556 [details] [review] gd-main-view: Fix "stuck drag" after rubberband selection We need to unset rubberband_select when we handle it or it will cause us to not correctly handle later buttton releases.
Comment on attachment 241556 [details] [review] gd-main-view: Fix "stuck drag" after rubberband selection Attachment 241556 [details] pushed as e6117cf - gd-main-view: Fix "stuck drag" after rubberband selection
This was committed.
Scrolling is still needed, filed that as bug 699221