Skip to content
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

Update widgets with default values on attribute form #31167

Merged
merged 9 commits into from
Aug 16, 2019

Conversation

signedav
Copy link
Contributor

@signedav signedav commented Aug 8, 2019

Updates widget values on real time while editing the referenced fields.
default_value_01

To avoid recursion it stores the currently updated fields index in mAlreadyUpdatedFields. The widgets contains as default value the value of the previous one plus 1:
default_value_02

To Do:
-[ ] Testfunction in testqgsattributeform

…ing to the currently changed widgets field.

On init the dependencies are stored into mDefaultValueDependencies. On attribute change the default values are written into the other widgets according to the dependency map.

To avoid recursions the widgets fields index is stored into mAlreadyUpdatedFields.
// need to check dstVar.isNull() != srcVar.isNull()
// otherwise if dstVar=NULL and scrVar=0, then dstVar = srcVar
// be careful- sometimes two null qvariants will be reported as not equal!! (e.g., different types)
bool changed = ( dstVar != srcVar && !dstVar.isNull() && !srcVar.isNull() )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would qgsVariantEqual take care of all that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would care for the case that when a NULL value changes to 0, but for what is mentioned in the comment (I took it from saveEdit because there the behavior is the same) qgsVariantEqual does not care for the case when a null value does not change, but "two null qvariants will be reported as not equal." This would mean both are isNull but the comparison is not true. This could be solved by changing the qgsVectorEqual as this:

  return lhs.isNull() == rhs.isNull() && lhs == rhs || ( lhs.isNull()  && rhs.isNull() ) ;

But I'm not sure about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lhs.isNull == rhs.isNull already takes care of exactly this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the && lhs == rhs makes it false again. When we pass two NULL values. the compared isNulls returns true, but are connected with the comparison that could be false (regarding to the comment).

What the lhs.isNull == rhs.isNull does is, that it returns false in the end in case we compare 0 with NULL what is nice...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because of the comment or did you experience this?
In the end I think qgsVariantCompare was implemented exactly for cases like this, so it would be a pity if it's not usable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because the comment. That's why I'm not sure at all. But it should be same like in saveEdits and so I didn't want that I risk issues with this case...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see, the check in saveEdits was introduced in 2fddc00 (by @nyalldawson)
The method qgsVariantEqual later in 90e2c45 (by @elpaso)

@nyalldawson / @elpaso, could the above patch be merged into qgsVariantEqual?

Copy link
Member

@m-kuhn m-kuhn Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the question is, should we add a testcase for

QVERIFY( qgsVariantEqual( QVariant( QVariant::Int ), QVariant( QVariant::Double ) ) )

I'd say yes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to all the above ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct that qgsVariantEqual should return true in case of two NULL values from different types, but false if one of them is invalid?
Means these test are correct:

  QVERIFY( qgsVariantEqual( QVariant( QVariant::Int ), QVariant( QVariant::String ) ) );

  QVERIFY( !qgsVariantEqual( QVariant( QVariant::Int ), QVariant() ) );

With that it won't have an issue anymore on the formerly failing test for the ValueRelationWidget but what is happening here, is still a little bit weird https://github.com/qgis/QGIS/blob/master/src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.cpp#L236

      QVariant v( mComboBox->itemData( i ) );
      if ( qgsVariantEqual( v, value ) )
      {
        idx = i;
        break;
      }

There it won't return true anymore if value is invalid, but still it wouldn't had returned true before also if v would not be recognized as NULL if mComboBox->itemData( i ) returns 0. So shouldn't here be set a type with e.g. toInt()?

src/gui/qgsattributeform.h Outdated Show resolved Hide resolved
src/gui/qgsattributeform.cpp Show resolved Hide resolved
// need to check dstVar.isNull() != srcVar.isNull()
// otherwise if dstVar=NULL and scrVar=0, then dstVar = srcVar
// be careful- sometimes two null qvariants will be reported as not equal!! (e.g., different types)
bool changed = ( dstVar != srcVar && !dstVar.isNull() && !srcVar.isNull() )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to all the above ;)

src/gui/qgsattributeform.cpp Outdated Show resolved Hide resolved
src/core/qgis.cpp Outdated Show resolved Hide resolved
Co-Authored-By: Matthias Kuhn <matthias@opengis.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants