*** Bug 333241 has been marked as a duplicate of this bug. ***

Yes, I'd also vote for the property_something(child) solution.

Created attachment 276580 [details] [review]
Add support for _WRAP_CHILD_PROPERTY in gmmproc.

Created attachment 276589 [details] [review]
Add infrastructure to handle child properties.

Created attachment 276590 [details] [review]
Child properties little demo.

I've uploaded patches to get child properties working, at least for setting, getting and resetting them. I haven't added support for ::child-notify signals until we make sure that we really want child properties implemented in this way. 

Hope you find this implementation pleasant; however I presume it will require changes before being pushed. For example, I'm not sure if we want to add a new method in generate_defs_gtk.cc. All comments are of course welcome; I'll try to make required changes at your request.

--- Add support for GtkContainer child properties (glibmm, gmmproc)
    Add support for _WRAP_CHILD_PROPERTY in gmmproc. (comment 3)

* tools/extra_defs_gen/generate_extra_defs.[h|cc]

  std::string get_property(GParamSpec* pParamSpec, std::string strObjectName)
I recommend
  std::string get_property(GParamSpec* pParamSpec, const std::string& strObjectName)


--- Add GtkContainer's child property handling (gtkmm)
    Add infrastructure to handle child properties. (comment 4)

* gtk/gtkmm/childpropertyproxy.[h|cc]

   // -*- c++ -*-
   /* $Id$ */
Probably a left-over from a previously used version handling program.
No reason to include it in new files.

* gtk/gtkmm/childpropertyproxy.h

The get_value() and set_value() methods should take [const] Gtk::Widget& child
instead of [const] Gtk::Widget* child.

* tools/extra_defs_gen/generate_defs_gtk.cc

   ppParamSpec = g_object_class_list_properties (pGClass, &iCount);
   ppParamSpec = gtk_container_class_list_child_properties (pGClass, &iCount);
Does the first of these two statements do anything useful here?

   strResult += ";; Warning: g_object_class_list_properties() ...
Should be ";; Warning: gtk_container_class_list_child_properties() ...

You might delete the deprecated GTK_TYPE_TABLE from the list of container
classes to get child properties from.

* tools/m4/child_property.m4

   dnl $Id$
I think this is a left-over from a version handling program that was used
before git. No reason to include it in a new file.

   ChildPropertyProxy$4< _QUOTE($3) >'dnl
should be
   Gtk::ChildPropertyProxy$4< _QUOTE($3) >'dnl
ChildPropertiesProxy can be useful in other packages than gtkmm, e.g.
class Canvas : public Gtk::Container
in goocanvasmm, where Canvas belongs to namespace Goocanvas.
(Goocanvas::Canvas does not have child properties, but there might be some
container widget in some package that does.)

GLIBMM_PROPERTIES_ENABLED is always defined nowadays. There's no need for
testing that.

   You rarely need to use properties because there are get_ and set_ methods
   for almost all of them.
Is that true of child properties? Don't you do this job because it's not true?


--- Example to demo child properties (gtkmm)
    Child properties little demo. (comment 5)

Ok


--- General comments

You assume that there can't be a child property with the same name as an
ordinary property. Are you sure of that? It looks like gtk+ keeps a separate
list of child properties, and uses a separate set of functions to handle
child properties.
I haven't found an example, but just imagine that a container class defines
a child property called "margin" while its base class GtkWidget has an
ordinary property also called "margin".

Instead of having a member
  Gtk::ChildPropertyProxy property_foo();
or
  Glib::PropertyProxy property_foo();
depending on which kind of property it is, wouldn't it be safer, and also
more informative for the users, to have
  Gtk::ChildPropertyProxy child_property_foo();
