GNOME Bugzilla – Bug 774444
ActionMap::add_action_with_parameter() does not set underlying parameter type, leaving it unusable
Last modified: 2017-05-05 10:10:51 UTC
I apologise in advance if I'm just doing this wrong... but it seems to me that Glibmm::ActionMap::add_action_with_parameter() is missing something important. Namely, it simply calls add_action() and then connects to the activate signal of the result. However, never does it actually set the parameter (Variant) type of the underlying GAction. This means that subsequent attempts to pass a target fail due to parameter type mismatch. I'm not sure if there is a solution to make this usable. I thought we could do something like Gio::SimpleAction::create("name", variant_type) and insert the result manually to the relevant action group/map, specifying the relevant slot, but I haven't found any way to do this yet, in either Action, Group or Map. By my understanding, the required fix is that add_action_with_parameter() should also set the Variant type for us, so that the resulting action is actually usable with targets passed to the parameter taken by the action callback. I hope I understand this correctly and this is useful. Thanks!
> I'm not sure if there is a solution to make this usable. > I thought we could do something like Gio::SimpleAction::create("name", > variant_type) and insert the result manually to the relevant action > group/map, specifying the relevant slot, but I haven't found any way > to do this yet, in either Action, Group or Map. Never mind. I should try being awake before writing bug reports. :) action.signal_activate() should work fine as a workaround.
Back to the main issue, can anyone comment on whether my conclusion is correct?
Created attachment 341102 [details] [review] [PATCH] ActionMap: Make add_action_with_parameter() work > It didn't register the parameter type for the action, so the slot > expected a null target, breaking it if we passed a non-null one. This builds fine, but I've not specifically tested it yet, though it seems to make sense. I'll try to test it in the coming days unless someone else beats me to it. This is against 2.50 and includes deprecation of the overload that fails to set the parameter type. (Which seemed more useful that patching master and making someone else write the deprecations.) Thanks!
Your patch looks fine, and quite obvious. You can push it to the master branch. Even better: Push a patch where your new corrected add_action_with_parameter() replaces the old one. I.e. remove the old one instead of deprecating it. The master branch will become the ABI/API- breaking glibmm-2.52, where all deprecated functions have been removed. Concerning the glibmm-2-50 branch, see the discussion in bug 774903. Don't add or deprecate anything now, but keep the bug open until we know what to do.
Thanks. Should the passed slot be wrapped in sigc::hide()? It is in add_action(), but I don't know what that's for. All the docs I can find on sigc::hide() on a quick search seem too old to be very relevant.
No, don't use sigc::hide(). It's used in combination with Gio::ActionMap:: ActivateSlot. SimpleAction::signal_activate().connect() requires a slot with a parameter, but ActivateSlot has no parameter. sigc::hide() hides the parameter, passed from glib before the signal handler call is passed on to the connected slot. In add_action_with_parameter() and ActivateWithParameterSlot the parameter shall not be hidden. I tried to compile ActionMap with sigc::hide() in add_action_with_parameter(). As I suspected, it does not compile.
Created attachment 342461 [details] [review] [PATCH 1/4] Gio::ActionMap: Fix add_action_with_parameter() Thanks for the explanation. I came up with a few other suggested patches, so I'll upload them all here for confirmation.
Created attachment 342462 [details] [review] [PATCH 2/4] Gio::ActionMap: Clarify doc of ActivateWithParameterSlot > This brings it in line with the existing doc comment for ActivateSlot.
Created attachment 342463 [details] [review] [PATCH 3/4] Gio::ActionMap: Add function to ActivateSlot doc > It's used in add_action_with_bool() too.
Created attachment 342464 [details] [review] [PATCH 4/4] Gio::ActionMap: Improve declaration order/spacing > This puts several things in more logical places and adds a couple of > line breaks that make it easier to see the key sections in the header. I felt that the current declarations are pretty jumbled up, for no apparent reason, hence this. It's hopefully more logic than just personal preference! I wasn't sure whether we can freely move _WRAP_VFUNCs around, so I left them where they are for now.
Review of attachment 342461 [details] [review]: Ok for the master branch, but you should add the documentation * @param parameter_type The type of parameter to be passed to the slot. like you did in your patch in comment 3.
Review of attachment 342462 [details] [review]: Ok. You can push it to the master branch, and also to the glibmm-2-50 branch if you like. Documentation fixes are allowed in stable branches.
Review of attachment 342463 [details] [review]: Ok. You can push it to the master branch, and also to the glibmm-2-50 branch if you like. I noticed some other details that should be fixed in the documentation. I'll fix them when you've pushed your patches. If you want to do it, you're welcome, of course. ActivateWithStringParameterSlot: * void on_slot_activated(const Glib::VariantBase& parameter); shall be * void on_slot_activated(const Glib::ustring& parameter); ActivateWithIntParameterSlot: * See add_action_radio_int(). * void on_slot_activated(const Glib::VariantBase& parameter); shall be * See add_action_radio_integer(). * void on_slot_activated(int parameter);
Review of attachment 342464 [details] [review]: Ok. You can push it to the master branch, and a similar patch to the glibmm-2-50 branch if you like. As the add_action_with_parameter() shall not (yet?) be fixed in the glibmm-2-50 branch, this patch won't apply there as is. If the locations of _WRAP_VFUNCs, _WRAP_SIGNALs with default handlers or other virtual methods are swapped, the compiler-generated vtable will change, meaning an ABI break. At the moment and a few months ahead, ABI breaks are accepted in the master branch.
Thanks for the reviews! Pushed to master and currently cherry-picking for glibmm-2-50. Now that we've branched, should TODOs about ABI breaks be removed as we go along?
(In reply to Kjell Ahlstedt from comment #14) > If the locations of _WRAP_VFUNCs, _WRAP_SIGNALs with default handlers or > other > virtual methods are swapped, the compiler-generated vtable will change, > meaning > an ABI break. At the moment and a few months ahead, ABI breaks are accepted > in > the master branch. Makes sense. Am I correct to think that in this case, rearranging _WRAP_METHOD calls is OK, as is moving around _WRAP_VFUNCS as a block (i.e. so long as they retain their relative order to other vfuncs)? It's not that important but would be nice to get a more consistent order with some of the tidier class declarations.
Comment on attachment 342461 [details] [review] [PATCH 1/4] Gio::ActionMap: Fix add_action_with_parameter() => master as f9db4fab71a5a93ada9e04117e311ab9d9ba83f3
Comment on attachment 342462 [details] [review] [PATCH 2/4] Gio::ActionMap: Clarify doc of ActivateWithParameterSlot => master as aa40819c5b7b8cd6e0e29ebe14efdc76d20d437f => glibmm-2-50 as 1b854ba341da0704dd2971dcc740c4d9af712dbd
Comment on attachment 342463 [details] [review] [PATCH 3/4] Gio::ActionMap: Add function to ActivateSlot doc => master as 2bb9c26b88388535decbd9fb0d9a069d0f877725 => glibmm-2-50 as 758135aaba5eb29d816dfb899734aa50f764bd42
Comment on attachment 342464 [details] [review] [PATCH 4/4] Gio::ActionMap: Improve declaration order/spacing => master as 22b2ecabf9c2d0c1f59f05e53f0ceef053b14103 (and cf23cff238aa882465bdcc44bf05ea67e155355b)
(In reply to Daniel Boles from comment #15) > Now that we've branched, should TODOs about ABI breaks be removed as we go > along? Yes, now is the time to fix TODOs that will break ABI (but only in the master branch). A quick search showed me that some remain to be fixed both in glibmm and gtkmm. (In reply to Daniel Boles from comment #16) > Makes sense. Am I correct to think that in this case, rearranging _WRAP_METHOD > calls is OK, as is moving around _WRAP_VFUNCS as a block (i.e. so long as they > retain their relative order to other vfuncs)? It's not that important but > would be nice to get a more consistent order with some of the tidier class > declarations. Yes, I'm almost certain that you can rearrange _WRAP_METHODs and _WRAP_PROPERTYs without affecting ABI. You can also rearrange _WRAP_SIGNALs without default handlers, and non-virtual hand-coded methods. It's quite common to group all _WRAP_* directives of the same kind together, with _WRAP_METHOD first and _WRAP_VFUNC last, even if a particular _WRAP_METHOD is closely related to a particular _WRAP_VFUNC or _WRAP_SIGNAL. Like you say, this is not that important.
(In reply to Kjell Ahlstedt from comment #21) > (In reply to Daniel Boles from comment #15) > > Now that we've branched, should TODOs about ABI breaks be removed as we go > > along? > > Yes, now is the time to fix TODOs that will break ABI (but only in the master > branch). A quick search showed me that some remain to be fixed both in glibmm > and gtkmm. Oh, it's definitely that time! But I should've been clearer: I was talking about removing TODOs from the glibmm-2-50 branch, when they talk about stuff we previously wanted to do after branching. These should presumably definitely be removed, so it's probably a bit silly of me to ask in the first place. :) I'll look at removing the obsolete TODOs I noticed later, in that case. (In reply to Kjell Ahlstedt from comment #21) > Yes, I'm almost certain that you can rearrange _WRAP_METHODs and > _WRAP_PROPERTYs > without affecting ABI. You can also rearrange _WRAP_SIGNALs without default > handlers, and non-virtual hand-coded methods. > > It's quite common to group all _WRAP_* directives of the same kind together, > with _WRAP_METHOD first and _WRAP_VFUNC last, even if a particular > _WRAP_METHOD > is closely related to a particular _WRAP_VFUNC or _WRAP_SIGNAL. > > Like you say, this is not that important. Thanks for the summary. It sounds like applying the same reordering commit to 2.50, but without adding the missing parameter, should be safe. By my understanding, this seems like a good order to aim for: * _IGNOREs * _WRAP_METHODs * hand-coded methods * _WRAP_VFUNCs et al.
Created attachment 342496 [details] [review] [patch for 2.50] Remove TODOs about the ABI break or adding VFUNCs (In reply to Daniel Boles from comment #22) > Oh, it's definitely that time! But I should've been clearer: I was talking > about removing TODOs from the glibmm-2-50 branch, when they talk about stuff > we previously wanted to do after branching. These should presumably > definitely be removed, so it's probably a bit silly of me to ask in the > first place. :) > > I'll look at removing the obsolete TODOs I noticed later, in that case. Behold: the power of > vim `grep -l -dskip '\(TODO\|FIXME\|XXX\).*\(abi\|ABI\|2[.-]5[12]\|VFUNC\)' **` If anyone knows of a way I can add even more punctuation to that, let me know!
Comment on attachment 342496 [details] [review] [patch for 2.50] Remove TODOs about the ABI break or adding VFUNCs Bah, I just realised this got based on a weird checkout somehow and doesn't apply to glibmm-2-50. I'll submit a proper version soon.
(In reply to Daniel Boles from comment #24) > Bah, I just realised this got based on a weird checkout somehow and doesn't > apply to glibmm-2-50. I'll submit a proper version soon. Apparently, that "weird checkout" was just master. That explains why I had so little work to do! I'll upload the fixed patch at this new bug: https://bugzilla.gnome.org/show_bug.cgi?id=776520
Created attachment 345684 [details] [review] ActionMap—Reorder add_action_with_parameter’s args Trivial, but if it's all the same to you, I'd like to swap these two arguments. This is more logical, matches e.g. the corresponding methods in SimpleAction, and I'm pretty sure this is the order I really wanted and used in a different draft of the original patch. Since the patch adding parameter_type has only hit master so far, ABI breaking is fair game, but still, I didn't feel right just pushing this without checking first
Review of attachment 345684 [details] [review]: Okay, please push to the master branch.
Comment on attachment 345684 [details] [review] ActionMap—Reorder add_action_with_parameter’s args Thanks, Kjell!
Thanks, Daniel. In future, please try to make your commits messages more like the others. For instance, I would prefer ActionMap: Reorder add_action_with_parameter()’s args to this: ActionMap—Reorder add_action_with_parameter’s args I find that clearer. Thanks.
Following on from the discussion about glibmm-2-52 being stable but API-modifying, could this be included in that release? I guess we would need to deprecate the nonfunctional original overload, and keep it around to maintain ABI compatibility. Thanks!
I have pushed some of these commits to the glibmm-2-52 branch. I guess these are the API-changing ones that you mean: https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-52&id=19ea8885ef17743c7c6e436e770b3b3835ca5016 and https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-52&id=61b774384195d5e0ec7dada546a3f69c6f273408 I have indeed added the previous API back, to maintain ABI: https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-52&id=ef4256e67dd3c9c02af62ae914627997a6a21bf6 Maybe you'd like to improve the deprecation comment. However, though I have not followed this discussion, it seems odd that this method doesn't actually call add_action(const Glib::RefPtr<Action>&). Like the others, it used to, but doesn't after the commit: https://git.gnome.org/browse/glibmm/commit/?id=f9db4fab71a5a93ada9e04117e311ab9d9ba83f3
Thanks! (In reply to Murray Cumming from comment #31) > Maybe you'd like to improve the deprecation comment. I think it would be good to clarify why the other overload should be used, i.e. because the now-deprecated one was missing a crucial argument. > However, though I have not followed this discussion, it seems odd that this > method doesn't actually call add_action(const Glib::RefPtr<Action>&). Like > the others, it used to, but doesn't after the commit: > https://git.gnome.org/browse/glibmm/commit/ > ?id=f9db4fab71a5a93ada9e04117e311ab9d9ba83f3 Ouch... How did I miss that? Yes, the newly created action should be added to self. D'oh. I'll prepare a patch for that. Thanks for catching it!
(In reply to Daniel Boles from comment #32) > I think it would be good to clarify why the other overload should be used, > i.e. because the now-deprecated one was missing a crucial argument. If the previous method can just never work, because it's meaningless without a parameter_type, then it's fine to say that. I have not given this any thought.
Created attachment 351055 [details] [review] ActionMap: Really fix add_action_with_parameter() I replaced the old line that both created an Action without a parameter and added it, only with a line that creates an Action with a parameter... but did not add it. Of course, we need to do that, too.
Created attachment 351056 [details] [review] ActionMap: Improve add_action_with_parameter docs • Clarify why the old overload has been deprecated: it simply does not work for the desired result because the parameter_type was not passed • Elaborate on the purpose of the parameter_type in the new overload.
Thanks.