After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768130 - Keyboard activation leads to a CRITICAL and opens the wrong item
Keyboard activation leads to a CRITICAL and opens the wrong item
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.20.x
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-06-28 09:22 UTC by Debarshi Ray
Modified: 2016-06-28 17:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
view-container: Don't leak the GtkTreePath (861 bytes, patch)
2016-06-28 09:58 UTC, Debarshi Ray
none Details | Review
embed, view-container: Consolidate keyboard and mouse activation (3.24 KB, patch)
2016-06-28 09:59 UTC, Debarshi Ray
none Details | Review
searchbar: Only use ENTER events when active (1004 bytes, patch)
2016-06-28 17:34 UTC, Debarshi Ray
committed Details | Review
view-container: Don't leak the GtkTreePath (861 bytes, patch)
2016-06-28 17:35 UTC, Debarshi Ray
committed Details | Review
embed, view-container: Add a method to activate the first item (3.37 KB, patch)
2016-06-28 17:35 UTC, Debarshi Ray
committed Details | Review
view-container: Consolidate searchbar and mouse activation (1.86 KB, patch)
2016-06-28 17:35 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2016-06-28 09:22:45 UTC
Hitting ENTER in the main window to open an item leads to:
  CRITICAL **: photos_preview_view_set_model: assertion
    'current_path != NULL' failed

If you tab your way to the GtkIconView and use the arrow keys to select an item (with the grey dotted lines around it), then hitting ENTER will open the wrong image.

The CRITICAL is caused by PhotosViewContainer->current_path only being set in response to GdMainView::item-activated, which is only emitted due to a mouse click. Therefore, when we try to use the current_path to handle the ENTER, it is still NULL.

On top of that we don't use gtk_icon_view_get_cursor to get the position of the keyboard cursor. Instead we always open the first item in the model.

The right thing to do, in my opinion, would be to use gtk_icon_view_get_cursor (so that the correct item is opened) and gtk_icon_view_item_activated (so that keyboard and mouse activation use the same code path).
Comment 1 Debarshi Ray 2016-06-28 09:24:27 UTC
I sketched out a solution in gnome-photos:wip/rishi/keyboard-activation

Will attach shortly.
Comment 2 Debarshi Ray 2016-06-28 09:58:10 UTC
Created attachment 330479 [details] [review]
view-container: Don't leak the GtkTreePath
Comment 3 Debarshi Ray 2016-06-28 09:59:55 UTC
Created attachment 330480 [details] [review]
embed, view-container: Consolidate keyboard and mouse activation
Comment 4 Debarshi Ray 2016-06-28 16:54:06 UTC
Also, hitting ENTER on, say, a GtkButton while the searchbar is not up will also open the first item in the view. See bug 733191 for the gnome-documents counterpart of this bug.
Comment 5 Debarshi Ray 2016-06-28 17:34:42 UTC
Created attachment 330500 [details] [review]
searchbar: Only use ENTER events when active

Alessandro sent this over IRC in #photos on GIMPNet.
Comment 6 Debarshi Ray 2016-06-28 17:35:09 UTC
Created attachment 330501 [details] [review]
view-container: Don't leak the GtkTreePath
Comment 7 Debarshi Ray 2016-06-28 17:35:35 UTC
Created attachment 330502 [details] [review]
embed, view-container: Add a method to activate the first item
Comment 8 Debarshi Ray 2016-06-28 17:35:56 UTC
Created attachment 330503 [details] [review]
view-container: Consolidate searchbar and mouse activation
Comment 9 Debarshi Ray 2016-06-28 17:43:21 UTC
Pushed to master.