Geom compatibility check release 2.18 fixes #15741 #16927 #5223

luipir · 2017-09-20T10:57:59Z

Description

Added geometry compatibility check in addFeature and changeGeometry of QgsVectorLayerEditBuffer. In this way the user is prompted of incompatibility errors during editing.
Also added in QgsVectorLayerProvider::convertToProviderType flat geometry in case provider does not support 3d or M => they are removed.
This patch would fix #15741 #16927
This fix has effect also on the issue #16948 during paste in case of incompatibilities.

NOTE: because now editBuffer addFeature and changeGeometry can return boolean error => also vectorLayer changeGeometry and addFeature can return error. I updated (I hope) any place where this method were used without taking into account a possible fails. Most of these code parts were not covered by test to know if the patch can introduce more regressions.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

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

m-kuhn

Nice one, make sure that there are no changes in terms of things that would have been committed before and are no longer committed because of early exit on failure, since we are in a stable release series.

m-kuhn · 2017-09-20T12:52:19Z

python/core/qgsvectorlayereditbuffer.sip

+     * In case of convertion a message is notified in the log
+     * @param geom pointer to the geometry that is adapted to provider
+     * @return bool true if success
+     * @note added in QGIS 2.18


Should be QGIS 2.18.patch

m-kuhn · 2017-09-20T12:52:28Z

python/core/qgsvectorlayereditbuffer.sip

@@ -148,4 +148,13 @@ class QgsVectorLayerEditBuffer : QObject
    void updateAttributeMapIndex( QgsAttributeMap& attrs, int index, int offset ) const;

    void updateLayerFields();
