Remove visibility changes in draw for *Cursor widgets by QuLogic · Pull Request #19763 · matplotlib/matplotlib · GitHub

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

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
[n/a] New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
[n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Why drop this?

I think we want to keep this short-circuit to avoid going to _update which avoids a draw_idle down the line.

If that's the case, we might want to clarify that for our future selves with a 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.

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?

I re-added a condition on the _update calls, which is hopefully sufficient.

ping @ianhi or @anntzer for a second review here?

Sidenote but as far as I can tell self.needclear is never actually used in MultiCursor

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.

Pro: rebased
Con: can still reproduce the issues @ianhi reported with disabling the horizontal line

I'm half inclined to merge this even with that issue as this is still a big step forward.

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.

I followed up on both @ianhi's issues in #24845.

These were added in matplotlib#19763.

By implementing `_onmove` in a similar manner to `Cursor`. Also, deprecate the related `needclear` attribute in both widgets. Followup to matplotlib#19763.

QuLogic added the topic: widgets/UI label Mar 24, 2021

QuLogic added this to the v3.5.0 milestone Mar 24, 2021

QuLogic force-pushed the widget-events branch from 5638ee7 to c4bcb5f Compare March 24, 2021 08:18

tacaswell reviewed Mar 24, 2021

View reviewed changes

tacaswell requested a review from ianhi March 24, 2021 14:28

QuLogic force-pushed the widget-events branch from c4bcb5f to 1c01eab Compare April 7, 2021 03:14

timhoffm approved these changes Apr 7, 2021

View reviewed changes

ianhi reviewed May 11, 2021

View reviewed changes

ianhi suggested changes May 11, 2021

View reviewed changes

jklymak marked this pull request as draft May 13, 2021 14:17

jklymak added the status: needs revision label May 13, 2021

QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 23, 2021

QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022

github-actions Bot added the status: needs rebase label Oct 18, 2022

QuLogic added 2 commits December 15, 2022 20:04

Deprecate access to Cursor/MultiCursor event handlers.

733fbb0

Remove visibility changes in draw for *Cursor widgets.

44405a0

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 December 16, 2022 01:06

github-actions Bot removed the status: needs rebase label Dec 16, 2022

tacaswell marked this pull request as ready for review December 16, 2022 16:24

tacaswell approved these changes Dec 16, 2022

View reviewed changes

tacaswell removed the status: needs revision label Dec 16, 2022

tacaswell merged commit c1588e6 into matplotlib:main Dec 16, 2022

QuLogic deleted the widget-events branch December 17, 2022 00:44

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 17, 2022

Fix deprecations of *Cursor widget event handlers

d2a3266

These were added in matplotlib#19763.

QuLogic mentioned this pull request Dec 17, 2022

Fix deprecations of *Cursor widget event handlers #24750

Merged

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 30, 2022

Fix toggling of MultiCursor.{horizOn,vertOn}

273cebe

By implementing `_onmove` in a similar manner to `Cursor`. Followup to matplotlib#19763.

QuLogic mentioned this pull request Dec 30, 2022

Fix toggling of MultiCursor.{horizOn,vertOn} #24845

Merged

3 tasks done

This was referenced Jan 30, 2023

[Doc]: annotated_cursor example seems broken #25107

Closed

Undeprecate Cursor event handlers #25129

Merged

raphaelquast pushed a commit to raphaelquast/matplotlib that referenced this pull request Mar 16, 2023

Fix deprecations of *Cursor widget event handlers

b6a59fc

These were added in matplotlib#19763.

imgbot Bot pushed a commit to kkipngenokoech/matplotlib that referenced this pull request Apr 19, 2026

Fix deprecations of *Cursor widget event handlers

16e8456

These were added in matplotlib#19763.

                       if not self.canvas.widgetlock.available(self):
                           return
                       self.needclear = True
-                      if not self.visible:

                           return
                       if not self.canvas.widgetlock.available(self):
                           return
                       self.needclear = True

Remove visibility changes in draw for *Cursor widgets by QuLogic · Pull Request #19763 · matplotlib/matplotlib · GitHub

Repository navigation

Remove visibility changes in draw for *Cursor widgets - #19763

Remove visibility changes in draw for *Cursor widgets#19763tacaswell merged 2 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom QuLogic:widget-eventsQuLogic/matplotlib:widget-eventsCopy head branch name to clipboard

Conversation

QuLogic commented Mar 24, 2021

PR Summary

PR Checklist

Uh oh!

tacaswell Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

tacaswell Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

QuLogic Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 11, 2021

Uh oh!

ianhi May 11, 2021

Choose a reason for hiding this comment

Uh oh!

ianhi left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic commented May 12, 2021

Uh oh!

tacaswell commented Dec 16, 2022

Uh oh!

ianhi commented Dec 16, 2022

Uh oh!

tacaswell commented Dec 16, 2022

Uh oh!

QuLogic commented Dec 30, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove visibility changes in draw for *Cursor widgets#19763
tacaswell merged 2 commits into
matplotlib:mainfrom
QuLogic:widget-events

QuLogic Mar 26, 2021 •
edited
Loading