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 some /Transfer/ annotations #3460

Merged
merged 1 commit into from Sep 23, 2016
Merged

Fix some /Transfer/ annotations #3460

merged 1 commit into from Sep 23, 2016

Conversation

mhugo
Copy link

@mhugo mhugo commented Sep 5, 2016

I was originally interested in fixing QgsGeometryRubberBand::setGeometry, but found some other candidates based on the surrounding comments ("takes ownership")

@nyalldawson
Copy link
Collaborator

Nice work! Looks good to merge

@@ -54,7 +54,7 @@ class QgsGeometryRubberBand: QgsMapCanvasItem
~QgsGeometryRubberBand();

/** Sets geometry (takes ownership). Geometry is expected to be in map coordinates */
void setGeometry( QgsAbstractGeometry* geom );
void setGeometry( QgsAbstractGeometry* geom /Transfer/ );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should fix this and make it accept a QgsGeometry instead. That'll avoid any potential leaks/deleted pointer issues.

Copy link
Member

Choose a reason for hiding this comment

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

Even better would be to merge QgsGeometryRubberBand with QgsRubberBand ;-)

Copy link
Author

Choose a reason for hiding this comment

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

True. So what was the idea of QgsGeometryRubberBand ? @mhugent ?

@m-kuhn
Copy link
Member

m-kuhn commented Sep 13, 2016

Ready to ship?

@wonder-sk wonder-sk added the Requires Changes! Waiting on the submitter to make requested changes label Sep 14, 2016
@m-kuhn
Copy link
Member

m-kuhn commented Sep 22, 2016

@mhugo does it take a lot to fix this so we can remove it from the PR queue?

@mhugo
Copy link
Author

mhugo commented Sep 23, 2016

@m-kuhn Sorry for the delay !

@m-kuhn
Copy link
Member

m-kuhn commented Sep 24, 2016

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Changes! Waiting on the submitter to make requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants