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

Don't loop through all selected features multiple times (once per field) when the attribute form is opened #41382

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

nyalldawson
Copy link
Collaborator

This is incredibly expensive, yet only required in a very very small
corner case (field is from a joined layer without the upsert on edit
capabilities).

Refine logic to avoid the scan wherever we can.

Fixes #41366

@nyalldawson nyalldawson added backport release-3_16 Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Feb 5, 2021
@nyalldawson
Copy link
Collaborator Author

@rduivenvoorde can you test please?

@rduivenvoorde
Copy link
Contributor

compiling....

@github-actions github-actions bot added this to the 3.18.0 milestone Feb 5, 2021
@rduivenvoorde
Copy link
Contributor

Thanks Nyall!

If I test the steps from the issue with that downloadable osm dataset, it works! No more freezing/blowing fans...

I also tried if the 'multi edit' was working, and that seems to work OK.

Did this come from #31234 ? I do see multi edit buttons, but I'm not sure how to test the auxiliary storage attribute buttons...

Ok, I tried using https://oslandia.com/en/2017/10/17/auxiliary-storage-support-in-qgis-3/

So I moved some labels using fid as primary key, and then in the form the multi edit looks like this (aka I see a button there...):

Screenshot-20210206005919-968x289

@troopa81 can you maybe test this better?

@uclaros
Copy link
Contributor

uclaros commented Feb 6, 2021

@nyalldawson Wouldn't it make sense to have all code in void QgsAttributeFormEditorWidget::updateWidgets() handled inside the switch( mode() ) conditional? Why bother at all with edit form widgets when we only use the search form?

@nyalldawson
Copy link
Collaborator Author

@uclaros

I agree - but I was trying to fix the deeper cause of the slowness here, since it would also affect use of the form in multi edit mode.

@gioman
Copy link
Contributor

gioman commented Feb 6, 2021

I also tried if the 'multi edit' was working, and that seems to work OK.

@rduivenvoorde @nyalldawson does this patch helps with #36863 ?

@uclaros
Copy link
Contributor

uclaros commented Feb 6, 2021

I agree - but I was trying to fix the deeper cause of the slowness here, since it would also affect use of the form in multi edit mode.

Yes, that I saw, and totally agree with the treat the cause, not the symptom idea. But while there, it might be a good idea to separate the logic of the search and edit form, those are needlessly mixed and might cause trouble in the future.

@nyalldawson
Copy link
Collaborator Author

@gioman

does this patch helps with #36863 ?

Should do, yes

@nyalldawson
Copy link
Collaborator Author

@uclaros does 724e1ba look right to you?

@uclaros
Copy link
Contributor

uclaros commented Feb 6, 2021

@uclaros does 724e1ba look right to you?

Can't the logic be handled in a single switch( mode() ) ? Two consecutive same switch statements look weird.

@nyalldawson
Copy link
Collaborator Author

Can't the logic be handled in a single switch( mode() ) ? Two consecutive same switch statements look weird.

I agree it looks odd, but they are handling completely different logic. I can't see any clean way to avoid this without unnecessary code duplication.

@nyalldawson nyalldawson removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Feb 8, 2021
@nyalldawson nyalldawson closed this Feb 8, 2021
@nyalldawson nyalldawson reopened this Feb 8, 2021
field) when the attribute form is opened.

This is incredibly expensive, yet only required in a very very small
corner case (field is from a joined layer without the upsert on edit
capabilities).

Refine logic to avoid the scan wherever we can.

Fixes qgis#41366
Fixes qgis#36863
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.

Subsequent selections kill QGIS ui performance
4 participants