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 741633 - GtkListBoxRow should implement GtkActionable
GtkListBoxRow should implement GtkActionable
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.15.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2014-12-17 10:09 UTC by Christian Hergert
Modified: 2018-01-03 01:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch against gnome-3-22. (7.13 KB, patch)
2017-11-24 13:56 UTC, Arnaud B.
committed Details | Review
Patch against master. (7.11 KB, patch)
2017-11-24 13:57 UTC, Arnaud B.
committed Details | Review
Test against gnome-3-22. (7.14 KB, patch)
2017-11-29 08:00 UTC, Arnaud B.
committed Details | Review
Test against master. (7.13 KB, patch)
2017-11-29 08:01 UTC, Arnaud B.
committed Details | Review

Description Christian Hergert 2014-12-17 10:09:56 UTC
It would be handy if GtkListBoxRow would implement GtkActionable so I can just set detailed-action-name and not need to wire up any "row-activated" handlers.

I tend to create rows like such, which may or may not be very common (rather than relying on ListBox to create them for me).

  g_object_new (GTK_TYPE_LIST_BOX_ROW,
                "visible", TRUE,
                NULL);

Simply being able to add "detailed-action-name" to that is really what I want.
Comment 1 Arnaud B. 2017-11-24 13:56:04 UTC
Created attachment 364329 [details] [review]
Patch against gnome-3-22.

Warning, I’m neither an usual C dev, nor an usual gtk+ dev, so there can be some oddities. But looks like this patch is doing the work, tested with dconf-editor (current code works as usual, and a modified code works as expected).
Comment 2 Arnaud B. 2017-11-24 13:57:23 UTC
Created attachment 364330 [details] [review]
Patch against master.

Same warning as before, patch rebased against master.
Comment 3 Christian Hergert 2017-11-26 06:46:59 UTC
A cursory look over the code and things look like they'll do what they claim to. However, we need tests to be able to know that for sure. Can you add a test inside of tests/ that can be used to ensure the implementation works as designed?

It probably only needs to be a simple small list that activates a label or something when the action is activated.
Comment 4 Arnaud B. 2017-11-29 08:00:53 UTC
Created attachment 364602 [details] [review]
Test against gnome-3-22.

Same warning as before.
Comment 5 Arnaud B. 2017-11-29 08:01:36 UTC
Created attachment 364603 [details] [review]
Test against master.

Same warning as before.
Comment 6 Christian Hergert 2017-12-08 11:10:16 UTC
Just wanted to let you know I haven't forgotten about this, just haven't had a chance to review yet this week. Will hopefully get to that tomorrow.
Comment 7 Christian Hergert 2017-12-09 07:37:46 UTC
Review of attachment 364329 [details] [review]:

I like this, looks good.

::: gtk/gtklistbox.c
@@ +171,3 @@
+  ROW_PROP_ACTION_TARGET,
+
+  LAST_ROW_PROPERTY = ROW_PROP_ACTION_NAME

My preferences is to have LAST_ROW_PROPERTY before ROW_PROP_ACTION_NAME, but I guess we do this style in a couple other files too, so that's fine.

@@ +1789,3 @@
+  if (!gtk_list_box_row_get_activatable (row))
+    return;
+

I wonder if we should always activate row-activated? Although, I can't really think of a use-case where you'd want both...
Comment 8 Christian Hergert 2017-12-09 07:42:25 UTC
Review of attachment 364602 [details] [review]:

Cool, thanks for adding this
Comment 9 Christian Hergert 2017-12-09 07:42:59 UTC
Review of attachment 364330 [details] [review]:

LGTM
Comment 10 Christian Hergert 2017-12-09 07:43:25 UTC
Review of attachment 364603 [details] [review]:

LGTM
Comment 11 Christian Hergert 2017-12-09 07:44:04 UTC
I these all seem fine, but will wait for mclasen to ack before merging.
Comment 12 Matthias Clasen 2018-01-02 22:56:53 UTC
Yes, looks ok to me too
Comment 13 Christian Hergert 2018-01-03 01:57:04 UTC
Many thanks for working on this!

Landed on both gtk-3-22 and master.