cginternals / libzeug

deprecated: C++ sanctuary for small but powerful and frequently required, stand alone features.
MIT License
16 stars 13 forks source link

Further improvements to reflectionzeug #103

Open mjendruk opened 9 years ago

mjendruk commented 9 years ago

Here is a list of things that currently bother me about reflectionzeug's implementation:

the complicated property class hierarchy

now and then: the hierarchy emerged so that different types of properties could have different interfaces way back when the Variant class and options didn't exist. now, it still exists to provide the serialization implementations (toString(), fromString()) for different types and also to provide unification interfaces for integral types etc. problems: it's too complex and you have to implement a toString() and fromString() method for every new type you want to store / access in a property. however, providing a those simple serialization methods is not sufficient for a variety of types, e.g., for integral types there different ways to represent them (academic, i.e., with mantissa and exponent, simple) and for floating point types you also need to specify the precision. you could also prefer to represent color value as simple floating point arrays in you gui rather in hex values. the floating point editor already circumvents toString() and fromString() for this reason. possible solution: remove the serialization methods from properties so that any value type can be stores in properties "as-is". move and implement specialized serialization methods into the serializer / deserializer and editors. you would only have to specialize the property template if you want to support a special interface.

the visitor pattern implementation

now: a "super-charged" visitor pattern implementation is currently used to determine the actual type of a property. it enables you to add new types to the visitor as a user of the library and is therefore relying on RTTI. problems: the visitor implementation needs to perform at least one dynamic_cast per property. there is no way to determine the type of a property in a switch/case way without interrupting the instruction flow through the indirection of visit methods. solution: replace it with some count of type system based on an integer value like in QVariant. But I have no exact idea yet how to do it.

there are too many ways to do the same thing

problem: I have heard that someone found that there were to many ways to do the same thing. I am not sure what that means. (needs confirmation)

the "owns property" flag of PropertyGroups

now: Properties can be part of multiple PropertyGroups. However, only one of them owns them, even though by default a group owns all of its children. To make it work, you have to set the "owns property" flag on every group but one to false. problem: this is error-prone. solution: store properties in groups within shared_ptrs.


I would like to invite everyone to comment on this and maybe add additional annoyances.

sbusch42 commented 9 years ago

One comment on the previous:

the floating point editor already circumvents toString() and fromString() for this reason.

I don't see why this is "circumventing". A specific editor for a specific property type should of course always work with the actual data type of the property, e.g., a color editor knows it works on a color, so it can and should create any type of widgets that seem suitable for editing a color. It was never intended that toString()/fromString() should be used for everything. toString() and fromString() were always meant as a "least common denominator" for properties - if there is no specific editor for my data type, the GUI can at least use a default string editor for this property, so that a user can at least edit each value, even if there is no nice editor for a certain data type.

To add a few more issues with the current reflectionzeug implementation:

Variant interface

problem: The over-generic variant type is hard to use, e.g., VariantMap and VariantArray are not Variants themselves which makes it very annoying to compose and work with variant hierarchies. Also, the conversion system for variants is cumbersome and not intuitive for users, this may lead to programming errors very easily. (See next issue) _solution_: Improve variant interface to improve working with variants and variant hierarchies.

Duplicated functionality between Property and Variant

problem: Both Property and Variant deal with storing values of a certain data type, and both implement different ways of converting between different data types (e.g., to convert a value of an arbitrary type into a string). Both implementations are totally unrelated, do not share the same code and do not offer the same feature set (e.g., with properties I can determine if a type is 'any integral number' or "any color type", with Variant I can't). This is hard to understand, not nice to work with and will most likely lead to inconsistencies (toString in one or the other will give different results). solution: Implement type handling as a central functionality in reflectionzeug and let both Variant and Property use this same functionality.

I suggest to leave the system as it is right now. When there is time in the future, I would recommend to re-design the whole property system in a way that it deals with types first, e.g., all conversions and type-grouping stuff has nothing to do with the storage of variables but depends only on the type. At this level, we could implement all type and conversion related functionality, then Property and Variant can be implemented on top of that. Also, the interface can then be cleaned up and refactored to resolve the problems listed in this issue.

mjendruk commented 9 years ago

Comment to

Variant interface

The Variant class is one of the many small things which I really like. The support of any type was one of the initial requirements. Furthermore, It would make me really feel uncomfortable if the Variant class had various methods to interact with VariantMaps and VariantArrays even though more often than not, it just stored a value. I think it also would violate one of OOPs main principles. I have added a wiki article for Variants.

sbusch42 commented 9 years ago

The Variant itself is OK, but still it is as I said: working with it to compose a hierarchy is just a pain, and that's a very important use case I have (and had from the beginning). Please acknowledge that this is a requirement that is just there. I'm sure there is a way to improve on that, it doesn't matter so much how it is done.

mjendruk commented 9 years ago

We will find a solution that is satisfying for all of us once I am back in January.