WIP Correct array string display in table and form #9610

troopa81 · 2019-03-22T09:32:57Z

Description

This is the following of PR #9048.
The previous PR fixes the read from postgres and it works with list widget but not with plain text widget.
This PR fixes the display with plain text widget for string array.

Be aware that there is still a problem to deal with integer and float array in plain text.

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 contain 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

nyalldawson · 2019-03-22T20:43:13Z

src/core/qgsfield.cpp

@@ -268,6 +268,11 @@ QString QgsField::displayString( const QVariant &v ) const
  {
    return QObject::tr( "BLOB" );
  }
+  else if ( d->type == QVariant::StringList )
+  {
+    return "{" + v.toStringList().join( "," ) + "}";


Suggested change

return "{" + v.toStringList().join( "," ) + "}";

return '{' + v.toStringList().join( ',' ) + '}';

What do you think about json format [x,y,z]?

Why not, but If the data comes from postgres and there is more than two dimensions then, you will have something displayed like this : [{"test", "titi"}, {"tutu", "tata"}], because only the first level of array is parsed.

But if the data comes from JSON (is it possible?), it will be the opposite problem.

Not sure what the ideal solution is here, support of multidimensionnal array is quite partial in QGIS (like I said before, integer or float array won't work)

But if the data comes from JSON (is it possible?), it will be the opposite problem.

Yes https://qgis.org/en/site/forusers/visualchangelog34/#feature-json-jsonb-type-support

Actually, displayString treats the specific case of json and jsonb and return the json text as is.
My modifications will only occurs when we encounter an array which is not a json array.

So :

In case of postgres array we would have : {{"test", "titi"}, {"tutu", "tata"}}

In case of postgres json array we would have : [["test", "titi"], ["tutu", "tata"]]

Don't you think it's OK because each one respect the original text description?

I understand your point, but I can't find an elegant way of fixing this.

Best I can propose is to add a method on QgsField to setup the way we have to format array. To set the default to [ and ] and call this method from postgres provider.

Is the underlying problem that the postgres provider is only able to parse non-nested arrays?

We can parse nested array, so 2 dimension string array would result in a QVariantList where each element is QStringList. I try it at first but I get other problems (I don't remember exactly why).

Some users seems to expect the original string when displaying PostGres array https://issues.qgis.org/issues/20872.

Sorry for the silence, let's see if we can define some basic requirements

internally in QGIS the data should be passed around as QVariantList (of QVariantLists) to allow for generic implementation for data aggregation and the like

the "default" representation should be as generic as it gets (and my gut feeling is that json is more in this area than hstore formatting) but having this configurable (or autoconfigured) sounds good

... so re-reading your proposal #9610 (comment) sounds quite good, I must have misread that before, sorry.

I made the modifications described above 24fd6ce

Best I can propose is to add a method on QgsField to setup the way we have to format array. To set the default to [ and ] and call this method from postgres provider.

stale · 2019-04-23T15:56:08Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

troopa81 · 2019-06-04T08:00:54Z

@m-kuhn Are you OK with the last modifications?

nyalldawson · 2019-06-17T20:21:33Z

Ping @m-kuhn

m-kuhn

Works for me the way it is.
Just some minor adjustments to be made before it's good to merge.

m-kuhn · 2019-07-05T09:38:17Z

src/core/qgsfield_p.h

@@ -60,6 +60,7 @@ class QgsFieldPrivate : public QSharedData
      , length( len )
      , precision( prec )
      , comment( comment )
+      , arrayFormatString( "[%1]" )


Suggested change

, arrayFormatString( "[%1]" )

, arrayFormatString( QStringLiteral( "[%1]" ) )

m-kuhn · 2019-07-05T09:39:30Z

src/providers/postgres/qgspostgresprovider.cpp

@@ -1046,6 +1046,7 @@ bool QgsPostgresProvider::loadFields()
    mDefaultValues.insert( mAttributeFields.size(), defValMap[tableoid][attnum] );

    QgsField newField = QgsField( fieldName, fieldType, fieldTypeName, fieldSize, fieldPrec, fieldComment, fieldSubType );
+    newField.setArrayFormatString( "{%1}" );


Shouldn't that be conditional for hstore fields?
And don't forget QStringLiteral

array field has to be formatted this way too, so it works for all postgres field, no?

Forget about my previous comment, I forget about the json fields

src/core/qgsfield.h

m-kuhn · 2019-07-05T09:54:17Z

@troopa81 what's your opinion about the current formatting possibilities?

I am kind of undecided, because on the one hand they pretend to be very flexible with the replacement pattern, on the other hand, they only go half the way because the delimiter is hardcoded to , and quoting cannot be fine tuned.
I wonder if an enum to specify the format wouldn't be preferable, it's less flexible but can be optimized better and I wonder if the current flexibility is actually ever used.

troopa81 · 2019-07-05T12:08:37Z

I wonder if an enum to specify the format wouldn't be preferable

I ask myself pretty much the same thing. Do you really think that one day, we will need to specify the delimeter, or quotes around the array values?

It should be several enum (one for {} or [], one for delimeter, one for quote around value) or maybe a flags combination. I'm not sure but il seems a little bit complicated just to overcome this issue, no?

Or maybe we can just start with an enum CURLY_BRACKETS and SQUARE_BRACKETS and see what comes next.

m-kuhn · 2019-07-05T23:24:43Z

It should be several enum (one for {} or [], one for delimeter, one for quote around value) or maybe a flags combination. I'm not sure but il seems a little bit complicated just to overcome this issue, no?

I was more thinking of a FormatJson and a FormatHstore one.
If we need the full flexibility I would opt for a more flexible system similar to what you have here. But it pretty much sounds like over engineering the system.

formatting arrays (postgres for instance)

elpaso · 2019-07-08T14:45:50Z

@troopa81 just to be sure, did you have a look to the related issues here: #30610 and #30565 ?
We need to agree on a common and robust representation of complex values, independently from the provider storage.

troopa81 · 2019-07-08T14:46:57Z

I was more thinking of a FormatJson and a FormatHstore one

Looks fine to me indeed. I made the modifications 1dc486f

I'm choose to define the enum as an integer in QgsField private class and convert it then https://github.com/qgis/QGIS/pull/9610/files#diff-b2de5811b366da6fc58a391b307efd38R207. Indeed private class cannot include public one or forward declare the enum. It's not so pretty but I don't see a better way.

elpaso · 2019-07-08T14:49:25Z

src/core/qgsfield.cpp

+  else if ( d->type == QVariant::StringList )
+  {
+    QString formatString = arrayFormatString() == QgsField::FormatHstore ? "{%1}" : "[%1]";
+    return formatString.arg( v.toStringList().join( "," ) );


What happens if an element of the string list contains commas?

I'll add a test to check it works

src/core/qgsfield.cpp

elpaso · 2019-07-08T14:50:33Z

tests/src/core/testqgsfield.cpp

+
+  // string list field
+  QgsField stringListField( QStringLiteral( "stringlist" ), QVariant::StringList, QStringLiteral( "_text" ) );
+  QCOMPARE( stringListField.displayString( QStringList() << "test1" << "test2" << "test3" ), QString( "[test1,test2,test3]" ) );


You need to test strings containing commas and []

Co-Authored-By: Alessandro Pasotti <elpaso@itopen.it>

troopa81 · 2019-07-08T15:09:38Z

@troopa81 just to be sure, did you have a look to the related issues here: #30610 and #30565 ?

Just now :)

We need to agree on a common and robust representation of complex values, independently from the provider storage.

I'm not sure this could be independent from the provider storage. For instance, when QGIS deals with multidimensionnal array (let say 2 dimension), it parses the first dimension and let the second dimension in raw format. For instance, if you have this kind of table


Paul	Eric
Hugo	Seb

You will have a QVariantList containing 2 string :

"[Paul, Eric]", "[Hugo, Seb]" for JSON provider
"{Paul, Eric}", "{Hugo, Seb}" for PG provider

So, for now, if you have a PG provider and you display your value with plain text widget, you will display
[{Paul, Eric}, {Hugo, Seb}], that's what my PR fixes.

If you want to force the representation, you will have to parse and store internally all array dimension. Futhermore, it seems that user wants to keep the original provider format https://issues.qgis.org/issues/20872#note-14

troopa81 · 2019-07-08T16:13:11Z

There is a big issue with edition, I have to think again about this.

troopa81 · 2019-07-10T13:41:01Z

When we use text edit on an array, we need to convert the array, QStringList (or QVariantList), to a string representation (it's partially done here) and back from QString to QStringList (or QVariantList). This last is not done so edition fails.

It's not as simple as splitting upon commas because of possible espaced character (,") or because we have multidimensionnal array.

Best option I think is to reuse the array parsing method developed in qgspostgresprovider (and move them to QgsArrayParser file). I made an (ugly) POC, it works. We could even add a method to format list to string in order to add quote and protect special character.

@elpaso @m-kuhn Does it sounds a good approach?

m-kuhn · 2019-07-12T04:41:43Z

I'm choose to define the enum as an integer in QgsField private class and convert it then https://github.com/qgis/QGIS/pull/9610/files#diff-b2de5811b366da6fc58a391b307efd38R207. Indeed private class cannot include public one or forward declare the enum. It's not so pretty but I don't see a better way.

Does it work if you make the enum an enum class?

stale · 2019-08-07T01:29:26Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2019-08-14T01:57:10Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

stale · 2019-08-28T05:56:13Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2019-09-04T06:53:49Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

nyalldawson reviewed Mar 22, 2019

View reviewed changes

nyalldawson added the Bugfix label Mar 31, 2019

nyalldawson added this to the 3.8 milestone Mar 31, 2019

stale bot added stale Uh oh! Seems this work is abandoned, and the PR is about to close. and removed stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Apr 23, 2019

nyalldawson modified the milestones: 3.8, 3.8.0 May 19, 2019

qgib mentioned this pull request May 25, 2019

QGIS 3.5 not loading or saving multi-dimensional arrays properly from PostGIS layers #28691

Closed

nyalldawson added Bug Either a bug report, or a bug fix. Let's hope for the latter! and removed Bugfix labels May 27, 2019

nyalldawson modified the milestones: 3.8.0, 3.8.1 Jun 23, 2019

m-kuhn reviewed Jul 5, 2019

View reviewed changes

troopa81 added 5 commits July 8, 2019 16:21

fixes qgis#20872 : Correct string array string display in table and form

c55ef7f

Add array formatting string on qgsfield to deal with different way of

b46879c

formatting arrays (postgres for instance)

Forget parameter name in documentation

73a8280

Update sip

186bd25

Replace formatting string with an enum

1dc486f

troopa81 force-pushed the fix_stringlist_display branch from 0ccbf2d to 1dc486f Compare July 8, 2019 14:39

elpaso reviewed Jul 8, 2019

View reviewed changes

src/core/qgsfield.cpp Outdated Show resolved Hide resolved

elpaso reviewed Jul 8, 2019

View reviewed changes

add QStringLiteral

913d4b8

Co-Authored-By: Alessandro Pasotti <elpaso@itopen.it>

troopa81 changed the title ~~Correct array string display in table and form~~ WIP Correct array string display in table and form Jul 8, 2019

troopa81 mentioned this pull request Jul 15, 2019

Fix multi-selection on value relation widget using string fields #30610

Merged

1 task

nyalldawson modified the milestones: 3.8.1, 3.8.2 Jul 23, 2019

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 7, 2019

stale bot closed this Aug 14, 2019

m-kuhn reopened this Aug 14, 2019

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 14, 2019

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 28, 2019

stale bot closed this Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Correct array string display in table and form #9610

WIP Correct array string display in table and form #9610

troopa81 commented Mar 22, 2019

nyalldawson Mar 22, 2019

m-kuhn Mar 25, 2019

troopa81 Mar 25, 2019

m-kuhn Apr 9, 2019

troopa81 Apr 9, 2019

troopa81 Apr 9, 2019

m-kuhn Apr 9, 2019

troopa81 Apr 9, 2019

m-kuhn Apr 23, 2019

troopa81 May 10, 2019

stale bot commented Apr 23, 2019

troopa81 commented Jun 4, 2019

nyalldawson commented Jun 17, 2019

m-kuhn left a comment

m-kuhn Jul 5, 2019

m-kuhn Jul 5, 2019

troopa81 Jul 8, 2019

troopa81 Jul 8, 2019

m-kuhn commented Jul 5, 2019 •

edited

Loading

troopa81 commented Jul 5, 2019

m-kuhn commented Jul 5, 2019

elpaso commented Jul 8, 2019

troopa81 commented Jul 8, 2019

elpaso Jul 8, 2019

troopa81 Jul 8, 2019

elpaso Jul 8, 2019

troopa81 commented Jul 8, 2019

troopa81 commented Jul 8, 2019

troopa81 commented Jul 10, 2019

m-kuhn commented Jul 12, 2019

stale bot commented Aug 7, 2019

stale bot commented Aug 14, 2019

stale bot commented Aug 28, 2019

stale bot commented Sep 4, 2019

	return "{" + v.toStringList().join( "," ) + "}";
	return '{' + v.toStringList().join( ',' ) + '}';

	, arrayFormatString( "[%1]" )
	, arrayFormatString( QStringLiteral( "[%1]" ) )

WIP Correct array string display in table and form #9610

WIP Correct array string display in table and form #9610

Conversation

troopa81 commented Mar 22, 2019

Description

Checklist

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

stale bot commented Apr 23, 2019

troopa81 commented Jun 4, 2019

nyalldawson commented Jun 17, 2019

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

m-kuhn commented Jul 5, 2019 • edited Loading

troopa81 commented Jul 5, 2019

m-kuhn commented Jul 5, 2019

elpaso commented Jul 8, 2019

troopa81 commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troopa81 commented Jul 8, 2019

troopa81 commented Jul 8, 2019

troopa81 commented Jul 10, 2019

m-kuhn commented Jul 12, 2019

stale bot commented Aug 7, 2019

stale bot commented Aug 14, 2019

stale bot commented Aug 28, 2019

stale bot commented Sep 4, 2019

m-kuhn commented Jul 5, 2019 •

edited

Loading