Deduplicate implementation of per-backend Tools. - #12760
Conversation
There was a problem hiding this comment.
| # Avoid using this (prefer self.canvas.manager.window), so that Tool | |
| # Avoid using self.window (prefer self.canvas.manager.window), so that Tool |
Does it make sense to make this a property or are there use-cases where window != self.canvas.manager.window?
There was a problem hiding this comment.
Actually I think this attribute should be deprecated (because it is not consistent across backend implementations, so any cross-backend code will need to use self.canvas.manager.window anyways, and that doesn't seem to be so hard to type that you'd also want a property for it (if anything, making the NavigationToolbar APIs consistent across toolkits should be the object of another PR)).
But that's orthogonal to the PR, because the point of the comment is that sometimes, NavigationToolbar methods get called without a true NavigationToolbar as first argument, but a proxy created with _make_classic_style_pseudo_toolbar(), and these won't have a window attribute.
|
|
||
| class RubberbandTk(backend_tools.RubberbandBase): | ||
| def __init__(self, *args, **kwargs): | ||
| backend_tools.RubberbandBase.__init__(self, *args, **kwargs) |
There was a problem hiding this comment.
It seems that RubberbandBase does not have an __init__. So technically, this change is possible.
Generally, is it wise not to call super().__init__(). The parent class may later add an __init__() method.
Here in particular, while RubberbandBase does not have __init__(), its parent ToolBase has, which does not get called also with the existing code. That doesn't look right to me.
There was a problem hiding this comment.
It will get called even after deleting this code block: when instantiating a RubberbandTk, Python will call RubberbandTk.__init__; because RubberbandTk does not define an __init__, it is inherited by looking up in the base classes; ultimately this resolves to ToolBase.init`.
In other words, the deleted code block does not do anything.
6a843e0 to
84ded1c
Compare
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
I think something should be done about self.window, because having an attribute that should not be used or at least not be used in certain situations is not reasonable. However, changing/deprecating window is beyond the scope of this PR.
With respect to keeping PRs small, I accept the introduced limitation on self.window within this PR. Ideally, we would first have a PR that cares for self.window and merge this PR only after that.
84ded1c to
afe3e76
Compare
... by reusing the old Toolbar-based implementations. If toolmanager ever becomes the default we can move the implementations there and conversely point the old Toolbar-based implementations to use the Tools.
afe3e76 to
d64890b
Compare
![MeeseeksDev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…760-on-v3.1.x Backport PR #12760 on branch v3.1.x (Deduplicate implementation of per-backend Tools.)
3 participants
... by reusing the old Toolbar-based implementations.
If toolmanager ever becomes the default we can move the implementations
there and conversely point the old Toolbar-based implementations to use
the Tools.
PR Summary
PR Checklist