Major State Refactoring

[rorystephenson]

This PR can be considered exploratory, prompted by #1547. I think overall the code quality is significantly improved but there are some major breaking changes so I want to see what other contributors think about these changes.

It's late where I am so I will try to update this summary better tomorrow or in the next couple of days but for now some highlights are:

• Immutable FlutterMapState: This will make testing far easier.
• Dropped children/nonrotatedChildren in favour of a builder. This resolves the issue of not being able to interlace rotated and non rotated layers. That said it's obviously a big breaking change although it's quite easy to migrate (see changes to examples). Could investigate re-adding children/nonrotatedChildren as deprecated options if we feel that's necessary.
• All changes to the map state are now managed by FlutterMapStateController.
• General tidy-ups.
• Added MapController.of(context) which plugins can use.
• Many map events now pass the FlutterMapState before & after the event instead of just the zoom/latLng.

Some more potential improvements which I haven't tackled yet, some of which are probably for another PR:

• More tidying up of the gesture code (splitting monster methods in to smaller more understandable ones).
• Replace methods in MapController that proxy to FlutterMapState with a getter for FlutterMapState.
• Make the InheritedWidget an InheritedModel with aspects (FlutterMapState/MapController)
• Consider whether MapOptions should be removed from FlutterMapState. This would allow us to expose it as a separate aspect if the previous point about InheritedWidget is implemented. It would also reduce builds caused by options changes which don't end up affecting the state.
• Consider merging PositionedTapDetector2 code in to the FlutterMapInteractiveViewer. This would mean we just have one gesture detector (raw gesture detector).

[JaffaKetchup]

Sure, I think that's probably a good idea! I've summarized in #1553, and we can continue the discussion there in the meantime.
In response to the above question (this is also in #1553), why not just swap the NonRotatedLayer and Container around? We could also offer both options for maximum flexibility - if it's the user's custom descendant, they can use the mixin, otherwise they can use the wrapper.

[JaffaKetchup]

On another note, is this ready for a preliminary review, or did you have more short-term plans?

[JaffaKetchup]

Just a few little nitpicks and questions, haven't gone through in detail yet. Feel free to argue if you think otherwise, I've probably misunderstood something.

[rorystephenson]

On another note, is this ready for a preliminary review, or did you have more short-term plans?

Actually I think I'm going to work on the InhertiedModel change so I would say wait for now.

I do, however, have a question which as arised from refactoring.

In MapOptions what is the difference between bounds, swPanBoundary/nePanBoundaryand maxBounds.

My rough understanding right now is that swPanBoundary/nePanBoundary are only for use when the adaptiveBoundaries is enabled. However the difference between bounds/maxBounds is confusing me.

[ibrierley]

I think...

bounds is basically fitBounds (i.e on initialisation), i.e make the map fit as best possible to the bounds provided, but doesn't restrict during future pan/zoom iirc.

maxbounds is basically, don't show outside these bounds at any point at all (to eliminate grey areas outside a map area for example on init and during pan/zooms).

I think you're right on the pan ones, but it's a while since I've tried, so may need to double check the code for the adaptive boundaries thing (unless anyone else can remember specifically).

[rorystephenson]

bounds is basically fitBounds (i.e on initialisation), i.e make the map fit as best possible to the bounds provided, but doesn't restrict during future pan/zoom iirc.

Ah yep looks like you're right, it would be worth renaming this/center/zoom to initialBounds/initialCenter/initialZoom (with deprecations) as it's quite confusing.

maxbounds is basically, don't show outside these bounds at any point at all (to eliminate grey areas outside a map area for example on init and during pan/zooms).

Hmm this is where it gets confusing for me because that's what I though adaptive bounds is supposed to do?

[JaffaKetchup]

It's explained in the docs, and I'm not really a fan of making breaking changes just for naming, but that will affect all users, at the moment.

Tbh, I can't remember what the point of adaptive boundaries was. I think it was based on screen size instead of coordinates, or something like that, but I have no idea.

[ibrierley]

I think when people get confused, it's always worth revisiting what the concepts are, just in case there are tweaks in method names or documentation etc. After all if people ask, there's a reason, I certainly forget :D.

I "think" there's separate things here though, i.e a boundary of the center you can drag to, vs a boundary of the edges displayed.

[rorystephenson]

It's explained in the docs, and I'm not really a fan of making breaking changes just for naming, but that will affect all users, at the moment.

I tried setting a bound limit on a personal project of mine not long ago but didn't bother in the end because bounds wasn't working and I couldn't be bothered jumping in to the code to figure out why. I also remember when I first used FlutterMap a few years ago I was confused why changing center/zoom did not update the map. I think if the changes are made with appropriate deprecations it's a trivial change for users.

---

As far as adaptive versus max bounds I've never used either so I'm also a bit lost.

This is the relevant part of FlutterMapState's move() method which uses these values:

flutter_map/lib/src/map/state.dart

Lines 406 to 420 in 79b54e4

	if (isOutOfBounds(newCenter)) {
	if (!options.slideOnBoundaries) return false;
	newCenter = containPoint(newCenter, _center);
	}
	if (options.maxBounds != null) {
	final adjustedCenter = adjustCenterIfOutsideMaxBounds(
	newCenter,
	newZoom,
	options.maxBounds!,
	);
	if (adjustedCenter == null) return false;
	newCenter = adjustedCenter;
	}

It uses them as follows:

1. It uses isOutOfBounds() to determine if we are out of bounds. isOutBounds relies on the adaptive bounds or, if they are not defined, ne/sw pan boundaries to determine if we are out of bounds. If outside of bounds and...
   a. If slidingBoundaries is disabled the move is aborted.
   b. Otherwise it uses containPoint() to move the center so that we are inside the bounds. containPoint uses the adaptive bounds, if they are defined, or ne/sw pan boundaries to determine the new center.
2. If maxBounds is defined adjustCenterIfOutsideMaxBounds() will try to find a center position which places us inside the maxBounds. If it fails to find one (maybe because we are zoomed too far out) the move is aborted.

So we seem to have two different ways of trying to stay within the bounds. The next thing to figure out is why there is two and if we can combine them in to one.

[rorystephenson]

@ibrierley looks like you're right from an old comment of yours: #177 (comment)

So I will integrate the various bounds options in to a single bounds option to make this clear.

[ibrierley]

As always, I forget what I wrote in the past :D

[rorystephenson]

So adaptive bounds come from this PR and they were added to stop the visible bounds from going outside of the defined LatLng bounds. This was initially intended for an asset tile provider to prevent loading tiles for which there is no tile asset (if only within a certain range were downloaded).

maxBounds was added a couple of years later in this PR. It was added to get rid of the grey borders when the map is zoomed out.

These options were added for different reasons and, whilst I'm not 100% certain, they seem to essentially achieve the same thing: prevent viewing of coordinates outside of the defined LatLng boundary. The algorithms are different although they may be different ways of achieving the same thing, some testing will be needed to determine that and choose a preferred one.

I did notice that maxBounds doesn't prevent seeing the gray borders when rotating (tested using the flutter_map demo, mobile web, home page). This may also be the case with adaptive boundaries but i'll need to modify the sliding map example to determine that because I don't see missing tiles but I'm not sure if the asset tiles finish where the provided adaptive bounds finish. In both cases some digging will be necessary to determine if there is a problem with the algorithm or something else (e.g. bounds checking isn't done whilst rotating, only when moving).

P.S.
I had a laugh when I noticed that all three of us have been confused by bounds before, see the comments from this comment onwards.

P.P.S
@JaffaKetchup the web demo is super slick and it's also really useful for quickly checking my changes against the previous version, thanks for setting it up.

[ibrierley]

Hah yes, it's always been a bit confusing, but in some ways I'm not surprised, as it's not even always clear when talking about it :D.
So nothing like actually testing it to see...and I was kinda surprised actually haha. You're kind of right, but the implementation has different effects, so they aren't quite the same afaics.

So firstly, maxBounds aim was to avoid grey areas and pretty much match leaflet.js implementation of maxBounds (hence why it's called that, and something not a bit more clear), i.e https://leafletjs.com/reference.html search for maxBounds "When this option is set, the map restricts the view to the given geographical bounds, bouncing the user back if the user tries to pan outside the view."

swPanBoundary (with adaptive off), seems to keep the center constrained to the boundary. Indeed adaptiveBoundaries true then appears to have the aim of keeping the visible area constrained, and it works similar, but if you zoom out for example, you will be able to see outside of the pan area (but can't pan). So it is more of a literal pan boundary, rather than a pure visible boundary.

I also note in the original PR for adaptiveBoundaries it says "Caching a few tiles outside the boundaries is recommended for cases where the user is allowed to zoom out so much that the entire area fits on the screen." so the implication is that it won't limit in the same way.

Here's an adjusted sliding_map example with markers draw at the boundary, just pop this in place of the existing example one in examples folder, and comment/uncomment the relevant lines around 32 & 39 to swap between them.

I think I did test originally maxBounds with rotation, but annoyingly my mobile is bust to be able to tell properly whether this is the case (kinda hard to tell with just a manual force), so it would be good for someone to test that at least and see if it does work or not, and whether I'm imagining it :D.

It's slightly annoying that there are two quite similar features, I suspect both are used, but may be wrong.

import 'package:flutter/material.dart';
import 'package:flutter_map/flutter_map.dart';
import 'package:flutter_map_example/widgets/drawer.dart';
import 'package:latlong2/latlong.dart';

class SlidingMapPage extends StatelessWidget {
  static const String route = '/sliding_map';

  const SlidingMapPage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(title: const Text('Sliding Map')),
      drawer: buildDrawer(context, route),
      body: Padding(
        padding: const EdgeInsets.all(8),
        child: Column(
          children: [
            const Padding(
              padding: EdgeInsets.only(top: 8, bottom: 8),
              child: Text(
                  'This is a map that can be panned smoothly when the boundaries are reached.'),
            ),
            Flexible(
              child: FlutterMap(
                options: MapOptions(
                  center: const LatLng(56.704173, 11.543808),
                  //minZoom: 12,
                  //maxZoom: 14,
                  zoom: 15,
                  //comment out these 4 lines
                  swPanBoundary: const LatLng(56.6877, 11.5089),
                  nePanBoundary: const LatLng(56.7378, 11.6644),
                  adaptiveBoundaries: true,
                  slideOnBoundaries: true,


                  //comment out this line
                  //maxBounds: LatLngBounds(const LatLng(56.6877, 11.5089),
                  //    const LatLng(56.7378, 11.6644)),

                  screenSize: MediaQuery.of(context).size,
                ),
                children: [
                  TileLayer(
                    urlTemplate:
                    'https://tile.openstreetmap.org/{z}/{x}/{y}.png',
                    userAgentPackageName: 'dev.fleaflet.flutter_map.example',
                  ),
                  MarkerLayer( markers: [
                    Marker(
                      point: const LatLng(56.6877, 11.5089),
                      width: 64,
                      height: 64,
                      anchorPos: AnchorPos.align(AnchorAlign.center),
                      builder: (context) => const ColoredBox(
                        color: Colors.lightBlue,
                        child: Align(
                          alignment: Alignment.centerRight,
                          child: Text('-->'),
                        ),
                      ),
                    ),
                    Marker(
                      point: const LatLng(56.7378, 11.6644),
                      width: 64,
                      height: 64,
                      anchorPos: AnchorPos.align(AnchorAlign.center),
                      builder: (context) => const ColoredBox(
                        color: Colors.lightBlue,
                        child: Align(
                          alignment: Alignment.centerRight,
                          child: Text('-->'),
                        ),
                      ),
                    ),
                  ],
                  ),
                ],
              ),
            ),
          ],
        ),
      ),
    );
  }
}

[rorystephenson]

Thanks for having a look @ibrierley ! So I can confirm that on mobile web using the demo when I zoom all the way out and rotate the grey border is partially visible in the corners. I'd need to have a further look to determine if that's due to the algorithm or because the bounds limiting is not called in all the places is should be (e.g. when calling rotate).

So right now I'm looking to create a MapBoundary base class with the following subclasses:

• VisibleCenterBoundary: This would match the old sw/ne pan boundary implementation so it prevents the visible center from going outside of a given LatLngBounds.
• VisibleEdgeBoundary: This would replace adaptive and max bounds. It would prevent points from outside of a given LatLngBounds from being shown on the map.

I'm thinking that MapBoundary would define methods something like this:

• contains(nonRotatedSize, center, rotation, zoom): Determine if a given center/rotation/zoom is within the boundary.
• minimumZoom(nonRotatedSize, center, rotation): Determine the minimum possible zoom for a given center & rotation. This can be used to determine if a given center/rotation/zoom can be moved to remain within the bounds without zooming (if the desired zoom is >= the minimum).
• clamp(nonRotatedSize, center, rotation, zoom): Determine the nearest center value which would place the visible boundary within this boundary.

I wonder if FitBoundsOptions can also become a subclass of MapBoundary. In fact it seems like it could be combined with VisibleEdgeBoundary by adding the padding, maxZoom, inside and forceIntegerZoomLevel options.

I'm not attached to the names, if we can come up with names which better describe the behaviour I'm all for it.

[rorystephenson]

I was diving in to the various bounds implementations when I noticed #1549 and #1550 which add a new fit bounds method and add a fix for fit bounds, respectively. I'm hopeful that we can create a MapBounds abstraction which can encapsulate all of this. So then rather than adding a new fitCoordinates method to the controller we can have a MapCoordinateBounds subclass with its own implementation for determining if we are within the coordinate bounds and the center zoom required to be inside the coordiante bounds if we are not already.

[JaffaKetchup]

minimumZoom(nonRotatedSize, center, rotation)
clamp(nonRotatedSize, center, rotation, zoom)

What about clampZoom & clampCenter? Makes them kind of related in a way, as they are.

I've merged #1550, feel free to refactor it however.

[jjoelson]

I was diving in to the various bounds implementations when I noticed #1549 and #1550 which add a new fit bounds method and add a fix for fit bounds, respectively. I'm hopeful that we can create a MapBounds abstraction which can encapsulate all of this. So then rather than adding a new fitCoordinates method to the controller we can have a MapCoordinateBounds subclass with its own implementation for determining if we are within the coordinate bounds and the center zoom required to be inside the coordiante bounds if we are not already.

Hi @rorystephenson, I really like the idea of unifying the various types of boundaries and performing boundary checks for rotation!

As far as unifying this with fitCoordinates, I think it's conceptually tricky. The idea behind fitCoordinates is not to define a "bounds" per se, but rather to set the center and zoom so that each of the individual coordinates is visible. In fact, as I'm thinking about this, fitCoordinates's parameter should probably be Set<LatLng> rather than List<LatLng>, because the order of the coordinates or duplicate coordinates have no effect on the result.

Using a collection of coordinates as a map boundary feels like a different and broader problem, and it could mean a few different things:

• It could be the bounding rectangle of the coordinates
• It could be the convex hull of the coordinates
• It could be an alpha shape of the coordinates
• It could be an arbitrary polygon defined by the order of the coordinates

Fitting coordinates doesn't have this type of ambiguity; given a set of coordinates, there's really only one way to fit them on the map.

So if we wanted to try to implement specific coordinate-based boundaries such as ConvexHullBoundary or BoundingBoxBoundary then there might be some opportunity to share logic internally with fitCoordinates. But I'm not sure it makes sense to create a single type like MapCoordinateBounds to try to unify the public API because fitting a set of coordinates and defining a map boundary based on a collection of coordinates are quite different things in my estimation.

[JaffaKetchup]

(

fitCoordinates's parameter should probably be Set<LatLng> rather than List<LatLng>, because the order of the coordinates or duplicate coordinates have no effect on the result.

Whilst that would make logical sense, it's probably fine as it is. It's less work to import from an API or export to a storage medium. Open to argument though.

Using a collection of coordinates as a map boundary feels like a different and broader problem, and it could mean a few different things

Of those options, I feel like bounding box and convex hull are enough for most users - and implementing arbitrary (potentially concave) polygon boundaries feels like it might be a lot of work (not saying a convex hull algorithm wouldn't be).
)

[rorystephenson]

• options.maxBounds needs a deprecation to CameraConstraint.contain
• MapCamera should be exposed through the standard import library, not the plugin API

Done 👍

---

I've also:

• Merged in @jjoelson's camera fit PR which looks great.
• Tidied up some exports/imports and added a lint to keep them alphabetically ordered (99% were already).
• Moved back the CameraFit parameters to the subclasses.

I don't feel strongly about the zoom limits for camera fit, happy to defer to @jjoelson and @Robbendebiene on that.

[jjoelson]

I think where we landed is that it's OK for maxZoom to be an option for all fits, but since applying a maxZoom can cause certain fits (like FitInsideBounds) to fail, then its default value should be a "no-op" value like double.infinity so people don't experience confusing default behavior. Please correct me if I'm wrong @Robbendebiene.

@JaffaKetchup Given that the old fitBounds and FitBoundsOptions continue to exist with their existing default values for backwards compatibility, would it be OK if all parameters for the new CameraFit abstraction have "no-op" values by default? I.e.:

padding = EdgeInsets.zero,
maxZoom = double.infinity,
forceIntegerZoomLevel = false,

I think that would be the best way to avoid unexpected behavior going forward, especially if we imagine that new fit types will be added in the future.

[JaffaKetchup]

@jjoelson Sorry for the slow response, I think having those defaults make sense.

[rorystephenson]

Your proposed defaults sound good @jjoelson , do you want to open a PR or shall I make the changes?

[JaffaKetchup]

Is this ready for a final review, or are more changes still needed?

[rorystephenson]

@JaffaKetchup I've just been upgrading some packages to use this branch:

• flutter_map_marker_popup
• flutter_map_supercluster
• flutter_map_animations

Those deprecation changes were a result of that. Otherwise no other changes were needed for these plugins to work.

So as far as I can see we are just missing the changed CameraFit defaults.

[rorystephenson]

@jjoelson and @JaffaKetchup I've just added a commit which updates the maxZoom default (the rest were already as suggested). I opted for null rather than double.infinity for the default value as I think that the intent is clearer; no maximum. It also makes it more explicit in the calculations.

Let me know what you think. If you're both happy with I think we're ready to review and release a dev version for more testing.

[JaffaKetchup]

LGTM for a preview release, although if bugs are found, a new PR will be needed, @rorystephenson if you're up for that.

Will try to merge the other remaining PRs before publishing the preview, and whilst those are being updated, I'm going to make a couple of changes in a PR as well. Just for reference, here's the v6 plan: https://github.com/fleaflet/flutter_map/projects/3.

[Robbendebiene]

Not sure if this is intentional or if I'm doing something wrong, but it seems like I can no longer access/import FlutterMapState?

Edit: I guess MapController.of(context) and MapCamera.of(context) are the new suggested methods to get/manipulate the state?