+
+    /** Apply geom modification basing on provider geometry type.


s/geom/geometry/

m-kuhn · 2017-09-20T12:52:47Z

python/core/qgsvectorlayereditbuffer.sip

+
+    /** Apply geom modification basing on provider geometry type.
+     * Geom is modified only if successful convertion is possibile.
+     * In case of convertion a message is notified in the log


conversion
sent to the log

m-kuhn · 2017-09-20T12:53:10Z

python/core/qgsvectorlayereditbuffer.sip

+    /** Apply geom modification basing on provider geometry type.
+     * Geom is modified only if successful convertion is possibile.
+     * In case of convertion a message is notified in the log
+     * @param geom pointer to the geometry that is adapted to provider


s/geom/geometry
"is adapted" -> "should be adapted"

m-kuhn · 2017-09-20T12:53:48Z

python/core/qgsvectorlayereditbuffer.sip

+     * Geom is modified only if successful convertion is possibile.
+     * In case of convertion a message is notified in the log
+     * @param geom pointer to the geometry that is adapted to provider
+     * @return bool true if success


How can success be interpreted here? Only if the geometry has been changed or also if it already was ok before?

m-kuhn · 2017-09-20T15:15:35Z

src/core/qgsofflineediting.cpp

-      remoteLayer->addAttribute( field );
+      if ( !remoteLayer->addAttribute( field ) )
+      {
+        this->syncError = true;


No need to use this.

m-kuhn · 2017-09-20T15:15:44Z

src/core/qgsofflineediting.cpp

-    remoteLayer->addFeature( f, false );
+    if ( !remoteLayer->addFeature( f, false ) )
+    {
+      this->syncError = true;


No need for this

m-kuhn · 2017-09-20T15:15:57Z

src/core/qgsofflineediting.cpp

-    remoteLayer->deleteFeature( fid );
+    if ( !remoteLayer->deleteFeature( fid ) )
+    {
+      this->syncError = true;


No need to use this.

m-kuhn · 2017-09-20T15:16:26Z

src/core/qgsofflineediting.cpp

-    remoteLayer->changeAttributeValue( fid, attrLookup[ values.at( i ).attr ], values.at( i ).value );
+    if ( !remoteLayer->changeAttributeValue( fid, attrLookup[ values.at( i ).attr ], values.at( i ).value ) )
+    {
+      this->syncError = true;


No more repetitions ;)

m-kuhn · 2017-09-20T15:22:45Z

src/core/qgsvectorlayereditbuffer.cpp

+      QgsGeometry* newGeom = provider->convertToProviderType( geom );
+      if ( !newGeom )
+      {
+        QgsMessageLog::logMessage( tr( "ERROR: geometry type is not compatible with the layer.", "not compatible geometry" ) );


Use level instead of an "ERROR" prefix

luipir · 2017-09-21T11:37:43Z

@m-kuhn about your comment:
"Nice one, make sure that there are no changes in terms of things that would have been committed before and are no longer committed because of early exit on failure, since we are in a stable release series."
I asked to some power users to do tests (a lot of old parts are not covered by unit tests). BTW I would prefer to see this patch merged before to have more extensive test in case of regressions.
There will obviously regression regarding the workflow! now the error is raised as soon as it happened. since 2.18.13, the error is raise only at commitChanges phase with the risk to loose all editing work because of one feature incompatibility.

m-kuhn · 2017-09-21T11:47:03Z

since 2.18.13, the error is raise only at commitChanges phase with the risk to loose all editing work because of one feature incompatibility.

Ah, sorry, I wasn't aware of that. This changes everything a bit.

In which scenario does this happen actually? I guess it's there for 2D->3D promotion and similar which should be easy to do, is it related to pasting multi-features on single-layers?

gioman · 2017-09-21T11:49:53Z

In which scenario does this happen actually?

in 2.14: you have a table that is not "multi" and you try to manually merge two features so to create a multi-geometry, a warning pop up saying that you can't and the operation is discarded.

in 2.18: you do the same, no error/warning, you continue with (a ton of) other edits and then try to save > error because of the "multi" geometry you created and you can only discard all the changes you made.

gioman · 2017-09-21T11:51:45Z

in 2.18: you do the same, no error/warning, you continue with (a ton of) other edits and then try to save > error because of the "multi" geometry you created and you can only discard all the changes you made.

moreover at some point in 2.18 when discarding changes there was also data loss (now this seems fixed).

m-kuhn · 2017-09-21T11:52:12Z

Why could we do a warning pop up back then and now we need to send it to the message log?

m-kuhn · 2017-09-21T11:53:57Z

python/core/qgsvectorlayereditbuffer.sip

+     * @return bool true if success.
+     *  Success means that input geometry is changed because conversion is applied or
+     *  geometry is untouched if geometry conversion is not necessary.
+     *  Fail if conversion is not possible and geometry is untuched.


Typos:
False if conversion is not possible and geometry is untouched.

Can you also change the "Geom" strings to "Geometry", while at it?

Thanks

gioman · 2017-09-21T11:55:02Z

Why could we do a warning pop up back then and now we need to send it to the message log?

good question.

m-kuhn · 2017-09-21T11:56:17Z

Bit +1 to go ahead with this fast

luipir · 2017-09-21T14:53:36Z

@gioman @m-kuhn
compatibility check weas moved from qgisapp:
https://github.com/qgis/QGIS/blob/release-2_14/src/app/qgisapp.cpp#L6539
from this @mhugent commit:
8d70a51
IMHO move the check to provider is a good solution, but lack of a complete user notification.

luipir · 2017-09-21T14:56:09Z

TestQgsServer now fail afte updated a doxygen comment?

m-kuhn · 2017-09-21T14:58:42Z

IIRC on the provider we have the infrastructure in place to pushError()

luipir · 2017-09-21T15:24:08Z

IIRC on the provider we have the infrastructure in place to pushError()

I'll give a look

gioman · 2017-09-21T16:52:40Z

@luipir I tested your branch and the patch seems to work fine to me! It certainly a much better user experience. I would suggest to merge asap for wider testing.

m-kuhn · 2017-09-21T16:55:30Z

Thanks for testing @gioman
@luipir, your call, if a better UX/error reporting is possible, it would be awesome, if not, let's get this merged.

m-kuhn

Thanks for all the improvements

luipir · 2017-09-21T17:14:40Z

@m-kuhn I'm doing some tests with pushMessages from QgsVectorDataProvider::convertToProviderType I'll give you more report tomorrows

luipir · 2017-09-22T10:54:54Z

@m-kuhn simplified error message management usign pushMessage

luipir · 2017-09-22T13:31:52Z

@m-kuhn now two test fails:
170 - PyQgsComposerPicture (Failed) <- (test passes locally)
265 - PyQgsServer (Failed) <- as usual

luipir · 2017-09-25T09:44:44Z

still PyQgsServer failing as usual? I've to ask to merge? PyQgsServer seems fail in every 2.18 build

m-kuhn · 2017-09-25T12:25:10Z

@luipir can you identify since when the tests fail?

luipir · 2017-09-25T13:15:42Z

@m-kuhn If I'm not wrong, would be this merge #5225

luipir · 2017-09-26T07:34:25Z

rebased on the latest upstream fuixes that should fix failing trest on QgsServer

luipir · 2017-09-26T08:37:11Z

now a new test fail that pass locally :/
qgis_legendrenderertest

m-kuhn · 2017-09-26T08:42:33Z

Haven't seen that one fail before, but this looks suspicious indeed:

Expected

Result

luipir · 2017-09-26T08:55:12Z

again I'm lost :(
thi sis my successful local test that here is failing:
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() src/core/symbology-ng/qgssymbollayerv2utils.cpp: 4074: (prettyBreaks) [0ms] pretty classes: 5
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() "testName:legend_diagram_size size=52.9219x89.9 dpmm=3.77953 s=200x339"
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() src/core/qgsscalecalculator.cpp: 40: (setMapUnits) [14ms] Map units set to 2
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() QgsRenderChecker using mask image
68: /tmp/legend_diagram_size.png
68: /home/ginetto/PROGRAMMING/QGIS/QGIS-2.18/tests/testdata/control_images/legend/expected_legend_diagram_size/expected_legend_diagram_size.png
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() Expected size: 200w x 339h
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() Actual size: 200w x 339h
68: /tmp/legend_diagram_size_result_diff.png
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() 6/67800 pixels mismatched (500 allowed)
68: 6/67800
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() src/core/layertree/qgslayertreeregistrybridge.cpp: 78: (layersWillBeRemoved) [7ms] 1 layers will be removed, enabled:1
68: QDEBUG : TestQgsLegendRenderer::testDiagramSizeLegend() src/core/layertree/qgslayertreeregistrybridge.cpp: 78: (layersWillBeRemoved) [0ms] 4 layers will be removed, enabled:1
68: PASS : TestQgsLegendRenderer::testDiagramSizeLegend()

m-kuhn · 2017-09-26T08:59:45Z

If you could use git blame to see who created the test we could ping him here to see if he has a hint on what's going wrong.

If it's unrelated to your changes (and only a flaky test) we can easily restart the tests, merge and leave the author of the legend tests to further investigate what could have gone wrong.

luipir · 2017-09-26T09:01:58Z

I always think it's my responsability... and I loose a lot of time for this :/... I'll look for the test author

m-kuhn · 2017-09-26T09:06:32Z

python/core/qgsvectorlayereditbuffer.sip

@@ -148,4 +148,16 @@ class QgsVectorLayerEditBuffer : QObject
    void updateAttributeMapIndex( QgsAttributeMap& attrs, int index, int offset ) const;

    void updateLayerFields();
+
+    /** Apply geometry modification basing on provider geometry type.


If you could add here a hint on what kind of modification can be expected from this method this would help a lot for a reader of the API. (I.e. some reference to Z, M, Multi, others?)

added more doc to the underling call that do the work: convertToProviderType

luipir · 2017-09-26T09:12:46Z

@nyalldawson any idea why:
qgis_legendrenderertest -> TestQgsLegendRenderer::testDiagramSizeLegend fails? (blame points to you)
It works locally and my PR is involved only in case of add features or change geometries and test just works on an empty layer.

nyalldawson · 2017-09-26T09:36:12Z

@nyalldawson any idea why:
qgis_legendrenderertest -> TestQgsLegendRenderer::testDiagramSizeLegend fails? (blame points to you)
It works locally and my PR is involved only in case of add features or change geometries and test just works on an empty layer.

Sorry - no idea. I write a LOT of tests without necessarily understanding all the underlying code! ;)

luipir · 2017-09-26T10:19:09Z

now success! :/ travis magics?

nyalldawson · 2017-09-26T10:20:12Z

Odd.. I don't recall seeing that test fail intermittently. Maybe another recent change has caused this...

m-kuhn · 2017-09-26T10:20:15Z

now success! :/ travis magics?

Probably more an unstable test...

luipir · 2017-09-26T10:22:04Z

me too... is the first time I'm facing this fail!

luipir · 2017-09-26T10:32:53Z

@m-kuhn merge?

m-kuhn · 2017-09-26T11:05:53Z

python/core/qgsvectordataprovider.sip

@@ -391,6 +391,13 @@ class QgsVectorDataProvider : QgsDataProvider
    void pushError( const QString& msg );

    /** Converts the geometry to the provider type if possible / necessary
+     * this is the list of possibile modifications:


s/possibile/possible/

m-kuhn · 2017-09-26T11:06:56Z

python/core/qgsvectordataprovider.sip

+     * - convert to multitype if necessary
+     * - convert to curved type if necessary
+     * - convert to linear type from curved type
+     * - Add or Remove z/m types


Should this say "values" instead of "types"?

Oh and either always capitalize the items or never

m-kuhn · 2017-09-26T11:07:34Z

python/core/qgsvectordataprovider.sip

+     * - convert to multitype if necessary
+     * - convert to curved type if necessary
+     * - convert to linear type from curved type
+     * - Add or Remove z/m types
    @return the converted geometry or nullptr if no conversion was necessary or possible*/


