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 identify on map in relation reference widget #10047

Merged

Conversation

@elpaso
Copy link
Contributor

commented May 21, 2019

Fixes #22071 - Relation reference widget wrong feature when "on map identification"

Fix identify on map in relation reference widget
Fixes #22071 - Relation reference widget wrong feature when "on map identification"
@elpaso

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@m-kuhn I'd like your blessing here.

@m-kuhn

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Do you have some information what was wrong with the previous approach? Reading the code, both approaches seem to make sense, so I'd be happy to get a bit of context as a decision aid.

@elpaso

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@m-kuhn thank you for looking at this PR, I think that the error was due to a wrong assumption about the underlying model: the original code assumed the model was an instance of QgsAttributeTableModel instead of QgsFeatureFilterModel, the role QgsAttributeTableModel::FeatureIdRole does not exist in QgsFeatureFilterModel and there is no feature ID stored in QgsFeatureFilterModel's data.

I didn't check if that error was introduced by a change of the underlying model, it dates 3 years back, what surprises me is that this bug went unnoticed for such a long time.

In any event I added a test case (which failed before my patch and passes after the patch).

QEventLoop loop;
// Populate model (I tried to listen to signals but the module reload() runs twice
// (the first load triggers a second one which does the population of the combo)
// and I haven't fin a way to properly wait for it.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn May 22, 2019

Member

Maybe casting the combobox to a QgsFeatureListComboBox and then connecting to the models isLoadingChanged signal?
But it's not trivial

This comment has been minimized.

Copy link
@elpaso

elpaso May 22, 2019

Author Contributor

Maybe casting the combobox to a QgsFeatureListComboBox and then connecting to the models isLoadingChanged signal?
But it's not trivial

I tried that, but the isLoadingChanged was triggered twice as well (4 times, really), and I missed the second reload (which actually loads the data in the combo), but I admit that the logic in that loading behavior is not clear to me. I thought about adding another signal but I'm not sure it would be useful out of the test, so I prefer to not add it.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn May 22, 2019

Member

IIRC: The logic is to as quickly as possible get the display value of the current selected item to show up to the user. And then with a little delay lazy-load also some additional values to fill the popup.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn May 22, 2019

Member

Maybe connecting to the signal and then checking if the rowCount of the model is > 1?

@m-kuhn
m-kuhn approved these changes May 22, 2019
Copy link
Member

left a comment

Looks all good to me, merge as is or handle the suggestions at will.

@elpaso elpaso merged commit 86f35e8 into qgis:master May 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@backporting

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

The backport to release-3_4 failed:

Commits ["2b14dacd51ca98061d7f14877df74006fd52e5c4","ad019c499bcd8d9800d9e7bafbcebad48d678bb2"] could not be cherry-picked on top of release-3_4

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport release-3_4
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 2b14dacd51ca98061d7f14877df74006fd52e5c4 ad019c499bcd8d9800d9e7bafbcebad48d678bb2
# Create a new branch with these backported commits.
git checkout -b backport-10047-to-release-3_4
# Push it to GitHub.
git push --set-upstream origin backport-10047-to-release-3_4
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is release-3_4 and the compare/head branch is backport-10047-to-release-3_4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.