Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PropertiesWindow to handle different Shape types #98

Open
synapticvoid opened this issue Oct 4, 2015 · 4 comments
Open

Refactor PropertiesWindow to handle different Shape types #98

synapticvoid opened this issue Oct 4, 2015 · 4 comments
Milestone

Comments

@synapticvoid
Copy link
Member

Several things to consider:

  • we should have a "generic" code in PropertiesWindow displaying different widgets based on the Shape type. Each of these widget should handle their sole properties and take for granted that the right ShapeType is used. I think that this is important because the PropertiesWindow class will grow into a monster after we added a few dozen properties.
  • Right now we have ShapeType, from the PropertiesWindow viewpoint, it doesn't matter that we are working with a Canvas/Rectangle/Ellipse. What matters is that it's a ShapeBezier or a ShapeText.

A possible solution could be to used a bitfield to store ShapeBezier and ShapeCanvas. The test would look like:

if(shapeType & ShapeType::Bezier) {
...
}
@synapticvoid
Copy link
Member Author

What's your opinion on this @GuillaumeLazar ?

@GuillaumeLazar
Copy link
Member

I agree with the refactor idea.

Your bitfield idea could be a solution, but in this case that PropertiesWindow that determine which properties is related to each Shape.

Another solution, maybe more generic, could be to define a enum for each properties available. Each Shape into the construcor (or an init function) fill a QList with dependent properties. In this case PropertiesWindow will only displayed properties of this QLIst, no matter if we add another Shape type in the future, PropertiesWindow will not be dependent of Shape type but only of new properties.

@synapticvoid
Copy link
Member Author

That's a good idea!

The only thing that bothers me is that the Shape model should not have any knowledge of how/which property is exposed for the UI (it simply holds and exposes its own data). I don't know how we could do it without "polluting" the Shape classes. Do you think we could keep it away from Shape (I don't know, maybe using some kind of QHash properties <ShapeType, <QList<PropertyWidget>>) ?

@GuillaumeLazar
Copy link
Member

In my mind, Shape will don't store "any knowledge of how/which property is exposed for the UI", but store "which property" it support. And UI will use this information to do display (or not) something.

For example, ShapeLine does not support the property "Background picture". So the ShapeLine constructor will not add PROPERTY_BACKGROUND_PICTURE on thier list of supported properties.

We can also put it outside of Shape as you suggest into a QMap/QHash but it seems to me more logical that each model store supported properties.

@synapticvoid synapticvoid added this to the alpha-07 milestone Nov 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants