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

[Feature] Auxiliary Storage #5086

Merged
merged 74 commits into from Oct 9, 2017

Conversation

Projects
None yet
6 participants
@pblottiere
Member

pblottiere commented Aug 29, 2017

Description

This PR adds the Auxiliary Storage feature discussed in the QEP qgis/QGIS-Enhancement-Proposals#27.

A new tab is available in vector layer properties to manage auxiliary storage :

as323

Moreover a new action Store data in the project is available in the data defined menu providing an easy way to manage auxiliary data for a property :

as00

Auxiliary data is stored in a sqlite database and managed thanks to the OGR data provider (instead of the spatialite provider) to keep as small as possible the database file. This database file (with extension .qgd) is either saved just next to the project file or directly embedded within the new .qgz format.

Units tests will come soon.

I look forward to your reactions and suggestions! Tank you.

Warning: there's some known issues which may disturb while testing this feature:

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
Show outdated Hide outdated src/core/qgsauxiliarystorage.h
Show outdated Hide outdated src/core/qgsauxiliarystorage.h
Show outdated Hide outdated src/core/qgsauxiliarystorage.h
Show outdated Hide outdated src/core/qgsauxiliarystorage.cpp
Show outdated Hide outdated src/core/qgsauxiliarystorage.cpp
Show outdated Hide outdated src/app/qgslabelinggui.cpp
Show outdated Hide outdated src/app/qgsvectorlayerproperties.cpp
Show outdated Hide outdated src/app/qgsmaptoollabel.cpp
Show outdated Hide outdated src/app/qgsvectorlayerproperties.h
Show outdated Hide outdated src/core/qgsauxiliarystorage.cpp
@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 29, 2017

Contributor

Lovely stuff! This is all very much appreciated 🎉

Looking over the original QEP, is this part still planned too? "The creation of such auxiliary fields will also be transparent for the user: the manual labeling tools will be available and if no fields already exist for data-defined properties, they will be automatically created through auxiliary fields."

Contributor

nyalldawson commented Aug 29, 2017

Lovely stuff! This is all very much appreciated 🎉

Looking over the original QEP, is this part still planned too? "The creation of such auxiliary fields will also be transparent for the user: the manual labeling tools will be available and if no fields already exist for data-defined properties, they will be automatically created through auxiliary fields."

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Aug 30, 2017

Member

Why's this restricted to spatial layers?

@haubourg, can you share your thoughts on this point?

Member

pblottiere commented Aug 30, 2017

Why's this restricted to spatial layers?

@haubourg, can you share your thoughts on this point?

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 30, 2017

Contributor

Looking over the original QEP, is this part still planned too? "The creation of such auxiliary fields will also be transparent for the user: the manual labeling tools will be available and if no fields already exist for data-defined properties, they will be automatically created through auxiliary fields."

@nyalldawson in fact, after testing I asked Paul to remove that. Opening a project was automatically adding tables to each layer (spatial or not) and that looked intrusive, and potentially slow.

I also found that it's always hard to teach users to first play with automatic placement settings when it's too easy to manually move labels. So I was wondering if making it too easy wasn't ruining efforts of trainers or GIS administrators somewhere

My idea to make it straight simple is now to create an assistant somewhere to automatically set all labeling widgets, add styles with geometry generators for label callouts, with multiple style choices for rendering / fonts and so on. In one word, we now have the core features to easily port EasyCustomLabeling plugin :). We had that in separated QEP about connectors, custom paths for plugins, but the guy funding that quit his job for any reason... (me indeed).

Contributor

haubourg commented Aug 30, 2017

Looking over the original QEP, is this part still planned too? "The creation of such auxiliary fields will also be transparent for the user: the manual labeling tools will be available and if no fields already exist for data-defined properties, they will be automatically created through auxiliary fields."

@nyalldawson in fact, after testing I asked Paul to remove that. Opening a project was automatically adding tables to each layer (spatial or not) and that looked intrusive, and potentially slow.

