-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix Cursor widget drawing behavior with blitting support #29971
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
Conversation
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Hello, it's just that I haven't heard from my contribution yet and it's been almost 2 weeks. The github bot suggested I leave a comment here so it would bring attention to any reviewers who are willing to check it and give me some feedback. So I would just like to ask if it is possible to review my PR soon so I could try to fix anything if needed. Hope to get some feedback soon! |
Hi @livlutz thank you for your contribution and sorry you did not get a response sooner. Is this PR in response to issue #29731? If so, please edit your summary above to include "closes #29731". I am not familiar with this module but I think the suggestion at #29731 (comment) was to make this method consistent with the one on |
Thanks for the feedback! Yes, it is related to #29731! I'll change it on my summary shortly. I did not try to fix it using MultiCursor though, I used self.canvas.blit() instead...should I take another look and try to use MultiCursor this time? |
As I said I'm not familiar with this module so definitely no expert. So I had a play with the code examples Cursor and MultiCursor to teach myself something about it. For I also tried the |
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 made a few comments to help you think about the change. I am inexperienced with blitting concepts so feel free to say so if you think I'm mistaken. I have just been looking at this example to try to understand what is happening.
lib/matplotlib/widgets.py
Outdated
self.canvas.restore_region(self.background) | ||
else: | ||
self.canvas.draw() | ||
self.canvas.restore_region(self.background) |
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.
self.background
may be None
here, in which case this fails. I think it is always None
if useblit
is False
.
lib/matplotlib/widgets.py
Outdated
else: | ||
self.canvas.draw() | ||
self.canvas.restore_region(self.background) | ||
self.canvas.blit(self.ax.bbox) |
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.
Since useblit
may be False
here, I think it does not make sense to use blit
.
lib/matplotlib/widgets.py
Outdated
@@ -1934,8 +1934,15 @@ def onmove(self, event): | ||
self.linev.set_visible(False) | ||
self.lineh.set_visible(False) | ||
if self.needclear: | ||
if self.useblit and self.background is not None: |
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 both needclear
and useblit
are True
, is it possible for background
to be None
? Looking at the code I do not think so, but maybe I missed something.
Hello, I tried to apply the suggested changes and it looks a bit better, could you try to take a look again and see if it is alright? |
I'm afraid I do not understand your changes. Can you explain how you chose what goes in the different branches of the if-loops? Also, please test your changes locally. Unfortunately there are no unit tests for this class, but you can run some example code that uses it. When you run an example against your branch, does the behaviour look right? Is it the same as without your changes? If not, fix it. I'm going to mark this as "draft" for now. Please mark it as "ready for review" when you are ready. In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room. |
PR summary
In class cursor,when the mouse moves out of the ax, redrawing takes a long time,
it solves the issue with the redrawing method, which takes too long when the mouse moves out of the ax, after fixing it should be more efficient
closes #29731
Summarising changes :
"Fix Cursor widget drawing behavior with blitting support"
PR checklist