GNOME Bugzilla – Bug 741633
GtkListBoxRow should implement GtkActionable
Last modified: 2018-01-03 01:57:10 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.
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).
Created attachment 364330 [details] [review] Patch against master. Same warning as before, patch rebased against master.
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.
Created attachment 364602 [details] [review] Test against gnome-3-22. Same warning as before.
Created attachment 364603 [details] [review] Test against master. Same warning as before.
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.
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...
Review of attachment 364602 [details] [review]: Cool, thanks for adding this
Review of attachment 364330 [details] [review]: LGTM
Review of attachment 364603 [details] [review]: LGTM
I these all seem fine, but will wait for mclasen to ack before merging.
Yes, looks ok to me too
Many thanks for working on this! Landed on both gtk-3-22 and master.