I also found that it's always hard to teach users to first play with automatic placement settings when it's too easy to manually move labels. So I was wondering if making it too easy wasn't ruining efforts of trainers or GIS administrators somewhere

My idea to make it straight simple is now to create an assistant somewhere to automatically set all labeling widgets, add styles with geometry generators for label callouts, with multiple style choices for rendering / fonts and so on. In one word, we now have the core features to easily port EasyCustomLabeling plugin :). We had that in separated QEP about connectors, custom paths for plugins, but the guy funding that quit his job for any reason... (me indeed).

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 30, 2017

Contributor

in fact, after testing I asked Paul to remove that. Opening a project was automatically adding tables to each layer (spatial or not) and that looked intrusive, and potentially slow.

Couldn't we just add these when they are first required? So e.g. always have the label tools available, but only create the auxiliary field when a label in that layer is first moved.

I really liked this idea - it was bringing us to feature parity with one of the few remaining areas MapInfo is ahead of us. I think even an having an assistant to automatically set this up is too hard for many users to understand.

Contributor

nyalldawson commented Aug 30, 2017

in fact, after testing I asked Paul to remove that. Opening a project was automatically adding tables to each layer (spatial or not) and that looked intrusive, and potentially slow.

Couldn't we just add these when they are first required? So e.g. always have the label tools available, but only create the auxiliary field when a label in that layer is first moved.

I really liked this idea - it was bringing us to feature parity with one of the few remaining areas MapInfo is ahead of us. I think even an having an assistant to automatically set this up is too hard for many users to understand.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 30, 2017

Contributor

@nyalldawson agreed for Mapinfo fully. From a user point of view I'm still unsure it is a good idea to have pinned labels so easily and then turn the map into a fixed scale map. And just moving / rotating labels in current map tools is not enough to reach mapinfo, we need callouts and alignment settings also, which is a lot of data defined settings to do. That's why I was thinking of an assistant to help users do that easily.
From a developper point of view, current maptools are not "active layer" aware, they might need some refactor to trigger the auxiliary field creation on the fly. I just had a look at the rotate Label (which is broken due to rotation changes ) and I'm afraid users will have random "auxiliary data creation" dialogs popping when they pick labels from other layers.
We might probably think a maptool refactor before doing that?
In fact, we face something similar with the new node tool, not active layer focused any more. We have now very heterogeneous ergonomics with active layer and maptools.

Contributor

haubourg commented Aug 30, 2017

@nyalldawson agreed for Mapinfo fully. From a user point of view I'm still unsure it is a good idea to have pinned labels so easily and then turn the map into a fixed scale map. And just moving / rotating labels in current map tools is not enough to reach mapinfo, we need callouts and alignment settings also, which is a lot of data defined settings to do. That's why I was thinking of an assistant to help users do that easily.
From a developper point of view, current maptools are not "active layer" aware, they might need some refactor to trigger the auxiliary field creation on the fly. I just had a look at the rotate Label (which is broken due to rotation changes ) and I'm afraid users will have random "auxiliary data creation" dialogs popping when they pick labels from other layers.
We might probably think a maptool refactor before doing that?
In fact, we face something similar with the new node tool, not active layer focused any more. We have now very heterogeneous ergonomics with active layer and maptools.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 30, 2017

Contributor

@nyalldawson so I'm not saying it's not possible but I'd like to be sure this is really what we want.

Contributor

haubourg commented Aug 30, 2017

@nyalldawson so I'm not saying it's not possible but I'd like to be sure this is really what we want.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Aug 30, 2017

Member

what happens/should happen when a vector layer with auxiliary storage is duplicated/cloned?

For now, the auxiliary layer is not kept in the clone but I don't have a strong opinion here.

But if we want to clone the auxiliary layer too, then the underlying sqlite table should be copied to avoid some conflicts.

Member

pblottiere commented Aug 30, 2017

what happens/should happen when a vector layer with auxiliary storage is duplicated/cloned?

For now, the auxiliary layer is not kept in the clone but I don't have a strong opinion here.

But if we want to clone the auxiliary layer too, then the underlying sqlite table should be copied to avoid some conflicts.

