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] Fix georeferencer crash when deleting a point through context menu #18227 #6626

Merged
merged 1 commit into from Apr 24, 2018

Conversation

alexispolti
Copy link
Contributor

@alexispolti alexispolti commented Mar 18, 2018

Description

fixes #18227
Georeferencer crashes when deleting a point through context menu. This should fix it.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • I have run the scripts/prepare-commit.sh script before each commit

@alexispolti alexispolti changed the title Fix geroreferencer crash when deleting a point through context menu Fix georeferencer crash when deleting a point through context menu Mar 18, 2018
@alexispolti alexispolti force-pushed the fix_georeferencer_crash_18227 branch from dacb435 to fe9e1cd Compare March 18, 2018 17:48
@alexispolti
Copy link
Contributor Author

Any news @nyalldawson ?

@alexispolti alexispolti changed the title Fix georeferencer crash when deleting a point through context menu [bugfix] Fix georeferencer crash when deleting a point through context menu #18227 Apr 19, 2018
@alexispolti
Copy link
Contributor Author

@m-kuhn or anyone: is there something beside lack of time preventing merge ? :-)

@m-kuhn
Copy link
Member

m-kuhn commented Apr 23, 2018

@m-kuhn or anyone: is there something beside lack of time preventing merge ? :-)

Nope, that's it pretty much :)

Looks ok to me to change it this way, but I'm not sure what caused the original crash. Was it that mPrevRow pointed to an invalid row or something else?
If it's the the invalid mPrevRow, will the crash appear again if you first recenter on this point and then delete it after?

@alexispolti
Copy link
Contributor Author

alexispolti commented Apr 23, 2018

Yes, mPrevRow / mPrevColumn were positioned on the selected point whether the action was "Recenter" or "Delete point". Which in the second case leads obviously to a crash as they pointed to a non existing row/column. The fix is to change them only when the action is "Recenter".
We could first recenter on the point, then delete it but IMHO it would be more confusing than helping: when you select a point to delete it, you already know which one it is (if not, you first select "Rencenter"). So is it really necessary to center on a now-non-existing point?

@m-kuhn
Copy link
Member

m-kuhn commented Apr 24, 2018

You are right of course, I meant to ask something different. Let me reformulate my question. If a user first clicks recenter (mPrevRow/mPrevColumn are set) and then he clicks delete, will this also crash or are mPrevRow/mPrevColumn invalidated with some other functionality in this scenario?

@alexispolti
Copy link
Contributor Author

(Sorry, ashamed of my poor English)
No, there won't be any crash, as mPrevRow is only used in updateItemCoords which cannot be called as the point doesn't exist anymore.

@m-kuhn m-kuhn merged commit 11741ad into qgis:master Apr 24, 2018
@m-kuhn
Copy link
Member

m-kuhn commented Apr 24, 2018

Thanks a lot!

@alexispolti
Copy link
Contributor Author

Thanks to you!

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.

None yet

2 participants