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

Sip keep reference to registry #9112

Merged
merged 2 commits into from Feb 6, 2019

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Feb 6, 2019

When adding item to registries they are transferred to C++ via /Transfer/.

This adds an additional reference to the wrapper of the registered instance from the registry wrapper. If the registry wrapper scoped out, this reference was deleted and the child sip wrapper therefore garbage collected. By adding /KeepReference/ annotation to the registry accessors, the wrapper of the registries will never be deleted. This is no big deal, since they are de facto singletons.

The only slight shortcoming here is, that the python wrappers of registered items will never be garbage collected. But that should really only lead to minimal overhead.

If they are deleted, any other python wrappers which have been parented to these singletons are deleted too.

One of the most common issues with this is, that subclasses of registry items are deleted and lost.
@m-kuhn m-kuhn force-pushed the sip-keep-reference-to-registry branch from 462b073 to b2aed60 Compare February 6, 2019 11:18
@m-kuhn m-kuhn merged commit db15057 into qgis:master Feb 6, 2019
@m-kuhn m-kuhn deleted the sip-keep-reference-to-registry branch February 6, 2019 16:13
@nyalldawson
Copy link
Collaborator

@m-kuhn i noticed you missed some registries here -- e.g. layoutItemRegistry, symbolLayerRegistry etc. Was this intentional or an oversight? I'm inclined to add this to ALL singletons, just to avoid annoyances in future if they gain registry like behaviour.

@m-kuhn
Copy link
Member Author

m-kuhn commented Feb 7, 2019

Oops, I just quickly grepped for Registry through the singletons, that was not on purpose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants