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

Fix issues for multiediting and updating for attribute form with more widgets pointing to the same field #50410

Merged
merged 5 commits into from
Nov 18, 2022

Conversation

domi4484
Copy link
Contributor

@domi4484 domi4484 commented Sep 30, 2022

Fix #50197 Multiedit not working correctly when multiple widgets are available for the same field, and at the same time reduce the usage of signals/slots logic inside the attribute form.

In the attribute form there can be multiple widgets for the same field. But the dependency widget -> field was stored as a map with field as key. It means only the last added widget was stored in the map. The updating of the values was done with a signal/slot connection between valuesChanged() and setValues() of all widgets sharing the field.

I changed that using a multi map and thus storing all widgets pointing to a field in a variable. When a field value is changed the values of the same field widgets are set explicitly by looking in the map if thre are other to update.

This probably solves other unreported issues where in functions onConstraintStatusChanged and synchronizeState only the last added widget was updated.

@github-actions github-actions bot added the GUI/UX Related to QGIS application GUI or User Experience label Sep 30, 2022
@github-actions github-actions bot added this to the 3.28.0 milestone Sep 30, 2022
@nyalldawson
Copy link
Collaborator

Merging this one should be deferred till after release -- touching this particular code introduces a nuclear level risk of regression 😱

@nyalldawson nyalldawson added the Frozen Feature freeze - Do not merge! label Sep 30, 2022
@m-kuhn
Copy link
Member

m-kuhn commented Oct 1, 2022

@domi4484 could you leave some notes about what was wrong and how this PR improves the situation?

@domi4484 domi4484 changed the title Fix #50197 and reduce usage of signal/slots in attribute form Fix issues for multiediting and updating for attribute form with more widgets pointing to the same field Oct 3, 2022
@domi4484
Copy link
Contributor Author

domi4484 commented Oct 3, 2022

@m-kuhn yes I added some.

@nyalldawson i hope this will be a small step making this code more predictable. If this doesn't show any regression do you think it will be possible to backport it?

@3nids
Copy link
Member

3nids commented Oct 3, 2022

This is something we'd need to land in upcoming LTR.

I see two approaches:

  • frozen for now, merge in next master 3.29 in, backport, in 3.28.1 or 2
  • merge now for 3.28.0

I am not sure what is best. The sooner before it gets flagged as LTR sounds good, but just a bit of testing in master before might be worth (although it's not bullet-proof, especially just right after the release).

@andreasneumann
Copy link
Member

I see two approaches:

* frozen for now, merge in next master 3.29 in, backport, in 3.28.1 or 2

* merge now for 3.28.0

If possible, I'd like to see this land in 3.28.x LTR. I'll be available to help testing. Both of Denis proposals would work for me - perhaps the sooner the better. People don't expect a .0 release to be production ready and it isn't yet labeled as LTR with 3.28.0

@nyalldawson
Copy link
Collaborator

@3nids

frozen for now, merge in next master 3.29 in, backport, in 3.28.1 or 2

This would be my vote. (Specifically backport for 3.28.1)

@3nids
Copy link
Member

3nids commented Oct 4, 2022

This would be my vote. (Specifically backport for 3.28.1)

I'm happy with this approach

@3nids 3nids added backport release-3_28 and removed Frozen Feature freeze - Do not merge! labels Nov 15, 2022
@3nids 3nids requested review from nirvn and removed request for nyalldawson November 15, 2022 13:09
Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

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

Code looks good to me -- could we add an additional test case that covers the scenario being fixed here? Other than that, +1 to merge.

@domi4484
Copy link
Contributor Author

@nirvn thanks I have added one test for it

Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the test (I've ran it against master and it does fail, which is perfect :) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI/UX Related to QGIS application GUI or User Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiedit does not play well with fields appearing multiple times on an editor form
6 participants