Properties framework #2857

Merged
merged 42 commits into from Jan 23, 2017

Conversation

Projects
None yet
5 participants
@nyalldawson
Contributor

nyalldawson commented Mar 2, 2016

Keen to hear ppls thoughts and feedback regarding this.

Basically, this implements a system for storing collections of properties and handling inheritance and priority of properties contained with collections.

The motivation here is:

  • to avoid the current multiple duplicate code paths handling storage, retrieval and evaluation of data defined properties and to make it easier to add data defined support to more things (eg diagrams) without incurring even more duplicate code
  • to allow creation of other property types, eg time based properties for animations, non linear scaled properties, etc
  • to allow a system of inherited and overridden properties. Eg default label font override by project default font overriden by label font setting

There's a lot still to do, including implementing saving and restoring collections and porting exiting code to use this, but first I'd like feedback on this approach.

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Mar 2, 2016

Contributor

@wonder-sk @m-kuhn @mhugo @vmora pinging you all because you've shown interest in this in the past

Contributor

nyalldawson commented Mar 2, 2016

@wonder-sk @m-kuhn @mhugo @vmora pinging you all because you've shown interest in this in the past

@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Mar 6, 2016

Member

Hi Nyall - great stuff so far!

My random thoughts:

  • it would be good to include also mapping between integer keys to strings, so e.g. python code that does not require superb speed can still do something like "collection["prop_name"] = 123" without having to look up the keys somewhere
  • would it make sense to store the default values somewhere in the property definition? That could make the life easier if a property is used multiple times - one does not need to repeat the default values in the code multiple times
  • combining the two notes above, there could be something like "property collection definition" class, which would define allowed keys, their string representation, default value and maybe some more things (like description / help?). This would be nice also for error checking, so you can't set value to a key that is not defined
  • would it make sense to have just one class for properties, without inheritance? From the PR description I see you would like to allow further property types, but don't they always boil down to some expressions in the end? But mainly, having a single class would be great so that we could do passing by value without a problem. Something like QVariant, just for properties...
  • it would be nice if property collection and property collection stack had some common interface (maybe stack derived from collection?) so that user does not need to care if they work with a simple collection or a whole stack of them.
Member

wonder-sk commented Mar 6, 2016

Hi Nyall - great stuff so far!

My random thoughts:

  • it would be good to include also mapping between integer keys to strings, so e.g. python code that does not require superb speed can still do something like "collection["prop_name"] = 123" without having to look up the keys somewhere
  • would it make sense to store the default values somewhere in the property definition? That could make the life easier if a property is used multiple times - one does not need to repeat the default values in the code multiple times
  • combining the two notes above, there could be something like "property collection definition" class, which would define allowed keys, their string representation, default value and maybe some more things (like description / help?). This would be nice also for error checking, so you can't set value to a key that is not defined
  • would it make sense to have just one class for properties, without inheritance? From the PR description I see you would like to allow further property types, but don't they always boil down to some expressions in the end? But mainly, having a single class would be great so that we could do passing by value without a problem. Something like QVariant, just for properties...
  • it would be nice if property collection and property collection stack had some common interface (maybe stack derived from collection?) so that user does not need to care if they work with a simple collection or a whole stack of them.
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Mar 11, 2016

Member

Nice, first results from the Qt5/Python3 build in under 10 minutes. Ccache is working 👍

Sorry for abusing the thread, I know I should have a look at the property system...

Member

m-kuhn commented Mar 11, 2016

Nice, first results from the Qt5/Python3 build in under 10 minutes. Ccache is working 👍

Sorry for abusing the thread, I know I should have a look at the property system...

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Mar 11, 2016

Contributor

@m-kuhn Nice! That's a huge saving, well done!

Back on topic, I've just pushed some large changes to this branch. This ports across the existing QgsScaleExpression (to QgsSizeScaleTransformer) and other data defined classes to properties, and adds a framework for other QgsPropertyTransformers including a new QgsColorRampTransformer for translating evaluated properties to an interpolated color.

With any luck the doxygen comments are sufficient to explain how this all fits together, but please ping me if not!

Contributor

nyalldawson commented Mar 11, 2016

@m-kuhn Nice! That's a huge saving, well done!

Back on topic, I've just pushed some large changes to this branch. This ports across the existing QgsScaleExpression (to QgsSizeScaleTransformer) and other data defined classes to properties, and adds a framework for other QgsPropertyTransformers including a new QgsColorRampTransformer for translating evaluated properties to an interpolated color.

With any luck the doxygen comments are sufficient to explain how this all fits together, but please ping me if not!

@nyalldawson nyalldawson referenced this pull request in qgis/QGIS-Enhancement-Proposals Mar 15, 2016

Open

