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 774444 - ActionMap::add_action_with_parameter() does not set underlying parameter type, leaving it unusable
ActionMap::add_action_with_parameter() does not set underlying parameter type...
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: giomm
2.50.x
Other All
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2016-11-15 01:26 UTC by Daniel Boles
Modified: 2017-05-05 10:10 UTC
See Also:
GNOME target: ---
GNOME version: 3.21/3.22


Attachments
[PATCH] ActionMap: Make add_action_with_parameter() work (2.89 KB, patch)
2016-12-01 02:21 UTC, Daniel Boles
none Details | Review
[PATCH 1/4] Gio::ActionMap: Fix add_action_with_parameter() (1.91 KB, patch)
2016-12-25 16:47 UTC, Daniel Boles
committed Details | Review
[PATCH 2/4] Gio::ActionMap: Clarify doc of ActivateWithParameterSlot (1.05 KB, patch)
2016-12-25 16:48 UTC, Daniel Boles
committed Details | Review
[PATCH 3/4] Gio::ActionMap: Add function to ActivateSlot doc (764 bytes, patch)
2016-12-25 16:49 UTC, Daniel Boles
committed Details | Review
[PATCH 4/4] Gio::ActionMap: Improve declaration order/spacing (6.04 KB, patch)
2016-12-25 16:50 UTC, Daniel Boles
committed Details | Review
[patch for 2.50] Remove TODOs about the ABI break or adding VFUNCs (8.18 KB, patch)
2016-12-27 13:04 UTC, Daniel Boles
none Details | Review
ActionMap—Reorder add_action_with_parameter’s args (2.07 KB, patch)
2017-02-14 00:12 UTC, Daniel Boles
committed Details | Review
ActionMap: Really fix add_action_with_parameter() (910 bytes, patch)
2017-05-04 14:10 UTC, Daniel Boles
committed Details | Review
ActionMap: Improve add_action_with_parameter docs (1.96 KB, patch)
2017-05-04 14:11 UTC, Daniel Boles
committed Details | Review

Description Daniel Boles 2016-11-15 01:26:02 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!
Comment 1 Daniel Boles 2016-11-15 01:30:06 UTC
> 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.
Comment 2 Daniel Boles 2016-11-29 20:05:07 UTC
Back to the main issue, can anyone comment on whether my conclusion is correct?
Comment 3 Daniel Boles 2016-12-01 02:21:26 UTC
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!
Comment 4 Kjell Ahlstedt 2016-12-23 12:31:24 UTC
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.
Comment 5 Daniel Boles 2016-12-24 14:30:52 UTC
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.
Comment 6 Kjell Ahlstedt 2016-12-25 10:04:42 UTC
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.
Comment 7 Daniel Boles 2016-12-25 16:47:33 UTC
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.
Comment 8 Daniel Boles 2016-12-25 16:48:30 UTC
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.
Comment 9 Daniel Boles 2016-12-25 16:49:00 UTC
Created attachment 342463 [details] [review]
[PATCH 3/4] Gio::ActionMap: Add function to ActivateSlot doc

> It's used in add_action_with_bool() too.
Comment 10 Daniel Boles 2016-12-25 16:50:34 UTC
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.
Comment 11 Kjell Ahlstedt 2016-12-26 15:32:42 UTC
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.
Comment 12 Kjell Ahlstedt 2016-12-26 15:34:23 UTC
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.
Comment 13 Kjell Ahlstedt 2016-12-26 15:35:31 UTC
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);
Comment 14 Kjell Ahlstedt 2016-12-26 15:36:54 UTC
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.
Comment 15 Daniel Boles 2016-12-26 16:11:33 UTC
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?
Comment 16 Daniel Boles 2016-12-26 16:23:15 UTC
(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 17 Daniel Boles 2016-12-26 16:52:02 UTC
Comment on attachment 342461 [details] [review]
[PATCH 1/4] Gio::ActionMap: Fix add_action_with_parameter()

=> master as f9db4fab71a5a93ada9e04117e311ab9d9ba83f3
Comment 18 Daniel Boles 2016-12-26 16:52:31 UTC
Comment on attachment 342462 [details] [review]
[PATCH 2/4] Gio::ActionMap: Clarify doc of ActivateWithParameterSlot

=> master      as aa40819c5b7b8cd6e0e29ebe14efdc76d20d437f
=> glibmm-2-50 as 1b854ba341da0704dd2971dcc740c4d9af712dbd
Comment 19 Daniel Boles 2016-12-26 16:53:06 UTC
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 20 Daniel Boles 2016-12-26 16:53:47 UTC
Comment on attachment 342464 [details] [review]
[PATCH 4/4] Gio::ActionMap: Improve declaration order/spacing

=> master as 22b2ecabf9c2d0c1f59f05e53f0ceef053b14103
        (and cf23cff238aa882465bdcc44bf05ea67e155355b)
Comment 21 Kjell Ahlstedt 2016-12-27 09:28:55 UTC
(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.
Comment 22 Daniel Boles 2016-12-27 10:19:35 UTC
(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.
Comment 23 Daniel Boles 2016-12-27 13:04:23 UTC
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 24 Daniel Boles 2016-12-27 13:09:31 UTC
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.
Comment 25 Daniel Boles 2016-12-27 13:26:42 UTC
(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
Comment 26 Daniel Boles 2017-02-14 00:12:17 UTC
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
Comment 27 Kjell Ahlstedt 2017-02-14 15:03:22 UTC
Review of attachment 345684 [details] [review]:

Okay, please push to the master branch.
Comment 28 Daniel Boles 2017-02-14 19:08:15 UTC
Comment on attachment 345684 [details] [review]
ActionMap—Reorder add_action_with_parameter’s args

Thanks, Kjell!
Comment 29 Murray Cumming 2017-02-23 11:20:34 UTC
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.
Comment 30 Daniel Boles 2017-05-04 09:57:50 UTC
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!
Comment 31 Murray Cumming 2017-05-04 13:38:08 UTC
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
Comment 32 Daniel Boles 2017-05-04 13:44:59 UTC
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!
Comment 33 Murray Cumming 2017-05-04 13:48:40 UTC
(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.
Comment 34 Daniel Boles 2017-05-04 14:10:56 UTC
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.
Comment 35 Daniel Boles 2017-05-04 14:11:45 UTC
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.
Comment 36 Murray Cumming 2017-05-05 10:10:51 UTC
Thanks.