@qgis qgis deleted a comment from nyalldawson Aug 30, 2017

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Aug 30, 2017

Member

@nyalldawson Firstly, thank you for your extensive review! I think I addressed all comments and questions. Let me know what you think about!

And by the way, what do you ( @nyalldawson and @haubourg ) decide about the map tool stuff?

Member

pblottiere commented Aug 30, 2017

@nyalldawson Firstly, thank you for your extensive review! I think I addressed all comments and questions. Let me know what you think about!

And by the way, what do you ( @nyalldawson and @haubourg ) decide about the map tool stuff?

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 31, 2017

Contributor

I've just given a test run. Here's some thoughts:

  • please don't forget about the "Change Label" toolbar action. It's currently disabled when the layer isn't editable, unlike the move/rotate label actions. But it should be treated the same as these.
  • i need to manually hit "apply" in the style dock to get the properties to apply - this should be automatic
  • do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)
  • should users be allowed to add their own auxiliary fields? i can see that there's potentially use cases for this...
Contributor

nyalldawson commented Aug 31, 2017

I've just given a test run. Here's some thoughts:

  • please don't forget about the "Change Label" toolbar action. It's currently disabled when the layer isn't editable, unlike the move/rotate label actions. But it should be treated the same as these.
  • i need to manually hit "apply" in the style dock to get the properties to apply - this should be automatic
  • do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)
  • should users be allowed to add their own auxiliary fields? i can see that there's potentially use cases for this...
@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 31, 2017

Contributor

what happens/should happen when a vector layer with auxiliary storage is duplicated/cloned?
For now, the auxiliary layer is not kept in the clone but I don't have a strong opinion here.

But if we want to clone the auxiliary layer too, then the underlying sqlite table should be copied to avoid some conflicts.

Agreed - a clone should duplicate the existing sqlite table, then update the clone to use the new table but retain all existing links for data defined properties (pointing to new table).

Contributor

nyalldawson commented Aug 31, 2017

what happens/should happen when a vector layer with auxiliary storage is duplicated/cloned?
For now, the auxiliary layer is not kept in the clone but I don't have a strong opinion here.

But if we want to clone the auxiliary layer too, then the underlying sqlite table should be copied to avoid some conflicts.

Agreed - a clone should duplicate the existing sqlite table, then update the clone to use the new table but retain all existing links for data defined properties (pointing to new table).

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 31, 2017

Contributor

@haubourg

agreed for Mapinfo fully. From a user point of view I'm still unsure it is a good idea to have pinned labels so easily and then turn the map into a fixed scale map.

I disagree. If a user wants to turn a map into a fixed scale map, let's make it easy for them. I used to manage a MapInfo rollout with ~400 non-GIS users (non techy people, too), yet they were totally comfortable with moving labels around in MapInfo and would frequently do this to improve their maps. This is the benchmark we need to hit here.

In QGIS it's easy to unfreeze labels anyway, and the ability to click and drag to unfreeze multiple at once would make this quick to do if needed. (incidentally, seems there's a bug in that freeze tool - it's not automatically refreshed on the map).

And just moving / rotating labels in current map tools is not enough to reach mapinfo, we need callouts and alignment settings also, which is a lot of data defined settings to do. That's why I was thinking of an assistant to help users do that easily.

True, but that's a different topic/function. Let's make label moving/rotating/hiding/custom styling as easy as possible first, given that it's being tackled in this PR :)

From a developper point of view, current maptools are not "active layer" aware, they might need some refactor to trigger the auxiliary field creation on the fly. I just had a look at the rotate Label (which is broken due to rotation changes ) and I'm afraid users will have random "auxiliary data creation" dialogs popping when they pick labels from other layers.

Again, I don't think that's a big problem. The "auxiliary data creation" dialog could be improved a bit so that it's not so technical and scary for users, but in the end if they WANT to move a label which is in a layer which doesn't currently have aux data attached, the dialog is really just making this easy for them to do that. I see it as QGIS saying "hey, you're wanting to move this label. That's great! You've just got to answer this one question for me, and then you I'll let you move and tweak those labels however you want." (what a friendly little fellow QGIS is!)

