Created attachment 179832 [details] [review]
libanjuta: correctly register gobject properties for AnjutaProjectNode

(In reply to comment #0)
> One thing I'm not sure about is memory management for *_properties 
> lists. If I understand correctly, native_properties isn't owned by the 
> node and thus shouldn't be freed, while custom_properties is (since it 
> is appended to in set_property).

Yes native properties are owned by the project backend. These are static data used by all nodes of the same type. They should be a const GList * but it doesn't work well with GList.

The custom properties are owned by the node, there is nothing special with them.

Review of attachment 179832 [details] [review]:

Thanks, it looks fine. I have just committed it.

Is there something else to come or could I close this bug?

I want to also fix/document how memory management is done:

If I understand correctly native_properties is owned (including the properties it contains) by the project backend (and so is correct as it is right now).

Now for custom_properties, how should it be done? If I understand correctly from {insert,remove}_property, the node owns the list but not the properties, but the list isn't freed in dispose/finalize. I understand that this may be because the project backend isn't supposed to keep a pointer to the property, but should rather remove all the properties before removing the node? If so, it should probably warn in dispose if it finds custom properties not yet freed.

Another issue is how to handle setting the property from gobject: should we just make custom_properties not writable and have the user only use {insert,remove}_property to edit it?

(In reply to comment #5)
> If I understand correctly native_properties is owned (including the properties
> it contains) by the project backend (and so is correct as it is right now).

Yes, right. 


> Now for custom_properties, how should it be done? If I understand correctly
> from {insert,remove}_property, the node owns the list but not the properties,
> but the list isn't freed in dispose/finalize.

No, the node owns the list and the properties. When you remove a node, you automatically removes all its properties. By example

amp-group.c:
static void
amp_group_node_finalize (GObject *object)
{
        AmpGroupNode *node = AMP_GROUP_NODE (object);

        g_list_foreach (node->base.custom_properties, (GFunc)amp_property_free, 


amp-properties.c:
amp_property_free (AnjutaProjectProperty *prop)
{
        if (prop->native == NULL) return;

        if ((prop->name != NULL) && (prop->name != prop->native->name))
        {
                g_free (prop->name);
        }
        if ((prop->value != NULL) && (prop->value != prop->native->value))
        {
                g_free (prop->value);
        }
        g_slice_free (AmpProperty, (AmpProperty *)prop);
}


> Another issue is how to handle setting the property from gobject: should we
> just make custom_properties not writable and have the user only use
> {insert,remove}_property to edit it?

Yes, all properties, custom and native ones, are not writable and the user only uses the functions provided by the interface to modify them.

(In reply to comment #6)
> > Now for custom_properties, how should it be done? If I understand correctly
> > from {insert,remove}_property, the node owns the list but not the properties,
> > but the list isn't freed in dispose/finalize.
> 
> No, the node owns the list and the properties. When you remove a node, you
> automatically removes all its properties. By example
> 
> amp-group.c:
> static void
> amp_group_node_finalize (GObject *object)
> {
>         AmpGroupNode *node = AMP_GROUP_NODE (object);
> 
>         g_list_foreach (node->base.custom_properties, (GFunc)amp_property_free, 

To me, this seems like the node owns the list but not the properties ;-)

When I say 'node', I'm talking about AnjutaProjectNode: to me a subclass in the project backend is part of the project backend.

Still, unless I'm missing something, the list should actually be freed in anjuta-project.c


> > Another issue is how to handle setting the property from gobject: should we
> > just make custom_properties not writable and have the user only use
> > {insert,remove}_property to edit it?
> 
> Yes, all properties, custom and native ones, are not writable and the user only
> uses the functions provided by the interface to modify them.

There is no function to set native_properties (except using g_object_set with my patch above), so I guess there is a misunderstanding here.

(In reply to comment #7)
> To me, this seems like the node owns the list but not the properties ;-)
> When I say 'node', I'm talking about AnjutaProjectNode: to me a subclass in
> the project backend is part of the project backend.

Ok, in my mind, backend was the plugin backend object, which is different from the node objects. I wasn't taking care if the implementation comes from libanjuta or the backend. 


> Still, unless I'm missing something, the list should actually be freed in
> anjuta-project.c

Yes right, there is a memory leak. I have forgotten to free, the list. I think the list has to be freed in the backend too (in amp_group_node_finalize in the example above).


> There is no function to set native_properties (except using g_object_set with
> my patch above), so I guess there is a misunderstanding here.

There is no function to set native_properties because they are read only. You cannot change their values. They are the default value for nodes of the corresponding kind. The list of native_properties defined all possible properties of a nodes. It is specific to each kind of nodes and cannot be changed, most properties will probably need some specific code to be saved and even loaded.

You normally set custom properties using ianjuta_project_set_property. The property argument of this function is normally a native property but it's working the same if it's a custom property.

It's a bit different for ianjuta_project_remove_property. Normally you expect a custom property, but you can use a native property. It will have the same effect if the native property has only one associated custom property. But you can have several custom properties (list of installation directories in group object). In this case you delete only the specified custom property.

(In reply to comment #8)
> > Still, unless I'm missing something, the list should actually be freed in
> > anjuta-project.c
> 
> Yes right, there is a memory leak. I have forgotten to free, the list. I think
> the list has to be freed in the backend too (in amp_group_node_finalize in the
> example above).
That's also fine.

> > There is no function to set native_properties (except using g_object_set with
> > my patch above), so I guess there is a misunderstanding here.
> 
> There is no function to set native_properties because they are read only. You
> cannot change their values. They are the default value for nodes of the
> corresponding kind.
Yeah, I know that. But this "default value" also needs to be set, right? :-)

The project backends in Anjuta tree all set it directly, but the doc comment for AnjutaProjectNode says it shouldn't be accessed directly. I guess the best way would be to make it construct-only, what do you think?

(In reply to comment #9)
>> I think the list has to be freed in the backend too (in
>> amp_group_node_finalize in the example above).
> That's also fine.

Ok, I will do it like this and fix the memory leak.


> Yeah, I know that. But this "default value" also needs to be set, right? :-)
> The project backends in Anjuta tree all set it directly, but the doc comment
> for AnjutaProjectNode says it shouldn't be accessed directly. I guess the best
> way would be to make it construct-only, what do you think?

I don't know how it maps in vala and introspection stuff but these native properties are created before any node. In C++ or in python, these properties should be implemented as class constants. You cannot change them even when you create a new node.

At creation time, you can only select which list of native properties will be used which is normally linked to the type of the node.

But you can never changed the value of these properties, it is like a constant string, initialized before the beginning of the program.

(In reply to comment #10)
> > Yeah, I know that. But this "default value" also needs to be set, right? :-)
> > The project backends in Anjuta tree all set it directly, but the doc comment
> > for AnjutaProjectNode says it shouldn't be accessed directly. I guess the best
> > way would be to make it construct-only, what do you think?
> 
> I don't know how it maps in vala and introspection stuff but these native
> properties are created before any node. In C++ or in python, these properties
> should be implemented as class constants. You cannot change them even when you
> create a new node.
Vala doesn't have class constants, only class fields: and they are implemented as fields in the *Class struct, like activate_signal in GtkWidgetClass or menu_item_type in GtkActionClass. The "you cannot change them" requirement cannot be enforced in Vala (like it isn't in C or Python), but the rest is ok.

As for introspection, *Class structs are in the gir file, but you must extract the needed info yourself, so binding is using this ATM.
 
> At creation time, you can only select which list of native properties will be
> used which is normally linked to the type of the node.

This describes (more or less) a construct-only gobject property, so I guess the easiest way to fix the problem is to register it as a construct-only property.

Review of attachment 179832 [details] [review]:

Corrected patch status...

This is now fixed after lengthy discussion and the patches in bug 666679