Add null handling to value map edit widget (fixes #15215) #3274

dgoedkoop · 2016-07-04T18:33:08Z

See title. :)

This makes it possible to use the NULL placeholder value in the value map widget, just like in the other editing widgets, so that NULL values can be displayed with an appropriate description, and so that values can be reset to NULL.

I hope this can also be applied to the LTR release 2.14.

m-kuhn · 2016-07-05T10:10:28Z

src/gui/editorwidgets/qgsvaluemapwidgetwrapper.cpp

@@ -24,10 +26,13 @@ QgsValueMapWidgetWrapper::QgsValueMapWidgetWrapper( QgsVectorLayer* vl, int fiel

 QVariant QgsValueMapWidgetWrapper::value() const
 {
-  QVariant v;
+  QString v;


Better to make that a QVariant and only have a single return in the end

dgoedkoop · 2016-07-05T16:13:39Z

@m-kuhn Is this what you meant by "single return in the end"?

m-kuhn · 2016-07-05T16:22:23Z

Yes, thanks.
It's mainly "good practice" because it avoids memory leaks when methods get very long.

I also wondered if the toString() is required if it's just after converted back to a QVariant?

Right now, the widget setting for NULL and the application representation setting for NULL have to match, is this correct?

dgoedkoop · 2016-07-05T16:33:31Z

The added toString() isn't required anymore, that's true. I'll fix that.

The idea of this PR is that you should be able to do something like this:

(assuming that 'NULL' is the application-wide representation for null values.)

Currently, this would just try to write the literal string value 'NULL' into the attribute when selecting the first option.

m-kuhn · 2016-07-05T16:46:45Z

I really like the functionality.

I'm just a bit worried about the portability of the project, mixing representation and data seems to be a bit brittle.

What would be great it to make the whole configuration for the Value Map a bit more advanced. At the moment it's just a key-value pair which is saved. This leads to ugly side-effects like leftovers from configurations from other widgets when you start configuring it. The proper way to go would be to move the configuration to a QVariantMap in a dedicated key in the configuration. (See e.g. relationreferencefactory that does a similar thing for a stringlist). Sorry, getting a bit off-topic here :)

If the above is implemented, there could also be a dedicated boolean key "AddNull" which would be semantically safe.

Would you be able to implement that for merging for QGIS 3 (I guess it's a bit late for QGIS 2 now).

To fix the current behavior, what do you think about this:

We make a hardcoded string __NULL__ which represents NULL when inserted as value (or even straight NULL)? This would be a system configuration independent option with little space for side-effects. And to make configuration easier, a button could be added "Insert NULL value" to make the configuration obvious.

dgoedkoop · 2016-07-05T17:00:21Z

I agree that the value map widget could use some improvements. In addition to a widget-specific null configuration it should perhaps also try to actually use the data type which the attribute actually has. Currently all configuration is done with strings, so that the result after choosing is always a string, which leads to queries like SET int_field = '3', which coincidentally works but probably isn't as correct as it could be.

As for a quick fix, my thought has been that the application-wide null setting is there for a reason, namely that some people apparently really want to write the string 'NULL' into a field. So I think that the value map should also allow to enter that value, if QGis is configured accordingly.

m-kuhn · 2016-07-05T17:05:54Z

But this string is nowhere to be written by the user (apart from the configuration). Normally it's written by the user into a text field and the converted to a null value which is appropriate for the backend.

If you think about it in an MVC concept way.

The system configured NULL should only be there on the view.
In this pull request it is in the model.

And the result is that if you like to write 'NULL' for NULL and your colleague likes to write empty string '' for NULL values and you give him your project it will be broken.

dgoedkoop · 2016-07-05T17:17:08Z

That is true. And perhaps that is a more common problem than that someone really wants to enter the string 'NULL' by using a value map. Ok, I'll see if I can change it to a hard-coded value and add a button, like you suggested. :)

dgoedkoop · 2016-07-05T20:31:53Z

So how about this?

The usage looks like this:

Internally the null value is represented as one particular hard-coded GUID. A bit of a hack, but I think it prevents both kinds of problem. If doing it like this, perhaps the GUID should be stored somewhere as a constant, but at the moment the widget itself and the configuration dialog don't seem to share a common header file.

m-kuhn · 2016-07-06T06:52:20Z

Wow, this is exactly how I thought it should be!
But now I am afraid merging this huge changeset so close to release.

Can you convince me? ;)

m-kuhn · 2016-07-06T06:53:28Z

src/gui/editorwidgets/qgsvaluemapconfigdlg.cpp

-    {
-      tableWidget->setItem( row, 0, new QTableWidgetItem( mit.key() ) );
-    }
+      setRow( row, mit.key(), QString( "" ) );


FYI, there's also a NULL string

… constructor

nyalldawson · 2016-07-06T08:48:58Z

src/gui/editorwidgets/qgsvaluemapconfigdlg.cpp

@@ -48,13 +49,17 @@ QgsEditorWidgetConfig QgsValueMapConfigDlg::config()
    if ( !ki )
      continue;

+    QString ks = ki->text();
+    if (( ks == QString( "NULL" ) ) && !( ki->flags() & Qt::ItemIsEditable ) )
+      ks = "{2839923C-8B7D-419E-B84B-CA2FE9B80EC7}";


can you make this a #define somewhere to avoid the multiple places it's manually reused?

... i.e. just include the config in the widget

dgoedkoop · 2016-07-06T09:41:11Z

Yes, it has turned out a bit bigger than I first expected!

I've done some more testing with a colleague just now. The functionality seems to be ok, but he complained that the value is displayed as "NULL" even if the global null representation setting is different. I think he is right, so I'll fix that.

m-kuhn · 2016-07-06T10:52:23Z

Cool, if it's for displaying only it's the right thing to do I think.

Did you consider to expand the editwidget test? https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgseditwidgets.py

dgoedkoop · 2016-07-06T19:49:58Z

@m-kuhn Thanks for the suggestion! I've added a few unit tests and they did indeed catch a minor issue with my code.

m-kuhn · 2016-07-07T10:43:49Z

Thanks a lot!

dgoedkoop · 2016-07-09T22:11:58Z

Great!

What would be great it to make the whole configuration for the Value Map a bit more advanced. At the moment it's just a key-value pair which is saved. This leads to ugly side-effects like leftovers from configurations from other widgets when you start configuring it. The proper way to go would be to move the configuration to a QVariantMap in a dedicated key in the configuration. (See e.g. relationreferencefactory that does a similar thing for a stringlist). Sorry, getting a bit off-topic here :)

I've been thinking about this a bit more, and I cannot think of a really suitable solution here. It is probably desired not to break things when you open a project saved by a previous QGIS version. So, then, a value map saved in the current format needs to be converted on-the-fly to the new format. But such a function would also 'upgrade' those left-overs from other widgets into a value map of the new type, right? So then, not much is solved after all.

m-kuhn · 2016-07-10T05:44:22Z

There is the possibility to upgrade on project load. Then it would only affect old configurations and no left-overs. There is a "projectupgrader" class (can't remember the name right now). How does that sound?

Smooth upgrade to 3.0 is not a strict requirement - but from my side appreciated.

) * Add null handling to value map edit widget (fixes qgis#15215) * Return QVariant type * Use hardcoded value for 'null' representation * Detect "null" value when loading value map from csv; use null QString constructor * Use configured "null" representation for display in value map * Use single definition for value map null representation guid * Added unit test for value map widget and fixed value displaying bug (cherry picked from commit d2c9863)

* Add null handling to value map edit widget (fixes #15215) * Return QVariant type * Use hardcoded value for 'null' representation * Detect "null" value when loading value map from csv; use null QString constructor * Use configured "null" representation for display in value map * Use single definition for value map null representation guid * Added unit test for value map widget and fixed value displaying bug (cherry picked from commit d2c9863)

Add null handling to value map edit widget (fixes qgis#15215)

50e2667

dgoedkoop force-pushed the valuemapnull branch from ea1ae52 to 50e2667 Compare July 5, 2016 06:27

m-kuhn reviewed Jul 5, 2016
View reviewed changes

Return QVariant type

42652c5

dgoedkoop force-pushed the valuemapnull branch from 1ca3701 to 42652c5 Compare July 5, 2016 16:48

Use hardcoded value for 'null' representation

24ed6da

m-kuhn reviewed Jul 6, 2016
View reviewed changes

Detect "null" value when loading value map from csv; use null QString…

5c38f80

… constructor

dgoedkoop force-pushed the valuemapnull branch from 2f7c69e to 5c38f80 Compare July 6, 2016 08:36

nyalldawson reviewed Jul 6, 2016
View reviewed changes

dgoedkoop added 2 commits July 6, 2016 12:57

Use configured "null" representation for display in value map

6b3cbca

Use single definition for value map null representation guid

5b19f48

Added unit test for value map widget and fixed value displaying bug

dda84fa

dgoedkoop force-pushed the valuemapnull branch from 5745e33 to dda84fa Compare July 7, 2016 09:24

m-kuhn merged commit d2c9863 into qgis:master Jul 9, 2016

dgoedkoop deleted the valuemapnull branch July 9, 2016 10:36

dgoedkoop mentioned this pull request Jul 9, 2016

Add handling of null value in value map to search widget #3289

Merged

dgoedkoop mentioned this pull request Aug 14, 2017

Add null handling to value map edit widget (fixes #15215) #5015

Closed

8 tasks

wonder-sk mentioned this pull request Apr 12, 2018

Backport to 2.18: Fix for null handling in value map #6784

Merged

qgib mentioned this pull request May 25, 2019

Cannot set value to NULL in value map #23152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add null handling to value map edit widget (fixes #15215) #3274

Add null handling to value map edit widget (fixes #15215) #3274

dgoedkoop commented Jul 4, 2016

m-kuhn Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 6, 2016

m-kuhn Jul 6, 2016

nyalldawson Jul 6, 2016

m-kuhn Jul 6, 2016

dgoedkoop commented Jul 6, 2016

m-kuhn commented Jul 6, 2016

dgoedkoop commented Jul 6, 2016

m-kuhn commented Jul 7, 2016

dgoedkoop commented Jul 9, 2016

m-kuhn commented Jul 10, 2016

Add null handling to value map edit widget (fixes #15215) #3274

Add null handling to value map edit widget (fixes #15215) #3274

Conversation

dgoedkoop commented Jul 4, 2016

m-kuhn Jul 5, 2016

Choose a reason for hiding this comment

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

dgoedkoop commented Jul 5, 2016

m-kuhn commented Jul 6, 2016

m-kuhn Jul 6, 2016

Choose a reason for hiding this comment

nyalldawson Jul 6, 2016

Choose a reason for hiding this comment

m-kuhn Jul 6, 2016

Choose a reason for hiding this comment

dgoedkoop commented Jul 6, 2016

m-kuhn commented Jul 6, 2016

dgoedkoop commented Jul 6, 2016

m-kuhn commented Jul 7, 2016

dgoedkoop commented Jul 9, 2016

m-kuhn commented Jul 10, 2016