Repository navigation
Unique values allow null value checkbox - #62750
MoritzMoreira wants to merge 5 commits into
Conversation
67d1047 to
0b1a434
Compare
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. 🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟
|
0b1a434 to
cf08eda
Compare
m-kuhn
left a comment
There was a problem hiding this comment.
Thanks for this pull request.
There are a few things to reconsider regarding where methods live, QgsFieldFormatter and QgsEditorWidgetWrapper are generic base classes, so they should not contain unique values widget specific code.
da8c72e to
0b0d78f
Compare
15ed34e to
7a131eb
Compare
7a131eb to
31c3f3a
Compare
31c3f3a to
05ec425
Compare
05ec425 to
da6acff
Compare
| if ( comboBox->findData( v ) == -1 ) | ||
| { | ||
| // v != "" is to check for the autogenerated null value, which should be represented as "()" instead of "(Null)". | ||
| if ( allowNull && !v.isEmpty() ) |
There was a problem hiding this comment.
I don't get that. On line 157 you set v to the NULL_VALUE in case it's null. Otherwise it's a string. Why is this check here needed?
There was a problem hiding this comment.
I think you are right, I cannot reproduce the situation where this autogenerated value is passed anymore. I have simplified the function accordingly and I think I have tested all cases (0/1 entry in value map table, allow null (un)checked, (un)valid default value, and all their combinations).
There was a problem hiding this comment.
What about backwards compatibility btw.? Is it concerned when the allowNull setting is not set (because configured with older QGIS) but NULL values are in the list?
There was a problem hiding this comment.
As it is, the NULL values from the table are added to the combobox, even when allow NULL is unchecked. Should the user get a message about it, and/or the addition of the null values be blocked? I think the rows should not be deleted automatically, because the user might want to go back to the old QGIS, correct?
There was a problem hiding this comment.
@m-kuhn What do you think about that? (the backwards compatibility part)
There was a problem hiding this comment.
@MoritzMoreira Could you implement the backwards compatibility here? Means that you are able to load old projects: e.g on widget init set allow null if you have a null value (and remove the null value) and same in the widget configuration (but then stored again the new way). So you are able to use older project on newer QGIS versions (not being able to use new projects on old QGIS versions is fine I guess).
There was a problem hiding this comment.
FYI: I hope I will get to do this soon, but I am not sure when I'll have the time. I will continue this whole project and some point.
There was a problem hiding this comment.
I have added the backwards compatibility.
da6acff to
cd17a17
Compare
cd17a17 to
26cb2f4
Compare
| // check if the value was already added to the combobox | ||
| if ( comboBox->findData( v ) == -1 ) | ||
| { | ||
| // only if "allow null" is checked a null value is added |
There was a problem hiding this comment.
Won't this add NULL multiple times?
There was a problem hiding this comment.
I could not produce the situation, I think it is because the function only get's called once. Should I add
&& comboBox->findData( QgsFieldFormatter::NULL_VALUE ) == -1 )
just in case, before adding NULL?
Maybe this would be necessary in the case of a set default value and the call of QgsEditorWidgetWrapper::setValues()? I could not work out how to do the latter in the python console and have it reflected in the added layer in the GUI yet.
There was a problem hiding this comment.
Not 100% sure, but I thinks that it should check if QgsFieldFormatter::NULL_VALUE is already in the list.
It's in my opinion a little bit confusing structured. See below.
| QString v = value.toString(); | ||
|
|
||
| if ( comboBox ) | ||
| { | ||
| // check if the value was already added to the combobox | ||
| if ( comboBox->findData( v ) == -1 ) | ||
| { | ||
| // only if "allow null" is checked a null value is added | ||
| if ( config( QStringLiteral( "AllowNull" ) ).toBool() ) | ||
| comboBox->addItem( QgsApplication::nullRepresentation().prepend( '(' ).append( ')' ), QgsFieldFormatter::NULL_VALUE ); | ||
| // add the value | ||
| if ( !QgsVariantUtils::isNull( value ) ) | ||
| comboBox->addItem( v.prepend( '(' ).append( ')' ), v ); | ||
| } |
There was a problem hiding this comment.
I think I would rather structure it like this (just draft - and untested)
if ( comboBox )
{
if ( !QgsVariantUtils::isNull( value ) )
{
QString v = value.toString();
if ( comboBox->findData( v ) == -1 )
{
comboBox->addItem( v.prepend( '(' ).append( ')' ), v );
}
}
else if ( config( QStringLiteral( "AllowNull" ) ).toBool() && comboBox->findData( QgsFieldFormatter::NULL_VALUE) == -1 )
{
comboBox->addItem( QgsApplication::nullRepresentation().prepend( '(' ).append( ')' ), QgsFieldFormatter::NULL_VALUE );
}
}
What do you think?
There was a problem hiding this comment.
yes, that is better. I have tested commited the changes.
|
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
|
|
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. |
|
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
|
5bec983 to
c191eb3
Compare
34bcd05 to
b04091f
Compare
Non-virtual version of updateValues() specific for all the widgets with a combobox and allow-null-value configuration parameter.
Addition of a "Add NULL value" checkbox, enabling the addition of a NULL value to the combobox when there are no features.
- the widget's combobox offers a null value for layers from older versions when there is a null value in the field - when opening the configuration widget for the first time, the allow null checkbox is set accordingly - Null values are no longer added to the combobox in qgsuniquevaluewidgetwrapper:initWidget()
b04091f to
320dcf2
Compare
|
Is there anything I can do about the failed WASM check ? |
Just ignore it. |
I first created the PR Value map allow null value checkbox which fixes this issue. As the modified version of the virtual
QgsEditorWidgetWrapper::updateValues()can be applied to all widgets with ComboBoxes, I decided to implement my modification as a non-virtual function beside the virtual one, to avoid identical overrides for all 6 widgets with comboboxes.I decided to create one PR for each widget type, to avoid one very large PR.
There are two motivations for making the change for the other 5 combobox widgets besides value map (unique values,, enumaration, relation reference, classification and value relation) now:
in the case of only one available value, this value has to be selected by default, because the combobox will be disabled
The null value is represented this way in the combobox, rather then with no representation as of now. Also, one of the available values is set as default on opening the widget editor, rather then setting the comboBox to index -1 (no selection).
The adaptation of the default value preview is done in this PR.