Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
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
Conversation
| ifnotself.canvas.widgetlock.available(self): | ||
| return | ||
| self.needclear=True | ||
| ifnotself.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.
jklymak commentedMay 11, 2021
| return | ||
| ifnotself.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
ianhi left 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.
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:
QuLogic commentedMay 12, 2021
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.
tacaswell commentedDec 16, 2022
Pro: rebased I'm half inclined to merge this even with that issue as this is still a big step forward. |
ianhi commentedDec 16, 2022
🚀 🚀 🚀 🚀 🚀 - the power to blit in notebooks awaits! |
tacaswell commentedDec 16, 2022
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.
QuLogic commentedDec 30, 2022
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).