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 573482 - Gtk::Builder: autoconnecting signals
Gtk::Builder: autoconnecting signals
Status: RESOLVED OBSOLETE
Product: gtkmm
Classification: Bindings
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2009-02-27 22:00 UTC by Stas Sergeev
Modified: 2018-05-22 12:11 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
sigc++ change (360 bytes, patch)
2009-02-27 22:01 UTC, Stas Sergeev
none Details | Review
slot lookup hack (4.21 KB, patch)
2009-02-27 22:02 UTC, Stas Sergeev
none Details | Review
updated patch (3.33 KB, patch)
2009-02-28 22:18 UTC, Stas Sergeev
none Details | Review
signal_autoconnect implementation (7.43 KB, patch)
2009-03-04 15:08 UTC, Stas Sergeev
none Details | Review
signal_autoconnect implementation (7.47 KB, patch)
2009-03-04 15:12 UTC, Stas Sergeev
none Details | Review
130024: signal_autoconnect implementation (10.89 KB, patch)
2009-03-04 15:27 UTC, Stas Sergeev
none Details | Review
signal_autoconnect implementation (10.94 KB, patch)
2009-03-04 19:05 UTC, Stas Sergeev
none Details | Review
libglade hack to give away signal info (2.22 KB, patch)
2009-03-05 10:53 UTC, Stas Sergeev
none Details | Review
signal_autoconnect implementation (7.17 KB, patch)
2009-03-05 10:56 UTC, Stas Sergeev
none Details | Review
hack for libglademm build (1.67 KB, patch)
2009-03-06 14:45 UTC, Stas Sergeev
none Details | Review
glibmm patch (12.41 KB, patch)
2009-03-11 19:38 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (4.70 KB, patch)
2009-03-11 19:43 UTC, Stas Sergeev
none Details | Review
example patch (1.71 KB, patch)
2009-03-11 19:45 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (4.69 KB, patch)
2009-03-11 20:33 UTC, Stas Sergeev
none Details | Review
gtk_builder_get_object_name() (1.50 KB, patch)
2009-03-12 09:43 UTC, Stas Sergeev
none Details | Review
use gtk_builder_get_object_name() (4.59 KB, patch)
2009-03-12 09:49 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (4.83 KB, patch)
2009-03-12 15:24 UTC, Stas Sergeev
none Details | Review
glibmm patch (15.48 KB, patch)
2009-03-18 15:46 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (10.03 KB, patch)
2009-03-18 15:49 UTC, Stas Sergeev
none Details | Review
example patch (5.18 KB, patch)
2009-03-18 15:55 UTC, Stas Sergeev
none Details | Review
glibmm patch (15.64 KB, patch)
2009-03-19 10:33 UTC, Stas Sergeev
none Details | Review
example patch (5.84 KB, patch)
2009-03-19 10:33 UTC, Stas Sergeev
none Details | Review
example patch (5.84 KB, patch)
2009-03-19 10:43 UTC, Stas Sergeev
none Details | Review
glibmm patch (15.63 KB, patch)
2009-03-24 10:33 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (15.41 KB, patch)
2009-03-24 10:38 UTC, Stas Sergeev
none Details | Review
example patch (6.71 KB, patch)
2009-03-24 10:40 UTC, Stas Sergeev
none Details | Review
example patch (11.56 KB, patch)
2009-06-05 13:51 UTC, Stas Sergeev
none Details | Review
example patch (11.63 KB, patch)
2009-06-22 23:07 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (8.38 KB, patch)
2009-06-27 13:39 UTC, Stas Sergeev
none Details | Review
Gtk::Builder::connect_signals() implementation (8.48 KB, patch)
2009-07-22 11:20 UTC, Stas Sergeev
none Details | Review
example patch (11.54 KB, patch)
2009-07-22 11:23 UTC, Stas Sergeev
none Details | Review

Description Stas Sergeev 2009-02-27 22:00:44 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.
Comment 1 Stas Sergeev 2009-02-27 22:01:57 UTC
Created attachment 129690 [details] [review]
sigc++ change

makes sigc::slot_base to have virtual dtor
Comment 2 Stas Sergeev 2009-02-27 22:02:40 UTC
Created attachment 129691 [details] [review]
slot lookup hack

