Skip to content
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

add source to movestart, move, moveend, zoomstart, zoom and zoomend events #6929

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Athorcis
Copy link

@Athorcis Athorcis commented Dec 4, 2019

It would be very useful to know when the map is moved or zoomed due to user interaction.

So here‘s a pull request which adds a 'source' property to movestart, move, moveend, zoomstart, zoom, and zoomend events.
The source is either user(.[a-z]+)? or imperative(.[a-z]+)?, depending on how was triggered the event.

This pull request is mostly based on this one #5247 which is three years old and was never merged for some unknown reason.

@IvanSanchez
Copy link
Member

As I said before, I'm not convinced by this approach. Something in the back of my brain tells me that there's gonna be some edge case where an event handler calls another event handler and things go inconsistent.

This would be better with some unit tests, specifically in the parts where we test user interaction via touch and mouse drag, e.g. #2934 (comment). The code in the PR looks good, but unfortunately doesn't give me any assurance that it'll work.

@Athorcis
Copy link
Author

Athorcis commented Dec 22, 2019

Wait before merging this pull request, I want to refactor these changes, instead of adding a parameter to a lot of methods, I'm going to use the existing parameter options. This way all the plugins which redefine methods of Leaflet will continue to work.

@johnd0e johnd0e marked this pull request as draft April 16, 2020 12:39
@rdev-software
Copy link

Any update on this ?

@rdev-software
Copy link

I've merged it on my fork, and it works fine.
If there is any other approach how to detect user, please let me know

@wieringen
Copy link

Any update? Would be nice to have this feature.

@AnthonyZink
Copy link

Any updates on this ? Would be great !

Copy link
Collaborator

@johnd0e johnd0e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just random thoughts:

@@ -22,12 +22,12 @@ import * as DomUtil from '../dom/DomUtil';

export var PosAnimation = Evented.extend({

// @method run(el: HTMLElement, newPos: Point, duration?: Number, easeLinearity?: Number)
// @method run(el: HTMLElement, newPos: Point, duration?: Number, easeLinearity?: Number, source?: String)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mention in description

// Sets the view of the map (geographical center and zoom) with the given
// animation options.
setView: function (center, zoom, options) {
setView: function (center, zoom, options, source) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps better include into existing options

// Sets the zoom of the map.
setZoom: function (zoom, options) {
setZoom: function (zoom, options, source) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, here and in a many other places

@johnd0e
Copy link
Collaborator

johnd0e commented Nov 6, 2021

And all this should be covered by tests..

@Malvoz Malvoz added the feature label Apr 19, 2022
@Malvoz Malvoz linked an issue Apr 19, 2022 that may be closed by this pull request
@nasainsbury
Copy link

Any update on this? Very interested and happy to investigate, wasn't sure if this is just being disregarded now in favour of other methods of tracking event source?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User-initiated zoom events?
9 participants