Contributor

nyalldawson commented Aug 31, 2017

@haubourg

agreed for Mapinfo fully. From a user point of view I'm still unsure it is a good idea to have pinned labels so easily and then turn the map into a fixed scale map.

I disagree. If a user wants to turn a map into a fixed scale map, let's make it easy for them. I used to manage a MapInfo rollout with ~400 non-GIS users (non techy people, too), yet they were totally comfortable with moving labels around in MapInfo and would frequently do this to improve their maps. This is the benchmark we need to hit here.

In QGIS it's easy to unfreeze labels anyway, and the ability to click and drag to unfreeze multiple at once would make this quick to do if needed. (incidentally, seems there's a bug in that freeze tool - it's not automatically refreshed on the map).

And just moving / rotating labels in current map tools is not enough to reach mapinfo, we need callouts and alignment settings also, which is a lot of data defined settings to do. That's why I was thinking of an assistant to help users do that easily.

True, but that's a different topic/function. Let's make label moving/rotating/hiding/custom styling as easy as possible first, given that it's being tackled in this PR :)

From a developper point of view, current maptools are not "active layer" aware, they might need some refactor to trigger the auxiliary field creation on the fly. I just had a look at the rotate Label (which is broken due to rotation changes ) and I'm afraid users will have random "auxiliary data creation" dialogs popping when they pick labels from other layers.

Again, I don't think that's a big problem. The "auxiliary data creation" dialog could be improved a bit so that it's not so technical and scary for users, but in the end if they WANT to move a label which is in a layer which doesn't currently have aux data attached, the dialog is really just making this easy for them to do that. I see it as QGIS saying "hey, you're wanting to move this label. That's great! You've just got to answer this one question for me, and then you I'll let you move and tweak those labels however you want." (what a friendly little fellow QGIS is!)

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 31, 2017

Contributor

@nyalldawson pretty convincing 👍
@pblottiere So let's activate the label maptools by default and create what's needed on the fly when required. Tell us if the state of the maptools requires more work than expected.

Contributor

haubourg commented Aug 31, 2017

@nyalldawson pretty convincing 👍
@pblottiere So let's activate the label maptools by default and create what's needed on the fly when required. Tell us if the state of the maptools requires more work than expected.

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 31, 2017

Contributor

@nyalldawson pretty convincing 👍

cheerydependablearcticseal-small

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?

Contributor

nyalldawson commented Aug 31, 2017

@nyalldawson pretty convincing 👍

cheerydependablearcticseal-small

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 31, 2017

Contributor

do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)

I thought some more on this - I think what should happen is:

  • aux field created which maps to any of the predefined label tools (e.g. x/y/rotation/those settings available through the label properties tool) should have their editor widget set to hidden by default
  • fields mapped to other properties should not be set to hidden (otherwise there's no way to edit the values for these). E.g. if i bind the "background shape radius" to a aux field, that field should be shown in the attribute table/identify results.
  • as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default

Thoughts?

Contributor

nyalldawson commented Aug 31, 2017

do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)

I thought some more on this - I think what should happen is:

  • aux field created which maps to any of the predefined label tools (e.g. x/y/rotation/those settings available through the label properties tool) should have their editor widget set to hidden by default
  • fields mapped to other properties should not be set to hidden (otherwise there's no way to edit the values for these). E.g. if i bind the "background shape radius" to a aux field, that field should be shown in the attribute table/identify results.
  • as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default

Thoughts?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 1, 2017

Member

do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)

visibility of fields in the attribute table is no longer controlled by hidden, it's controlled by "organize fields".

Member

m-kuhn commented Sep 1, 2017

do we want to hide auxilliary fields from the attribute table and identify results? (i'd say yes - they could default to the "hidden" editor widget, but be aware that there's a bug where hidden fields are being shown in the attribute table - i can't find the issue report for this, but it's there somewhere)

