long post ahead
Not a bug fix, just a refactor of a few core pieces of code. POC build here: https://github.com/vosk/FreeCAD/pull/2/files
I tried to mentally define what a Property has to share with a DynamicProperty and there are pieces of code that I would really like to see cleaned up.
Key concepts as I derived them from the code, and the beginning of the suggested changes:
- Properties are owned (lifecycle) by PropertyContainers, and are constructed inside containers. The Property::setContainer is a key point of ownership.
- Somehow, Properties delegate their OWN PropertySpec attributes (name,group,doc) to the container. This is IMHO poorly cohesive code
- DynamicProperties apparently differ only at the point of construction, they do not get instantiated during container construction. that is IMHO a triviality.
- Ownership is not stated explicitly
- For legacy reasons, there exists PropertyType, a typeless "attribute" and StatusBits all bunched up together, setting and getting these is scattered in a few places, and there is this weird Property::syncType that is to me a sore point.
- The Touched property status bit is a poorly defined as a triggering mechanism for recomputations, since you can setValue to a property and this will Touch it, but you may also Touch it, but never actually change anything in it.
So unraveling the changes I tried to capture in the PR:
A)I created a StatusCollection class that templates on an enum type, thus using type safety to capture the difference between a single status bit, and a set of bits (cough bitset, not the best choice, but it sort of works). I created an operator+ overload so that eg Hidden+ReadOnly create a StatusCollection that can be captured during Property construction. The Property attribute enum for status bits, I named PropertyStatus, and Prop_None becomes and empty collection of prop, and a constexpr.
B) Since many classes use StatusBits (bitset) , I created a StatusContainer class, in sort of, a similar scheme to PropertyContainer. The class defines strongly typed setStatus, testStatus overloaded to capture the needs of FreeCAD as far as I explored it. There is an event capturing callback function onStatusChanged. Property now inherits StatusContainer, and implements onStatusChanged to notify the father container. No need to implement other set/test methods in Property, but isTouched() etc, I kept to minimize the carnage.
C)To state ownership explicitly, PropertyContainer now implements isOwnerOf(Property) that simply tests Property.getContainer()==this. ExtensionContainer overrides this, and tests extensions also. This eliminates the need for these awkward container->getPropertyName(prop), and they are instead written much more beautifully as container->isOwnerOf(prop), that are needed mostly to distinguish ViewProviderDocumentObject properties from DocumentObject properties. This captures the existance of dangling Properties without an owner (no parent container).
D)Having done C) I shared the ownership of PropertySpec (ie. name,group,doc) into Property. This simplifies getName() getDocumentation() getGroup() greatly, just check this->PropertySpec. These three const char* I turned into const string, so THIS avoids the strange situation in DynamiProperty, where the c-strings there are dynamically allocated, but not owned explicitly when instantiating from python, which may lead to bugs. Now, some null checks are rendered useless.
E)Having done D), DynamicProperty simplifies the ownership of properties, by just keeping PropertSpecs and a Property* it now does not have a name and a pName, the most confounding thing I have seen the past year in software.
F)Since the Property is now co-owning (name,group,doc), PropertyContainer (and its derived classes) do not need a few getProperty<Name,Group,Documentation,Type>, they are deleted. The only getProperty of any real use, is getPropertyByName.
G)Since PropertyType is now NOT needed, nor are "short attr", wherever they are loaded from XML files for backward compatibility I created LegacyPropertyType, that transforms the old values to the new StatusCollection<PropertyStatus>
Open issues:
1)Having eliminated propertyType from PropertySpec, we are now left with awkward ifs that thest for Hidden and Prop_Hidden, Readonly and Prop_Readonly etc. While I have NOT ensured that Prop_* may not be altered, this can be captured in StatusContainer, as deemed necessary. I believe it is not, I stipulate that we remove Prop_* entirely from the enumeration. If Prop_Hidden was enough, then Hidden wouldn't exist, etc. This will need another aspect to be explored, how do we read from the XML files in a single place where all legacy values are transformed into the current state of the enumeration, regardless of serialization concerns. Also, handling legacy python status stuff can be confined to its specific file, and not tested anywhere else.
2) Status change callbacks are a little bit lacking in definition in FreeCAD, I suspect this has caused a lot of grievance in the past with double recomputes, no recomputes etc.
3) What the HELL are those User1 User2 User3 User4 enum values? A B C D would have been BETTER, since User implies the user has something to do with them, which he does not.
Open to ideas and criticism.
