Remove visibility changes in draw for *Cursor widgets #19763
Conversation
| if not self.canvas.widgetlock.available(self): | ||
| return | ||
| self.needclear = True | ||
| if not self.visible: |
There was a problem hiding this comment.
I think we want to keep this short-circuit to avoid going to _update which avoids a draw_idle down the line.
There was a problem hiding this comment.
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.
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.
Oh, I see now. We used to always make the lines not visible so if not self.visible we could safely bail.
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?
There was a problem hiding this comment.
I re-added a condition on the _update calls, which is hopefully sufficient.
| return | ||
| if not self.canvas.widgetlock.available(self): | ||
| return | ||
| self.needclear = True |
There was a problem hiding this comment.
Sidenote but as far as I can tell self.needclear is never actually used in MultiCursor
ianhi
left a comment
ianhi
left a comment
There was a problem hiding this comment.
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 setting multi.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, and prevents triggering extra draws in nbAgg. Fixes matplotlib#19633.
tacaswell
force-pushed
the
widget-events
branch
from
1c01eab to
44405a0
Compare
|
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 in matplotlib#19763.
By implementing `_onmove` in a similar manner to `Cursor`. Followup to matplotlib#19763.
By implementing `_onmove` in a similar manner to `Cursor`. Also, deprecate the related `needclear` attribute in both widgets. Followup to matplotlib#19763.
These were added in matplotlib#19763.
5 participants
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).