visibility of fields in the attribute table is no longer controlled by hidden, it's controlled by "organize fields".

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Sep 1, 2017

Contributor

I thought some more on this - I think what should happen is:

  • aux field created which maps to any of the predefined label tools (e.g. x/y/rotation/those settings available through the label properties tool) should have their editor widget set to hidden by default

+1

  • fields mapped to other properties should not be set to hidden (otherwise there's no way to edit the values for these). E.g. if i bind the "background shape radius" to a aux field, that field should be shown in the attribute table/identify results.

+1. Good catch, I only focused on labeling tests.

  • as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default
    Thoughts?

of course, this would be better if easy to do.

Contributor

haubourg commented Sep 1, 2017

I thought some more on this - I think what should happen is:

  • aux field created which maps to any of the predefined label tools (e.g. x/y/rotation/those settings available through the label properties tool) should have their editor widget set to hidden by default

+1

  • fields mapped to other properties should not be set to hidden (otherwise there's no way to edit the values for these). E.g. if i bind the "background shape radius" to a aux field, that field should be shown in the attribute table/identify results.

+1. Good catch, I only focused on labeling tests.

  • as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default
    Thoughts?

of course, this would be better if easy to do.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Sep 1, 2017

Contributor

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?

I'm not totally convinced here. Editing toolbars strongly suggest you are editing main data source.I would let the current "Store data in the project" checkbox do the job in that case.

Contributor

haubourg commented Sep 1, 2017

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?

I'm not totally convinced here. Editing toolbars strongly suggest you are editing main data source.I would let the current "Store data in the project" checkbox do the job in that case.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 1, 2017

Member

So let's activate the label maptools by default and create what's needed on the fly when required.

Done but it still remains some work to do on Change Label map tool. I'm taking a look.

Member

pblottiere commented Sep 1, 2017

So let's activate the label maptools by default and create what's needed on the fly when required.

Done but it still remains some work to do on Change Label map tool. I'm taking a look.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 1, 2017

Member

please don't forget about the "Change Label" toolbar action. It's currently disabled when the layer isn't editable, unlike the move/rotate label actions. But it should be treated the same as these.

Done. I just noticed that max/min scale data defined properties are just took into account after reloading the project... I don't know why (yet).

Member

pblottiere commented Sep 1, 2017

please don't forget about the "Change Label" toolbar action. It's currently disabled when the layer isn't editable, unlike the move/rotate label actions. But it should be treated the same as these.

Done. I just noticed that max/min scale data defined properties are just took into account after reloading the project... I don't know why (yet).

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 1, 2017

Contributor

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?
I'm not totally convinced here. Editing toolbars strongly suggest you are editing main data source.I would let the current "Store data in the project" checkbox do the job in that case.

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

Contributor

nyalldawson commented Sep 1, 2017

One thing that occurred to me last night - we also have some map tools for working with data defined symbol properties (rotate point symbol tool, offset point symbol tool). Should these also be addressed?
I'm not totally convinced here. Editing toolbars strongly suggest you are editing main data source.I would let the current "Store data in the project" checkbox do the job in that case.

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 1, 2017

Contributor

visibility of fields in the attribute table is no longer controlled by hidden, it's controlled by "organize fields".

Is this predictable behavior though? It seems odd to me that the fields won't show in form view, but will in the table view.

Contributor

nyalldawson commented Sep 1, 2017

visibility of fields in the attribute table is no longer controlled by hidden, it's controlled by "organize fields".

Is this predictable behavior though? It seems odd to me that the fields won't show in form view, but will in the table view.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Sep 1, 2017

Contributor

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

True, We have the "store data " checkbox not doing anything currently outside of Labeling fields.
@pblottiere do yo have to handle by hand every auxiliary field name or is that something automated?

Contributor

haubourg commented Sep 1, 2017

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

True, We have the "store data " checkbox not doing anything currently outside of Labeling fields.
@pblottiere do yo have to handle by hand every auxiliary field name or is that something automated?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 1, 2017

Member

Is this predictable behavior though? It seems odd to me that the fields won't show in form view, but will in the table view.

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

Member

m-kuhn commented Sep 1, 2017

Is this predictable behavior though? It seems odd to me that the fields won't show in form view, but will in the table view.

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 1, 2017

Member

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

It's done (but I needed a non const vector layer in QgsSymbolLayerWidget to be able to create the auxiliary field)

Member

pblottiere commented Sep 1, 2017

That's fine by me - but I don't think it's handled in this PR yet (creating aux fields for symbology data defined properties)

It's done (but I needed a non const vector layer in QgsSymbolLayerWidget to be able to create the auxiliary field)

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 2, 2017

Contributor

@nirvn can I request you to checkout this branch and give a test run? You've always got good UX insights, so I'm keen to hear your thoughts on how this behaves.

Contributor

nyalldawson commented Sep 2, 2017

@nirvn can I request you to checkout this branch and give a test run? You've always got good UX insights, so I'm keen to hear your thoughts on how this behaves.

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 2, 2017

Contributor

@m-kuhn

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

+1 to this

Contributor

nyalldawson commented Sep 2, 2017

@m-kuhn

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

+1 to this

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Sep 2, 2017

Contributor

Latest test results:

  1. If I click the "label properties" tool, and click on a label for the first time for a layer, I get the "set primary key" dialog. After clicking here I'd expect the label properties dialog to show, but instead I get nothing and need to reclick the label.

  2. Being able to change the font style of a single label without any config or table changes is killer. Thank you SO much for this!

  3. Note to self: add a "show buffer" checkbox to the label properties dialog so that buffers can be enabled/disabled for individual labels too.

  4. There's no effect if I choose "store data in the project" for point symbol rotation or size (at the symbol level, not symbol layer level)

  5. I see "Store data in the project" options for property override buttons where they should not be. E.g. composer. Selecting the option here crashes.

  6. The approach used to hide the aux fields is incorrect. They should just auto set the widget type to "hidden" initially, but leave the option for users to later change this if desired. The current approach prevents them from ever being shown.

Contributor

nyalldawson commented Sep 2, 2017

Latest test results:

  1. If I click the "label properties" tool, and click on a label for the first time for a layer, I get the "set primary key" dialog. After clicking here I'd expect the label properties dialog to show, but instead I get nothing and need to reclick the label.

  2. Being able to change the font style of a single label without any config or table changes is killer. Thank you SO much for this!

  3. Note to self: add a "show buffer" checkbox to the label properties dialog so that buffers can be enabled/disabled for individual labels too.

  4. There's no effect if I choose "store data in the project" for point symbol rotation or size (at the symbol level, not symbol layer level)

  5. I see "Store data in the project" options for property override buttons where they should not be. E.g. composer. Selecting the option here crashes.

  6. The approach used to hide the aux fields is incorrect. They should just auto set the widget type to "hidden" initially, but leave the option for users to later change this if desired. The current approach prevents them from ever being shown.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 2, 2017

Member

Sorry in advance if this question has been answered before, it's just a random thought.

Is it possible to "promote" an auxiliary field to a provider field? I imagine the situation where I start working on an isolated "branch" of a project to bring it back to the production "master" data source later on.

Member

m-kuhn commented Sep 2, 2017

Sorry in advance if this question has been answered before, it's just a random thought.

Is it possible to "promote" an auxiliary field to a provider field? I imagine the situation where I start working on an isolated "branch" of a project to bring it back to the production "master" data source later on.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 2, 2017

Member

The approach used to hide the aux fields is incorrect. They should just auto set the widget type to "hidden" initially, but leave the option for users to later change this if desired. The current approach prevents them from ever being shown.

I probably didn't understand what you mean but if you click on the Organize columns button, you have the next window where auxiliary widgets may be displayed if necessary:

organize

Member

pblottiere commented Sep 2, 2017

The approach used to hide the aux fields is incorrect. They should just auto set the widget type to "hidden" initially, but leave the option for users to later change this if desired. The current approach prevents them from ever being shown.

I probably didn't understand what you mean but if you click on the Organize columns button, you have the next window where auxiliary widgets may be displayed if necessary:

organize

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 2, 2017

Member

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

+1 to this

Should this only happen when it's done manually (vector layer properties) or also when setting it through the API?
For both I can imagine situations where it will be unexpected.

Member

m-kuhn commented Sep 2, 2017

It could be adjusted that when a widget type is changed to hidden it will also be deactivated for the attribute table. And when it's changed from hidden to something else, that it's set to visible. This would leave maximum configurability available while keeping it easy to configure for the 90% default cases.

+1 to this

Should this only happen when it's done manually (vector layer properties) or also when setting it through the API?
For both I can imagine situations where it will be unexpected.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 2, 2017

Member

as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default

It's now automatically done when a new auxiliary field is created.

Member

pblottiere commented Sep 2, 2017

as a follow up to this, any fields bound to a QgsPropertyDefinition::ColorWithAlpha or ColorNoAlpha property should have their editor widget set to the color widget by default

It's now automatically done when a new auxiliary field is created.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 2, 2017

Member

If I click the "label properties" tool, and click on a label for the first time for a layer, I get the "set primary key" dialog. After clicking here I'd expect the label properties dialog to show, but instead I get nothing and need to reclick the label.

It was desired but you're right, it's much better this way :). Done.

Member

pblottiere commented Sep 2, 2017

If I click the "label properties" tool, and click on a label for the first time for a layer, I get the "set primary key" dialog. After clicking here I'd expect the label properties dialog to show, but instead I get nothing and need to reclick the label.

It was desired but you're right, it's much better this way :). Done.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 2, 2017