Better us \returns, most code is now written this way

luipir · 2017-09-27T13:52:06Z

@m-kuhn is it safe to merge? do you need more reviewer to merge?

m-kuhn · 2017-09-27T14:02:23Z

It looks good for me. I can't tell if it is safe.

m-kuhn requested changes Sep 20, 2017

View reviewed changes

m-kuhn reviewed Sep 21, 2017

View reviewed changes

m-kuhn approved these changes Sep 21, 2017

View reviewed changes

luipir force-pushed the geom_compatibility_check_release-2_18-fix15741 branch from 76e9199 to 45f041d Compare September 26, 2017 07:33

luipir added 6 commits September 26, 2017 09:34

return geom compatibility check in addFeature and changeGeometry

c33ae01

adapted test to addFeature new check geometry

101c5cc

manage new geom compatibility check when changeGeometry

7012c12

manage addFeatures fails (mainly due to geom compatibility check)

016363f

flat geom (remove Z) if provider is 2D and geom is 3D + test

3264709

added more unit test

99cf08f

luipir added 4 commits September 26, 2017 09:34

better doxy description and spelling

43ef3f3

notify adaptGeometry error via pushMessage

1e2a213

remove unuseful message managed by pushMessage from provider

8a4d32f

fixed misleading error return code in addPart

