Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add touch event handlers to the webagg and nbagg backends#18851
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This also collides with#16931 |
def1394
to58fb0d9
Compareon_touch_event_closure('touch_move') | ||
); | ||
rubberband_canvas.addEventListener( | ||
'touchenter', |
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.
Honestly I've not seen this event (nor touchleave) actually fire. Perhaps I don't have the right kind of device (mobile or chrome developer tools).
anntzer commentedOct 31, 2020 • 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.
From a very quick look, it seems like on the GUI toolkits side (especially gtk, from which we got the event names (button_press_event, etc.), but also the others) typically have a single touch_event handler, and then let the single callback differentiate based on some flag present on the event; unless there's a strong reason to do things differently here I'd suggest also sticking to just It may also be nice to review how the various GUI toolkits handle multitouch (which is relevant for zooming), keeping in mind that we'd like to keep the API the same across all backends (except for the guiEvent attribute which refers, well, to the native event). Again, from a quick look:
So I guess another question would be whether to expose at all the low-level touch events or the higher level gestures in Matplotlib (after all the actual event handlers may be easier to write against a gesture API?) |
Thanks for taking the time to look over the various touch event options across the backends@anntzer. I definitely agree with the idea of standardising on something which is consistent across the backends, and it isn't a given that the JS approach is the one we should necessarily opt for. In terms of implementation, it doesn't change a huge amount I think - we just need to make a decision and then it can be implemented. How would you like to proceed with making the decision - I definitely don't have enough experience of touch events across backends to be able to make an informed decision that doesn't risk precluding certain types of touch event handling. |
I don't actually know anything about touch event handling other than what I listed above (which basically just comes from some quick googling...). So I don't have much to contribute on the API design side. However, once again I think that even if you don't provide implementation (of whatever API you choose) for all backends, it would be nice to at least be sure that everything you propose is at least in theory implementable across them. |
timhoffm commentedNov 23, 2020 • 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.
I'm also not experienced with touch events. But from a usability point of view, a gesture API is simpler to use. And it's simpler to get right because it's a higher level abstraction that's less dependent on the low-level API of the frameworks (we can wrap that). We can always add an additional low-level touch API later if needed. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
PR Summary
This PR compliments#8041 to add touch support for the webagg and nbagg backends. This will also unlock the door for the ipympl backend to be able to implement touch handlers for free, should they so wish.
I didn't want to tread on#8041's implementation, and believe it is healthy to keep a strong distinction between new backend behaviour and introducing an entirely new class of event handlers (something that perhaps should be done for the original PR too), so whilst the events are being fired and sent to the backend, they currently don't do anything (not even fire any matplotlib event). So in other words, this is a non-zero change for zero immediate benefit, and I'm completely willing to accept that we must have the backend_bases event infrastructure in place before merging this PR.
This also fixes a bug which prevents the webagg from running in device compatibility mode in chrome (Developer Tools -> Toggle device) in which there was a problem with the websocket having not yet been connected. The error message there was:
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).