QEP 38: Inheritable property collections #38

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Mar 16, 2016

Contributor

I've done an initial port for diagrams to use the properties system to implement data defined styling, as a demonstration of how this will work in practice.

There's no GUI for now, but you can set data defined overrides using the API.

Eg:

Setting a static override for a the diagram background color property:

vl=iface.activeLayer()
vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.BackgroundColor, '#ff0000')

Setting an expression based outline width:

vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.OutlineWidth, QgsExpressionBasedProperty('rand(0,3)',True))

Field based outline width:

vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.OutlineWidth, QgsFieldBasedProperty('width_field',True))

Here's an example of a transformer used on a property. This will transform the value in the 'passengers' field (with values ranging from 0 to 10000) to an interpolated background color from the 'YlOrRd' ramp:

p=QgsFieldBasedProperty('passengers',True)
p.setTransformer( QgsColorRampTransformer(0, 10000, QgsStyleV2.defaultStyle().colorRamp('YlOrRd') ) )
vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.BackgroundColor,p)
Contributor

nyalldawson commented Mar 16, 2016

I've done an initial port for diagrams to use the properties system to implement data defined styling, as a demonstration of how this will work in practice.

There's no GUI for now, but you can set data defined overrides using the API.

Eg:

Setting a static override for a the diagram background color property:

vl=iface.activeLayer()
vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.BackgroundColor, '#ff0000')

Setting an expression based outline width:

vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.OutlineWidth, QgsExpressionBasedProperty('rand(0,3)',True))

Field based outline width:

vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.OutlineWidth, QgsFieldBasedProperty('width_field',True))

Here's an example of a transformer used on a property. This will transform the value in the 'passengers' field (with values ranging from 0 to 10000) to an interpolated background color from the 'YlOrRd' ramp:

p=QgsFieldBasedProperty('passengers',True)
p.setTransformer( QgsColorRampTransformer(0, 10000, QgsStyleV2.defaultStyle().colorRamp('YlOrRd') ) )
vl.diagramLayerSettings().properties().setProperty(QgsDiagramLayerSettings.BackgroundColor,p)
@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Apr 4, 2016

Contributor

I'm keen to move forward with this and finalise these changes so that I can start working on the GUI. Anyone want to review this latest iteration for me? @wonder-sk @m-kuhn @mhugo @vmora ?

Contributor

nyalldawson commented Apr 4, 2016

I'm keen to move forward with this and finalise these changes so that I can start working on the GUI. Anyone want to review this latest iteration for me? @wonder-sk @m-kuhn @mhugo @vmora ?

@nirvn

This comment has been minimized.

Show comment
Hide comment
@nirvn

nirvn Apr 16, 2016

Contributor

@nyalldawson , I just ran into a need for diagrams to show only within a given atlas feature. Crossing fingers this will get review in time for 2.16.

Contributor

nirvn commented Apr 16, 2016

@nyalldawson , I just ran into a need for diagrams to show only within a given atlas feature. Crossing fingers this will get review in time for 2.16.

python/core/qgsproperty.sip
+%End
+
+%ConvertToSubClassCode
+ if (dynamic_cast<QgsSizeScaleTransformer*>(sipCpp) != NULL)

This comment has been minimized.

@m-kuhn

m-kuhn Apr 18, 2016

Member

I would either go for the propertyType() here or use RTTI everywhere.

@m-kuhn

m-kuhn Apr 18, 2016

Member

I would either go for the propertyType() here or use RTTI everywhere.

This comment has been minimized.

@m-kuhn

m-kuhn Apr 18, 2016

Member

And use nullptr and spacing like in the C++ source even if it's not enforced by astyle ;)

@m-kuhn

m-kuhn Apr 18, 2016

Member

And use nullptr and spacing like in the C++ source even if it's not enforced by astyle ;)

This comment has been minimized.

@jef-n

jef-n Apr 18, 2016

Member

No, don't compare with nullptr use it like a boolean.

@jef-n

jef-n Apr 18, 2016

Member

No, don't compare with nullptr use it like a boolean.

src/core/qgsproperty.h
+
+ /** Returns a clone of the property.
+ */
+ virtual QgsAbstractProperty* clone() = 0;

This comment has been minimized.

@m-kuhn

m-kuhn Apr 18, 2016

Member

This will force us to always use properties with pointers.
Would it be possible to have them implicitly shared and not having to care for the memory management? I think it would make usage much more comfortable.

@m-kuhn

m-kuhn Apr 18, 2016

Member

This will force us to always use properties with pointers.
Would it be possible to have them implicitly shared and not having to care for the memory management? I think it would make usage much more comfortable.

This comment has been minimized.

@m-kuhn

m-kuhn Apr 18, 2016

Member

