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.

Sign up for GitHub

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

Jump to bottom

Merge features: immediately update widgets for attributes with a provider default value #59960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Merge features: immediately update widgets for attributes with a provider default value #59960

wants to merge 1 commit into from

Conversation

JuhoErvasti
Copy link
Contributor

@JuhoErvasti JuhoErvasti commented Dec 18, 2024 •
edited
Loading

Apply for widgets which show a key/description instead of the actual values. Addresses part of #59494 (the additional context part, the main part AFAIK is the intended behavior and is meant to be addressed with a "merge policy" feature)

Backport to 3.40 requested.

before:
before

after:
after

@github-actions github-actions bot added this to the 3.42.0 milestone Dec 18, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 18, 2024 •
edited
Loading

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 88078a2)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 88078a2)

@JuhoErvasti JuhoErvasti changed the title Merge features: immediately update widgets for attributes with a default value Merge features: immediately update widgets for attributes with a provider default value Dec 18, 2024
m-kuhn
m-kuhn reviewed Dec 19, 2024
View reviewed changes
src/app/qgsmergeattributesdialog.cpp Outdated
@@ -267,6 +267,15 @@ void QgsMergeAttributesDialog::createTableWidgetContents()
currentComboBox->setCurrentIndex( currentComboBox->findData( QStringLiteral( "manual" ) ) );
currentComboBox->blockSignals( false );
}

const QString widgetType = QgsGui::editorWidgetRegistry()->findBest( mVectorLayer, mFields.at( idx ).name() ).type();
Copy link
Member

Choose a reason for hiding this comment

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

Should this be field.editorWidgetSetup().type() instead? Iirc findBest is using some heuristics while we can use a configuration here.

m-kuhn
m-kuhn reviewed Dec 19, 2024
View reviewed changes
src/app/qgsmergeattributesdialog.cpp Outdated

const QString widgetType = QgsGui::editorWidgetRegistry()->findBest( mVectorLayer, mFields.at( idx ).name() ).type();

const QStringList updateWidgetFor = QStringList() << QStringLiteral( "ValueMap" )
Copy link
Member

@m-kuhn m-kuhn Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the reason for this hardcoded list of widget types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m-kuhn Apologies for taking so long to respond! My thinking was that these are the widgets where you can have a situation where the default value should be displayed differently, whereas for others such as text edit / range widget it doesn't necessarily matter.

Should I change it so that the widget gets updated regardless of its type?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025

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.

@github-actions github-actions 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 Jan 3, 2025
@JuhoErvasti JuhoErvasti force-pushed the 59494_db_default_values branch from 69fde6a to 527d77f Compare January 9, 2025 12:06
@JuhoErvasti JuhoErvasti force-pushed the 59494_db_default_values branch from 527d77f to 88078a2 Compare January 9, 2025 12:08
@JuhoErvasti
Copy link
Contributor Author

@m-kuhn
I pushed an update if you can/want to take a look, it should address both your comments. I explained my initial reasoning for using the list of widgets but thinking about it more I don't think it makes sense to only update certain types of widgets.

@m-kuhn
Copy link
Member

m-kuhn commented Jan 9, 2025

@JuhoErvasti thanks, looks good now.

If we have hardcoded lists like this, it's good to have a comment that explains the why. Or often it is better to bring this knowledge into the widget registry or widget wrapper, so if someone implements a new widget in a plugin, they can define how it should behave. Just FYI, as this seems to be no longer an issue now.

@JuhoErvasti
Copy link
Contributor Author

@m-kuhn thank you, I'll keep that in mind.

If it's okay now, could this change be backported to 3.40 as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-kuhn m-kuhn m-kuhn approved these changes

Assignees
No one assigned
Labels
backport release-3_40
Projects
None yet
Milestone
3.42.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JuhoErvasti @m-kuhn