-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix for #9608: RectangleSelector now reports mouse button press and release events correctly #20611
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
base: main
Are you sure you want to change the base?
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 while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. 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.
This reverts commit eba1886.
Mouse release is now taken as the coordinates of the actual mouse button release. Previously, the onmove set the extent of rectangle, and the extent was used as the mouse press and release coordinates. Now they are decoupled, and the test must be updated.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
…atplotlib into RectSel_press-release
lib/matplotlib/widgets.py
Outdated
x = self._eventpress.xdata | ||
y = self._eventpress.ydata |
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.
Please keep this using event
; I might remove _eventpress
as a state.
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.
Removing _eventpress
might be a little difficult, as _release(event)
refers to _eventpress
to calculate span, and also for calling onselect
. The reference for the span calculation might be able to be gotten around, but we'd then need to reconstruct the press event to send to onselect
.
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'm aware, but at this point event == self._eventpress
, so it should be fine to go back.
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.
Done. Also updated _release
to refer to event
instead of _eventrelease
. _release
still needs _eventpress
to properly calculate the span.
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.
Would it be more useful to change the docstring rather than returning the eventpress
and eventrelease
position? Mainly, because otherwise, it will be useless with interactive=True
- typically when dragging or resizing the selector. With interactive=False
, the eventpress
and eventrelease
will match the extents the selector.
What would be the benefit of returning the eventpress
and eventrelease
position? I guess the docstring was correct before the interactive functionality was added.
event == self._eventpress in _press and event == self._eventrelease in _release, so this makes sure the _release and _press functions refer to the event that is passed to them.
@ericpre - the extents of the selector always refer to the bottom left and top right of the rectangle, regardless of the order of the actual clicking. Sometimes you need to know the direction in which the rectangle was drawn, so you need to differentiate between click and release. If you want the extent, call |
Yes, the issue is that |
This is not passing the doc build. Also please rebase on master if you know how to do that. |
I don't understand the problem. Just sort the coordinates if you need them strictly increasing in the callback? Sorting them before passing to the callback reduces the possible usage. |
I haven't touched any of the docs for this function/file (afaik). How is it failing/where do I need to read about building docs? I don't know about rebasing on master. |
Sorry, I was not very clear. When using a selector on an axis, I would expect that in the callback that I get the current selection, as it is for example in the case of the import matplotlib.pyplot as plt
from matplotlib.widgets import RectangleSelector
def onselect(eclick, erelease):
print("onselect callback triggered")
print("Press event: ", eclick.xdata, eclick.ydata)
print("Release event: ", erelease.xdata, erelease.ydata)
fig = plt.figure()
ax = fig.add_subplot()
ax.plot([0, 10], [0, 20])
rs = RectangleSelector(ax, onselect, interactive=True) will give the following output: onselect callback triggered
Press event: 2.493951612903226 15.32142857142857
Release event: 5.709677419354839 5.142857142857142
onselect callback triggered
Press event: 5.6875 10.261904761904761
Release event: 6.818548387096775 10.142857142857142 The last press and release events can't be used to retrieve the data selection. |
This comes back to my initial question. To make it conditional on |
In my opinion, using the extent makes more sense and is consistent with the |
Related to my comment in #9608 I am 👎🏻 on merging this as is. I think @ericpre 's point about the interteractive case in very strong that in interactive mode you expect to get back the corners of rectangle, not necessarily the point the user just clicked on (eg, when they dragged on the edge) as the thing of interest is the "rectangle area". By just returning the point the user release on they won't get that anymore. A case could be made for remembering the "first" corner and the "release" corner on the initial selection and then normalizing to that so As I mentioned in #9608 I think the best course of action here is to
@rowlesmr Even though we are having a mostly critical response to your work, please do not be discouraged! We appreciate your time and effort and hopefully will end up someplace that is better than either the status quo or your initial proposal. |
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.
see long comment is inline.
The main concern is that the onselect
calls from interactive usage (when the user drags the handles around) is still gives the full rectangle.
Anyone can dismiss this review.
I've moved to draft but feel free to move back... |
Hi @rowlesmr - are you interested in continuing this work? If so, please mark the PR as ready for review and let us know if you need help with the rebase, we have instructions for that here. Thanks! |
PR Summary
Fix for #9608
"Normalisation" of the click and release mouse button events for the RectangleSelector is removed. The click and release mouse button events reported by the onselect function now reflect the actual press and release coordinates, and not the extent of the rectangle.
If the extent of the rectangle is required, then extent should be explicitly called.
The test_widgets test was updated to reflect the change in behaviour of the release event. Previously, the extent was used, now the actual release coordinates are used.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and 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).widgets.py definitely needs work on style conventions - about 200 lines of output on python -m flake8 --docstring-convention=all C:\Users_____\Documents\GitHub\matplotlib\lib\matplotlib\widgets.py