Created attachment 194235 [details] [review]
libanjuta, am-project: add an ID string to AnjutaProjectProperty

Review of attachment 194235 [details] [review]:

I have looked at your patch. I think we don't need to add a new id argument to the anjuta_project_property_new procedure because the id of each property is the id of its corresponding native property. The id doesn't need to be allocated dynamically neither, as it's already done in the native property.

The native property is an static object containing all information on a particular property, its type (string, integer ...), the default value and other informations. In fact, perhaps it would be better to use a different type (the native property will be rather a property description type) and we keep in the anjuta property type only for the value of the property.

I have used a single type because I haven't much attributes in a property and some could be shared by example the value attribute of a property could be understand as a default value in the property description. Moreover it could avoid to have two set of functions on the property and on the property description. But it seems that it's not clear. Do you understand how the properties are working? and do you think it will be better to make two types?

(In reply to comment #2)
> Review of attachment 194235 [details] [review]:
> 
> I have looked at your patch. I think we don't need to add a new id argument to
> the anjuta_project_property_new procedure because the id of each property is
> the id of its corresponding native property. The id doesn't need to be
> allocated dynamically neither, as it's already done in the native property.

But the native property itself needs to be allocated dynamically in dynamic languages (and also for vala, if we use introspection bindings as is).
 
> The native property is an static object containing all information on a
> particular property, its type (string, integer ...), the default value and
> other informations. In fact, perhaps it would be better to use a different type
> (the native property will be rather a property description type) and we keep in
> the anjuta property type only for the value of the property.
> I have used a single type because I haven't much attributes in a property and
> some could be shared by example the value attribute of a property could be
> understand as a default value in the property description. Moreover it could
> avoid to have two set of functions on the property and on the property
> description. But it seems that it's not clear. Do you understand how the
> properties are working? and do you think it will be better to make two types?

Yes, more or less. (but I don't think I understand how the am-project is using them).
I feel it would be better to use two types, but again, I'm not sure how this would fit in
the way the am-project plugin is using it. If we have two types, would am-project need
to extend only one or both?

And btw, I still don't like the way memory management is done for properties, but that's
probably for another bug.

(In reply to comment #3)
> But the native property itself needs to be allocated dynamically in dynamic
> languages (and also for vala, if we use introspection bindings as is).

Currently, the native property are never allocated dynamically in C.
We need a special function to allocate them.
If we keep the same type, we can need to allocate the id only if native is NULL.


> I feel it would be better to use two types, but again, I'm not sure how this
> would fit in the way the am-project plugin is using it. If we have two types,
> would am-project need to extend only one or both?

Both. The am-project keeps additional data in the property object with its value like the position in the file where the property is defined. And in the "native property" object, there are additional data like the kind of token used by this property or the name of the variable used to set such property.

(In reply to comment #4)
> (In reply to comment #3)
> > But the native property itself needs to be allocated dynamically in dynamic
> > languages (and also for vala, if we use introspection bindings as is).
> 
> Currently, the native property are never allocated dynamically in C.
> We need a special function to allocate them.
> If we keep the same type, we can need to allocate the id only if native is
> NULL.

That's ok with me.

> > I feel it would be better to use two types, but again, I'm not sure how this
> > would fit in the way the am-project plugin is using it. If we have two types,
> > would am-project need to extend only one or both?
> 
> Both. The am-project keeps additional data in the property object with its
> value like the position in the file where the property is defined. And in the
> "native property" object, there are additional data like the kind of token used
> by this property or the name of the variable used to set such property.

So what do you think, should we use 2 types? If yes, what would each of them contain?

If I'm following correctly with the code (what's currently in am-project), the "native properties" use everything in AnjutaProjectProperty except value, and the "custom properties" use only name and value (and could use id instead of name, or even just use the value from).

So if we use 2 types, they should be: native property {id, name, type, flags, detail} and custom property {native, value}. Or am I missing something?

Anyway, it's up to you to decide whether we want to use 2 types or not, but I think it may be easier to continue using one type for now, even if we decide to use 2 types in the long run (I would like to have my patches in bug 657491 in 3.2)

Review of attachment 194235 [details] [review]:

I have committed your patch, so you can apply our others changes. I have only removed the "AM_" prefix on all properties id.

Anyway, the way the properties are designed looks quite bad now. This need some work to redesign it in a better way. I don't know if we can do it before the next release. Anyway, at that point it shouldn't affect the user so I prefer to concentrate on more visible bugs.

This is fixed in git master