Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
figure_enter_event uses now LocationEvent instead of Event. Fix issue #9812.#9814
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
Should perhaps change the backends to pass inn their coordinates as well. As for now the fix is having in FigureCanvasBase.enter_notify_event, as in the commit. |
Thanks for the PR. You'll probably get more testers if you give us a sample script to try that shows this works better. |
wow, that is a pretty big likely long standing bug! |
tacaswell commentedNov 19, 2017 • edited by dopplershift
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by dopplershift
Uh oh!
There was an error while loading.Please reload this page.
ok, so it looks like the sub-classes of Doing this from scratch I think the second pattern is better, but it is probably not worth the effort to deprecate and rename the bridge methods in {gtk,gtk3, tk} (the work would be to create new methods rename the methods to |
The method name The bridge method for tk |
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.
Some minor comments, but 👍 as-is
Tested on
- qt5
- tk
| @@ -268,7 +268,8 @@ def get_width_height(self): | |||
| return int(w / self._dpi_ratio), int(h / self._dpi_ratio) | |||
| def enterEvent(self, event): | |||
| FigureCanvasBase.enter_notify_event(self, guiEvent=event) | |||
| x, y = self.mouseEventCoords(event.pos()) | |||
| FigureCanvasBase.enter_notify_event(self, guiEvent=event, xy=(x, y)) | |||
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.
This can just beself.enter_notif_event(guiEvent=event, xy=(x, y))
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.
Could do but that would break some inconsistency with the other event as well. For example
def wheelEvent(self, event): x, y = self.mouseEventCoords(event) # from QWheelEvent::delta doc if event.pixelDelta().x() == 0 and event.pixelDelta().y() == 0: steps = event.angleDelta().y() / 120 else: steps = event.pixelDelta().y() if steps: FigureCanvasBase.scroll_event(self, x, y, steps, guiEvent=event)def keyPressEvent(self, event): key = self._get_key(event) if key is not None: FigureCanvasBase.key_press_event(self, key, guiEvent=event)def keyReleaseEvent(self, event): key = self._get_key(event) if key is not None: FigureCanvasBase.key_release_event(self, key, guiEvent=event)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.
Fair. I suppose those could also all be changed to normal method calls, but that is beyond the scope of this PR.
| @@ -2017,8 +2017,11 @@ def enter_notify_event(self, guiEvent=None, xy=None): | |||
| if xy is not None: | |||
| x, y = xy | |||
| self._lastx, self._lasty = x, y | |||
| else: | |||
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.
Can you add a warning here like
warnings.warn('enter_notify_event expects a location but your backend did not pass one. ''This may become mandatory in the future',stacklevel=2)
?
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.
cbook.warn_deprecated?
| x = evt.GetX() | ||
| y = self.figure.bbox.height - evt.GetY() | ||
| evt.Skip() | ||
| FigureCanvasBase.enter_notify_event(self, guiEvent=evt, xy=(x, y)) |
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.
This can also just beself.enter_notify_event.
| @@ -340,6 +342,11 @@ def motion_notify_event(self, event): | |||
| y = self.figure.bbox.height - event.y | |||
| FigureCanvasBase.motion_notify_event(self, x, y, guiEvent=event) | |||
| def enter_notify_event(self, event): | |||
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.
This is probably better named something else (as it is new)
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.
Could but breaks naming scheme used in the class.
@tacaswell Will this get merged soon or there need more testing? |
This one has gone too long@tacaswell . I needed to strip some commits since there where some confilics with the master branch and I simple do not have the time to solve the merge conflics. |
@lkjell Sorry this has sat for so long, I assure you it is not personal! I am confused by your comment though as it looks like have fixed the merge conflicts (as the tests are running). |
lib/matplotlib/backend_bases.py Outdated
| else: | ||
| x = None | ||
| y = None | ||
| warn_deprecated('2.2', 'enter_notify_event expects a location but your backend did not pass one.') |
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.
Can this be changed to '3.0' ?
tacaswell commentedMar 2, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
This is a bug fix.https://matplotlib.org/users/event_handling.html |
🐑 your right, that is what I get for reading from the bottom! |
If I understand correctly, the original PR included fixes to backends; the present version of this PR adds a warning, but does not have any fixes to the backends. Therefore, if this PR is merged, an attempt to use the figure enter/leave events will generate the deprecation warnings until another PR provides the backend fixes. It is unfortunate that those original fixes were wiped out because they needed to be rebased. Am I correct, or hopelessly confused? |
@efiring something like that. If memory serve right some backend did pass in None, None. Some did not register an Enter event(gtk). The backend fix had merge confict and you need to edit the super to python3 only. |
My fault for not looking into deeper. I restored the non-conflicting commits. The only conflicting backend is tk. It appears that file was renamed to _backend_tk.py |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
In the documentation1 figure_enter_event uses LocationEvent. However, previously it just uses the base class Event. This cause some problem when trying to access LocationEvent attributes because they did not exist. This addresses issue#9812.
Fix also an issue that tkagg backend does not have a figure_enter_event.
gtk3, Qt5 and wx pass in coordinates when entering figure.
The tkagg backend did not have a figure_leave_event. Enabling that as well.
Sample script using Qt5 backend. Move the cursor in and out of the figure.
PR Checklist