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

[bugfix] Crash on undo layout legend item on deleted item #7240

Merged
merged 3 commits into from
Jun 18, 2018

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jun 14, 2018

Fixes #19155

@wonder-sk please have a look: I think it's a safe move since layer() is not accessed in removeLegendFromLayer()

@nyalldawson nyalldawson added this to the 3.2 milestone Jun 14, 2018
@wonder-sk
Copy link
Member

I have looked into this and I think the correct fix should be in QgsLayerTreeLayer::layerWillBeDeleted() - by emitting layerWillBeUnloaded() signal before the weak pointer to the layer is cleared. Layer tree model listens to that signal and tries to clean up everything, but it can't because the layer pointer is null at the point when the slot is called.

We should also have a unit test for this case: 1. create a layer, 2. create a layer tree with the layer, 3. delete the layer, 4. check that layerLegendNodes() for the layer's node is empty.

@elpaso
Copy link
Contributor Author

elpaso commented Jun 17, 2018

@wonder-sk bingo! Added a test (that was failing without the patch, as you suggested).

(I'll squash before commit if the code looks good to you)

Copy link
Member

@wonder-sk wonder-sk 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, thanks!

@elpaso elpaso merged commit e39abc5 into qgis:master Jun 18, 2018
@elpaso elpaso deleted the bugfix-19155-layout-legend-undo-crash branch June 18, 2018 06:05
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.

3 participants