This patch allows for the slot look-ups
by name.
Comment 3 Stas Sergeev 2009-02-28 22:06:54 UTC
The beginning of the discussion is here:
http://mail.gnome.org/archives/gtkmm-list/2009-February/msg00154.html
Comment 4 Stas Sergeev 2009-02-28 22:18:17 UTC
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.
Comment 5 Stas Sergeev 2009-03-03 15:51:41 UTC
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?
Comment 6 Stas Sergeev 2009-03-04 15:08:46 UTC
Created attachment 130023 [details] [review]
signal_autoconnect implementation

I finally implemented Xml::signal_autoconnect().
And it works.
Comment 7 Stas Sergeev 2009-03-04 15:12:49 UTC
Created attachment 130024 [details] [review]
signal_autoconnect implementation
Comment 8 Stas Sergeev 2009-03-04 15:27:25 UTC
Created attachment 130026 [details] [review]
130024: signal_autoconnect implementation

Oops, forgot "svn add" for the new file, fixed patch.
Comment 9 Stas Sergeev 2009-03-04 19:05:03 UTC
Created attachment 130050 [details] [review]
signal_autoconnect implementation

move glade-private.h to private.
Comment 10 Murray Cumming 2009-03-04 23:16:08 UTC
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.

Comment 11 Stas Sergeev 2009-03-05 06:11:59 UTC
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.
Comment 12 Murray Cumming 2009-03-05 09:05:23 UTC
(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.


Comment 13 Stas Sergeev 2009-03-05 10:53:42 UTC
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.
Comment 14 Stas Sergeev 2009-03-05 10:56:09 UTC
Created attachment 130107 [details] [review]
signal_autoconnect implementation

New patch that uses the aforementioned
libglade hook.
Comment 15 Murray Cumming 2009-03-05 11:04:16 UTC
Again, this would only make sense for GtkBuilder, which replaces libglade.
Comment 16 Stas Sergeev 2009-03-05 14:40:10 UTC
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? :)
Comment 17 Murray Cumming 2009-03-05 15:02:46 UTC
(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. 

Comment 18 Stas Sergeev 2009-03-06 14:45:40 UTC
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.
Comment 19 Murray Cumming 2009-03-06 14:57:53 UTC
If you use autogen.sh then you should be in maintainer mode, meaning that the files will be regenerated. Are you using autogen.sh?
Comment 20 Stas Sergeev 2009-03-06 20:27:32 UTC
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. :)
Comment 21 Murray Cumming 2009-03-06 22:05:22 UTC
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.
Comment 22 Stas Sergeev 2009-03-07 00:12:31 UTC
> 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. :)
Comment 23 Stas Sergeev 2009-03-11 19:38:31 UTC
Created attachment 130475 [details] [review]
glibmm patch

Add SlotsContainer into glibmm.
Comment 24 Stas Sergeev 2009-03-11 19:43:24 UTC
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? :)
Comment 25 Stas Sergeev 2009-03-11 19:45:28 UTC
Created attachment 130477 [details] [review]
example patch

gtkmm-documentation change.

Btw, change this bug finally to ASSIGNED,
if you please. :)
Comment 26 Stas Sergeev 2009-03-11 19:49:59 UTC
Hmm, I forgot to absolete the old
patches, and now it doesn't seem to
be possible any more...
Comment 27 Stas Sergeev 2009-03-11 20:33:25 UTC
Created attachment 130479 [details] [review]
Gtk::Builder::connect_signals() implementation

Absolete old patches.
Comment 28 Stas Sergeev 2009-03-12 09:43:18 UTC
Created attachment 130502 [details] [review]
gtk_builder_get_object_name()

Same as glade_get_widget_name()
Comment 29 Stas Sergeev 2009-03-12 09:49:33 UTC
Created attachment 130503 [details] [review]
use gtk_builder_get_object_name()

Gtk::Builder::connect_signals() implementation that uses
gtk_builder_get_object_name()
Comment 30 Stas Sergeev 2009-03-12 15:24:44 UTC
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?
Comment 31 Murray Cumming 2009-03-16 13:34:53 UTC
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.


