[0.20] - Eliminate PropertyType and typeless "attr" for Property

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
User avatar
voskos
Posts: 67
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

[0.20] - Eliminate PropertyType and typeless "attr" for Property

Postby voskos » Tue Feb 16, 2021 9:32 pm

Hello everyone,
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.
The idiom container.getPropertyName(Property) is used to depict ownership. This is IMO utterly, completely unreadable and leads to bugs, by confusing the name attribute of a Property with the concept of ownership.

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. :lol:

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.
User avatar
Kunda1
Posts: 10528
Joined: Thu Jan 05, 2017 9:03 pm

Re: [0.20] - Eliminate PropertyType and typeless "attr" for Property

Postby Kunda1 » Wed Mar 03, 2021 8:35 pm

bumping for discussion
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
realthunder
Posts: 1981
Joined: Tue Jan 03, 2017 10:55 am

Re: [0.20] - Eliminate PropertyType and typeless "attr" for Property

Postby realthunder » Thu Jun 10, 2021 10:02 am

I think you missed the fact that normal 'Property', i.e. the static property has all its meta data (i.e. PropertySpec) stored as a static data inside each defining class derived from PropertyContainer. The PropertyType served as the static property attributes that are shared by all instances of the same type of object, and cannot be overridden by end-user on per-object/property basis. The StatusBits inside each property is used for per-object/property customization, but has lower priority than static attributes.

I can see that the static property system design is probably borrowed from Coin3D code base, which I do think is a bit over-complicated, just to save some memory I presume. But I am not sure whether or not this warrants a major refactor that you are proposing.
Try Assembly3 (latest version 0.11) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
voskos
Posts: 67
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

Re: [0.20] - Eliminate PropertyType and typeless "attr" for Property

Postby voskos » Tue Jun 15, 2021 7:21 am

Thank you for taking the time to read and respond. I highly respect you and your work !
Storage details for a given object does not warrant (IMHO of course) creating a new type. In fact, I would say this is the DEFINITION of encapsulation.
These details belong in implementation details of Property (in this case).
realthunder wrote: Thu Jun 10, 2021 10:02 am The PropertyType served as the static property attributes that are shared by all instances of the same type of object, and cannot be overridden by end-user on per-object/property basis
Again, sharing and not sharing is a) an implementation detail with respect to storage space and ownership, and b) proven to be a mistake by the fact that duplicate bits are now required. The FACT is, that sometimes readonly is changed, hence a duplicate bit was added. Saying (not YOU specifically) that the readonly bit MUST be read only, and then proceeding to create a parallel storage bit, just to be able to say THIS bit doesn't change, does not reflect the actual requirements of the codebase, of the software model, only our own mental model. This creates just creates dissonance. Questions pop up and the answers is "well actually this, but actually that". These answers indicate that people WANT the software model to reflect X "but actually" it reflects Y.

Again, thank you for time in responding to a n00b in this forum :D
realthunder
Posts: 1981
Joined: Tue Jan 03, 2017 10:55 am

Re: [0.20] - Eliminate PropertyType and typeless "attr" for Property

Postby realthunder » Wed Jun 16, 2021 10:28 am

voskos wrote: Tue Jun 15, 2021 7:21 am Again, sharing and not sharing is a) an implementation detail with respect to storage space and ownership, and b) proven to be a mistake by the fact that duplicate bits are now required. The FACT is, that sometimes readonly is changed, hence a duplicate bit was added. Saying (not YOU specifically) that the readonly bit MUST be read only, and then proceeding to create a parallel storage bit, just to be able to say THIS bit doesn't change, does not reflect the actual requirements of the codebase, of the software model, only our own mental model. This creates just creates dissonance. Questions pop up and the answers is "well actually this, but actually that". These answers indicate that people WANT the software model to reflect X "but actually" it reflects Y.
A bit of history here. The semantics of 'attr' is extended by me. Originally, it only has 'readonly' and 'hidden' bits which are used for property editor access control. I extended it to mirror most PropertyType bits so that it can be set at runtime. It is not complete duplication, so to speak. The actual semantics is supposed to be that if either PropertyType or attr is marked as readonly, then the property is readonly, and so are all the other attribute bits.

The static PropertyType is shared by all instances of the same C++ type. But the same C++ type can be inherited by multiple other types, or extended into different types of Python objects, where they may have different reasons to change some attributes of a static property, which is what 'attr' (i.e. Property::StatusBits) is for. For example, the 'Shape' property by default will save the OCC shape into the document. The Draft Array object can set the 'Transient' attribute bit to the property so that it won't waste storage.

But then, there might be reason for a C++ type to force some attribute on to a static property, such that the python code or the end user can't revert, and that's what the PropertyType is for. Once it is marked as readonly, 'attr' can't unset it. The implementation of the static property system (which exists long before I was involved) dictates that only the type that actually owns the property can set its PropertyType, not any derived types. I'd say that this is probably not by design, and just a side effect. But it's good to have, and not so easy to implement without some sort of static storage like the PropertyType for synchronization.
Try Assembly3 (latest version 0.11) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal