Added MapController.fitCoordinates method

[jjoelson]

Hi all! With this PR I'm proposing to add a new map controller method called fitCoordinates.

When we have a list of lat/lng coordinates and want to fit the map viewport to those coordinates, the current best practice is to first calculate a LatLngBounds, and then use fitBounds:

final coordinates = <LatLng>[...];
final bounds = LatLngBounds.fromPoints(coordinates);
mapController.fitBounds(bounds);

With this PR, we can instead write:

mapController.fitCoordinates(coordinates);

The origin of this PR is that I was trying to implement a fix for #1342 to make fitBounds work correctly when the map is rotated. I implemented this fix which was proposed by @JosefWN but the result was a bit surprising to me:

I was puzzled by the additional padding to the left of the green marker, until I remembered fitBounds is fitting the LatLngBounds, not the 4 coordinates themselves. Given LatLngBounds can only represent a rectangle bounds in a north-up orientation, then what is happening here is correct; that extra margin on the left is required to fit the northwestern most point of the bounds. If we draw a black rectangle polyline that represents the LatLngBounds of the four points, then we can see it's working as it should:

So I realized fitBounds, even if corrected to work with rotation, is not what I actually need, and is probably not what most users of this package need when trying to fit a list of coordinates in the viewport when the map is rotated. Rather, I needed a method like fitCoordinates that fits specific coordinate points, regardless of rotation. With fitCoordinates, the result looks like this:

Rather than fitting a north-up bounding box of the coordinates, fitCoordinates fits the coordinates themselves.

I plan to add tests and make some minor tweaks to the code to avoid some duplicate computations, but I wanted to post this PR before putting in too much more work so I can get feedback from the experienced maintainers of this package about this idea. Am I correct that fitCoordinates does something useful and different than fitBounds when the map is rotated?

[JosefWN]

Hi @jjoelson!

Yes, you are spot-on. I only found the bug in fitBounds, but just like you, I realized that fitBounds might not be all that good for coordinate/vector-based layers. I have since been pretty busy in other projects, but I'm glad you have come up with a solution. I'm off to bed now, but will have a look at your PR when I wake up!

[jjoelson]

Hi @jjoelson!

Yes, you are spot-on. I only found the bug in fitBounds, but just like you, I realized that fitBounds might not be all that good for coordinate/vector-based layers. I have since been pretty busy in other projects, but I'm glad you have come up with a solution. I'm off to bed now, but will have a look at your PR when I wake up!

Great, thank you!

While writing tests I realized that the calculations to handle padding are not quite correct, so I'm going to push an update at some point today or tomorrow to address that.

[JosefWN]

Okay, I will have a look a bit later then!

Oh, and if you have the fixed fitBounds-method, feel free to make a PR with that too (or I can do it, but my fork looks a bit different so I would need to port my fix). The fitBounds-method is still relevant for raster layers etc.

[jjoelson]

OK, I pushed an update with the fixed padding computation and tests. I have two questions:

• Currently fitCoordinates uses FitBoundsOptions, as none of the properties in FitBoundsOptions seem to be specific to the bounds use-case. But would it make sense to create a separate FitCoordinatesOptions class in case there's a need to add use-case specific properties in the future?
• Are there any other things that need to be updated as part of this PR e.g. documentation, examples, changelog, etc.?

Oh, and if you have the fixed fitBounds-method, feel free to make a PR with that too

I should be able to put up a separate PR for that some time next week. 👍

[JaffaKetchup]

Hey @jjoelson, apologies for not popping in sooner!

Thanks for the contributions so far! Fixes and improvements are always appriciated.
To add something sort of potentially related: #1342 may have related?

Just to let you know, the checks appear to be failing atm due to an unnecessary import.

Are there any other things that need to be updated as part of this PR e.g. documentation, examples, changelog, etc.?

Don't worry about documentation or changelog, I do that before a release. If you can add an example, please do!

Currently fitCoordinates uses FitBoundsOptions, as none of the properties in FitBoundsOptions seem to be specific to the bounds use-case. But would it make sense to create a separate FitCoordinatesOptions class in case there's a need to add use-case specific properties in the future?

If they can share now, they should share now. However, the class does need some improvements in terms of documentation, clarity, and ensuring consistency.

[jjoelson]

Hey @jjoelson, apologies for not popping in sooner!

No problem at all!

I pushed an update this branch to fix the unused import, and I put up a second PR to update fitBounds: #1550

[rorystephenson]

Hey @jjoelson, nice work on this PR and the fitBounds fix!

I was looking at bounds related code in #1551 before I noticed these two PRs. We have three different ways of expressing map bounds right now (four with your coordinate bounds) and I was hoping we could clean that up by creating a MapBounds base class which the different bounds implementations can subclass. I mentioned it in this comment but disregard the suggestions for the specific methods as I'm starting to think they don't make sense. I think we have the following variations of map bounds (I've given them names for convenience, the names can change):

• VisibleCenterBounds - Map bounds that limit the center point of the map.
• VisibleEdgeBounds - Map bounds that limit the visible edges of the map, i.e. prevents coordinates from outside of a defined LatLng bounds from being viewed. This is useful for preventing grey borders when zooming out or stopping missing tiles from being loaded when using asset tiles.
• FitBounds - bounds that should contain LatLng bounds, with optional padding etc.
• CoordinateBounds - Map bounds that should contain a list of coordinates (your addition).

These would all subclass MapBounds and expose relevant methods to detmine if we are inside the bounds, CenterZoom, etc. This would allow us to use a single method in the controller for fitting the map to any of these different bounds. MapBounds would also be useful in MapOptions:

• bounds (which is initial bounds) could be a MapBounds instead of just a LatLngBounds.
• maxBounds/adaptive bounds/swPanBoundary/nePanBoundary would all become a single option (perhaps maBounds).

If you think this is doable and a good idea, maybe you could help me determine what methods MapBounds would need to expose?

EDIT:
It may be enough for the MapBounds subclasses to just implement a centerZoom method which returns a CenterZoom? given crs, center, zoom, rotation, nonRotatedSize. If the CenterZoom's center is null movement is abandoned (this can be the case if we are outside of bounds and sliding is disabled (see slideOnBoundaries options). Otherwise we use the returned center/zoom.

[JaffaKetchup]

(I've converted this to a draft due to it's conflicting interaction with #1551. @rorystephenson Any commits that integrate this change, if this PR is no longer needed, please add @jjoelson as co-author and mention this PR ID in commit message to give appropriate credit.)

[JaffaKetchup]

Replaced by #1551. Thanks @jjoelson for your contribution!