Member

There's no effect if I choose "store data in the project" for point symbol rotation or size (at the symbol level, not symbol layer level)

It's fixed.

Member

pblottiere commented Sep 2, 2017

There's no effect if I choose "store data in the project" for point symbol rotation or size (at the symbol level, not symbol layer level)

It's fixed.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Sep 2, 2017

Contributor

Sorry in advance if this question has been answered before, it's just a random thought.

Is it possible to "promote" an auxiliary field to a provider field? I imagine the situation where I start working on an isolated "branch" of a project to bring it back to the production "master" data source later on.

Hi Matthias, this was not in the list, no. Users might use the field calculator, waiting for something more integrated.
An additional button could be shown in the 'auxiliary storage' tab of layer properties.

Contributor

haubourg commented Sep 2, 2017

Sorry in advance if this question has been answered before, it's just a random thought.

Is it possible to "promote" an auxiliary field to a provider field? I imagine the situation where I start working on an isolated "branch" of a project to bring it back to the production "master" data source later on.

Hi Matthias, this was not in the list, no. Users might use the field calculator, waiting for something more integrated.
An additional button could be shown in the 'auxiliary storage' tab of layer properties.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Sep 2, 2017

Member

I see "Store data in the project" options for property override buttons where they should not be. E.g. composer.

"Store data in the project" action is now only available in :

  • QgsDiagramProperties
  • QgsLabelingGui
  • QgsSymbolLayerWidget
  • QgsSymbolsListWidget
Member

pblottiere commented Sep 2, 2017

I see "Store data in the project" options for property override buttons where they should not be. E.g. composer.

"Store data in the project" action is now only available in :

  • QgsDiagramProperties
  • QgsLabelingGui
  • QgsSymbolLayerWidget
  • QgsSymbolsListWidget

pblottiere added some commits Sep 1, 2017

@pblottiere pblottiere merged commit ac66ced into qgis:master Oct 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Oct 9, 2017

Member

Yay!!
🎉

Member

m-kuhn commented Oct 9, 2017

Yay!!
🎉

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Oct 9, 2017

Contributor

giphy

Contributor

haubourg commented Oct 9, 2017

giphy

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Oct 9, 2017

Contributor

Bravo Paul.. huge work (and this is not just to move labels.... )

Contributor

haubourg commented Oct 9, 2017

Bravo Paul.. huge work (and this is not just to move labels.... )

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