Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Remove visibility changes in draw for *Cursor widgets#19763
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if self.ignore(event): | ||
| return | ||
| if event.inaxes not in self.axes: | ||
| return | ||
| if not self.canvas.widgetlock.available(self): | ||
| return | ||
| self.needclear = True | ||
| if not self.visible: |
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.
Why drop this?
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.
I think we want to keep this short-circuit to avoid going to_update which avoids adraw_idle down the line.
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.
If that's the case, we might want to clarify that for our future selves with a comment.
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.
Well, we want to remove it from here so that the later.set_visible is effective. I'll see if we can still skip the_update at least. Alternatively, we should turn the visibility attributes into property setters.
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.
Oh, I see now. We used toalways make the lines not visible so ifnot self.visible we could safely bail.
Do we need some more careful logic on this like to callself._update() if visible or just made invisible? I suspect there is now also some path where we can setself.vertOn orself.horizOn to False and then they never get removed?
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.
I re-added a condition on the_update calls, which is hopefully sufficient.
| def onmove(self, event): | ||
| onmove = _api.deprecate_privatize_attribute('3.5') | ||
| def _onmove(self, event): | ||
| if self.ignore(event): | ||
| return | ||
| if event.inaxes not in self.axes: | ||
| return | ||
| if not self.canvas.widgetlock.available(self): | ||
| return | ||
| self.needclear = True |
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.
Sidenote but as far as I can tellself.needclear is never actually used inMultiCursor
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.
I think this has introduced some incorrect behavior where the cursor is not removed when the mouse leaves the axes. Also@tacaswell noted:
Do we need some more careful logic on this like to call self._update() if visible or just made invisible? I suspect there is now also some path where we can set self.vertOn or self.horizOn to False and then they never get removed
I found that moving the cursor around and then settingmulti.horizOn = False leaves an artifact:
Hmm, I tested changing those settings, but it may have been only while the mouse was already over the window. |
This can be all handled in the mouse move event handler instead, andprevents triggering extra draws in nbAgg.Fixesmatplotlib#19633.
Pro: rebased I'm half inclined to merge this even with that issue as this is still a big step forward. |
🚀 🚀 🚀 🚀 🚀 - the power to blit in notebooks awaits! |
Given Ian's comment I marked this non-draft and approved. I'm holding off on merging at@timhoffm 's review is very old now so this should get another set of eyes. |
These were added inmatplotlib#19763.
By implementing `_onmove` in a similar manner to `Cursor`.Followup tomatplotlib#19763.
By implementing `_onmove` in a similar manner to `Cursor`. Also,deprecate the related `needclear` attribute in both widgets.Followup tomatplotlib#19763.
These were added inmatplotlib#19763.
PR Summary
This can be all handled in the mouse move event handler instead, and prevents triggering extra draws in nbAgg.
Fixes#19633.
Additionally, mark access to said event handlers as deprecated.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).