search

Leaflet / Leaflet.VectorGrid

Display gridded vector data (sliced GeoJSON or protobuf vector tiles) in Leaflet 1.0.0
598 stars 194 forks source link

Use _map._animateToZoom only if zoom is animating in _isCurrentTile #179

Closed mattesCZ closed 6 years ago

mattesCZ commented 6 years ago

Map property _animateToZoom is not reset after animation, so _isCurrentTile() check could return false after map.setView() without animation. As I can see in Leaflet source, Map property _animatingZoom is truthy only if animating (reset in Map._onZoomTransitionEnd) so we can rely on it when checking _isCurrentTile().

IvanSanchez commented 6 years ago

Which problem does this exactly solve? Is there an open issue about this?

mattesCZ commented 6 years ago

No, there isn't. I've only found a bug and created pull request. Problem is explained in the first comment/description. Should I open one?

mattesCZ commented 6 years ago

Oh sorry, it's definitely related to #173 And I think it fixes this.

IvanSanchez commented 6 years ago

Should I open one?

It depends on the project and the project maintainers and the scope of the bug. If it was a typo in a text file or in a comment, then it's trivial and not needed. But for something like this, I feel better if I can see the bug for myself (preferably in a fiddle/codepen/plunkr/etc), and how the bug goes away when monkey-patching the fix.

I mean, you seem to have tracked down a tricky combination of conditions, and that gives me reassurance that the fix might be good. But actually seeing the bug go away would give me more reassurance. On those regards, the How To Report Bugs Effectively essay has very good advice.

IvanSanchez commented 6 years ago

Oh sorry, it's definitely related to #173 And I think it fixes this.

Oh, that's very good to know :+1:

IvanSanchez commented 6 years ago

Oh, I see now, this seems to be an oversight from #151 / https://github.com/Leaflet/Leaflet.VectorGrid/pull/151/commits/ff9e007022621dbc8e1fa4cc0eae9845fd44f38b .

So I guess this fixes blank tiles when performing several zoom operations in quick succession, including fitBounds() calls.

mattesCZ commented 6 years ago

Great! Do you think I should report to the Leaflet repo directly, that both properties Map._animateToZoom and Map._animateToCenter are not reset together with Map._animatingZoom? Leaflet itself is not affected, but some plugins like this one may rely on this...

IvanSanchez commented 6 years ago

Yeah, reporting upstream might be worth it. It should be easy to patch up, too.