Comment 32 Murray Cumming 2009-03-16 13:36:09 UTC
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.
Comment 33 Stas Sergeev 2009-03-17 14:27:14 UTC
> 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. :)
Comment 34 Stas Sergeev 2009-03-17 14:43:41 UTC
> 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?
Comment 35 Stas Sergeev 2009-03-17 14:53:48 UTC
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.
Comment 36 Murray Cumming 2009-03-17 14:59:56 UTC
(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.


Comment 37 Murray Cumming 2009-03-17 15:02:09 UTC
If we decide that the API works and is useful then I will do the gmmproc work. We are not quite there yet.
Comment 38 Stas Sergeev 2009-03-17 20:01:06 UTC
> 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.
Comment 39 Murray Cumming 2009-03-17 21:10:24 UTC
(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.
Comment 40 Stas Sergeev 2009-03-18 15:46:27 UTC
Created attachment 130894 [details] [review]
glibmm patch

Extended SlotsContainer to handle class members.
Comment 41 Stas Sergeev 2009-03-18 15:49:26 UTC
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. :)
Comment 42 Stas Sergeev 2009-03-18 15:55:18 UTC
Created attachment 130898 [details] [review]
example patch

Example for registering functions and class members, with and without the
use of the macros.
Comment 43 Stas Sergeev 2009-03-18 16:00:43 UTC
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.
Comment 44 Stas Sergeev 2009-03-19 10:33:01 UTC
Created attachment 130949 [details] [review]
glibmm patch

Simplify registering the slots from outside the class def.
Comment 45 Stas Sergeev 2009-03-19 10:33:58 UTC
Created attachment 130950 [details] [review]
example patch

more examples.
Comment 46 Stas Sergeev 2009-03-19 10:43:13 UTC
Created attachment 130953 [details] [review]
example patch
Comment 47 Murray Cumming 2009-03-19 11:16:37 UTC
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.
Comment 48 Stas Sergeev 2009-03-19 15:04:13 UTC
> 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? :)
Comment 49 Murray Cumming 2009-03-20 12:39:00 UTC
(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.


Comment 50 Stas Sergeev 2009-03-21 15:59:41 UTC
> 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.
Comment 51 Stas Sergeev 2009-03-24 10:33:01 UTC
Created attachment 131239 [details] [review]
glibmm patch

remove __
Comment 52 Stas Sergeev 2009-03-24 10:38:34 UTC
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.
Comment 53 Stas Sergeev 2009-03-24 10:40:43 UTC
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?
Comment 54 Stas Sergeev 2009-04-06 14:50:53 UTC
Any progress?
Comment 55 Murray Cumming 2009-04-09 08:18:14 UTC
Sorry, I forgot about this. I'll deal with it soon.
Comment 56 Stas Sergeev 2009-04-17 11:31:56 UTC
You can miss the Gnome3 release this way. :)
Comment 57 Stas Sergeev 2009-05-21 11:48:13 UTC
OK, so what is it?
Any hopes to get the patch review?
Comment 58 Murray Cumming 2009-05-21 13:17:12 UTC
Yes, I'm sorry. I will find time.
Comment 59 Murray Cumming 2009-06-02 09:33:56 UTC
(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.
Comment 60 Stas Sergeev 2009-06-05 13:51:12 UTC
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?
Comment 61 Murray Cumming 2009-06-22 15:56:31 UTC
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
Comment 62 Stas Sergeev 2009-06-22 23:05:56 UTC
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.
Comment 63 Stas Sergeev 2009-06-22 23:07:40 UTC
Created attachment 137218 [details] [review]
example patch

Patch against current svn.
Rejects fixed but not tested.
Comment 64 Stas Sergeev 2009-06-27 13:39:05 UTC
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?
Comment 65 Murray Cumming 2009-07-20 10:54:02 UTC
(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.
Comment 66 Stas Sergeev 2009-07-22 11:20:23 UTC
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.
Comment 67 Stas Sergeev 2009-07-22 11:23:43 UTC
Created attachment 138987 [details] [review]
example patch

Demonstrate that.
Comment 68 Stas Sergeev 2009-08-14 10:19:10 UTC
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?
Comment 69 Stas Sergeev 2010-09-30 18:26:41 UTC
Are there really no hopes to ever get this in?
Comment 70 rhl 2011-05-13 15:48:23 UTC
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.
Comment 71 Stas Sergeev 2011-05-13 16:35:42 UTC
> 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.
Comment 72 wilson.foo 2012-08-21 17:42:46 UTC
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>
Comment 73 wilson.foo 2012-08-21 17:55:17 UTC
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;
};
Comment 74 Stas Sergeev 2012-08-21 19:35:15 UTC
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
Comment 75 wilson.foo 2012-08-22 04:22:40 UTC
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.
Comment 76 Stas Sergeev 2012-08-22 08:48:25 UTC
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?
Comment 77 GNOME Infrastructure Team 2018-05-22 12:11:41 UTC
-- 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.