Created attachment 77131 [details] [review]
Proposed Patch

This is my patch proposal. It works the same way as the code for GtkContainer does. By having the #ifdef construct, this works also if CC_TYPE_LAYOUT is not defined.

May I commit this one?

I'd prefer not to add project-specific stuff to gtk-doc.

I'd prefer to add an option to gtkdoc-scangobj like
   "--query-child-properties=my_query_child_properties_function".

That function would then call the appropriate list_child_properties() function depending on the class.

Working on a patch...

Created attachment 77240 [details] [review]
Proposed Patch #2

So, here's a patch that extends gtkdoc-scangobj by two command line parameters which can be used like this (from Makefile.am):

# Extra options to pass to gtkdoc-scangobj. Not normally needed.
SCANGOBJ_OPTIONS=--type-init-func="g_type_init()" \
                 --check-child-props="CC_IS_LAYOUT_IMPL" \
                 --query-child-props="cc_layout_iface_list_child_properties"

May I commit this patch?

I don't really like the "--check-child-props" argument, I'm afraid.
This feature is going to be very rarely used, so 2 command-line arguments
for it seems too much. Also, we need to support libraries that have more than
one class with child properties.

I'd rather the "--query-child-props" function just do the test itself.

Also, leave the GtkContainer check separate, just in case a library has
GtkContainer subclasses as well as other classes with child properties.

Okay, then please tell me how you'd like to see it done. I'm quite frustrated right now that you didn't like my first patch, claimed some requirements and now you say »hmm, I somehow don't like it still…«. (In that case you could have written a patch on your own.)

So can you please tell me whether you'd like to apply a patch that does the following:

1. Refactor the current code to support multiple checks for child and style properties (my first patch silently assumed that you won't have a GtkContainer implementing CcLayout).

2. Come up with a file format that allows the specification of custom property pools. Requirements: Pool name (»Child Properties«, »Style Properties«, »Foobar Properties«), type check (GTK_IS_CONTAINER(), GTK_IS_WIDGET(), CC_IS_LAYOUT_IMPLEMENTATION()), and pool query function: void function(GObjectClass*, guint*).

3. Add code to the result of #1 to support this file - if existing.

Created attachment 77254 [details] [review]
Patch for 1

So, here's the first patch. It removes the while(TRUE) loop and tries to make the child and style properties a little less hackish (at least easier to extend with custom code).

Now that this patch looks almost nice (I think I'll need to check indentation and spacing before braces again, then it should be fine).

We should discuss how to achieve extensibility. The problem is that eg. multiple child property blocks would have to be split correctly in the code. If you have any input on this, I'd really appreciate it. If not, I'll cook up my own idea tomorrow night.

I should have been clearer in my first comment - sorry about that.

I just want a "--query-child-properties" argument, then code like this after
outputting the GtkContainer and style properties:

if ($QUERY_CHILD_PROPERTIES) {
  print OUTPUT <<EOT;
    if (!child_prop) {
      properties = $QUERY_CHILD_PROPERTIES (class, &n_properties);
      if (properties)
        {
          child_prop = TRUE;
          continue;
        }
   }
EOT
}




So you'd use it like this:

# Extra options to pass to gtkdoc-scangobj. Not normally needed.
SCANGOBJ_OPTIONS=--type-init-func="g_type_init()" \
                 --query-child-properties="cc_query_child_properties"


And your cc_query_child_properties() function would do something like:

  if (CC_IS_LAYOUT_IMPL (class))
    return cc_layout_iface_list_child_properties (class, &n_properties);
  else
    return NULL;


That should work, shouldn't it? It means you need an extra function in your
library, but it's fairly simple.

I can add it to gtk-doc if you like, but it will take me a few days to get
around to it.

(In reply to comment #9)
> I just want a "--query-child-properties" argument, then code like this after
> outputting the GtkContainer and style properties:
> 
> if ($QUERY_CHILD_PROPERTIES) {
>   print OUTPUT <<EOT;
>     if (!child_prop) {
>       properties = $QUERY_CHILD_PROPERTIES (class, &n_properties);
>       if (properties)
>         {
>           child_prop = TRUE;
>           continue;
>         }
>    }
> EOT
> }

To this looks broken. According to comment #5 you'd like to support multiple child properties per class (eg. GtkContainer and CcLayout). This won't work in this loop code because you would need a different child_prop variable for each param spec pool (that's why my latest patch breaks the loop).

> And your cc_query_child_properties() function would do something like:
> 
>   if (CC_IS_LAYOUT_IMPL (class))
>     return cc_layout_iface_list_child_properties (class, &n_properties);
>   else
>     return NULL;

I absolutely disagree here for two reasons:
1. You should only imply as much about the code as necessary.
2. Passing wrong parameters to a function is broken. I protect my code using
   stuff like g_return_if_fail() against abusing and mistakes. Turning off
   these warnings because the documentation tool requires it doesn't sound
   like a good idea to me.

> That should work, shouldn't it? It means you need an extra function in your
> library, but it's fairly simple.

Well, it "would" work. But IMHO it would raise too much requirements about the code to be documented.

I'd rather have a file like this:
c:GTK_TYPE_CONTAINER:gtk_container_class_list_child_properties
s:GTK_TYPE_WIDGET:gtk_widget_class_list_style_properties
c:CC_TYPE_LAYOUT:cc_layout_class_list_child_properties

Then we could easily have one block like this:

if(g_type_is_a(object_type, CC_TYPE_LAYOUT))
{
  properties = cc_layout_class_list_child_properties(object_class, &n_properties);
  object_out_props (fp, object_type, properties, n_properties, "c");
}

Another problem I see with your proposal of the command line argument is the question of how to specify two functions in your project (like GTK+ would need to if widget/container weren't special-cased)? I don't think you'd like to have something like --query-props-func=cc_foo_query:cc_bar_query:…

Created attachment 77260 [details] [review]
Updated Patch

Fixed one small bug and updated indentation to match to the rest of the c code.

Firstly, I didn't say I wanted to support multiple child properties per class.
I said I wanted to support child properties for multiple classes.
Your second patch only supported child properties for one class (& subclasses).
I'm not bothered about multiple types of child properties per class as that
is unlikely to happen.


I don't think you've understood my proposal. I'm only suggesting that we
support a "--query-child-properties" argument, which is passed a class and
returns any child properties for it. That is very simple really.


I'll add support for it when I've got child properties working in GooCanvas.

(In reply to comment #12)
> I don't think you've understood my proposal.

And I don't think you got my point of »just having one function is broken«. To repeat myself:

(My comment #10)
> (Your comment #9)
> > And your cc_query_child_properties() function would do something like:
> > 
> >   if (CC_IS_LAYOUT_IMPL (class))
> >     return cc_layout_iface_list_child_properties (class, &n_properties);
> >   else
> >     return NULL;
>
> I absolutely disagree here for two reasons:
> 1. You should only imply as much about the code as necessary.
> 2. Passing wrong parameters to a function is broken. I protect my code using
>    stuff like g_return_if_fail() against abusing and mistakes. Turning off
>    these warnings because the documentation tool requires it doesn't sound
>    like a good idea to me.

If someone uses the API in a way that's not supposed to be (eg. calling cc_layout_iface_list_child_properties() on a class that doesn't implement this interface) is just wrong. That's why there should be a warning in the code (in MY code). Silently ignoring broken code - as proposed by you - won't make it easier to find passages where people call functions on arguments they are not supposed to.

Just to repeat myself this check has to be in gtk-doc, not in the API to be documented (this is about getting code documented, right? Not about forcing API developers to got a gtk-doc-forced way of ignoring wrong parameters).

I still don't understand your problem.

It is up to you to write the query_child_properties() function in your library.
You can do any type-checking there that you want, and make sure you call any
xxx_list_child_properties() functions only when necessary.

Anyway, let's wait until I get child properties working in GooCanvas, then I can try supporting them in gtk-doc. Maybe I'll understand you then.

(In reply to comment #14)
> It is up to you to write the query_child_properties() function in your library.
> You can do any type-checking there that you want, and make sure you call any
> xxx_list_child_properties() functions only when necessary.

Okay, once again. I want to write my code like this:

GParamSpec**
list_child_props(GObjectClass* my_class, guint* n_properties)
{
  g_return_val_if_fail(n_properties, NULL);
  *n_properties = 0;
  g_return_val_if_fail(g_type_is_a(G_TYPE_FROM_CLASS(my_class),
                                   CC_TYPE_LAYOUT),
                       NULL);

  ...
}

If this code would be called with a wrong parameter, there will be a call to g_warning() to print on the command line that there's a programming mistake (as it's the same kind of error as you would have when you try to cast a GtkHBox to a GtkWindow using the macro GTK_WINDOW()).

gtk-doc should not produce warnings like these. In fact, gtk-doc even tries to not throw any warnings while compiling (to be able to compile with -Wall -Werror). The same level of caution should be respected here, as I might want to run the scanner with --g-fatal-warnings (which is useful to stop when interfaces not not implemented correctly).

Also. it could be possible to not have a class_list_properties(GObjectClass*) but an iface_list_properties(GTypeInterface*) (I think you'll need this in GooCanvas as you are using more interfaces than I do). How would you want to specify this with just one command line argument?

Guess the problem is, that you Damon want a single reflection method for child properties per library, whilest Herzi wants one reflection method per type or maybe even per container type and child-property type.

Herzi's approach has the advantage of being a KISS (Keep It Simple, Stupid) approach, which is generallly a good thing. So if you seek for a short term solution this should be done (IMHO).

A really solution for the problem would follow Damon's path, but I would take it even further: Have one single reflection method for all kind of child properties in the entire type system. For achieving that goal I'd suggest to introduce a new interface GChildPropertiesIface. This interface would require two methods:

GSList* g_child_properties_query_property_types (
    GChildPropertiesIface* container);

GSList* g_child_properties_query_child_properties (
    GChildPropertiesIface* container, 
    GChildPropertyType* type);

First you'd call g_child_properties_query_property_types to figure out what kind of child properties are supported (widgets, styles, candy...) and then you query the child properties for each property type.

Style properties are not child properties so you should subsume it under another name like additional properties.

(In reply to comment #17)
> Style properties are not child properties so you should subsume it under
> another name like additional properties.

Ok, "styles" might be an bad example...

Created attachment 77536 [details] [review]
Proposed patch that works for me

Here's my proposed patch. I've got this working with GooCanvas and it seems to work fine.

Is there any major reason why this shouldn't go in?

because your query function serves just the purpose of documentation and I don't see any reason to bloat the library with a function that's just used for documentation (a library is to be used by applications in 99% of the use cases right?).

Not really taking any position here, but a lot of the overhead of properties is already purely for documentation (nicks/blurbs), so your argument is a bit weak...

I don't wanto to block this from going into gtk-doc I just don't think that this is the way to go. As Damon is the maintainer he's got any right to ignore this objection (as long as I can document those properties).

I've committed my patch to cvs.

This is going to be a very rarely used feature so I don't want to add lots of
complicated stuff to gtk-doc to support it. (We may be the only 2 people who
ever use it!)

It is fairly easy to write the extra function in the library to be documented.
Here's what I've got in GooCanvas:

/* This is a semi-private function to help gtk-doc find child properties. */
GParamSpec**
goo_canvas_query_child_properties (gpointer  class,
				   guint     *n_properties)
{
  if (g_type_interface_peek (class, GOO_TYPE_CANVAS_ITEM))
    return goo_canvas_item_class_list_child_properties (class,
							n_properties);

  if (g_type_interface_peek (class, GOO_TYPE_CANVAS_ITEM_MODEL))
    return goo_canvas_item_model_class_list_child_properties (class,
							      n_properties);

  return NULL;
}