Hi Eric,
Le 10 juin 09 à 09:26, Eric Wasylishen a écrit :
> Here's my first attempt at the refactoring.
>
> I changed any implementations of the -(NSArray *)properties methods to
> -(NSArray*)propertyNames, and removed the [super properties]
> arrayByAddingObjectsInArray: properties]; bits, so they just return
> the properties for that class.
Perfect.
> The -(NSArray *)allPropertyNames method is implemented in
> EtoileFoundation/ETPropertyValueCoding.m, in an NSObject category, but
> not part of the ETPropertyValueCoding protocol.
I'm not convinced by that. ETPropertyValueCoding protocol is only just
a way to formalize the protocol, since all classes will automatically
inherit it from NSObject. Currently it isn't declared on NSObject, but
it should be since NSObject implements all the requested methods.
The protocol also makes clearer that a custom root class other than
NSObject must conform to it to be Étoilé-compliant.
> I thought this is the
> best approach, since allPropertyNames only needs to be overridden if
> an object wants to hide/not inherit properties from its superclasses.
Right, but this shouldn't be allowed normally, in order to let
developers fully introspect every properties that make up an object
and not only the visible/displayed ones. This will be useful when you
switch to the 'development mode' in a running Étoilé-native application.
-[ETLayout setDisplayedProperties:] in EtoileUI is available to let
you set which properties are visible in the UI.
In the long run, we might want to add an extra method -
allVisiblePropertyNames, then a layout can transparently use these as
displayed properties. However I'm not sure we should add it to
NSObject. When this flexibility will be needed, it could be a better
choice to specify that with an ETModelDescription.
Once ETModelDescription will be finished, we could reimplement -
propertyNames and -allPropertyNames to behind the scene just store the
property names in a model description object per class, then this
model description could also be used as a cache to avoid recursing up
to the root class each time we call -allPropertyNames. The cache could
be implemented with a simple dictionary keyed by class in the current
implementation though.
So in the end, I'm in favor of the following in NSObject+Model:
NSObject (Model) <ETPropertyValueCoding>
// ETPropertyValueCoding protocol
-propertyNames;
-allPropertyNames;
-valueForProperty;
-setValue:forProperty:
// all the other methods
-isCollection;
-isMutable;
etc.
@end
Does that make sense or do you still feel it isn't the best approach?
Everything looks mostly fine in your patch. Here are some additional
comments…
You should probably use the A() macro everywhere it's possible. Not
really critical but this would make the code a bit more concise.
> +// FIXME: Why are these static?
> static id (*valueForKeyIMP)(id, SEL, NSString *) = NULL;
> static void (*setValueForKeyIMP)(id, SEL, id, NSString *) = NULL;
Hm, it looks wrong. I wanted to make these variables static to avoid
collisions in case identically named function pointers also declared
in other files. This should have been int (static *valueForKey)(id,
SEL, NSString *) = NULL;. However I'm not sure at all this is valid C
or even makes sense. Are C function pointers normal variables or not
really?
> +...@implementation NSObject (ETPropertyValueCoding)
>
> +- (NSArray *) allPropertyNames
> +{
> + NSMutableArray *properties = [[self propertyNames] mutableCopy];
> + for (Class c = [self superclass]; c != Nil; c = [c superclass])
> + {
> + id (*propertyNamesIMP)(id, SEL) =
> + (id (*)(id, SEL))[c instanceMethodForSelector:
> @selector(propertyNames)];
> +
> + [properties addObjectsFromArray:
> + propertyNamesIMP(self, @selector(propertyNames))];
> + }
> + return properties;
> +}
> +
> +...@end
I doubt caching the IMP is faster than calling the method directly, it
could even be slower.
Although you call the same selector -propertyNames, the method
implementation is not shared. Each class has its own method
implementation. The IMP cannot be reuse on several objects in the
loop. Which is usually the case where an IMP call can make a difference.
Great work. Feel free to commit once you have made the necessary
adjustments.
Cheers,
Quentin.
_______________________________________________
Etoile-dev mailing list
Etoile-dev@gna.org
https://mail.gna.org/listinfo/etoile-dev
