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

Move from QModelIndex to QPersistentModelIndex in the layer tree proxy model class #4705

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

nirvn
Copy link
Member

@nirvn nirvn commented Oct 30, 2023

Trying to fix this (https://opengisch.sentry.io/issues/4569883968/?project=6119013&query=is%3Aunresolved&referrer=issue-stream&sort=user&statsPeriod=7d&stream_index=10) which has been a longstanding crasher, but appears to be more frequent in Qt6.

The stacktrace is really unhelpful, but it does tell us that FlatLayerTreeModelBase::data's check for a valid source model QModelIndex doesn't appear to be enough.

I think moving from a QList<QModelIndex> to a QList<QPersistentModelIndex> could improve things here.

@qfield-fairy
Copy link
Collaborator

@m-kuhn
Copy link
Member

m-kuhn commented Nov 2, 2023

Do we also want to check for validity as advised in the docs?
Either by not using the index or at least add a debug print to see if it precedes crashes to have a hint that this is actually the culprit?

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

According to the docs, the QPersistentModelIndex is the way to go anyways.

There is this important bit of info from the docs here, I think this might improve the situation with random crashes:

It is good practice to check that persistent model indexes are valid before using them.

https://doc.qt.io/qt-6/qpersistentmodelindex.html#details

@nirvn nirvn merged commit c6c7dc4 into master Nov 2, 2023
21 checks passed
@nirvn nirvn deleted the persistent branch November 2, 2023 12:59
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.

4 participants