GNOME Bugzilla – Bug 560228
Add "action-controller" property to GtkWidgetClass
Last modified: 2009-02-06 18:03:14 UTC
Im attaching a 20 min. rough writeup for this. It will allow me to do powerful things in glade.
Created attachment 122353 [details] [review] A rough writeup Obviously it needs to allow for proxy disconnection when setting the property to NULL, and probably move the action qdata to GtkWidget entirely, I just wanted to share the idea and hear your thoughts on this first.
(In reply to comment #1) > Created an attachment (id=122353) [edit] > A rough writeup > > Obviously it needs to allow for proxy disconnection when > setting the property to NULL, and probably move the action qdata to > GtkWidget entirely, I just wanted to share the > idea and hear your thoughts on this first. Could you briefly summarize the intention of that patch? I suspect it's incomplete since I cannot see what "action-controller" actually is. What I can see is that you aliased gtk_action_connect_proxy with gtk_widget_set_action.
The nomanclature is a problem, by "controller" I am looking for whatever english word that would be the opposite of "proxy", the widget is the action's "proxy", so what is the action to the widget ? maybe a controller ? The patch is incomplete, inasmuch as it needs to do more things :) but it does at least provide a property setting access to connecting a widget to its action (which means we can use actions in builder created interfaces, which just currently isnt the case without using a UIManager, which is also limited in what widgets you can use actions with). Another thing the patch needs to do (besides be more elegant in general), is to allow a widget to register itself as a proxy, but only receive sensitivity state/toggle state updates and funnel action signals, so that widgets with custom graphics and custom labels can also connect to actions.
Created attachment 122889 [details] [review] revised Heres an update. This is pretty much what I'm aiming for, theres currently a bug wrt custom appearances, I think it revolves around how I keep the default TRUE property as qdata. Note I did this all as qdata and from gtkaction.c with the intention of avoiding bloat on the GtkWidget structure. This new patch also includes an "action-controls-appearance" property on the GtkWidgetClass, this way we can distinguish between widgets that should and should not have their labels and images updated by the action code.
Created attachment 122912 [details] [review] a good patch Ok I polished it off, I left the old gtk_widget_get_action() api in gtkaction.c, it uses the gtkaction private qdata so I didnt touch that... I added gtk_widget_set_action() and gtk_widget_[set/get]_use_action_appearance() to gtkwidget.[ch], plus properties "action-controller" and "use-action-appearance". The use-action-appearance property is now stored as a private flag on gtkwidget, handling is cleaner (and fixes my old bug), and also takes no added space on widget private data. What you get with this patch is the ability to connect widgets to actions as proxies using pure gobject properties, plus the ability to customize the look and feel of your widget while inheriting the sensitive/visible/toggle state of the action and proxying the action signals as usual.
Some comments: - I'm somewhat dubious on the use-action-appearance thing. I'll reserve some more thought on that. - I'm also somewhat dubious on setting actions on arbitrary widgets, when only a handful of widget types can actually do something sensible with them. What does happen when I set a toggle action on a GtkFrame ? (I guess the same that currently happens when you call gtk_action_connect_proxy on it.) This might be cleaner with an interface, but also more work... - I think the property should just be called GtkWidget:action. That gives nice setter/getter consistency with gtk_widget_get/set_action and it avoids further confusion by introducing another new term ("controller"). Is there any good reason why gtk_widget_set_action is not just a setter for the action-controller property, but instead sets both ? I'd prefer straight setters, 1 per property, if at all possible. If not possible, the bastardized setter needs to freeze/thaw notifications. Does gtk_widget_set_action handle gtk_widget_set_action (widget, actiona) gtk_widget_set_action (widget, actionb) correctly ? It seems that maybe it needs to disconnect from the current action before connecting to a new one ? I should probably just look at gtkaction.c...
(In reply to comment #5) > Created an attachment (id=122912) [edit] > a good patch I'm seeing "action-controller" and "action" used like synonyms, I suspect it was by mistake. I actually second that "action" should be *the* property, there's no need for new terminology and it only really becomes straight forward that way.
Fwiw, I am rewriting this whole patch as per the discussion in the last meeting, (interfaces on proxies...) currently I'm working now with the property name "related-action", it seems to look coherent with the rest of the api, and I can change it after... still working... I would like to use "action" too :) but thats a segfault in filechooser widgets, who use the "action" keyword for an alltogether different type and meaning.
(In reply to comment #6) > Some comments: > > - I'm somewhat dubious on the use-action-appearance thing. I'll reserve some > more thought on that. Thats the stickler it is... the thing is, I think its a must that you can use an action and not have it set your label, icon, or remove your custom child widget etc, and have an option, the proper way to do this would probably be to provise a "use-%s" auxillary property on the proxy for every action property that needs to be communicated (i.e. "use-sensitive", "use-visible", "use-stock-id"). That will result in lots more private data on the proxies, and add complexity when you initially set the related action (the proxy is supposed to modify the heirarchy at that time) I'm writing up a large scale migration of the action case specific code into interface implementations at the moment, would be much better than this patch. The idea simply put is that we have a GtkActivatableIface implemented by proxies, the iface has the 2 properties mentioned in the previous patch, "related-action" and "use-action-appearance". When the "related-action" property is set, the activatable must connect to the "notify" signal of the new action, and call gtk_activatable_update() from that handler. Then the implementations go ahead and activate the action directly when appropriate. I'll attach a work in progress for perspective, it doesnt compile exactly cause of some link errors Im dealing with but it will give you an idea of what I have in mind.
Created attachment 123486 [details] [review] the iface header
Created attachment 123487 [details] [review] the iface with properties
Created attachment 123488 [details] [review] gtkmenuitem/gtkimagemenuitem patch to implement iface
Created attachment 123573 [details] [review] The writeup Ok heres an all in one patch for the proposed implementation. NOTE: This is unfinished, I need to implement the remaining recentchooser activatables, but gtk-demo uimanager works fine, so does loading the demo.ui I stayed up late last night and I moved mountains, there is no more list traversal in action code to update actions, with the exception of some technical recentchooser stuff that should really call the apis of their proxies directly (seems it doesnt make sence to make properties out of everything, I think I will have to revert the way gtk_recent_chooser_set_sort_func() is handled for that reason...) The activatables are responsible for activating the actions directly when they activate, and updating all relevant things in notify callbacks. I gave a little thought to the "use-action-appearance" property, maybe we can implement a GtkActivatableFlags to say what action property should be synced or not, then we can give control over all properties instead and have a minimal memory hit on the activatables, a problem with this is that we probably need a flags for each action derivative... I'm not exactly sure how it could play out in the long run but I think the important part is only to be able to setup your widget appearance by hand, defining what is appearance can be the tricky part...
Created attachment 123580 [details] [review] bug fixes I had some bug in GtkActivatable:update() implementations, resulting in no icons in toolbars, I also fixed up recursions when updating states of proxies that result in activations (since we no longer use a signal...). With this update, I dont know of any remaining bugs besides the unfinished recentaction code (recent menus dont work yet...).
Created attachment 123581 [details] [review] Clean candidate patch Ok, this one nails recentchoosermenu into the picture, I looked over the patch as a whole and tweaked and cleaned house, as far as I know this patch can be applied to gtk+ trunk without regressions. Unless I find some bugs I'll wait for a review on this one...
Created attachment 123615 [details] [review] some typos This one removes an old diff to gtkprivate.h (oops, that was remaining from the old implementation), and fixed some copy paste errors (had copy/pasted the license inline.., fixed author).
Before looking at the patch in depth, there is a naming conflict with the GtkActivatable interface that has been proposed in bug 532795. That needs to be solved somehow, maybe by moving this one to something like GtkActionProxy.
Or maybe they should be merged, since it really makes most sense to connect a widget to an action if it is has an ::activate signal...
I was actually going to reply the same thing... (mid air collision ;-) The bug on bug 532795 targets 3.0 and... I'm not sure what we can do so far as api breaks, I wonder, if a button and an expander use "activate" to do something, while a menu or an entry clearly use "activate" to denote user input (i.e. useful "activate" to listen to...) if its a good idea to mix this implementation into an interface. I would really like that kind of thing to be resolved for 3.0, "activate" should mean user activated, or it should mean do a fancy dance (I would really like the name "dance" for this actually ;-)) or it could even mean both... (I'll bring this part of the discussion to the other bug...)
Created attachment 125310 [details] [review] New patch with running gimp as I said on the list, this patch had crashers with gimp. I assumed gimp is deriving actions, so I corrected my refactoring work to maintain the proxy list inside the connect_proxy() vfunc, and keep it installed on the base action class incase derived classes chain up to it. I ran gimp afterwords both with and without the patch applied. I didnt encounter noticeable differences in navigating the complex menus. are there some specific areas in gimp where menus may be slower due to the notify handling ?
How does it compare memory-wise ? Can you measure how many signal handlers are created ?
On my system, top shows that the gimp takes 34 megs of resident memory, with or without the patch applied. Thats obviously a very vague comparison, how does one measure signal handler connections ? can it be done without hacking/compiling the gimp itself ?
Some numbers from quick testing of starting up and closing the gimp: with 2.14.6: total signal connections: 52023, current: 13600 with 2.15.0 + GtkActivatable: total signal connections: 53195, current: 13695 which doesn't seem to be a significant increase. So that is good. Looking briefly over the patch, some things seem to be a bit convoluted: gtk_activatable_set_related_action() calls g_object_set (obj, "related-action"... calls gtk_tool_item_set_related_action() or similar calls _gtk_activatable_do_set_related_action() calls g_signal_connect (action, "notify"... and the notify handler calls gtk_activatable_update() Since all of the set_related_action implementations seem to do essentially the same, can't you do most of this (ie calling _do_set_related_action(), connecting to "notify", etc) in gtk_activatable_set_related_action ? or at least move the "notify" handling into _do_set_related_action ? Why does GtkAction establish a weak ref on the proxy, when all proxy implementation maintain the proxy list manually anyway, by calling _do_set_related_action from dispose ? Another case of convolutedness is that the notify handlers of each activatable implementation call gtk_activatable_update, which in turn calls the activatable's implemenation of update Calling gtk_activatable_update with property_name == NULL to update all properties seems to lead to somewhat messy, repetitive implementations of update... The documentation for use-action-appearance needs to spell out which action properties are considered 'appearance' and which are not. What about GtkToggleAction::draw-as-radio, doesn't that need to get handled in the updating code too ? It looks to me like the update code in gtkrecentchooser will trigger this warning: g_warning ("Choosers of type `%s' do not support showing numbers", G_OBJECT_TYPE_NAME (chooser)); One regression: The "Bold" checkbutton in testmerge doesn't update the "Bold" menuitem anymore. Some typos: s/recieve/receive/ s/relavent/relevant/
And I believe the default for ::use-actiona-appearance should be TRUE
(In reply to comment #23) [...] > > Looking briefly over the patch, some things seem to be a bit convoluted: > > gtk_activatable_set_related_action() > calls g_object_set (obj, "related-action"... > calls gtk_tool_item_set_related_action() or similar > calls _gtk_activatable_do_set_related_action() > calls g_signal_connect (action, "notify"... > and the notify handler calls gtk_activatable_update() > > Since all of the set_related_action implementations seem to do essentially the > same, can't you do most of this (ie calling _do_set_related_action(), > connecting to "notify", etc) in gtk_activatable_set_related_action ? or at > least move the "notify" handling into _do_set_related_action ? I didnt do it in _set_related_action() directly because g_object_set() wouldnt work correctly (required also by builder). I can move the signal connection to do_set_related_action(), it will reduce a bunch of code. I would actually like to make that function public (and documented to be used in activatable implementations...), if you dont mind. > Why does GtkAction establish a weak ref on the proxy, when all proxy > implementation maintain the proxy list manually anyway, by calling > _do_set_related_action from dispose ? Remaining code from the old implementation, good point I'll clean that out too. > Another case of convolutedness is that the notify handlers of each > activatable implementation call gtk_activatable_update, which in turn > calls the activatable's implemenation of update That will at least look nicer with the signal connection done in _do_set_related_action()... the vfunc I think is a good convenient alternative to having each subclass handle the iface "related-action" property themselves, and connect a notify themselves. > Calling gtk_activatable_update with property_name == NULL to update all > properties seems to lead to somewhat messy, repetitive implementations of > update... Right, hmmm, the relevant properties that need updating are subclass implementation specific - so if we move that code outside of ->update(), we cant exactly just put it in _set_related_action() or _set_use_action_appearance()... ...so I guess I will try adding a ->reset() vfunc to GtkActivatable unless I hear objections or better ideas ;-) ... > The documentation for use-action-appearance needs to spell out which action > properties are considered 'appearance' and which are not. > Todo Noted... > > What about GtkToggleAction::draw-as-radio, doesn't that need to get handled > in the updating code too ? That is handled by gtkcheckmenuitem.c, can we use it anywhere else ? > > It looks to me like the update code in gtkrecentchooser will trigger this > warning: > g_warning ("Choosers of type `%s' do not support showing numbers", > G_OBJECT_TYPE_NAME (chooser)); > > > One regression: The "Bold" checkbutton in testmerge doesn't update the "Bold" > menuitem anymore. > > Some typos: > s/recieve/receive/ > s/relavent/relevant/ > Thanks so much for the lengthly review :) I will get on it this week; by tuesday or wednesday I should have a patch to address all of these issues.
Created attachment 125815 [details] [review] Patch to adress problems pointed out by Matthias This one differs from the last patch in the following ways: - Added docs to GtkAction properties about use-action-appearance - Fixed GtkToggleButton to chain up in ->clicked() (fixes bold action in testmerge) - Added ->reset() vfunc to interface to be used instead of a NULL property name in ->update(). - gtk_activatable_do_set_related_action() public - moved notify connections and calls to gtk_activatable_update() into gtk_activatable_do_set_related_action() and its common notify handler. - fixed typos - made default of use-action-appearance = TRUE - removed weak ref to activatable from action.
The documentation for the gtk_activatable_ functions needs to be clearer on which functions are just used in GTK+ itself (gtk_activatable_update, gtk_activatable_reset) vs which are used by GtkActivatable implementations (+gtk_activatable_do_set_related_action) vs third-party code (gtk_activatable_set_related_action, gtk_activatable_set_use_action_appearance) The docs for gtk_activatable_do_set_related_action() need to spell out what it does, otherwise it is hard to know for an implementor what is left to do (well, assuming we also spell out the responsibilities of a GtkActivtable implementor somewhere). s/handleing/handling/ This feels like an implementation detail not worth calling out specifically: + * <para><note>If set to %TRUE with a related action set, + * this will also update the appearance.</note></para> Shouldn't the call to gtk_activatable_reset() be moved to do_set_related_action as well ? I notice that some set_related_action implementations don't call it, currently, and some are still calling gtk_activatable_update(..., NULL) What is the commented out code in gtkactiongroup.c about ? + /* activatable properties */ + PROP_ACTIVATABLE_ACTION, + PROP_ACTIVATABLE_USE_APPEARANCE Should use the same name as the property here, to stick with established patterns, ie PROP_ACTIVATABLE_RELATED_ACTION and PROP_ACTIVATABLE_USE_ACTION_APPEARANCE. gtk_action_add_to_proxy_list gtk_action_remove_from_proxy_list Do these really need to be exported, now that the management of the proxy list is hidden in gtk_activatable_do_set_related_action ? And why is there a ton of unrelated gtk_action api in this patch ? Nobody asked for gtk_action_set_label, gtk_action_set_stock_id, etc, etc + * Deprecated 2.16: activatables are now responsible for activating the + * action directly so this doesnt apply anymore. So, how is this compatible with the current practise, where you can hook up any widget that has an "activate" signal to an action, and have it work ? I'm growing less and less fond of this whole interface idea...
(In reply to comment #27) [...] > Shouldn't the call to gtk_activatable_reset() be moved to do_set_related_action > as well ? I notice that some set_related_action implementations don't call it, > currently, and some are still calling gtk_activatable_update(..., NULL) I can put it there, I felt that since it also needed to be called when use-action-appearance is set, that I would leave it on the implementor's side of things entirely, I guess since you point out I already had mistakes in my patch it might be better to move it into do_set_related_action(). > > What is the commented out code in gtkactiongroup.c about ? Sorry that was to be removed, I just removed it locally... > > + /* activatable properties */ > + PROP_ACTIVATABLE_ACTION, > + PROP_ACTIVATABLE_USE_APPEARANCE > > Should use the same name as the property here, to stick with established > patterns, ie PROP_ACTIVATABLE_RELATED_ACTION and > PROP_ACTIVATABLE_USE_ACTION_APPEARANCE. > > > gtk_action_add_to_proxy_list > gtk_action_remove_from_proxy_list > > Do these really need to be exported, now that the management of the proxy list > is hidden in gtk_activatable_do_set_related_action ? Very nice point, we can manage to hide these functions yes. > > And why is there a ton of unrelated gtk_action api in this patch ? > Nobody asked for gtk_action_set_label, gtk_action_set_stock_id, etc, etc They were private implementations before that used to loop over the proxies, I suppose that setting the values of properties on actions that have widgets connected to them is the preferred way of interfacing with activatable widgets, I threw in the grunt work of properly exporting the api to the action properties for free... was that a bad idea ? Should I remove those exported apis for some reason ? > + * Deprecated 2.16: activatables are now responsible for activating the > + * action directly so this doesnt apply anymore. > > So, how is this compatible with the current practise, where you can hook up any > widget that has an "activate" signal to an action, and have it work ? > > > I'm growing less and less fond of this whole interface idea... > current practice is broken, its just not true that you can connect any widget that has an activate signal to an action, not every widget that emits "activate" uses that signal for anything useful for an action, and furthermore, GtkButton proxies are currently connected by way of the "clicked" signal. This is compatible because the widgets that were known to work with actions still work, and so do derived actions in gimp, and any activate signals that were fired before - are still fired - it shouldnt seriously effect the program flow that the action is now activating on a function call, that used to be envoked by a signal but now is called directly. Maybe one day when the issues are addressed on bug 532795, we could make actions use a unified "activated" signal - but I rather like the proposed implementation better - the view listens to the model and accesses the model directly, the model never listens or accesses the view directly (except for backwards compatible handling of the proxy list and weird exceptions like recentchooseraction).
would you prefer keeping the proxy type casing code that connects signals selectively in gtkaction.c instead of calling gtk_action_activate() from the activatible directly ? (and then change that part to use a unified signal once we sort out the activatable signal)
Created attachment 126204 [details] [review] Latest patch In this new patch: - Enriched documentation of GtkActivatable - Fixed merge conflicts with trunk, i.e. handle "gicon" property in imagemenuitem/button/toolbutton activatable->update and factor out the code from gtkaction.c - Renamed PROP_* as per Matthias's request - Removes the commented code in gtkactiongroup.c - Makes gtk_action_add_to_proxy_list and counterpart private. - Fixes update/reset mistakes in last patch and now calls reset from gtk_activatable_do_set_related_action() (and is documented as such). Some notes: About the new api's, mostly it was because I had to expose half of the accessors that were missing, to call functions like gtk_action_get_gicon() from activatable implementations, it seemed natural to me to add the _set_*() counterparts, again, I can remove that code, and then it will only be available by way of a property setting, I'm not sure I understand why that is desirable, but I can easily back that out if you so request. About the "activate"/"clicked" signals, in the current implementation the direct activate call is always made in the corresponding signal class handler (theres an exception that uses a connection that was there, not sure why...), I intended on providing 2 patches tonight with an optional signal driven reverted version for taste testing, but ran into that merge conflict and have to sleep and work tomorrow. As you did point out, very strictly speaking, its something of an api change/break - but it wasnt exactly working in a uniform way to begin with (already pointed out), while this detail is not so important to me than just actually having the properties sorted out and the whole thing working, I still favor the implementation without using the activate signal, for reasons outlined in previous comments, again - I can back that out and revert to "activate"/"clicked" from GtkAction->[dis]connect_proxy() and it wont effect the rest of the patch. Anyway, now some sleep...
> About the new api's, mostly it was because I had to expose > half of the accessors that were missing, to call functions like > gtk_action_get_gicon() from activatable implementations, it seemed > natural to me to add the _set_*() counterparts, again, I can > remove that code, and then it will only be available by way of a > property setting, I'm not sure I understand why that is desirable, > but I can easily back that out if you so request. I'd be more comfortable leaving out all the new action setters and getters for now. + * gtk_activatable_update: + * @activatable: a #GtkActivatable + * @action: the related #GtkAction + * @property_name: the name of the action property to update from, or %NULL + * The docs still say property_name can be NULL, but the implementations don't handle NULL anymore... Also, do we need to make this function public at all ? I see that we need to make reset public, since activatable implementations are expected to call that under certain circumstances, but I don't think that is the case for update. If we make this non-public, the relevant docs need to be moved to the update vfunc. And the docs should mention that update implementations are responsible for looking at ::use-action-appearance. In the same vein, the reset docs should explain what 'relevant' (spelling !) properties are. +static void +gtk_button_set_related_action (GtkButton *button, + GtkAction *action) +{ + GtkButtonPrivate *priv = GTK_BUTTON_GET_PRIVATE (button); + + if (priv->action == action) + return; + + gtk_activatable_do_set_related_action (GTK_ACTIVATABLE (button), action); + + priv->action = action; +} Should we take a reference here ? If not, why ? And the docs for do_set_related_action should better explain that... Also note that you miss property change notification here (and in other property setters in the patch. static void +gtk_button_set_use_action_appearance (GtkButton *button, + gboolean use_appearance) +{ + GtkButtonPrivate *priv = GTK_BUTTON_GET_PRIVATE (button); + + if (priv->use_action_appearance != use_appearance) + { + priv->use_action_appearance = use_appearance; + + if (priv->action && use_appearance) + gtk_activatable_reset (GTK_ACTIVATABLE (button), priv->action); + } +} Shouldn't this call gtk_activable_reset unconditionally ? The docs for reset don't indicate that it is ok to sometimes not call it here... Can you explain why we need gtk_action_add_to/remove_from_proxy_list, when we already have gtk_action_connect/disconnect_proxy ? The functions seem virtually identical...
+typedef struct _GtkActionGroup GtkActionGroup; Hmm, why add this to gtkwidget.h ?
Finally, I'd like to see some docs for GtkActivatable that explain how to make a widget into an action proxy.
Ok realistically no time for a patch tonight and I wont try to hurry one in, so I'll fix it in a couple days... (In reply to comment #31) > Also, do we need to make this function public at all ? I see that we need to > make reset public, since activatable implementations are expected to call that > under certain circumstances, but I don't think that is the case for update. If > we make this non-public, the relevant docs need to be moved to the update > vfunc. And the docs should mention that update implementations are responsible > for looking at ::use-action-appearance. In the same vein, the reset docs should > explain what 'relevant' (spelling !) properties are. Ok lets put it private, its was only valuable for the doc comment anyway > +static void > +gtk_button_set_related_action (GtkButton *button, > + GtkAction *action) > +{ > + GtkButtonPrivate *priv = GTK_BUTTON_GET_PRIVATE (button); > + > + if (priv->action == action) > + return; > + > + gtk_activatable_do_set_related_action (GTK_ACTIVATABLE (button), action); > + > + priv->action = action; > +} > > Should we take a reference here ? If not, why ? And the docs for > do_set_related_action should better explain that... do_set_related_action() takes care of that, will document it better... > Also note that you miss property change notification here (and in other > property setters in the patch. As we discussed on irc... no need because they are always called from _set_property()... > static void > +gtk_button_set_use_action_appearance (GtkButton *button, > + gboolean use_appearance) > +{ > + GtkButtonPrivate *priv = GTK_BUTTON_GET_PRIVATE (button); > + > + if (priv->use_action_appearance != use_appearance) > + { > + priv->use_action_appearance = use_appearance; > + > + if (priv->action && use_appearance) > + gtk_activatable_reset (GTK_ACTIVATABLE (button), priv->action); > + } > +} > > > Shouldn't this call gtk_activable_reset unconditionally ? The docs for reset > don't indicate that it is ok to sometimes not call it here... Well, right now calling it unconditionally will not break anything or slow anything down afaics, and would allow activatables better control. I will just make sure the implementations check for a NULL action - it will also make more sense and thus be easier to document. > Can you explain why we need gtk_action_add_to/remove_from_proxy_list, when we > already have gtk_action_connect/disconnect_proxy ? The functions seem virtually > identical... Well, gtk_activatable_set_related_action() is what is now responsible for that job - so gtk_action_connect_proxy() - the old way of connecting a proxy, must get the activatable to connect itself on the action's behalf (so older code now runs through the set_related_action() code path). Now the action has a proxy list to maintain, for recentchooser actions and exotic actions that want to call functions on their proxies, and furthermore - there is existing code (I think in gimp for instance) that derives actions and calls parent_class->connect_proxy(), which is why the proxy list is still updated from the base connect_proxy() implementation, since action implementing code probably assumes the connection/disconnection to happen in that vfunc. So we need a.) to alias the old api to use the new one b.) a way for an activatable to register to the actions proxy list and emit the action signals and call the supported derived actions connect_proxy() vfuncs Maybe we should call it _gtk_action_connect_proxy_internal() or something else ? (In reply to comment #32) > +typedef struct _GtkActionGroup GtkActionGroup; > > Hmm, why add this to gtkwidget.h ? > I can remove that now, it was needed for gtk_action_get_group() to be exposed, which was needed to emit the group change, and none of that is needed anymore (sorry for not catching that, I must have been in a hurry).
To get this moving, I've committed the GtkAction setters and getters now. Can you rebase the patch on top of that ? * gtk/gtk.symbols: * gtk/gtkaction.[hc]: Add setters and getters for GtkAction properties, in preparation for bug 560228.
Great, I was writing up another patch around meeting time today but didnt have time to finish (or time to re-review my patch before resubmitting it...) I'll update and have something by tomorrow afternoon for sure.
That would be good. This is the last thing that I want to get into 2.15.1, and the last major api piece I want to get into 2.16
Created attachment 126943 [details] [review] Another patch Ok, so this one pretty much does everything we talked about plus rebasing to svn with the exported apis (thanks) : - Make _activatable_update() private - Document the vfuncs on GtkActivatableIface - Call gtk_activatable_reset() unconditionally when use-action-appearance OR related-action changes, and documented to do so. - tried a retake on gtk_activatable_do_set_related_action() docs - Removed unneeded gtk_action_get_group and put typedef GtkActionGroup back into its own header - Finally, I added more descriptions about making a widget activatable and added example code to the description, and I tried and tried to build the docs but couldnt figure where to include the new entity in gtk-docs.sgml - I'm only unsure if the codelisting will show up and be parsed correctly through gtk-doc...
The example looks a bit long, but that can be improved later on; I think it is worth pointing out in the example that this * gtk_activatable_do_set_related_action (GTK_ACTIVATABLE (bar), action); * * priv->action = action; is ok because do_set_related_action refs the action. +/** + * gtk_activatable_reset: + * @activatable: a #GtkActivatable + * @action: the related #GtkAction Should probably mention that @action may be NULL here (or rather, the docs of the reset vfunc need to mention that action == NULL has to be handled). On the other hand, it seems odd that every implementation of reset has a if (!action) return; Why is it important to let reset be called with a NULL action ? + /* virtual table */ + void (* update) (GtkActivatable *, GtkAction *, const gchar *); + void (* reset) (GtkActivatable *, GtkAction *); Even if the compiler can do without, we tend to put parameter names in here, and put each argument on a line of its own: void (* update) (GtkActivatable *activatable, GtkAction *action, const gchar *property_name); +gtk_action_get_icon_name +gtk_action_get_is_important gtk_action_get_sensitive +gtk_action_get_short_label +gtk_action_get_stock_id +gtk_action_get_tooltip gtk_action_get_type G_GNUC_CONST gtk_action_get_visible +gtk_action_get_visible_horizontal +gtk_action_get_visible_vertical gtk_action_is_sensitive gtk_action_is_visible gtk_action_new gtk_action_set_accel_group gtk_action_set_accel_path +gtk_action_set_gicon +gtk_action_set_icon_name +gtk_action_set_is_important +gtk_action_set_label gtk_action_set_sensitive +gtk_action_set_short_label +gtk_action_set_stock_id +gtk_action_set_tooltip gtk_action_set_visible +gtk_action_set_visible_horizontal +gtk_action_set_visible_vertical +gtk_action_unblock_activate Hmm, didn't I already add those ? gtk_widget_get_action (GtkWidget *widget) { g_return_val_if_fail (GTK_IS_WIDGET (widget), NULL); + g_return_val_if_fail (GTK_IS_ACTIVATABLE (widget), NULL); I think this function should continue to silently return NULL for non-activatable widgets, as it used to do. Looks ok otherwise. Scary patch, though.
(In reply to comment #39) [...] > Should probably mention that @action may be NULL here (or rather, the docs of > the reset vfunc need to mention that action == NULL has to be handled). On the > other hand, it seems odd that every implementation of reset has a > > if (!action) > return; > > Why is it important to let reset be called with a NULL action ? This is a hard call to make - I changed it to handle NULLs so that we could call gtk_activatable_reset() unconditionally, as you pointed out in the last comments that it seemed that it should be called unconditionally (as specially since the implementor is expected to call it). This way: - The documentation will be clearer wrt when reset is called/must be called - Allows more flexability in implementing action - i.e. the NULL action could theoretically be handled by some implementors - either inside or outside of gtk+ I guess the alternative question would be: "Why does my ->reset() vfunc get called when an action is set but not when its unset ?" While it is a hard call to make, I would still rather be asked why a null action should be handled; then be asked why reset() only resets when an action is set and not when it goes away.
Fair enough, leave it as is, then.
Created attachment 127029 [details] [review] Updated patch. Ok this one makes the last requested changes; - Fixed gtk.symbols (strange my merge didnt leave me any conflicts there...) - Adjusted docs on gtk_activatable_reset & GtkActivatableIface->reset - Added note in the example about gtk_activatable_do_set_related_action() referencing the action. - Silent NULL return from gtk_widget_get_action() for non-activatable widgets
Ok, I guess we should go with this. Can you add the new functions to gtk-sections.txt when committing ? (Need to add a new section for GtkActivatable, add the get_type function to gtk.types, and include the new entity next to the other action-related stuff in gtk-docs.sgml)
Actually, before committing this, it would be good to check that it work fine with epiphany as well. The seem to be doing some semi-funky stuff with actions.
Unfortunately, quite a bit of fallout in ephy: epiphany:10265): Gtk-CRITICAL **: gtk_label_set_use_underline: assertion `GTK_IS_LABEL (label)' failed (epiphany:10265): Gtk-CRITICAL **: gtk_label_set_ellipsize: assertion `GTK_IS_LABEL (label)' failed (epiphany:10265): Gtk-CRITICAL **: gtk_label_set_max_width_chars: assertion `GTK_IS_LABEL (label)' failed ** (epiphany:10265): CRITICAL **: tool_item_enter_cb: assertion `action != NULL' failed ** (epiphany:10265): CRITICAL **: tool_item_enter_cb: assertion `action != NULL' failed ** (epiphany:10265): CRITICAL **: tool_item_enter_cb: assertion `action != NULL' failed bookmarks don't work at all
The warnings I can get rid of by two changes to do_set_related_action: 1) g_object_set_data (activatable, "gtk-action", action); Since ephy looks at that... 2) move gtk_activatable_reset (activatable, action); before the add_to_proxy_list call, since ephy expects the menuitems label to be set up in its ::connect-proxy signal handler. But still, no working bookmark menu. Bookmark toolitems do work.
Seems that the bookmarks don't work because ephy relies on actions to be activated when a menuitem proxy with a submenu is activated. The code in gtkmenuitem.c only calls gtk_action_activate if it doesn't have a submenu...
removing that check makes ephy bookmarks work again.
Thanks alot for doing that debugging, I know were on a tight timeline, and I'm working early tomorrow... I will be back at 5 or 6:00pm EST tomorrow...
I'm sure there will still be some fallout. I've committed the version that makes epiphany work. 2009-01-23 Matthias Clasen <mclasen@redhat.com> Bug 560228 – Add "action-controller" property to GtkWidgetClass Rework the way actions and proxies interact, to make the interaction less ad hoc, more extensible, and better suited for support in GUI builders like glade. To be used as a proxy, a widget must now implement the GtkActivatable interface, and GtkActivatable implementations are responsible for syncing their appearance with the action and for activating the action. All the widgets that are commonly used as proxies implement GtkActivatable now. Patch by Tristan van Berkom. * gtk/gtkactivatable.[hc]: The GtkActivatable interface. * gtk/gtkbutton.c: * gtk/gtktogglebutton.c: * gtk/gtktoolitem.c: * gtk/gtktoolbutton.c: * gtk/gtktoggletoolbutton.c: * gtk/gtkmenuitem.c: * gtk/gtkcheckmenuitem.c: * gtk/gtkimagemenuitem.c: * gtk/gtkradiomenuitem.c: * gtk/gtkrecentchooserprivate.h: * gtk/gtkrecentchooser.c: * gtk/gtkrecentchooserdefault.c: * gtk/gtkrecentchoosermenu.c: Implement GtkActivatable. * gtk/gtkaction.[hc]: Move appearance synchronization to GtkActivatable implementations. * gtk/gtkradioaction.c: * gtk/gtkrecentaction.c: * gtk/gtktoggleaction.c: * gtk/gtkactiongroup.c: Adapt. * gtk/gtk.h: Include gtkactivatable.h * gtk/gtk.symbols: Add new functions
(In reply to comment #47) > Seems that the bookmarks don't work because ephy relies on actions to be > activated when a menuitem proxy with a submenu is activated. The code in > gtkmenuitem.c only calls gtk_action_activate if it doesn't have a submenu... Just for reassurance, does this still happen in gtk+ trunk with the final patch? I rely on that not only in epiphany but also in other programmes I've written, and probably others rely on that too...
No, I've fixed that. menuitems always activate their action, submenu or not.
Created attachment 127149 [details] [review] I believe this fixes buggy toolbuttons Matthias, does this fix your toolbuttons ? I fired up evolution after making some changes and didnt see any labelless or iconless toolbuttons (but I'm not a regular evo user...), there was an obvious flaw I was quickly able to fix.
Ah I missed that you had mentioned which buttons on irc yes I definitely have the 'Send' and 'Attach' toolbuttons in the compose mail window working with this patch.
Yes, that seems to fix the toolbutton issues in evo and rhythmbox, thanks. I've committed that and added a testcase for such toolbuttons to testactions.c
This has broken PolicyKit-gnome that uses a custom PolKitGnomeToggleAction GObject. The Red Hat bug is at https://bugzilla.redhat.com/show_bug.cgi?id=484357 and I would appreciate some ideas on how to fix things. I guess this is a regression, but PolKitGnomeToggleAction is doing some funky things. Code is here if anyone is interested: http://hal.freedesktop.org/releases/PolicyKit-gnome-0.9.2.tar.bz2 Thanks.
I'm running off to work now but from a glance at the bug reffered to: That code is asserting on GTK_IS_TOGGLE_ACTION, PolKitGnomeToggleAction is supposed to be a toggle action derivative it seems - this has nothing at all to do with its proxies being activatable or not. I dont see what it is that we could have changed in GTK+ to make your PolKitGnomeToggleAction suddently start failing to derive from the right object class, but hey - anything is possible ;-) Your PolKitGnomeToggleAction exists for a long time ? when you back out activatables from your GTK+ tree then your toggle actions start working again ?
Tristan, if I understand things correctly, PolKitGnomeToggleAction derives from PolKitGnomeAction, which in turn derives from GtkAction. I assumed that GtkAction needs to implement GtkActivatable, and because PolKitGnomeToggleAction doesn't there would be that warning. The code in PolicyKit-gnome looks incorrect, but I think you're correct in saying it's not this patch that's broken it. Thanks.
No, actions don't implement the activatable interface, proxies do.