-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
remove .rect member (clashes with QWidget) #2946
Conversation
FigureCanvasQTAgg inherits from QWidget, which already has a 'rect' property and getter. Also, consolidate .rect and (boolean) .drawRect into a single _drawRect member. This is only used for the zoom rect and not accessed from other classes (other than indirectly via the drawRectangle() function).
@@ -115,10 +113,10 @@ def paintEvent(self, e): | |||
p.drawPixmap(QtCore.QPoint(0, 0), QtGui.QPixmap.fromImage(qImage)) | |||
|
|||
# draw the zoom rectangle to the QPainter | |||
if self.drawRect: | |||
if self._drawRect: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to if self._drawRect is not None:
? I know that it is valid and correct python as-written, however I think it is clearer to do the conversion to bool explicitly.
Could you also add a note in api_change.rst and CHANGELOG? It looks like this was always an internal-only attribute, but this is an api change and should be documented as such. It looks like a bunch of widgets in |
Thanks for the comments; I have made the suggested changes. I also looked at |
Great. Looks good to me. Going to hold off on merging for further feed back and so I can use this day-to-day for a bit but it will get in for 1.4. |
remove .rect member (clashes with QWidget)
FigureCanvasQTAgg inherits from QWidget, which already has a 'rect'
property and getter.