On a second thought I'm not sure what's the best way to do it. I.e. how to expose methods of the subclasses in such a scenario.

Maybe adding methods to get a pointer to the (subclass type) "private" data member. Or adding all methods of the subclasses to the parent class and return immediately if they shouldn't be called on the current type.

@m-kuhn

m-kuhn Apr 18, 2016

Member

On a second thought I'm not sure what's the best way to do it. I.e. how to expose methods of the subclasses in such a scenario.

Maybe adding methods to get a pointer to the (subclass type) "private" data member. Or adding all methods of the subclasses to the parent class and return immediately if they shouldn't be called on the current type.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 14, 2016

Member

Hey, just crossed my mind that you still have this PR open (and will possibly work on it again in the near future).

Just wondering, did you consider using Qt's property system?
It comes with a whole logic of properties and possibilities to react to changes in them including transition animations.

http://doc.qt.io/qt-5/properties.html

I think they need not to be known at compile time (see the Dynamic Properties section).

Not sure if we are reinventing the wheel...

Member

m-kuhn commented Sep 14, 2016

Hey, just crossed my mind that you still have this PR open (and will possibly work on it again in the near future).

Just wondering, did you consider using Qt's property system?
It comes with a whole logic of properties and possibilities to react to changes in them including transition animations.

http://doc.qt.io/qt-5/properties.html

I think they need not to be known at compile time (see the Dynamic Properties section).

Not sure if we are reinventing the wheel...

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 14, 2016

Contributor

I don't think there's really any overlap between this and qt properties (except for the name!). You could potentially embed individual properties using Qt's system, but it had no handling of inheritance, collections, etc. The direct getters/setters won't be going away, so it will still be possible to tie QGIS properties into Qt's framework using Q_PROPERTY.

Contributor

nyalldawson commented Sep 14, 2016

I don't think there's really any overlap between this and qt properties (except for the name!). You could potentially embed individual properties using Qt's system, but it had no handling of inheritance, collections, etc. The direct getters/setters won't be going away, so it will still be possible to tie QGIS properties into Qt's framework using Q_PROPERTY.

@nyalldawson nyalldawson changed the title from Properties system prototype to Properties framework Jan 19, 2017

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jan 20, 2017

Contributor

@wonder-sk

it would be good to include also mapping between integer keys to strings, so e.g. python code that does not require superb speed can still do something like "collection["prop_name"] = 123" without having to look up the keys somewhere
would it make sense to store the default values somewhere in the property definition? That could make the life easier if a property is used multiple times - one does not need to repeat the default values in the code multiple times
combining the two notes above, there could be something like "property collection definition" class, which would define allowed keys, their string representation, default value and maybe some more things (like description / help?). This would be nice also for error checking, so you can't set value to a key that is not defined

Ok, I've added QgsPropertyDefinition/QgsPropertyDefinitions for handling this kind of thing. Previously property behavior was only defined in gui, but now it's defined up front for each part of the code which uses properties, and gui uses these definitions to control the appearance and behavior of the property override button (the replacement for QgsDataDefinedButton). I'll use this in part 2 to provide assistants for the standard property templates (eg a property defined as a ColorWithAlpha property will show a color-from-ramp assistant, a property defined as a Transparency property will show a transparency from value assistant).

I haven't added default values yet, but they can be added to QgsPropertyDefinition when required. Similarly I haven't added any python sugar to retrieve properties by string, but this should now be possible by using the corresponding QgsPropertyDefinitions. (Although I tend to disagree re using strings here - part of the design here was to avoid arbitrary string use which requires reading the source to determine the correct string value to use. At least with enum values you can easily see all the acceptable values just by reading the api docs).

would it make sense to have just one class for properties, without inheritance? From the PR description I see you would like to allow further property types, but don't they always boil down to some expressions in the end? But mainly, having a single class would be great so that we could do passing by value without a problem. Something like QVariant, just for properties...

