GNOME Bugzilla – Bug 778675
Activating selection mode only works via selection mode button
Last modified: 2017-04-28 22:52:58 UTC
I happen to be renaming my task lists a lot, and to do so I need to activate selection mode. Selection mode is primarily activated by the selection mode button which works well. But this is also tedious to do if you rename things often. The HIG[1] suggests alternative ways to activate selection mode: "Selection mode can also be activated by rubber band selection, or by holding Ctrl or Shift and clicking/pressing content items." I would like to suggest that these alternative approaches is implemented in GNOME To Do. [1] https://developer.gnome.org/hig/stable/selection-mode.html.en
Hovering the "colour dot" at the far left of a list item presents a checkbox that also starts selection mode. I'm fiddling a bit with the "ctrl + click" solution mostly for practice. But I noticed the "hover the colour dot" feature when I was testing and thought I'd let you know.
Created attachment 350603 [details] [review] Partial patch, adds the ctrl+click functionality to grid view I added the functionality to the grid view as suggested in issue. Ctrl-clicking a task list grid item now selects it and initiates selection mode. I'm having trouble getting the event to replicate the same functionality in the GtkListBox since this has a 'row-activated' callback which has already consumed the GdkEventKey event so I'm unable to check the GDK_CONTROL_MASK. Input regarding this is welcome. Any other suggestions regarding code are also most welcome including links to coding standards etc.
(In reply to Linus Probert from comment #2) > Created attachment 350603 [details] [review] [review] > Partial patch, adds the ctrl+click functionality to grid view > > I added the functionality to the grid view as suggested in issue. > Ctrl-clicking a task list grid item now selects it and initiates selection > mode. > > I'm having trouble getting the event to replicate the same functionality in > the GtkListBox since this has a 'row-activated' callback which has already > consumed the GdkEventKey event so I'm unable to check the GDK_CONTROL_MASK. > > Input regarding this is welcome. Any other suggestions regarding code are > also most welcome including links to coding standards etc. For me, right clicking in grid view puts me in selection mode. It does not do the same in list view, though.
Yeah I noticed that too. In list view you can start selection by clicking on the coloured circle to the far left of the list item.
Review of attachment 350603 [details] [review]: This patch actually looks good. Can you please rewrite the commit message based on the guidelines available at https://wiki.gnome.org/Newcomers/SubmitPatch ?
Review of attachment 350603 [details] [review]: Oh, and I forgot something: please rename the variables to "right_click" and "left_click_with_ctrl"
Created attachment 350647 [details] [review] Applied sugested changes (commit msg and variable names) Think I got the commit msg right according to the spec now :D
Review of attachment 350647 [details] [review]: Almost there! The title should use the class name without prefix ("list-selector-grid-item"), not the file name ("gtd-list-selector-grid-item.c"). Fix that and your patch is ready to go (and of course, we'll need another patch for the list-item)
Created attachment 350654 [details] [review] ctrl-click select on grid items (enables selection mode) Commit msg fixed. I'm having some trouble with the list items but I'll find someone to discuss this with on IRC.
Created attachment 350669 [details] [review] Allow control clicking on list items to select and enter select mode I figured out a solution to detect if ctrl was held when "activating" a list item. Essentially we listen for button presses check if it was a left-click and if CTRL was held then we let the event propagate. Not sure if this is the best solution but it was the only one I could come up with. Any feedback is most welcome. Hopefully I got the commit msg right first try this time. :o)
Created attachment 350671 [details] [review] Allow control-click to select list items NOTE: Something got wrong with the last attachment so I'm doing it again. Sorry for the spam. Allow control clicking on list items to select and enter select mode I figured out a solution to detect if ctrl was held when "activating" a list item. Essentially we listen for button presses check if it was a left-click and if CTRL was held then we let the event propagate. Not sure if this is the best solution but it was the only one I could come up with. Any feedback is most welcome. Hopefully I got the commit msg right first try this time. :o)
(In reply to Linus Probert from comment #11) > NOTE: Something got wrong with the last attachment so I'm doing it again. > Sorry for the spam. Still wrong. How are you formatting your patch?
Created attachment 350672 [details] [review] Allow control-click to select list items I was piping stdout wrong, sorry. Last retry now, I promise.
Review of attachment 350672 [details] [review]: A few comments below. About the commit message: always use the present tense. So, "Add" instead of "Added" ::: src/views/gtd-list-selector-list.c @@ +33,3 @@ GtdWindowMode mode; + + gboolean control_mask_on_click; Rename to "ctrl_pressed" @@ +52,2 @@ static void +gtd_list_selector_list_on_row_selected (GtdListSelectorItem *item, Please keep the old function name. This change is unrelated to the patch. @@ +68,3 @@ +static gboolean +gtd_list_selector_list_on_click (GtdListSelectorList *selector, Rename to 'on_click()' @@ +72,3 @@ +{ + selector->control_mask_on_click = event->button == 1 && event->state & GDK_CONTROL_MASK; + return FALSE; Please use "GDK_EVENT_PROPAGATE" rather than "FALSE". It's much more readable and semantic. @@ +106,3 @@ selector); + /* In order to determine if a user held control when left-clicking In GNOME To Do, multiline comments always have a blank '/*' as the first line. @@ +110,3 @@ + * the control mask. The event is always propagated. + */ + g_signal_connect (GTK_CONTAINER (selector), You can simply pass "selector", without casting to GtkContainer. @@ +410,3 @@ + gtd_list_selector_item_set_selected (item, !gtd_list_selector_item_get_selected (item)); + gtd_list_selector_set_mode (GTD_LIST_SELECTOR (self), GTD_WINDOW_MODE_SELECTION); + } You can simplify this by doing: if (self->control_mask_on_click) gtd_list_selector_set_mode (self, GTD_WINDOW_MODE_SELECTION); if (self->mode == GTD_WINDOW_MODE_SELECTION) gtd_list_selector_item_set_selected (...);
Created attachment 350689 [details] [review] Allow control-click to select list items Adapted code according to review comments.
Review of attachment 350689 [details] [review]: Two minor nitpicks and this patch will be ready to go! :) ::: src/views/gtd-list-selector-list.c @@ +69,3 @@ +static gboolean +on_click (GtdListSelectorList *selector, + GdkEventButton *event) The parameters are misaligned. @@ +404,3 @@ + if (self->ctrl_pressed) + gtd_list_selector_set_mode (GTD_LIST_SELECTOR (self), GTD_WINDOW_MODE_SELECTION); Please jump a line below. Also, add a comment above the 'if()' explaining what's going on.
Review of attachment 350654 [details] [review]: LGTM
Created attachment 350690 [details] [review] Allow control-click to select list items Fixed arg alignment in on_click() Added some comments to explain logic.
Review of attachment 350690 [details] [review]: LGTM
Thanks for the patches! Attachment 350654 [details] pushed as 4d7c297 - list-selector-grid-item: Allow control-click for selection mode. Attachment 350690 [details] pushed as 2fb0d52 - list-selector-list: Add ability to ctrl-click select