45f041d

m-kuhn reviewed Sep 26, 2017

View reviewed changes

more explicit documentation

c54e202

m-kuhn reviewed Sep 26, 2017

View reviewed changes

luipir added 3 commits September 26, 2017 13:13

explicit 0 value in case add Z or M

fbaf966

updated doxigen directives

027a715

typo

278a88c

dakcarto merged commit d3d8f4d into qgis:release-2_18 Sep 27, 2017

luipir deleted the geom_compatibility_check_release-2_18-fix15741 branch September 27, 2017 15:11

Geom compatibility check release 2.18 fixes #15741 #16927 #5223

Geom compatibility check release 2.18 fixes #15741 #16927 #5223

Conversation

luipir commented Sep 20, 2017 • edited Loading

Description

Checklist

m-kuhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Sep 21, 2017

m-kuhn commented Sep 21, 2017 • edited Loading

gioman commented Sep 21, 2017

gioman commented Sep 21, 2017

m-kuhn commented Sep 21, 2017

Choose a reason for hiding this comment

gioman commented Sep 21, 2017

m-kuhn commented Sep 21, 2017

luipir commented Sep 21, 2017

luipir commented Sep 21, 2017

m-kuhn commented Sep 21, 2017

luipir commented Sep 21, 2017

gioman commented Sep 21, 2017

m-kuhn commented Sep 21, 2017

m-kuhn left a comment

Choose a reason for hiding this comment

luipir commented Sep 21, 2017

luipir commented Sep 22, 2017

luipir commented Sep 22, 2017

luipir commented Sep 25, 2017

m-kuhn commented Sep 25, 2017

luipir commented Sep 25, 2017

luipir commented Sep 26, 2017

luipir commented Sep 26, 2017

m-kuhn commented Sep 26, 2017

Expected

Result

luipir commented Sep 26, 2017 • edited Loading

m-kuhn commented Sep 26, 2017

luipir commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Sep 26, 2017

nyalldawson commented Sep 26, 2017

luipir commented Sep 26, 2017 • edited Loading

nyalldawson commented Sep 26, 2017

m-kuhn commented Sep 26, 2017

luipir commented Sep 26, 2017 • edited Loading

luipir commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Sep 27, 2017

m-kuhn commented Sep 27, 2017

luipir commented Sep 20, 2017 •

edited

Loading

m-kuhn commented Sep 21, 2017 •

edited

Loading

luipir commented Sep 26, 2017 •

edited

Loading

luipir commented Sep 26, 2017 •

edited

Loading

luipir commented Sep 26, 2017 •

edited

Loading