GNOME Bugzilla – Bug 134161
get_widget_derived() alternative
Last modified: 2016-01-28 10:07:28 UTC
Here's an alternate way to implement derived widgets in libglademm, as discussed here: http://mail.gnome.org/archives/gtkmm-list/2003-October/msg00070.html
Created attachment 24326 [details] derived.cpp - Source for Gnome::Glade::Derived class
Created attachment 24328 [details] derived.h - Header file for Gnome::Glade::Derived class
Created attachment 24329 [details] [review] DerivedExample.cpp - Source for simple Gnome::Glade::Derived example
Created attachment 24330 [details] DerivedExample.glade - Glade file for simple Gnome::Glade::Derived example
Unfortunately, setting up all the prereqs for building the latest CVS libglademm turned out to be much harder than I first thought. As I lack the time to do that now, I'm attaching my source files instead of a patch. Merging it should be extremely straightforward anyway as no existing code is modified. All suggestions for improvement that I have received since posting the initial code have been included. Please feel free to modify as desired.
> setting up all the prereqs for building the latest CVS libglademm In future, I recommend jhbuild.
Created attachment 46453 [details] [review] Patch
I've finally had the chance to look at the patch. It looks pretty good, and seems to work. In summary, it lets you do this: class HelloDialog : public Gnome::Glade::Derived<Gtk::Dialog> { public HelloDialog() : Gnome::Glade::Derived<Gtk::Dialog>("derivedclass.glade", "exampleDialog") {} }; or class HelloButton : public Gnome::Glade::Derived<Gtk::Button> { public HelloDialog(const Glib::RefPtr<Gnome::Glade::Xml>& xml) : Gnome::Glade::Derived<Gtk::Dialog>(xml, "exampleButton") {} }; However, in almost all cases you would want the constructor to take a Glade::Xml, so you can do this: class HelloDialog : public Gnome::Glade::Derived<Gtk::Dialog> { public HelloDialog() : Gnome::Glade::Derived<Gtk::Dialog>("derivedclass.glade", "exampleDialog"), m_button(get_xml(), "button") {} }; So the only advantage to this method that I see is that it requires less code, and can be used in the initializer list. That's good though. However, 1. it uses the SomeWidget(cobject) constructor directly, without checking whether there is an existing instance that it should return (See ObjectBase::_get_current_wrapper() in the existing get_widget_derived()), so this would cause problems if two widgets were instantiated for the same item in the glade file. 2. It doesn't check whether the instantiated C widget is of the expected type. But that should be easy to add.
I like the look of this patch from a brief inspection but I was wondering whether it would make sense for those constructors that don't take a reference to a Gnome::Glade::Xml to also take another string argument representing the domain for I18N? As I remember the create*() methods used to create a Gnome::Glade::Xml reference take a string representing the translation domain, but if this constructor is used DerivedBase(const std::string& filename, const std::string& widgetname) there seems to be no way to specify the translation domain.
> whether it would make sense for those constructors that don't take a reference > to a Gnome::Glade::Xml to also take another string argument representing the > domain for I18N I guess we need to know when/if that is useful.
(In reply to comment #10) > > whether it would make sense for those constructors that don't take a reference > > to a Gnome::Glade::Xml to also take another string argument representing the > > domain for I18N > > I guess we need to know when/if that is useful. > This might be able to sum it up better than I can http://developer.gnome.org/doc/API/libglade/libglade-i18n.html So from my understanding it would be useful when you want to support I18N when the translation domain used to look up the translations is different from the default AND you want to use the version of the constructor that only takes a filename/widgetname(/domain) rather than the constructor that takes a Gnome::Glade::Xml reference. As I've said above, it is still possible to specify the translation domain when creating the glade Xml reference and then using that in the Derived<T> constructor. I only brought it up because it seemed a little inconsistant, I don't see it as an issue.
Ok so, Andre Gaul sent this patch to the list in 2007, which can be found here: http://mail.gnome.org/archives/gtkmm-list/2007-October/msg00074.html, which is way simpler than the one proposed here. Hmm I just saw it does the same thing as the latest version here, but I guess that then doesn't make a difference. I've fixed the code up a little, and made a version for Gtk::Builder, see attached patches. If someone could review the _get_current_wrapper() stuff please, that would be nice.
Created attachment 127341 [details] [review] Gtk::BuilderWidgetLoader, simpler derived instantiation for Gtk::Builder
Created attachment 127342 [details] [review] Gnome::Glade::WidgetLoader, same thing but for libglademm
Could you show how this would be used in application code, please? Let's stick to libglademm for now, because that is what we are most familiar with.
Please?
Time is running out. There's a slim chance that this could be considered before the API freeze if you could reply.
If you replied to comment #15 then maybe we could consider this for Gtk::Builder.
Created attachment 202504 [details] Gtk::BuilderWidgetLoader and an example program. This is a rewritten version of Gtk::BuilderWidgetLoader from comment 13, and a version of the example program gtkmm-documentation/examples/book/builder/ derived that uses Gtk::BuilderWidgetLoader instead of Gtk::Builder::get_widget_derived(). If it's considered worth implementing, I can make two patches (one for gtkmm, one for gtkmm-documentation). Then I will also have to add some text in the Gtkmm tutorial, chapter Glade and Gtk::Builder, section Using derived widgets.
The libglademm bug 303044 contains another idea that makes it possible to use derived widgets with arbitrary constructors. I don't know if it's better or worse than this one, or just different. That bug contains no example how to use the new get_widget_derived() overload, suggested there.
Created attachment 304399 [details] [review] Arbitrary constructor parameters with variadic templates Here is another way to add support for (mostly) arbitrary constructors, using C++11 variadic templates and std::forward. I called the function create_widget_derived since its semantics are slightly different compared to get_widget_derived in case a C++ wrapper exists already for the C instance. The new function always fails, while get_widget_derived returns the existing C++ instance if it is compatible. I know there have been some discussions about using C++11 features in gtkmm. I don't know what the current status is, but I put this here in any case as an idea.
Created attachment 304400 [details] Example program for create_widget_derived An example program that shows how create_widget_derived could be used.
I vote for Armin's proposal with a variadic template. Gtkmm 3.18 will use C++11 features. Some details need discussion: - create_widget_derived() or get_widget_derived()? It's a mixture of getting and creating. It gets the underlying C widget from the GtkBuilder and creates a C++ wrapper for it. I have no clear preference for one name over the other. - g_critical() or throw? In my code in comment 19, I use throw to report errors. A problem with throw is that there are no suitable error codes in Gtk::BuilderError. I use std::runtime_error, and that's not ideal. But code in glibmm can throw std::out_of_range, std::overflow_error and std::bad_cast, so I guess it's acceptible.
With C++11 it's easy to make get_widget_derived() accept constructors with extra parameters: Just make it a variadic template. No need for a separate create_widget_derived(). get_widget_derived() can handle constructors both with and without extra parameters. Since it was a template also before this modification, it has not been part of the ABI. Fixed with this commit: https://git.gnome.org/browse/gtkmm/commit/?id=6e98903fc458c96fbf87b2080c9a06a0f221ead3