Ok, done. There's no more QgsAbstractProperty or subclasses, and everything is done in an implicitly shared QgsProperty class. No more pointer use for storing/retrieving properties either. I'm happy with this - it was never intended that custom property types (eg via PyQGIS) were supported anyway, and we can add future types directly into QgsProperty (eg time based property is high on my list). (BTW - if you're wondering about the struct use in qgsproperty_p.h, it's because originally I was trying to use a union there to reduce the memory footprint of the class... I thought this may be possible in c++11 but I couldn't get it to work.)

it would be nice if property collection and property collection stack had some common interface (maybe stack derived from collection?) so that user does not need to care if they work with a simple collection or a whole stack of them.

Done

So apart from the test failures I need to fix (and API break docs to write up), I'd like to merge this now. Part 2 will expand on the UI for defining properties but I'd like this API locked in before I proceed with that. Any chance of a final review?

Contributor

nyalldawson commented Jan 20, 2017

@wonder-sk

it would be good to include also mapping between integer keys to strings, so e.g. python code that does not require superb speed can still do something like "collection["prop_name"] = 123" without having to look up the keys somewhere
would it make sense to store the default values somewhere in the property definition? That could make the life easier if a property is used multiple times - one does not need to repeat the default values in the code multiple times
combining the two notes above, there could be something like "property collection definition" class, which would define allowed keys, their string representation, default value and maybe some more things (like description / help?). This would be nice also for error checking, so you can't set value to a key that is not defined

Ok, I've added QgsPropertyDefinition/QgsPropertyDefinitions for handling this kind of thing. Previously property behavior was only defined in gui, but now it's defined up front for each part of the code which uses properties, and gui uses these definitions to control the appearance and behavior of the property override button (the replacement for QgsDataDefinedButton). I'll use this in part 2 to provide assistants for the standard property templates (eg a property defined as a ColorWithAlpha property will show a color-from-ramp assistant, a property defined as a Transparency property will show a transparency from value assistant).

I haven't added default values yet, but they can be added to QgsPropertyDefinition when required. Similarly I haven't added any python sugar to retrieve properties by string, but this should now be possible by using the corresponding QgsPropertyDefinitions. (Although I tend to disagree re using strings here - part of the design here was to avoid arbitrary string use which requires reading the source to determine the correct string value to use. At least with enum values you can easily see all the acceptable values just by reading the api docs).

would it make sense to have just one class for properties, without inheritance? From the PR description I see you would like to allow further property types, but don't they always boil down to some expressions in the end? But mainly, having a single class would be great so that we could do passing by value without a problem. Something like QVariant, just for properties...

Ok, done. There's no more QgsAbstractProperty or subclasses, and everything is done in an implicitly shared QgsProperty class. No more pointer use for storing/retrieving properties either. I'm happy with this - it was never intended that custom property types (eg via PyQGIS) were supported anyway, and we can add future types directly into QgsProperty (eg time based property is high on my list). (BTW - if you're wondering about the struct use in qgsproperty_p.h, it's because originally I was trying to use a union there to reduce the memory footprint of the class... I thought this may be possible in c++11 but I couldn't get it to work.)

it would be nice if property collection and property collection stack had some common interface (maybe stack derived from collection?) so that user does not need to care if they work with a simple collection or a whole stack of them.

Done

So apart from the test failures I need to fix (and API break docs to write up), I'd like to merge this now. Part 2 will expand on the UI for defining properties but I'd like this API locked in before I proceed with that. Any chance of a final review?

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jan 23, 2017

Contributor

Phew... one unrelated test failure (OTB processing), but this is ready to merge in my opinion

Contributor

nyalldawson commented Jan 23, 2017

Phew... one unrelated test failure (OTB processing), but this is ready to merge in my opinion

nyalldawson added some commits Jan 17, 2017

Stronger definitions of properties in collections
Instead of defining the suitable field types and values for
properties when registering them to a data defined button,
now properties are fully defined when the valid
property keys are defined.
Refactor properties
Remove QgsAbstractProperty and subclasses, and instead use a single
QgsProperty class which covers the use of QgsAbstractProperty
and its subclasses. This simplifies the API and avoids pointer
handling. QgsProperty is implicitly shared for memory efficiency
and inexpensive copies.
Crazy fix to avoid shared data getting corrupted in sip bindings
Without this (should be unnecessary) virtual destructor the
shared data member for QgsProperty gets filled with garbage if
a QgsProperty is created from python code.

I can't explain it...!
@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Jan 23, 2017

Member

Insane amount of work - well done Nyall!

I have checked mainly the new classes and things look good to me - feel free to merge!

Member

wonder-sk commented Jan 23, 2017

Insane amount of work - well done Nyall!

I have checked mainly the new classes and things look good to me - feel free to merge!

@nyalldawson nyalldawson merged commit ea116fd into qgis:master Jan 23, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@nyalldawson nyalldawson deleted the nyalldawson:properties2 branch Jan 23, 2017

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jan 25, 2017

Member

Woohoo!!!! 👍

Member

m-kuhn commented Jan 25, 2017

Woohoo!!!! 👍

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Mar 6, 2017

Member

@nyalldawson Do you mind if this is renamed to just Path or do you prefer to add a separate new enum for that?

Member

m-kuhn commented on src/core/qgsproperty.cpp in 90e80c1 Mar 6, 2017

@nyalldawson Do you mind if this is renamed to just Path or do you prefer to add a separate new enum for that?

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Mar 6, 2017

Member

Forget about that, I've misunderstood it.

Member

m-kuhn replied Mar 6, 2017

Forget about that, I've misunderstood it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment