GNOME Bugzilla – Bug 740682
gtkapplication: Use actions from focused widget to activate accel
Last modified: 2014-12-09 09:22:11 UTC
Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, traverse from the focused widget until the window following the chain and recollecting the GActionGroups found on those widgets, the window inclusive, and fire the right action when an accel is activated, following the widget chain order.
Created attachment 291449 [details] [review] gtkapplication: Use actions from focused widget to activate accel
Created attachment 291451 [details] [review] gtkapplication: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, traverse from the focused widget until the window following the chain and recollecting the GActionGroups found on those widgets, the window inclusive, and fire the right action when an accel is activated, following the widget chain order.
What is the usecase for this, in terms of specific actions and what widgets they're defined on?
This would be really nice for me to have in Builder. I have multiple editors (both in a stack, and side by side) and want to have the keyboard focus actions attached to the container of these side-by-side tabs (A "multi-notebook"). For example, I want <Control>J and <Control>K to move up and down within the editor stack, and <Control>H and <Control>L to move to the stack left or right. Doing this at the toplevel only is unnecessarily complex when I don't want my toplevel widget to know about the details of its children.
(In reply to comment #3) > What is the usecase for this, in terms of specific actions and what widgets > they're defined on? In Nautilus currently all actions are performed on the children using GtkUIManager. Now, with the port to GAction I have multiple widgets (nautilus-view, nautilus-canvas-view, nautilus-desktop-view, nautilus-list-view) which they define it's own set of actions (that's the reason of that big inheritance). In the big case, nautilus-view, it have near 30 actions, so I defined a local GActionGroup which is attached both to the nautilus-view and nautilus-toolbar. Most of the actions of nautilus-view use accelerators. In case I have to implement them on the app or window, I will have to have public API in nautilus-view for 30 actions, and some song and dance each time an item is activated which triggers an action of nautilus-view, like the toolbar menus and the right click menus. Not only that, nautilus has a problem with implementing them on the window. Most of the actions in nautilus-view expect a window, to it's ok and they can be implemented on the window. But the desktop view doesn't use a window. So I will have to implement some of the nautilus-view actions in the window, and some of them in the app, which will make it confusing. I know though that not having a window is not something very expected in a application. So since GtkUIManager allowed that (because it was another beast indeed) and I think it makes sense to be able to have accelerators in local GActionGroups as it currently makes sense to have accelerators in the window (as a local widget of the application).
Review of attachment 291451 [details] [review]: I think it should be possible to just directly pass in the muxer of the focused widget. Muxers chain to their parent internally, which would accomplish what I think you're trying to do here. One thing to note: you should try to avoid creating muxers for widgets that do not already have them....
Created attachment 292058 [details] [review] gtkwindow: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, simply use the muxer of the focused widget, which already contains the actions of the parents.
Created attachment 292059 [details] [review] gtkwindow: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, simply use the muxer of the focused widget, which already contains the actions of the parents.
Review of attachment 292059 [details] [review]: For consistency, it seems like the code that follows widget-to-parent in search of the muxer should use the same algorithm for picking the parent as found in _gtk_widget_update_parent_muxer(). That suggests also that we should maybe do the logic inside of _gtk_widget_get_action_muxer() in the !create case. What do we expect to happen in case of a popover, for example? I'd imagine we want to go via its relative-to... ::: gtk/gtkwidget.c @@ +16511,3 @@ GtkActionMuxer * +_gtk_widget_get_action_muxer (GtkWidget *widget, + gboolean create_if_no_existent) Please just call this 'create'. ::: gtk/gtkwidgetprivate.h @@ +153,3 @@ void _gtk_widget_update_parent_muxer (GtkWidget *widget); +GtkActionMuxer * _gtk_widget_get_action_muxer (GtkWidget *widget, + gboolean create_if_no_existent); 'create' please. ::: gtk/gtkwindow.c @@ +482,3 @@ static GtkKeyHash *gtk_window_get_key_hash (GtkWindow *window); static void gtk_window_free_key_hash (GtkWindow *window); +static GtkActionMuxer* get_innermost_action_muxer (GtkWidget *origin); Seems like maybe this is a bit of a style issue -- I'll let Gtk hackers comment about that. @@ +11143,3 @@ + muxer = _gtk_widget_get_action_muxer (widget, FALSE); + + while (widget != NULL && muxer == NULL) This loop will exit with a NULL muxer if you run out of widgets... @@ +11239,2 @@ return gtk_application_activate_accel (window->priv->application, G_ACTION_GROUP (muxer), ... but here you assume that muxer is non-NULL.
Created attachment 292073 [details] [review] gtkwindow: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, simply use the muxer of the focused widget, which already contains the actions of the parents.
this was fixed
where?(In reply to comment #11) > this was fixed where?
sorry - I was sure I had seen your patch go in. Fake memories...
Review of attachment 292073 [details] [review]: ::: gtk/gtkwidget.c @@ +16494,3 @@ + { + /* This always exists */ + muxer = gtk_application_get_parent_muxer_for_window (GTK_WINDOW (widget)); Misleading comment: gtk_application_get_parent_muxer_for_window _can_ return NULL ::: gtk/gtkwindow.c @@ +11218,3 @@ + GtkActionMuxer *muxer = _gtk_widget_get_action_muxer (focused_widget, FALSE); + if (muxer == NULL) + return FALSE; Careful, gtk_window_get_focus can return NULL
Created attachment 292247 [details] [review] gtkwindow: Use actions from focused widget to activate accel This is something like what I had in mind (note: doesn't duplicate the logic about how to find the parent widget).
Review of attachment 292247 [details] [review]: Other than the NULL check, LGTM. ::: gtk/gtkwindow.c @@ +11215,3 @@ if (window->priv->application) { + GtkWidget *focused_widget = gtk_window_get_focus (window); As mentioned, this can return NULL.
Tested this on my use case and it works as expected. If no objections from Matthias, I'd be thrilled to have this commited ASAP.
Created attachment 292298 [details] [review] gtkwindow: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, simply use the muxer of the focused widget, which already contains the actions of the parents.
(In reply to comment #15) > Created an attachment (id=292247) [details] [review] > gtkwindow: Use actions from focused widget to activate accel > > This is something like what I had in mind (note: doesn't duplicate the logic > about how to find the parent widget). Oh, indeed, it's already recursive..., much better. Thanks for the code!
Review of attachment 292298 [details] [review]: ::: gtk/gtkwindow.c @@ +11218,3 @@ + if (focused_widget = NULL) + return FALSE; + GtkActionMuxer *muxer = _gtk_widget_get_action_muxer (focused_widget, FALSE); no declaration after statement: we are C89-compatible. this should have triggered a compiler warning for you.
Created attachment 292299 [details] [review] gtkwindow: Use actions from focused widget to activate accel Currently we only take into account the window GActionGroup for activating the accels. However, the application could have some custom GActionGroup in the chain of focused widgets that could want to activate some action if some accel is activated while that widget is focused. To allow applications to set accels on widgets that use custom GActionGroups, simply use the muxer of the focused widget, which already contains the actions of the parents.
(In reply to comment #20) > Review of attachment 292298 [details] [review]: > > ::: gtk/gtkwindow.c > @@ +11218,3 @@ > + if (focused_widget = NULL) > + return FALSE; > + GtkActionMuxer *muxer = _gtk_widget_get_action_muxer > (focused_widget, FALSE); > > no declaration after statement: we are C89-compatible. > > this should have triggered a compiler warning for you. whoops... thinking it was an easy change I didn't even compiled. Sorry for that.
Review of attachment 292299 [details] [review]: With that one fixup, good to go. ::: gtk/gtkwindow.c @@ +11218,3 @@ + if (focused_widget == NULL) + return FALSE; + GtkActionMuxer *muxer = _gtk_widget_get_action_muxer (focused_widget, FALSE); As mentioned: move the variable declaration to the beginning of the block.
There is one other issue with this patch set that breaks an existing feature. I can no longer use my accelerators registered with the GtkApplication (win.foo) unless a widget is focused.
sounds easily fixable. just rewrite it as if (focused_widget) muxer = _gtk_widget_get_action_muxer (focused_widget, FALSE); else muxer = _gtk_widget_get_action_muxer (window, FALSE);
Comment on attachment 292299 [details] [review] gtkwindow: Use actions from focused widget to activate accel Committed with followup patch to fix c89 and fallback to window muxer.
(In reply to comment #24) > There is one other issue with this patch set that breaks an existing feature. > > I can no longer use my accelerators registered with the GtkApplication > (win.foo) unless a widget is focused. I thought a window or a children of the window should be focused to actually trigger an accel, if not, what's the point/use case? A no focused window/children activating an accel? Sounds wrong for me at first though. Thanks for the review and push! =)
(In reply to comment #23) > Review of attachment 292299 [details] [review]: > > With that one fixup, good to go. > > ::: gtk/gtkwindow.c > @@ +11218,3 @@ > + if (focused_widget == NULL) > + return FALSE; > + GtkActionMuxer *muxer = _gtk_widget_get_action_muxer > (focused_widget, FALSE); > > As mentioned: move the variable declaration to the beginning of the block. oh ok, misunderstood Emanuelle review. That was not trigering a warning for me thougth...