for a child property? And
  (define-child-property foo
in the .defs file?

Comment 0 and comment 2 discuss the alternative
  Gtk::ChildPropertyProxy property_foo(child);
(or perhaps also
  Gtk::ChildPropertyProxy child_property_foo(child);)
Would that be more difficult to implement?

Then you could write, in your demo example
  m_Notebook.child_property_tab_expand(m_TextWidget_Info) = true;
similar to what can be done with ordinary properties.

I do not think implementing the alternative property_foo(child) would be more difficult. I see however something I do not know how to solve; ideas are welcome. For implementing this alternative, I would need to add a new constructor to ChildPropertyProxy that takes three arguments: container, child, and property name. However, argument child could be const and non-const, depending on the property being read only or not. E.g.:

Gtk::ChildPropertyProxy< bool > Notebook::child_property_tab_expand(Gtk::Widget& child)                                                                                                                            
{
  return Gtk::ChildPropertyProxy< bool >(this, child, "tab-expand");
}

Gtk::ChildPropertyProxy_ReadOnly< bool > Notebook::child_property_tab_expand(const Gtk::Widget& child) const
{
  return Gtk::ChildPropertyProxy_ReadOnly< bool >(this, child, "tab-expand");
}

ChildPropertyProxy would need to hold a reference to a Gtk::Widget; however, if I make this reference non-const, then I would need to do a const_cast in the constructor of ChildPropertyProxy_ReadOnly. I do not think this is acceptable. Maybe I can hold a pointer instead of a reference...

Sorry, I just realized that there is a const_cast right in the constructor of PropertyProxy_ReadOnly. Then I would try to implement this using references, unless you state otherwise.

Created attachment 278493 [details] [review]
Add support for _WRAP_CHILD_PROPERTY and define-child-property in gmmproc.

Created attachment 278494 [details] [review]
Add infrastructure to handle child properties.

Created attachment 278495 [details] [review]
Child properties little demo.

Please disregard my two last comments. Please find the new proposed solution in the last patches. I have included the comments made on the first review. Please let me know of anything that needs improvements and of errors.

Now that get_value() takes no parameter, and set_value() takes one parameter,
ChildPropertyProxy can be made even more similar to PropertyProxy.

ChildPropertyProxy and ChildPropertyProxy_ReadOnly can have
    operator PropertyType() const
    { return this->get_value(); }

The code snippet in the description of ChildPropertyProxy does not reflect
the changes in get_value() and set_value(). Compare the description of
PropertyProxy.

Except for this detail and the missing signal_changed(), which you're aware of,
it looks really fine.

Thank you for the quick review.

I'll add operator().

Yes, the description now is erroneous. In fact, documentation of get/set_value() is also wrong. I'll fix that.

Now that I know this is the expected implementation, I will continue implementing (maybe no additional code is needed) and testing signal_changed().

I see a problem in implementing signal_changed() the way ChildPropertyProxy is right now. As it inherits from PropertyProxy_Base, and that class has already a signal_changed() method that returns a SignalProxyProperty, we cannot overload it to return our own SignalProxyChildProperty. We need to overload it because SignalProxyProperty has hardcoded in it the "notify::" prefix, that becomes "child-notify::" with child properties.

I think it is reasonable to stop inheriting from PropertyProxy_Base and make ChildPropertyProxy a top class; I don't know exactly what we could potentially loose by doing this and what we will need to reimplement. Would it be reasonable to create a ChildPropertyProxy_Base?

A non-template base class can be useful in some situations.
E.g. if you want to create a std::vector<Glib::PropertyProxy_Base*>, or in
this case perhaps a std::vector<Gtk::ChildPropertyProxy_Base*>.
I haven't found any std::vector<Glib::PropertyProxy_Base*> in glibmm or
gtkmm, but there are some methods that take a const Glib::PropertyProxy_Base&
parameter.

Conclusion: If you can't use Glib::PropertyProxy_Base as a base class, make
a Gtk::ChildPropertyProxy_Base.

It's a pity that you probably have to duplicate a lot of code just because of
a tiny difference in signal_changed().connect().

I think I have not duplicated much code; it would be worse if I had to reimplement SignalProxyProperty, but for that case I simply subclassed SignalProxyProperty and overwrite the connect() method. I'll soon update some patches. I just started also an example in gtkmm-documentation.

Created attachment 278803 [details] [review]
Add infrastructure to handle GtkContainer child properties.

I'm not sure about the TODO part (if it is true what comes after that) in ChildPropeprtyProxy :S

Only Gtk::Container and its subclasses can have child properties.
Why not change
  Glib::ObjectBase* obj_;
in ChildPropertyProxy_Base to
  Container* container_;
or to
  Container* parent_;
?
The calls to gtk_container_child_[set|get]_property() can be simplified.
Calling GTK_CONTAINER() becomes unnecessary.


I also find the text after TODO in childpropertyproxy.h (and the identical
text in propertyproxy.h) confusing. It was added to the description of
PropertyProxy as a result of bug 523043.
The text is misleading. PropertyProxy and ChildPropertyProxy don't register
properties. They give access to properties that have been registered by other
means. I wonder if the text was added to the wrong class. It seems more like
a description of Glib::Property.
Perhaps it refers to a special use of PropertyProxy, which is illustrated in
https://git.gnome.org/browse/gtkmm-documentation/tree/examples/others/cellrenderercustom/cellrenderertoggle.cc
where a Glib::PropertyProxy is a proxy for a Glib::Property.

Anyway, I recommend that you delete that text from the description of
Gtk::ChildPropertyProxy. As long as there is no Gtk::ChildProperty that can
register custom child properties, the text would be plain wrong in
ChildPropertyProxy. And the need for a ChildProperty class is probably very
low or non-existent. Don't waste time on it, unless someone asks for it.

Created attachment 279472 [details] [review]
Add infrastructure to handle GtkContainer child properties.

Now that ChildPropertyProxy_Base depends directly on Gtk::Container, in order to avoid a circular dependency, gtkmm/childpropertyproxy.h cannot be included in container.hg, and therefore it needs to be included in each .hg that wraps child properties.

Since you don't include container.h in childpropertyproxy_base.h or in
childpropertyproxy.h, it's possible to include childpropertyproxy.h in
container.h. I can build gtkmm with your latest patch and
#include <gtkmm/childpropertyproxy.h> in container.hg. Can't you?

It's the same idea which is used for Glib::PropertyProxy:
Include propertyproxy.h in objectbase.h. Include glibmm/object.h only in
propertyproxy_base.cc. Add only an incomplete class ObjectBase declaration
in propertyproxy_base.h.

If you want to, you can also change the SignalProxyChildProperty constructor to 
  SignalProxyChildProperty(Widget* child, const char* property_name)
    : Glib::SignalProxyProperty(child, property_name) {}

You are completely right. It didn't compile for some reason; maybe because I tested this on osx. On Fedora it compiles without any problem.

Created attachment 279635 [details] [review]
Add infrastructure to handle GtkContainer child properties.

The patches in comment 10 (glibmm) and comment 26 (gtkmm) look good.
I recommend that you push them.

Git does not accept the update of gtk_signals.defs in comment 12 after I
updated that file for the Gesture classes. But perhaps that patch is not meant
to be pushed?

Remaining work:
  Describe _WRAP_CHILD_PROPERTY in the gtkmm tutorial, appendix G.
  Add _WRAP_CHILD_PROPORTY in some .hg files.

If I understood correctly, the recommendation is to push the aforementioned patches *before* completing the tasks under "Remaining work"; is this correct? Anyways the remaining work is, I presume, pretty straightforward, so it can be completed a short period of time.

Do as you like. You can push what's ready now, and other patches later,
or wait and push all at the same time. I don't think it matters.

I expect that adding _WRAP_CHILD_PROPERTY to .hg files will be an ongoing
activity, just like we add some _WRAP_METHOD/SIGNAL/PROPERTY now and then.

Created attachment 279982 [details] [review]
Add _WRAP_CHILD_PROPERTY to tutorial.

In the description, mostly copied from _WRAP_PROPERTY, I wouldn't like to talk much about GtkContainer child properties, that's why a placed a link, but I don't know if that is allowed.

I could translate the new section to castilian spanish, and I tried to do that, but I wasn't able to find how to generate the .po file... sorry.

I don't plan to upload patches here for actually wrapping the child properties, as I think it is easy enough to commit directly.

(In reply to comment #31)
> In the description, mostly copied from _WRAP_PROPERTY, I wouldn't like to talk
> much about GtkContainer child properties, that's why a placed a link, but I
> don't know if that is allowed.

There are lots of links in the gtkmm tutorial. I can't find any other link to
developer.gnome.org/gtk3, but there are links to developer.gnome.org/pango and
developer.gnome.org/jhbuild. So why not to gtk3?

Your patch is good.

> I could translate the new section to castilian spanish, and I tried to do that,
> but I wasn't able to find how to generate the .po file... sorry.

Don't know. If you don't get a useful reply here, perhaps you can e-mail
your question directly to someone who has updated a .po file in
gtkmm-documentation.

> I don't plan to upload patches here for actually wrapping the child properties,
> as I think it is easy enough to commit directly.

I agree.

Juan, a note for patches that you push in the future:
If a patch is related to a bug report, include the bug number in the commit
message.

That's how I found bug 523043 that I mentioned in comment 21.
First use 'git blame' to see when the suspicious code or comment was committed.
Then find the bug number in the commit message.

Sorry, I forgot. Thank you for reminding me. I'll link to this bug report when committing changes to wrap child properties in GtkContainer subclasses.

I just pushed notebook.hg with child properties wrapped, and again forgot to mention the bug report :( I'm really sorry. I'll be more careful hereon.

Isn't this bug fixed?

I think all child properties are wrapped. So yes, I think this can be closed. Even if some child properties were not wrapped I think it is normal work.