GNOME Bugzilla – Bug 573482
Gtk::Builder: autoconnecting signals
Last modified: 2018-05-22 12:11:41 UTC
I'd like to discuss the possibility of implementing the autoconnect feature. I have the POC patch that allows to look up the slots by names. I'll attach it here.
Created attachment 129690 [details] [review] sigc++ change makes sigc::slot_base to have virtual dtor
Created attachment 129691 [details] [review] slot lookup hack This patch allows for the slot look-ups by name.
The beginning of the discussion is here: http://mail.gnome.org/archives/gtkmm-list/2009-February/msg00154.html
Created attachment 129753 [details] [review] updated patch In reply to: http://mail.gnome.org/archives/gtkmm-list/2009-February/msg00177.html Murray Cumming <murrayc murrayc com> wrote: > again from your get_slot() via dynamic_cast<>. Of course the application > code needs to specify the get_slot()<> template type (via > get_widget()<>) so that the dynamic_cast<> can succeed. With the attached new patch, there is no need to specify the type, as it uses the SlotType typedef of the SignalProxy classes. The example still uses get_widget()<>, but it should use singal_autoconnect(). signal_autoconnect() can be implemented by iterating the widget tree and calling Slots::connect() for each. To implement that, I need to know how can I iterate the widget tree.
I should probably clarify why I implemented the string-based lookup, as it seems you are getting me wrong all the time. Glade saves the handler info in the form of: <signal name="clicked" handler="on_button_clicked" object="NULL"/> When you read this xml, you have only those names in a string form. So, you need a mapping to get the handler by its name. That's what my patch currently does. It seems you are expacting something else - what exactly then? Of course, you can't make it more "automatic" or compile-time type-safe, simply because the xml is being loaded at runtime. The runtime type-safety is, however, guaranteed. So could you please explain what you see wrong with my patch or approach?
Created attachment 130023 [details] [review] signal_autoconnect implementation I finally implemented Xml::signal_autoconnect(). And it works.
Created attachment 130024 [details] [review] signal_autoconnect implementation
Created attachment 130026 [details] [review] 130024: signal_autoconnect implementation Oops, forgot "svn add" for the new file, fixed patch.
Created attachment 130050 [details] [review] signal_autoconnect implementation move glade-private.h to private.
So, if I understand it, this is based on these ideas: 1. You know what slot (signal handler) type to expect for the specified signal, by using the sigc::slot<>::SlotType tyepdef. The dynamic_cast<> checks that it's the correct slot type. For instance: void connect(T_sig sig, const Glib::ustring& name) { sig.connect(get_slot<typename T_sig::SlotType>(name)); } That's nice. 2. You can't connect to any signal. Your libglademm hard-codes checks for each signal for each widget, so it can have the specific signal accessor (and type information) to call connect() in 1. That doesn't seem acceptable. I can't imagine how we would document its limitations without including a big ugly list of signals. 3. You have to parse the glade XML file because libglade doesn't give you any API to get these signal handler names (and their widgets) for you. That's unacceptable, but maybe you can check if GtkBuilder (which replaces libglade) has API for it, and maybe you could request that if necessary. By the way, this patch would be more readable if it did not use macros.
Hi, thanks for the review. 1. Yes. :) 2. Yes. :( The solution for that can be to move the part of the Slots class to Gtk::Bin, add the virtual connect() function to Gtk::Bin, then require all the widgets that emit signals to implement that function. Libglade will then only use the Gtk::Bin when connecting, no hardcodes. But, that would be a massive change in gtkmm, so before I start, I'd like to hear from you about that. 3. No, actually not. It appears that libglade already stores all the needed info in the GladeXML struct, but they use a private header that describes it. :( I had only to copy that header to libglademm, only to get the definitions of the private structs. But I am not doing any parsing. If you still find that bad, then the solution would be to patch libglade and add 1-2 simple functions to it, to give away all the info I need. But copying the header is imho also not all that bad, so what do you think? > By the way, this patch would be more readable if it did not use macros. Some macros do provide the syntactical sugar for the user to easier define his slots. Others are used to easier adding the support for the new widgets. I think the first ones should stay, the second ones can be removed, esp if we are going to move the connection code to the widgets.
(In reply to comment #11) > 2. Yes. :( > The solution for that can be to move the > part of the Slots class to Gtk::Bin, add > the virtual connect() function to Gtk::Bin, > then require all the widgets that emit signals > to implement that function. Libglade will then > only use the Gtk::Bin when connecting, no hardcodes. > But, that would be a massive change in gtkmm, > so before I start, I'd like to hear from you > about that. That's an interesting idea - adding a templated registration/retrieval system for signals to Glib::Object. It seems possible with changes to gmmproc, though we would need to break ABI to add the std::map member. I wonder if that std::map could be static rather than per-object. Even so, I would worry a bit about the registration causing a slowdown. Maybe we could delay it until the function is used, which would be rare. Note that an ABI break is likely in the next few months, because GTK+ 3 is going to have an ABI break, forcing us to break ABI. I don't think that's a wise decision but it does give us the opportunity to do things like this. u > 3. No, actually not. It appears that libglade > already stores all the needed info in the > GladeXML struct, but they use a private header > that describes it. :( I had only to copy that > header to libglademm, only to get the definitions > of the private structs. But I am not doing any > parsing. > If you still find that bad, then the solution > would be to patch libglade and add 1-2 simple > functions to it, to give away all the info I > need. But copying the header is imho also not > all that bad, so what do you think? Copying the header is awful. It's private for a reason. Yes, you would need to add API to GtkBuilder.
Created attachment 130106 [details] [review] libglade hack to give away signal info Attached is the patch for libglade to give away the signal info. I am a bit worried that people may dislike adding the function that only returns some private pointer... but I can't help it. We need that pointer.
Created attachment 130107 [details] [review] signal_autoconnect implementation New patch that uses the aforementioned libglade hook.
Again, this would only make sense for GtkBuilder, which replaces libglade.
How? It seems, my libglademm uses the libglade API, not gtkbuilder. I don't seem to even have the gtkbuilder installed, yet libglademm works. Do you mean gtkbuilder will replace libglade _in the future_, and libglademm will start using it one day? Or what did you mean? About ABI breakage: its good to know there is such an opportunity approaching. There is already a sigc++ ABI-incompatible change here. How would you deal with that one? About Glib/gmmproc: are there any chances that you would prototype the ideas you have, or is this all for me alone? :)
(In reply to comment #16) > How? > It seems, my libglademm uses the libglade API, > not gtkbuilder. I don't seem to even have the > gtkbuilder installed, yet libglademm works. > Do you mean gtkbuilder will replace libglade > _in the future_, and libglademm will start > using it one day? Or what did you mean? Gtk::Builder (in gtkmm) replaces libglademm, as GtkBuilder (in GTK+) replaces libglade. When a reasonable amount of people are using Gtk::Builder successfully, I will document libglademm as deprecated. > About ABI breakage: its good to know there is > such an opportunity approaching. There is already > a sigc++ ABI-incompatible change here. How would > you deal with that one? sigc++ could break ABI too. > About Glib/gmmproc: are there any chances that > you would prototype the ideas you have, or is > this all for me alone? :) It's not a high priority for me. I would probably do that part if you did the rest.
Created attachment 130198 [details] [review] hack for libglademm build Hi. OK, I'll try to do my best, but please fix the build system somehow! When I modify the .hg or .ccg file, the cc/h gets regenerated only by "make" in the topdir or in libglade/src, but not in libglade/libglademm. And when I remove any generated file, it doesn't get regenerated at all. The attached hack fixes this, yet I am sure it is far from being correct (but using BUILT_SOURCES is a must when you have the generated headers, AFAIK). With glibmm the things are much worse. It doesn't regenerate anything for me, no matter what. So I am a kind of stuck. And I hate autotools. Please fix the build system for glibmm somehow so that I can go on.
If you use autogen.sh then you should be in maintainer mode, meaning that the files will be regenerated. Are you using autogen.sh?
No because I've installed everything from src.rpm and there was no autogen.sh. Now I enabled maintainer mode via configure and the things are better. Yet, it would be nice if: 1. "make" would rebuild the files whose sources are modified, even if you type "make" in glib/glibmm directory, not only in glib/src. This can be done by specifying the proper dependencies. 2. "make" would rebuild the deleted files. This can be done by using BUILT_SOURCES macro of automake. Its really a relief when the build system works flawlessly. :)
You should really be using svn and making an svn diff. I don't see any great problem with the build system, and I'm actually glad that the source files cannot be accidentally regenerated (with different results, depending on the installed gmmproc version) by packagers. But please open a separate bug if you wish to pursue that.
> You should really be using svn and making an svn diff. Now that's really a PITA. Because of the ABI-incompatible changes I do, I need to rebuild everything. I wanted to keep the dependencies right, so I was building rpms. I used libglademm from svn regardless, but now, to install gtkmm from svn, I need to upgrade gtk for some reasons (mine is 2.14.7), and gtk have _so many_ dependencies, that I think I can't move to the svn code right now and have to stick to srpms. :( (I currently use Fedora 10 here) > I don't see any great problem with the build system, and I'm actually glad that > the source files cannot be accidentally regenerated (with different results, > depending on the installed gmmproc version) by packagers. That could be done still in the maintainer mode only. Then it will probably not hurt. > But please open a > separate bug if you wish to pursue that. OK, I think it worth a bugreport. :)
Created attachment 130475 [details] [review] glibmm patch Add SlotsContainer into glibmm.
Created attachment 130476 [details] [review] Gtk::Builder::connect_signals() implementation Good news: I made it a simple wrapper around gtk_builder_connect_signals_full(). Not so good: There seem to be no alternative to glade_get_widget_name(), so the way I am getting the name, is a bit obscure. This is not a POC now - a real implementation. :) Only Gtk::Button is supported in this patch though, and as a demo only - its a gmmproc work now, there is no need to change the ccg/hg files themselves. That's what you were supposed to do - may I count on that? :)
Created attachment 130477 [details] [review] example patch gtkmm-documentation change. Btw, change this bug finally to ASSIGNED, if you please. :)
Hmm, I forgot to absolete the old patches, and now it doesn't seem to be possible any more...
Created attachment 130479 [details] [review] Gtk::Builder::connect_signals() implementation Absolete old patches.
Created attachment 130502 [details] [review] gtk_builder_get_object_name() Same as glade_get_widget_name()
Created attachment 130503 [details] [review] use gtk_builder_get_object_name() Gtk::Builder::connect_signals() implementation that uses gtk_builder_get_object_name()
Created attachment 130529 [details] [review] Gtk::Builder::connect_signals() implementation OK, the GTK guys pointed me to an alternative of glade_get_widget_name(). So there is no need to change a GTK API - what a relief! So I think now everything is fine, and what remains is the gmmproc change. Are there still any issues with this patch that you can think of?
About example.patch, aren't these in the wrong order? Doesn't the signal handler need to be declared _before_ you use it? class MySlots : public Glib::SlotsContainer { private: GTKBUILDER_DECLARE_SLOTS( GTKBUILDER_SLOT(MySlots, on_button_clicked) ); void on_button_clicked() { if(pDialog) pDialog->hide(); //hide() will cause main::run() to end. } Also, I would really really like to avoid use of macros at least in the public API. Can you try that, please? And I unfortunately still don't find this much nicer than connecting manually. It only saves me from having to remember the names of the signals that I am connecting to, though a) that's usually fairly obvious from my signal handler names. b) I still need to remember what parameters the signal handlers take, which I can only know if I know the signal names.
Note to self: If I implement the signal registration system that would be needed to make this work, I would want that registration to only happen just before it is actually used, to avoid unnecessary slowdowns during normal gtkmm initialization.
> About example.patch, aren't these in the wrong order? Doesn't the signal > handler need to be declared _before_ you use it? That doesn't really matter - the class members are allowed to access the other members of the same class that were declared later. The patch is, of course, tested. :) > Also, I would really really like to avoid use of macros at least in the public > API. Can you try that, please? Its not an issue to remove the macros, its just a question of syntax. I wanted to hide the fact that I am building the map. If you remove macros, then the user will have to define the function __init_slots() and call register_slot() from it for every slot. If that's what the cpp can do, then I thought why not. What's so bad about the macros? wxwidgets does everything with macros. Are there any other ways to build the map automatically? I am not aware of them, so do you really hate the macros so much so to force the user to fill the map himself? The macros are not mandatory here, but I find them convenient. Maybe, it is possible to document both ways, so that people may use the macros, or may not? > And I unfortunately still don't find this much nicer than connecting manually. > It only saves me from having to remember the names of the signals that I am > connecting to, though You first need to remember all the widgets you need to connect. Recall their names. Their type. Get those widgets by names. Recall their respective handlers. Then connect. You write the wrapper by doing so. Wrapper needs to be recompiled when the form changes. You change the widget name in the GUI builder - adjust the wrapper + recompile. You add another button that can use the already existing handler - you nevertheless adjust the wrapper + recompile. I agree to recompile when I change or add the handler itself - this is unavoidable and obviously justified. But I don't want to have the wrapper, neither do I want to write it, nor to recompile every time I am changing the form. I want only a single call that loads the GUI from XML and makes it alive. A single call, not more. :) QT uses wrappers too, but at least, they are generated there. Here, the wrappers are hand-written. I also have that mail in my mail-box now: --- Nice feature. The only thing why I'm not currently using gtkmm anymore is because of the lack of autoconnecting signals. I'm using python bindings instead. Please, make this feature mainstream for the c++ bindings. --- Hope that clarifies. :)
> Note to self: If I implement the signal registration system that would be > needed to make this work, I would want that registration to only happen just > before it is actually used, to avoid unnecessary slowdowns during normal gtkmm > initialization. I'd like to discuss _my_ implementation a bit. :) In my implementation, the dynamic registration of slots happens, not of the signals. And yes, it is delayed up to the first use. I am not asking you to implement the dynamic registration of anything - I don't actually even understand what you propose. :) My system is almost complete - unless you can point a serious problems with it, the only thing remained, is to generate the connect() method for every widget. This is a very easy task for gmmproc. I propose to add the dedicated section to which the _WRAP_SIGNAL() will emit the code like in the change to button.ccg in my patch. At the second stage, that section will go into a .cc file as the implementation of connect(), and into a .h file as the prototype of connect(). And thats it - no more changes are required at all, the task is complete. What do you think?
I mean, I can even do such a change to gmmproc myself, it should be very simple. Unfortunately, the gmmproc is a bit complex and undocumented, and is written on the languages I haven't used much. So I hope you can do that in a reasonable time frame. But if not - let me know about that. The change will be very simple, but I am afraid I'll spend too much time learning how gmmproc really works, so that's why I am reluctant to start.
(In reply to comment #33) > > Also, I would really really like to avoid use of macros at least in the public > > API. Can you try that, please? > Its not an issue to remove the macros, its just > a question of syntax. I wanted to hide the fact > that I am building the map. If you remove macros, > then the user will have to define the function > __init_slots() and call register_slot() from it > for every slot. I think I'd like to force the user to implement the __init_slots() method overload, or a constructor, if that's possible. That use of a macro obscures things too much. Then the other macro will not look too bad. The macro should have a GTKMM_ prefix, by the way. However, I would like to see an example/test that: 1. Uses both the macro and the method, so it's easier to see what the macro does. 2. Uses a non-global signal handler, such as sigc::mem_fun(*this, ExampleWindow::on_test_button_clicked). That would probably have to use get_widget_derived(). If this only works with global non-class functions then it's not very useful.
If we decide that the API works and is useful then I will do the gmmproc work. We are not quite there yet.
> I think I'd like to force the user to implement the __init_slots() method > overload Uhm. So you really hate macros... :( > or a constructor, if that's possible. I decided against using the constructor, because not using it gives: 1. Syntactical sugar: you don't have to pass the class name into GTKBUILDER_DECLARE_SLOTS macro. I'd like to avoid passing it to the GTKBUILDER_SLOT macro too, but for the completely stupid reasons I can't. The completely stupid reasons are requiring me to qualify the class name when I take the pointer to the class member _inside that class declaration_! I find that silly. For example, qualifying the member declaration with the class name is not even allowed. So why do they require that when I take the pointer? 2. Compile-time check. It is easy for the user to forget to put the necessary macro, especially if he doesn't know what this macro do. :) I wanted a compile-time error for that mistake (and it is there), while with the constructor I don't see how to impement the compile-time check, and the run-time error is not that good. IIRC, wxWidgets has only the link-time error for that, and with the meaningless error message, so my implementation is even better. :) 3. Saner documentation. Do it in a constructor, and you will have to prohibit the user to define the custom constructor. I don't think someone will want to define a custom constructor for the helper class like this, but nevertheless you will have to mention that in the docs, while it is too silly to mention in the docs. :) 4. Delayed allocation. Probably completely unimportant here, but you can't do that with the ctor. So overall, I think having these things in the constructor is not very flexible. It might be better to have the dedicated function and a few macros to support. :) > That use of a macro obscures > things too much. Then the other macro will not look too bad. What do you mean? Leave one macro instead of 2? wxWidgets has way more macros for the purpose of declaring the event handlers. :) > However, I would like to see an example/test that: > 1. Uses both the macro and the method, so it's easier to see what the macro > does. > 2. Uses a non-global signal handler, such as sigc::mem_fun(*this, > ExampleWindow::on_test_button_clicked). I'll try to implement 1 tomorrow. Having more than one slotscontainer was not supposed to happen, but I'll try to add the support for that. 2 will simply require one more macro... :) > If we decide that the API works and is useful then I will do the gmmproc work. > We are not quite there yet. As for API - do you prefer SlotsContainer or SlotContainer name? Or anything else? I am asking because this looks like the only glibmm change, so it can be "stabilized" before the other bits.
(In reply to comment #38) > > I think I'd like to force the user to implement the __init_slots() method > > overload > Uhm. So you really hate macros... :( Yes. > What do you mean? Leave one macro instead of 2? > wxWidgets has way more macros for the purpose > of declaring the event handlers. :) wxWidgets is awful mostly because it uses macros. I am being clear about what I want, I believe. > > However, I would like to see an example/test that: > > 1. Uses both the macro and the method, so it's easier to see what the macro > > does. > > 2. Uses a non-global signal handler, such as sigc::mem_fun(*this, > > ExampleWindow::on_test_button_clicked). > I'll try to implement 1 tomorrow. Having more > than one slotscontainer was not supposed to > happen, but I'll try to add the support for that. > 2 will simply require one more macro... :) Thanks. > As for API - do you prefer SlotsContainer or > SlotContainer name? SlotsContainer, please. Whatever is causing you to split all your lines here, please try to stop it. It makes it hard to read your replies, particularly when you are so verbose.
Created attachment 130894 [details] [review] glibmm patch Extended SlotsContainer to handle class members.
Created attachment 130895 [details] [review] Gtk::Builder::connect_signals() implementation Extended the implementation to handle more than one container. I think its a useless feature, but if you think its necessary - here it is. :)
Created attachment 130898 [details] [review] example patch Example for registering functions and class members, with and without the use of the macros.
New patches are here, please take a look. Please note how small and nice is the change to the "derived" example - you won't regret those macros! :) Please make this bug ASSIGNED, rather than UNCONFIRMED - it is confirmed.
Created attachment 130949 [details] [review] glibmm patch Simplify registering the slots from outside the class def.
Created attachment 130950 [details] [review] example patch more examples.
Created attachment 130953 [details] [review] example patch
So, using it for member methods requires that the class derives from Glib::SlotsContainer, if I understand this. OK. Is there any way to use this when just using get_widget() instead of get_widget_derived()? I notice that you are still using the GLIBMM_DECLARE_SLOTS() macro, though I've said that I'd like that to be removed.
> So, using it for member methods requires that the class derives from > Glib::SlotsContainer, if I understand this. It is the preferred solution, yes. But I also implemented the way of adding the members from the non-derived classes. There is no demo for this, but it will look like slots.register_slot(GLIBMM_MAKE_SLOT(SomeClass::some_handler), obj_ptr); So I think all the possibilities are now supported. > Is there any way to use this > when just using get_widget() instead of get_widget_derived()? Could you please clarify? Yep, the changes to the "basic" example are with get_widget(), so I guess there are no problems. > I notice that you are still using the GLIBMM_DECLARE_SLOTS() macro, though > I've > said that I'd like that to be removed. MySlots2 class in the "basic" example is implemented without any macros at all. I've just demonstrated all the possibilities. I am not asking you to apply that example patch anywhere, so if I still use that macro in some place, its probably not a problem. :) Other than that, if you want to remove it entirely - it is just a single-line removal, it can be done later, or by yourself. :) My patch currently demonstrates both ways. So where do we go from here? Is the API now good enough (modulo the macro), or are there any other things to be done? And if it is OK, then what are the plans? :)
(In reply to comment #48) > > So, using it for member methods requires that the class derives from > > Glib::SlotsContainer, if I understand this. > It is the preferred solution, yes. But I also implemented the way of adding > the members from the non-derived classes. There is no demo for this, but > it will look like > slots.register_slot(GLIBMM_MAKE_SLOT(SomeClass::some_handler), obj_ptr); > So I think all the possibilities are now supported. It would be nice to add a test for this too, please. > > Is there any way to use this > > when just using get_widget() instead of get_widget_derived()? > Could you please clarify? > Yep, the changes to the "basic" example are with get_widget(), so I guess > there are no problems. The example seems to connect the signals before get_widget() is called. But the instance does not exist until get_widget() is called, and the instance is needed for the call to sigc::mem_fun(). It would be nice to see a test/example of this in the patch too. > > I notice that you are still using the GLIBMM_DECLARE_SLOTS() macro, though > > I've > > said that I'd like that to be removed. > MySlots2 class in the "basic" example is implemented without any macros at all. Ah, thanks. Could you please remove the __ prefix and change () to (void). > So where do we go from here? Is the API now good enough (modulo the macro), > or are there any other things to be done? And if it is OK, then what are > the plans? :) Once the above things are done then I will send an email to the mailing list showing the proposed API via examples, asking if people think it is useful.
> The example seems to connect the signals before get_widget() is called. But the > instance does not exist until get_widget() is called, and the instance is > needed for the call to sigc::mem_fun(). The autoconnect code calls get_widget() itself, so the instance is always there. The derived widgets are more problematic. Right now my solution for them is to make all the derived widgets to inherit SlotsContainer and call connect_signals() from their ctor. There is no problem to call connect_signals() more than once, so every derived widget can connect its own signals from its ctor. That means however, that writing a wrapper is unavoidable if you have the derived widgets. You have to call get_widget_derived() for all of them, simply to create them. Is this right? And if so - are there any considerations to make this automatic too? Doesn't look too difficult to me, but probably more work than with the signals... Something like register_derived_widgets() can be implemented. > Once the above things are done then I will send an email to the mailing list > showing the proposed API via examples, asking if people think it is useful. OK, I'll do that within a few days.
Created attachment 131239 [details] [review] glibmm patch remove __
Created attachment 131240 [details] [review] Gtk::Builder::connect_signals() implementation Clean up the implementation. Also some API additions. I find get_widget_derived() ugly and confusing. I added create_widget_derived(), and get_widget_derived() can be deprecated. Also, I find the habit of returning the pointers via the pointer reference argument to be very strange. I added get_widget() that just returns the pointer via the return value.
Created attachment 131241 [details] [review] example patch New set of examples. The example that registers the handler of the derived widget that does not inherit SlotsContainer, is now there, among others. So, what do you think?
Any progress?
Sorry, I forgot about this. I'll deal with it soon.
You can miss the Gnome3 release this way. :)
OK, so what is it? Any hopes to get the patch review?
Yes, I'm sorry. I will find time.
(In reply to comment #48) > > I notice that you are still using the GLIBMM_DECLARE_SLOTS() macro, though > > I've > > said that I'd like that to be removed. > MySlots2 class in the "basic" example is implemented without any macros at all. > I've just demonstrated all the possibilities. Please keep the basic example as simple as possible rather than making it a test case for all ways to use the API. You could add something in tests/ to actually test things.
Created attachment 136021 [details] [review] example patch Both basic and derived examples now have only the minimal editings. Things added as the new example. Are we getting any closer?
That doesn't patch against the current git master: $ patch -p0 < /home/murrayc/Desktop/patches/doc3.diff patching file examples/book/builder/basic/main.cc Hunk #1 FAILED at 3. Hunk #2 FAILED at 41. 2 out of 2 hunks FAILED -- saving rejects to file examples/book/builder/basic/main.cc.rej patching file examples/book/builder/basic/basic.ui patching file examples/book/builder/other/main.cc patching file examples/book/builder/other/Makefile.am patching file examples/book/builder/other/other.ui patching file examples/book/builder/derived/deriveddialog.cc patching file examples/book/builder/derived/deriveddialog.h patching file examples/book/builder/derived/main.cc Hunk #1 succeeded at 51 with fuzz 1 (offset 10 lines). patching file examples/book/builder/derived/basic.ui patching file examples/book/builder/Makefile.am
I unfortunately can't compile the svn code because of the Bug 586700 I filled, and the other problem that says: --- ./configure: line 2722: syntax error near unexpected token `scripts' ./configure: line 2722: `AL_ACLOCAL_INCLUDE(scripts)' --- which I don't know how to fix. Any hints are appreciated. I fixed up the patch without any testing, hope it still works.
Created attachment 137218 [details] [review] example patch Patch against current svn. Rejects fixed but not tested.
Created attachment 137454 [details] [review] Gtk::Builder::connect_signals() implementation Updated the patch for svn head. But it is a real pain maintaining these patches. I can't give them a sufficient testing any more, as there are too many dependancies on sigc++ and glibmm. I guess if you won't start taking a look, this will go nowhere. Are there any chances to start pushing a trivial changes, like the sigc++ and glibmm one?
(In reply to comment #64) > Created an attachment (id=137454) [edit] > Gtk::Builder::connect_signals() implementation > > Updated the patch for svn head. You mean git, right? Anyway, it applies cleanly. Thanks. However, there seem to be some unrelated changes in builder.hg for get_widget() and get_widget_derived(). Maybe that's something you'd like to submit separately. > But it is a real pain maintaining these patches. > I can't give them a sufficient testing any more, > as there are too many dependancies on sigc++ and > glibmm. > I guess if you won't start taking a look, this > will go nowhere. > Are there any chances to start pushing a trivial > changes, like the sigc++ and glibmm one? Well, the glibmm changes are not trivial, right? And the sigc++ one is a ABI break (adding a virtual destructor), so no, we can't do that until we are sure that this will be used. I'm starting a discussion about this now on the mailing list.
Created attachment 138986 [details] [review] Gtk::Builder::connect_signals() implementation Added connect_signals<T>(void) template to make it clear that you do _not_ need to fill the SlotsContainer object.
Created attachment 138987 [details] [review] example patch Demonstrate that.
OK, so there wasn't too much of a support, even the people who posted me privately, were now silent. Still, no objections usually means acceptance, and after all, this is yet another missing function being wrapped for good. So what would be a decision?
Are there really no hopes to ever get this in?
As a user of Gtkmm, I can say, that I definitely want a feature like this. There is alot of history on this patch, over a year of deliberations, is there some way we can get this feature implemented? It should be obvious that this feature is useful for Gtkmm users.
> is there > some way we can get this feature implemented? The feature is implemented in this patch set, except for the gmmproc part. It is tested and works, with many usage examples being provided here too (most of which are completely useless, and only exist to distract people from the original intents, but some are good). See comment #37 about the gmmproc part. I personally have no idea what more could have I done to get that included.
Hi, I have an alternative connect signals implementation that does not require macros and uses some new features of C++ 11 which I think will make it more convenient for programmers to specify signal handlers in derived classes. Hopefully you can consider this. Basically, with this method all one has to do is something like this: builder->add_slots({ {"glade handler name",[this](Glib::RefPtr<Glib::Object>,gpointer){ //handler code goes here } //, ...more event handlers }); Here is a demo: signaltest.cpp: #include <gtkmm.h> #include <gtk/gtk.h> #include <map> #include <list> using namespace std; class Builder:public Gtk::Builder{ protected: map<Glib::ustring,sigc::signal<void,Glib::RefPtr<Glib::Object>,gpointer>> handlers; struct signal_connect_info{ Builder *builder; Glib::ustring handler; gpointer user_data; }; list<signal_connect_info> connectInfo; static void BuilderConnectFunc(GtkBuilder *builder, GObject *object, const gchar *signal_name, const gchar *handler_name, GObject *connect_object, GConnectFlags flags, gpointer user_data){ if(Glib::RefPtr<Builder> b=Glib::RefPtr<Builder>::cast_dynamic(Glib::wrap(builder))){ b->connect_func(object,signal_name,handler_name,connect_object,flags,user_data); } } static void signal_handler(GObject* obj,gpointer data){ Builder::signal_connect_info *info=reinterpret_cast<Builder::signal_connect_info*>(data); info->builder->handle_signal(obj,info->handler,info->user_data); } void connect_func(GObject *object, const gchar *signal_name, const gchar *handler_name, GObject *connect_object, GConnectFlags flags, gpointer user_data){ connectInfo.push_back({this,g_strdup(handler_name),user_data}); if(connect_object){ g_signal_connect_object(object,signal_name,G_CALLBACK(signal_handler),&connectInfo.back(),flags); }else{ g_signal_connect_data(object,signal_name,G_CALLBACK(signal_handler),&connectInfo.back(),0,flags); } reference(); } void handle_signal(GObject* obj,const Glib::ustring& handler,gpointer user_data){ Glib::RefPtr<Glib::Object> ptr=Glib::wrap(obj); ptr->reference(); if(handlers.find(handler)!=handlers.end())handlers[handler](ptr,user_data); } public: Builder(GtkBuilder *cast_item):Gtk::Builder(cast_item){ } void connect_signals(){ gtk_builder_connect_signals_full(gobj(),(GtkBuilderConnectFunc)BuilderConnectFunc,0); } sigc::signal<void,Glib::RefPtr<Glib::Object>,gpointer>& signals(const Glib::ustring& handler){ return handlers[handler]; } void add_slots(multimap<Glib::ustring,sigc::slot<void,Glib::RefPtr<Glib::Object>,gpointer>> slots){ for(auto i:slots){ handlers[i.first].connect(i.second); } } }; class DerivedWindow:public Gtk::Window{ public: DerivedWindow(GtkWindow* cobject,Glib::RefPtr<Gtk::Builder> builder):Gtk::Window(cobject){ if(Glib::RefPtr<Builder> b=Glib::RefPtr<Builder>::cast_dynamic(builder)){ b->add_slots({ {"signal 2",[this](Glib::RefPtr<Glib::Object>,gpointer){ Gtk::MessageDialog(*this,"Signal 2 handled by "+get_name()+"\n").run(); }} }); } } protected: //Signal handlers: virtual void on_button_quit(){hide();} Glib::RefPtr<Gtk::Builder> m_refGlade; Gtk::Button* m_pButton; }; int main(int argc,char** argv){ Glib::RefPtr<Gtk::Application> app = Gtk::Application::create(argc, argv,"gtkmm.signaltest"); Glib::RefPtr<Builder> builder(new Builder(gtk_builder_new())); builder->add_from_file("signaltest.glade"); DerivedWindow *window; builder->get_widget_derived("window", window); builder->signals("signal 1").connect([window](Glib::RefPtr<Glib::Object>,gpointer){ Gtk::MessageDialog("Signal 1\n").run(); }); builder->connect_signals(); app->run(*window); delete window; return 0; } signaltest.glade: <?xml version="1.0" encoding="UTF-8"?> <interface> <!-- interface-requires gtk+ 3.0 --> <object class="GtkWindow" id="window"> <property name="can_focus">False</property> <property name="default_width">400</property> <property name="default_height">100</property> <child> <object class="GtkLayout" id="layout1"> <property name="visible">True</property> <property name="can_focus">False</property> <child> <object class="GtkButton" id="button1"> <property name="label" translatable="yes">Button 1</property> <property name="use_action_appearance">False</property> <property name="width_request">166</property> <property name="height_request">72</property> <property name="visible">True</property> <property name="can_focus">True</property> <property name="receives_default">True</property> <property name="halign">center</property> <property name="use_action_appearance">False</property> <signal name="clicked" handler="signal 1" swapped="no"/> </object> <packing> <property name="x">16</property> <property name="y">25</property> </packing> </child> <child> <object class="GtkButton" id="button2"> <property name="label" translatable="yes">Button 2</property> <property name="use_action_appearance">False</property> <property name="width_request">166</property> <property name="height_request">70</property> <property name="visible">True</property> <property name="can_focus">True</property> <property name="receives_default">True</property> <property name="use_action_appearance">False</property> <signal name="clicked" handler="signal 2" swapped="no"/> </object> <packing> <property name="x">220</property> <property name="y">26</property> </packing> </child> </object> </child> </object> </interface>
Sorry, I forgot about the on_button_quit. It could also have been: class DerivedWindow:public Gtk::Window{ public: DerivedWindow(GtkWindow* cobject,Glib::RefPtr<Gtk::Builder> builder):Gtk::Window(cobject){ if(Glib::RefPtr<Builder> b=Glib::RefPtr<Builder>::cast_dynamic(builder)){ b->add_slots({ {"signal 2",sigc::mem_fun(this,&DerivedWindow::on_button_quit)} }); } } protected: virtual void on_button_quit(Glib::RefPtr<Glib::Object>,gpointer){hide();} Glib::RefPtr<Gtk::Builder> m_refGlade; Gtk::Button* m_pButton; };
If you are doing --- b->add_slots({ {"signal 2",sigc::mem_fun(this,&DerivedWindow::on_button_quit)} --- by hands, then what problem does this solve? My implementation was taking all the info from XML. Please see this: https://mail.gnome.org/archives/gtkmm-list/2009-July/msg00088.html and this: https://mail.gnome.org/archives/gtkmm-list/2009-July/msg00108.html
It solves the problem of not being able to connect to named signal handlers in glade. The same glade file might be accessed by multiple languages, for instance. Even in your implementation, you still need to declare slots using macros so there is still some manual connection. The idea is more *not* to use member functions but closures. If you do this instead b->add_slots({ {"button1_clicked",[this](Glib::RefPtr<Glib::Object>,gpointer){ /*...*/ }}, {"button2_clicked",[this](Glib::RefPtr<Glib::Object>,gpointer){ /*...*/ }}, //etc etc {"button10_clicked",[this](Glib::RefPtr<Glib::Object>,gpointer){ /*...*/ }} }); Then you would not need to declare anything additional, saving time. Also, the same handler could then be triggered by multiple signals without the need for manual connection (e.g. menu/keyboard/button all point to the same handler - without a function to add all the slots at one go you would have to connect the same lambda 3 times, or even more in other cases). It also eliminates the need to use macros.
Mine does't require macros, they just add a syntactical sugar. The macro-haters can still do --- virtual void init_slots(void) { register_slot("MySlots2::on_button2_clicked", &MySlots2::on_button2_clicked); } --- if they want to. Lambdas can probably be used here as well but I haven't tried. Note that this only registers the slot. The signal->slot mapping is taken from XML, so there is no "some manual connection" as you suggest. "some manual connection" is in your approach, and in the old (current) approach, but not in mine. Also, please note that all the registration, that may optionally involve a macro, can be done in a class definition in a header file, so it won't pollute your main code: --- +private: +GLIBMM_DECLARE_SLOTS( + GLIBMM_SLOT(DerivedDialog::on_button_quit) +); --- is all you need to add to your class definition. Also, could you please point me to your implementation of connect_signals(), which would work like this: --- + m_refGlade->connect_signals(*this); --- and will take all the signal->slot mapping from XML, without ever mentioning the signal name in a code?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtkmm/issues/4.