GNOME Bugzilla – Bug 777953
SimpleAction: Docs say a signal_change_state handler must call set_state, but latter is protected
Last modified: 2017-05-04 13:45:33 UTC
docs of signal_change_value(): https://developer.gnome.org/glibmm/stable/classGio_1_1SimpleAction.html#afeadbbab5698569fd6a992ad4e8e6b78 >If no handler is connected to this signal then the default behaviour is to call >g_simple_action_set_state() to set the state to the requested value. If you >connect a signal handler then no default action is taken. If the state should >change then you must call g_simple_action_set_state() from the handler. ...but its implementation of SimpleAction::set_state() is protected, so you can't call it. What have I missed? This seems broken. Shouldn't that function be public, if we're explicitly supposed to use it to make the requested value change actually take effect? If so, can the accessibility be fixed? That doesn't break ABI afaict. [looks at the source] and oh hey, the header currently says: > //TODO: Add templated version of this, renaming this to set_state_variant(), like Action::change_state()? > //This is protected because the C docs say "This should only be called by the implementor of the action." > // though that is not entirely clear. We can make this public if somebody needs it. Yeah, as the other bit of the docs make clear, this means the implementor of the signal handler. So it should be public. Anyone who actually wants to use a change_value handler and actually make the requested value take effect needs it. Surely?
Created attachment 344582 [details] [review] [PATCH 1/3] Gio::Action: #include <glibmm/variant.h> in hg Some bonus patches! They don't quite seem worth a ticket of their own. > Since some methods return VariantBase by value, just forward-declaring > would mean that users would have to manually #include <glibmm/variant.h> > themselves to actually use the methods, which isn’t particularly useful. ...though if it's your policy to require that they do so, of course disregard this.
Created attachment 344583 [details] [review] [PATCH 2/3] Gio::Action: Remove unneeded #includes in ccg If you reject patch #1, then we can trim this to only remove the include of exceptionhandler.h, which isn't needed in either case.
Created attachment 344584 [details] [review] [PATCH 3/3] Gio::SimpleAction: Make set_state() public > This is needed to actually apply the requested state in a custom handler > for signal_change_state(). It was protected on an assumption that when > the C docs for g_simple_action_set_state() say “This should only be > called by the implementor of the action”, that meant the implementor of > a derived class. But the C docs for signal change-state make it clear > that it really means the implementor of the signal handler. Without > being able to call this, that implementor can’t apply the new state.
The patch is against master, but I would say we really ought to push it to 2.50 also (unless there's some bizarre way that it harms ABI, which I can't imagine)
-- About include directives (your first two patches): action.h can be compiled without including it in action.cc, which is surprising at first, because it uses not only VariantBase but also Variant<>, which is not forward-declared. Explanation: giomm/action.h includes glibmm/interface.h glibmm/interface.h includes glibmm/object.h glibmm/object.h includes glibmm/containerhandle_shared.h glibmm/containerhandle_shared.h includes glibmm/variant.h Your patches make the dependence obvious. Please push them. These patches certainly don't break API or ABI. They can be pushed also to the glibmm-2-50 branch. If you look at some header files in glibmm and gtkmm, you'll notice that they often just forward-declare classes that are used in some of the methods. That's most common if the forward-declared classes are used only in methods that only a few users of the header file are expected to use. -- About set_state/change_state: I assume that you mean signal_change_state() and set_state(). There are no signal_change_value() and set_value() in Action or SimpleAction. What is meant by "the implementor of the action"? Despite the example code in the documentation of GSimpleAction::change-state, I prefer to interpret it as SimpleAction or a subclass of SimpleAction in a C++ environment. In C++ it's easy to subclass Gio::SimpleAction. In C it's much more difficult to make a subclass. Therefore it's tempting to connect a signel handler where it would be more natural to subclass and override the default signal handler. This is discussed in an appendix in the gtkmm tutorial, https://developer.gnome.org/gtkmm-tutorial/stable/sec-signals-comparison.html.en In this case you can't override the default signal handler. Gio::SimpleAction:: signal_change_state() has no default signal handler. But you can subclass Gio::SimpleAction, put a signal handler in the subclass and connect the signal handler in the subclass's constructor. If you've got an application, where you find it more natural to let a signal handler in another class call set_state(), there is a simple workaround. class MySimpleAction : public Gio::SimpleAction { public: void my_set_state(const Glib::VariantBase& value) { set_state(value); } }; To summarize, I think it's reasonable to let set_state() remain protected. But as usual I let Murray decide.
Thanks for reading! (In reply to Kjell Ahlstedt from comment #5) > -- About include directives (your first two patches): > [....] > Your patches make the dependence obvious. Please push them. These patches > certainly don't break API or ABI. They can be pushed also to the glibmm-2-50 > branch. Cool, thanks! I'll do so later. > -- About set_state/change_state: > I assume that you mean signal_change_state() and set_state(). There are no > signal_change_value() and set_value() in Action or SimpleAction. Ouch, yes. I've fixed the title. > What is meant by "the implementor of the action"? Despite the example code in > the documentation of GSimpleAction::change-state, I prefer to interpret it as > SimpleAction or a subclass of SimpleAction in a C++ environment. > In C++ it's easy to subclass Gio::SimpleAction. In C it's much more difficult > to make a subclass. Therefore it's tempting to connect a signel handler where > it would be more natural to subclass and override the default signal handler. > This is discussed in an appendix in the gtkmm tutorial, > https://developer.gnome.org/gtkmm-tutorial/stable/sec-signals-comparison. > html.en > > In this case you can't override the default signal handler. > Gio::SimpleAction:: > signal_change_state() has no default signal handler. > > But you can subclass > Gio::SimpleAction, put a signal handler in the subclass and connect the > signal > handler in the subclass's constructor. Well, signal_change_state() is pretty useless without a way to actually apply the requested change, so this means it's mostly useless in Gio::SimpleAction itself. By that logic - and note: I don't advocate this! - then maybe this signal should be protected in SimpleAction and overridden to public visibility if a derived class is required. But requiring subclassing here seems heavy-handed to me. Making SimpleActions with state actually useful out-of-the-box is nicer.* I generally prefer to enhance *mm classes through aggregation/composition, rather then inheritance. This avoids lots of space being consumed in my binary for largely unused vtables, for one. And it's generally a mantra in C++ not to inherit unless it's actually needed. * I see that it is kind of bad that C necessarily has to expose this method (and many others) as open to public calling, but then say 'don't call this, or at least in 99% of situations'. And that we would somewhat be duplicating that issue here if set_state() were public. But I don't think it's unreasonable to have it public and caution users only to call it in a few appropriate scenarios. > If you've got an application, where you find it more natural to let a signal > handler in another class call set_state(), there is a simple workaround. > class MySimpleAction : public Gio::SimpleAction > { > public: > void my_set_state(const Glib::VariantBase& value) > { set_state(value); } > }; It's not from another class; I just want to be able to say > auto action = Gio::SimpleAction::create_boolean("my-action"); > action->signal_change_state().connect( > [=](auto const& requested_state) > { > if ( changing_is_permitted() ) { > action->change_state(requested_state); > > // Do something about the new state: turn something off/on, etc. > } > } ...without having to define a new subclass for such a very simple case as this. As it is, I just stayed with a stateless action, and track the toggled state on my own. I'd already written it this way before I realised create_boolean() existed and thought it would be better. :P But ithas some drawbacks elsewhere. > To summarize, I think it's reasonable to let set_state() remain protected. > But as usual I let Murray decide. OK, I'm interested to hear what Murray thinks too. Thanks for the thoughts so far.
D'oh: (In reply to Daniel Boles from comment #6) > It's not from another class; I just want to be able to say > > auto action = Gio::SimpleAction::create_boolean("my-action"); > > action->signal_change_state().connect( > > [=](auto const& requested_state) > > { > > if ( changing_is_permitted() ) { > > action->change_state(requested_state); // NOPE > > > > // Do something about the new state: turn something off/on, etc. > > } > > } > ...without having to define a new subclass for such a very simple case as > this. Of course, that call to change_state() should be set_state(). Otherwise it defeats the point of this bz, and we get an infinite loop. I've made that silly mistake before!
Ok, I change my mind. You can push the patch to the master branch. The comment "We can make this public if somebody needs it." is written by Murray. I don't think he would object. The glibmm-2-50 branch is more problematic. I can't imagine that your patch would break ABI, but it adds public API. See the discussion in bug 774903, comments 2-6. You'd better do as with that bug: Don't push to glibmm-2-50 now, but leave this bug open.
Thanks! Pushed with updated commit messages as: [PATCH 1/3] Gio::Action: #include <glibmm/variant.h> in hg master : commit 25cf234fc1146a35869c169bade05ad6edeb80e5 glibmm-2-50: commit 8b05dc9046e0e2a731f5674d64c63a6ecbdb3e6a [PATCH 2/3] Gio::Action: Remove unneeded #includes in ccg master : commit ca52847529ab43f4b1aed10b6bc26bcb08712592 glibmm-2-50: commit 98b4861ff813ce82cca5b9a8b52ab53ca59539ee [PATCH 3/3] Gio::SimpleAction: Make set_state() public master : commit c5bed834d97fecc59c431fd87a2081a64007afee I'll continue to hope that 2.52 becomes a 2.4 version. :D
In an effort to summarise my length ramblings above, here's the jist of how this arises and how it affects using boolean and other stateful GSimpleActions. via https://bugzilla.gnome.org/show_bug.cgi?id=705655#c2 * If unconnected, signal activate handles toggling the new bool/radio state * If unconnected, signal change-state just forwards that request to set_state() * There is no signal state-changed, so the only way to actually implement our response to a new state is to do that in signal_change_state(). * But by connecting to that, we are now responsible for doing the set_state(). If we don't, the Action won't update properly. And it's that last point that's the sticker. I've determined that in glibmm-2-50, we need to do this: > g_simple_action_set_state( action->gobj(), const_cast<GVariant*>( requested_state.gobj() ); This works, but doesn't look very nice!
Comment on attachment 344584 [details] [review] [PATCH 3/3] Gio::SimpleAction: Make set_state() public committed to master a while back; 2.50 remains an open question
Following on from the discussion about glibmm-2-52 being stable but API-modifying, could this be included in that release? It does not break ABI since external users simply could not compile when attempting to use this function previously. Thanks!
I pushed these two commits to the glibmm-2-52 branch: https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-52&id=61b774384195d5e0ec7dada546a3f69c6f273408 and https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-52&id=e432aef9f2fc81f98ba8fe2008cac7f85575c1b4 Please reopen this if there are other commits that need to be pushed. Thanks.