GNOME Bugzilla – Bug 731484
Usage of Glib::Property is limited to direct data member of the containing object
Last modified: 2016-01-25 13:52:49 UTC
Created attachment 278223 [details] [review] Rename ambiguously-named overridden property variables and types The attached patches reduce this limitation to the requirement that the same number of properties must be initialized in the same order for a certain object. Also, see https://mail.gnome.org/archives/gtkmm-list/2014-June/msg00011.html The patches are preliminary and so far received VERY MINIMAL testing.
Created attachment 278224 [details] [review] PropertyBase: Use Pimpl to increase struct size without breaking the ABI
Created attachment 278225 [details] [review] Use g_object_notify_by_pspec instead of g_object_notify
Created attachment 278226 [details] [review] PropertyBase: Make custom properties much more flexible
Glib::ObjectBase::destroy_notify_() is virtual. There is a Gtk::Object:: destroy_notify_() override. Glib::ObjectBase::destroy_notify_() is not called when gtkmm widgets are deleted. It's not a good place for PropertiesListType* props = get_properties_list(gobject_); if (props) delete props; ObjectBase::~ObjectBase is even worse, because gobject_ is usually 0 when ObjectBase::~ObjectBase is called. The best way to delete the properties list is to set the GQuark with g_object_set_qdata_full() and have a GDestroyNotify function called when the C object is finalized. BTW, if the code could have been kept in destroy_notify_(), it could have been simplified to delete get_properties_list(gobject_); The 'delete' operator does nothing, if it's called with a null pointer. This is the only flaw I'm noticed by looking at the code. I like your approach. It loosens the restrictions on Glib::Property<>. Some of the changes formally break ABI or API: Your first patch that changes some names in Class. The change from GParamSpec* to Private* in PropertyBase. I think this is acceptable. The changes should affect only code in glibmm. But you never know. There may be application programs that use glibmm in unforeseen ways. See e.g. bug 681206 comment 5 on what happened when I made a change in a protected member variable in Glib::OptionGroup. If your changes are accepted (which I think they should be, perhaps after more testing), then the description of Glib::Property also needs some changes. Property<> can't be used in every conceivable way. E.g. not like so: class Strange : public Glib::Object { Glib::Property<int>* m_pa; Glib::Property<int>* m_pb; public: Strange(int i) : m_pa(0), m_pb(0) { if (i < 0) m_a = new Glib::Property<int>(this, "prop_a", i); else m_b = new Glib::Property<int>(this, "prop_b", i); } };
In objectbase.cc: Object::PropertiesListType* ObjectBase::get_properties_list(GObject* obj) Object::PropertiesListType* ObjectBase::init_properties_list(GObject* obj) Shall be ObjectBase::PropertiesListType* ObjectBase::get_properties_list(GObject* obj) ObjectBase::PropertiesListType* ObjectBase::init_properties_list(GObject* obj)
I have pushed the patches in comment 0 and comment 2 with minor changes. I'm working on a solution to bug 732746, which will also affect property.cc.
If the new properties list is not deleted in Glib::ObjectBase:: destroy_notify_(), but in a callback function, set with g_object_set_qdata_full(), like I suggest in comment 4, then the properties list is used only in property.cc. Its GQuark can be local data in property.cc.
Whoops, indeed Gtk::ObjectBase::destroy_notify_() is not a good place for deletion of properties. I will update the patches with a fix as you suggest. By the way, thanks for taking care of the interfaces in bug 732746.
Is it really necessary to replace Glib::PropertyBase::param_spec_ by Glib::PropertyBase::priv_? In the patch in comment 3, priv_->property_id is written twice, first in the constructor, then in install_property(), but it's never read. The only time a PropertyBase needs to know its own property_id is in install_property().
Created attachment 280525 [details] Test case, source code and results The attached tar file contains a test case and results with and without the patches in comment 1 and comment 3. The test case shows that the approach in the main patch (comment 3) does not work. Each object is supposed to get a GQuark with a vector of PropertyBase pointers in PropertyBase::install_property(). But only the first constructed object gets such a GQuark. install_property() is called only if lookup_property() does not find the property.
Do you intend to give this a new try? The basic idea is good. I think it will work if you move the new code in install_property() to a new function that's unconditionally called from Property<>'s constructors, or perhaps from PropertyBase's constructor. I have pushed a commit that fixes bug 732746. You should get the latest source code from git before you make new changes in property.cc.
Correction: Of course some new code must be kept in install_property(). But props->push_back(this) must be moved somewhere else.
> Do you intend to give this a new try? Sure. The only problem is that I'm a bit busy at the moment, but this is temporary issue :-)
Are you still busy?
Review of attachment 278223 [details] [review]: Was committed with minimal changes 18 months ago.
Review of attachment 278225 [details] [review]: Was committed 18 months ago.
I have pushed an improved version of the patch in comment 3- https://git.gnome.org/browse/glibmm/commit/?id=858195b75b36e2d8958a946f2559eb7beae98355
Created attachment 319683 [details] Modified test case, source code and results A corrected version of the test case. I have also added two custom properties in an array, std::unique_ptr<Glib::Property<int>> custom_property_array[2]; That's possible only with the newly pushed patch. Before the patch was applied, the test case with the property array crashed. The result file without the patch shows the result without the property array.