Control animation in setView/panTo/setZoom/fitBounds/etc.

[mourner]

Implements #1616. Since zooming and panning have different animation mechanisms, I propose the following API:

map.setView(center, zoom, <ZoomPanOptions>);
map.fitBounds(bounds, <ZoomPanOptions>);
map.panTo(center, <PanOptions>);
map.setZoom(center, <ZoomOptions>);
...

ZoomPanOptions:

{
  // true will reset the view to clear state completely (without any animations etc.)
  reset: false,
  zoom: <ZoomOptions>, // will be used for zoom change animation
  // will be used for panning animation (also includes duration, easing etc.)
  pan: <PanOptions>,
  // passing animate will be an equivalent of passing animate to both zoom and pan options
}

Both zoomOptions and panOptions have animate option. Panning behavior:

// not specifying `animate` will cause animated pan if animating not more 
// than a screen away or reset the view if it is
map.setView(center, zoom); 
map.panTo(center);

// animate: true will force it to always animate pan if possible
map.setView(center, zoom, {pan: {animate: true}}); 
map.panTo(center, {animate: true});

// animate: false will turn off animation, just shifting the map pane
// if not more than a screen away, resetting the view otherwise
map.panTo(center, {animate: false});

// panBy is similar to panTo but it never does the pan distance check,
// so it either animates or shifts map pane depending on animate options
map.panBy(offset, {animate: false});

Zooming behavior:

// no `animate` will cause animated zoom if zoom origin is inside the current view
map.setView(center, zoom);

// animate: true will attempt animation disregarding where the zoom origin is
map.setView(center, zoom, {zoom: {animate: true});

// animate: false will always cause full view reset when zooming
map.setView(center, zoom, {zoom: {animate: false});

It may look a bit complicated at first but it's the simplest API I could think of that covers all the use cases.

@danzel @tmcw @jfirebaugh Comments? Suggestions?

[jfirebaugh]

Can we keep compatibility here and when options === true set options = {reset: true}?

[mourner]

We can, but the more general question is whether we should keep any compatibility/legacy code in major releases, or always force users to upgrade the code in breaking places (documenting upgrade thoroughly) and keep the codebase clean. It's hard to compromise between these options, as it's a half-measure. Full measure on the compatibility side would lead to OpenLayers-like API hell, and full measure on the other side is pretty acceptable for Leaflet in my opinion. That's one of the reasons why I didn't release 1.0 yet BTW — I want the API to mature first.

[kamalmarhubi]

You've decided differently in the latest patches, but I would fall on the side of dumping the compatibility. Or at least deprecating the option, to be removed before 1.0. Warn to console?

[mourner]

@jfirebaugh can you comment more on the compatibility here? it it more of a "nice to have" or a dependency in the Mapbox code?

[jfirebaugh]

Looks like we have one example that uses it, but we don't use it directly. Downstream users of mapbox.js could be using it, of course.

[kamalmarhubi]

In that case, how about deprecating it? Warn users calling on the console if they pass in true. This won't affect site visitors, but will be seen by developers.

[jfirebaugh]

Do you have a fooOptions naming convention established anywhere else? If not, I suggest dropping "Options". It's obvious from context that they are options.

map.setView(center, zoom, {pan: {animate: true}}); 

[mourner]

Good suggestion, renamed to pan and zoom.

[danzel]

Do we need zoomOptions and panOptions to be sub-things or could we just put all the options in the main closure?
So:
map.setView(center, zoom, {pan: {animate: true}});
becomes:
map.setView(center, zoom, {animate: true});

Not sure if it is necessary to differentiate between the two. Thoughts?
Based on the above examples I think this would make it simpler, not sure if it would lead to any confusion however.

[kamalmarhubi]

Do we need zoomOptions and panOptions to be sub-things or could we just put all the options in the main closure? [...] Not sure if it is necessary to differentiate between the two. Thoughts?

I can imagine wanting only one or the other of the animations. However I can also see wanting a single flag to turn both on or off for convenience. You could achieve both by combining both approaches, possibly with the more specific animate (or other) flag prevailing, eg

map.setView(center, zoom, { animate: false, zoom: { animate: true } });

would be equivalent to

map.setView(center, zoom, { pan: { animate: false }, zoom: { animate: true } });

and you'd also be able to just do

map.setView(center, zoom, { animate: true });

as you suggest.

Documentation would fairly straightforward: just state the more specific overrides the less specific. Implementation should not be too hard either. Not sure what I think of it, though.

[mourner]

Do we need zoomOptions and panOptions to be sub-things or could we just put all the options in the main closure?

I think we should have the possibility to configure it separately, because zooming and panning have different animations. ZoomOptions is currently barebones (only animate), but PanOptions has some additional options like animation duration, curvature etc. Also ZoomOptions will probably be extended with additional options in future.

I like the suggestion from @kmrhb though. Best of both worlds. The only problem is to document it clearly, as it gets a bit confusing with all the different options...

[mourner]

Also decided to add true as a shortcut for {reset: true} @jfirebaugh asked about, it doesn't hurt much.

[mourner]

So, good to merge then I guess? Any objections?

[kamalmarhubi]

Meant to look over this sooner. Oops. Taking a look now.

[kamalmarhubi]

Maybe pull this out into a user-callable normalizeOptions or similar? This is good for now, but if more properties get added that are shared, would be nice to simplify it. Also allows users to get the behaviour without copy/paste.

[mourner]

I'll rewrite this part when more options get shared in future. No need for normalizeOptions option I think.

[kamalmarhubi]

Reasonable.

I'm new to the codebase, so I'm not clear on the flow: this is the only place you've added this normalization code. Is it the only entry point where the full options object could be passed in?

[mourner]

Yep. All other entry points (such as fitBounds) use setView internally so they pass options through as is.

[mourner]

No further comments, so merging the pull.

[kamalmarhubi]

I had a minor comment above that I don't think were looked at, but it's not important to the API. And API-wise I'm happy! Is there a release scheduled? I'm looking forward to scrapping my _rawPanBy calls.

[mourner]

I think there's no need to comment every simple statement in the code, otherwise it will get overcrowded with comments. If there's a strict equality comparison of an expected object with true, we have a shortcut in place — easy enough in my opinion.

[kamalmarhubi]

Sure, not every comment, but maybe ones that are a little counter-intuitive. My only thought here was that as a newcomer to this code base—or even returning to it after some time—it would look strange, and I would wonder why that was going on; a comment stating it was to maintain backwards compatibility would help. Since the documentation is in the gh-pages branch, and removed from the code, that rationale is not obvious.

Of course, as a newcomer to the codebase, I defer to your style.

[hon]

Is there a way to set animation option's in L.map? One won't invoke setVeiw under some circumstances.