Skip to content

Conversation

jfirebaugh
Copy link
Member

This causes unnecessary nondeterminism in the sequence of events emitted. Instead, the caller should be responsible for throttling, as e.g. Map#_onResize does.

@mourner Does this make sense? The setTimeout is causing a problem for OSM.org -- sometimes the moveend triggers stuff 200ms later that should have happened synchronously with invalidateSize.

This causes unnecessary nondeterminism in the sequence of
events emitted. Instead, the caller should be responsible for
throttling, as e.g. Map#_onResize does.
@jfirebaugh
Copy link
Member Author

osmlab/openstreetmap-website@9ed8664 is the relevant OSM commit.

@mourner
Copy link
Member

mourner commented Nov 11, 2013

Hmm, that one needs some thought... Some internal Leaflet stuff is happening on moveend as well, such as vector layer clipping and simplification, so that could introduce a performance hit. Also, it would make the event inconsistent with the standard panning behavior — e.g. I'd expect it to behave similarly to panning, fire move during resize movement and moveend when you stop.

@mourner
Copy link
Member

mourner commented Nov 11, 2013

@jfirebaugh can you clarify the problem with the 200ms delay on OSM.org? Perhaps we could explore alternative ways to fix this.

@jfirebaugh
Copy link
Member Author

Yes, the problem is that the OSM.org code keeps track of the map state via history APIs -- there's a moveend event binding that updates the state via replaceState. Meanwhile, when the sidebar is shown or hidden in response to navigation events, we need to invalidateSize. But because invalidateSize fires moveend asynchronously, we have no proper control over which history entry is updated with the new state. It ends up being the wrong one if you click the back or forward button too quickly. Simply put, this use cases depends on invalidateSize firing moveend synchronously.

@jfirebaugh
Copy link
Member Author

fire move during resize movement and moveend when you stop

FWIW, a 200ms timeout doesn't really accomplish that either -- you'll still get intermittent moveend when pausing for more than 200ms while resizing.

@mourner
Copy link
Member

mourner commented Nov 12, 2013

If the hash depends on moveend being sync, why not update it on move instead of moveend? Performance concerns?

@jfirebaugh
Copy link
Member Author

Yeah, move happens a bit too often to update the hash, which flashes a loading indicator and may add a "Full History" entry.

@mourner
Copy link
Member

mourner commented Nov 12, 2013

But then you would fire lots of moveend events while resizing, having the same performance problem.

FWIW, a 200ms timeout doesn't really accomplish that either -- you'll still get intermittent moveend when pausing for more than 200ms while resizing.

It's at least an approximation, which is much better.

I'm sure there's a better solution to this problem, lets think more about other options...

@mourner
Copy link
Member

mourner commented Nov 13, 2013

@jfirebaugh what do you think about this: fire moveend sync by default, but debounce if some special option is specified (which is then passed in resize handler)? Should solve the issue.

@mourner
Copy link
Member

mourner commented Nov 13, 2013

Lets solve this before 0.7 release so that the redesigned OSM.org can use the stable version.

@jfirebaugh
Copy link
Member Author

👍, added a debounceMoveend option.

@mourner mourner merged commit 33263e5 into master Nov 13, 2013
@mourner mourner deleted the invalidateSize branch November 13, 2013 19:51
@mourner
Copy link
Member

mourner commented Nov 13, 2013

Thanks, merged!

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

Successfully merging this pull request may close these issues.

2 participants