Thoughts on refactoring architecture

[escamoteur]

Hi,

I implemented my own map view recently because we needed a completely different coordinate system for this app but I looked a lot at yours to get inspirations.
One thing that I always didn't like was the way the Layers were created with flutter_map and I decided here to go a different way which I want to show you

This here is my mapview (I removed unimportant stuff):

class LocalMapView extends StatefulWidget {
  final LocalMapOptions options;
  final List<LayerDefinition> layers;

  const LocalMapView({Key key, this.options, this.layers}) : super(key: key);
  @override
  LocalMapViewState createState() => LocalMapViewState();
}

As you can see I add factories for my layers als parameter here. LayerDefinition id defined:

abstract class LayerDefinition {
  Widget createLayer({
    LocalMapOptions options,
    CustomPoint<double> centerInMeter,
    double scale,
    CustomPoint<double> canvasSize,
  });
}

abstract class Layer  {
  final LocalMapOptions mapOptions;
  final CustomPoint<double> centerInMeter;
  final double scale;
  final CustomPoint<double> canvasSize;

  Layer(
      {Key key,
      this.mapOptions,
      this.centerInMeter,
      this.scale,
      this.canvasSize});
}

and for the different layers you derive a new LayerDefinition which can get any additional parameters like:

class PolylineLayerDefinition implements LayerDefinition {
  Stream<List<Polyline>> polylineUpdates;

  PolylineLayerDefinition({this.polylineUpdates});
  @override
  Widget createLayer(
      {LocalMapOptions options,
      CustomPoint<double> centerInMeter,
      double scale,
      CustomPoint<double> canvasSize}) {
    return PolylineLayer(
      polylineUpdates: polylineUpdates,
      canvasSize: canvasSize,
      centerInMeter: centerInMeter,
      mapOptions: options,
      scale: scale,
    );
  }
}

This approach is much cleaner and more direct than the current one with a big switch to check which type the LayerOption is and then call a factory function.
By this I can easy pass in a Stream to forward any data changes only the Layer it concerns instead to rebuild the whole map_view.

In the map_view build function it looks like this:

      var layerStack = Stack(
        children: widget.layers
            .map((layerDef) => layerDef.createLayer(
                options: options,
                canvasSize: canvasSize,
                centerInMeter: centerMeter,
                scale: scale))
            .toList(),
      );

      Widget mapRoot;

      if (!options.interactive) {
        mapRoot = layerStack;
      } else {
        mapRoot = GestureDetector(
          onScaleStart: handleScaleStart,
          onScaleUpdate: handleScaleUpdate,
          onScaleEnd: handleScaleEnd,
          child: layerStack,
        );
   return mapRoot;   
}

When using this MapView it looks like this:

              return LocalMapView(
                options: LocalMapOptions(
                  initialCenter: LatLng(map.latitude, map.longitude),
                  mapCenter: LatLng(map.latitude, map.longitude),
                  initialScale: 0.5,
                  maxScale: 2,
                  minScale: 0.05,
                  mPerPixel: map.mpp,
                  initialRotation: 0,
                  mapWidthPixel: map.width,
                  mapHeightPixel: map.height,
                ),
                layers: [
                  TileLayerDefinition(
                    tileImageProvider: mapTileService.getTile,
                  ),
                  PolygonLayerDefinition(
                      polygonUpdates:
                          polyStreamController.stream.asBroadcastStream()),
                  MarkerLayerDefinition(
                    markerUpdates:
                        markerStreamController.stream.asBroadcastStream(),
                  ),
                  PolylineLayerDefinition(
                    polylineUpdates:
                        lineStreamController.stream.asBroadcastStream(),
                  ),
                ],
              );

I just wanted to show this as a starting point if the package could move in this direction instead of moving all to plugins. The direct passing of LayerFactories is so much easier to understand and more powerfull than this here:

  Widget _createLayer(LayerOptions options, List<MapPlugin> plugins) {
    if (options is TileLayerOptions) {
      return TileLayer(
          options: options, mapState: mapState, stream: _merge(options));
    }
    if (options is MarkerLayerOptions) {
      return MarkerLayer(options, mapState, _merge(options));
    }
    if (options is PolylineLayerOptions) {
      return PolylineLayer(options, mapState, _merge(options));
    }
    if (options is PolygonLayerOptions) {
      return PolygonLayer(options, mapState, _merge(options));
    }

Which requires that all Layer types have to use the same parameters and to pass any other options over an Options object.

Please don't get me wrong, you created an awesome package. Perhaps we can improve the architecture a bit.

Cheers
Thomas

[escamoteur]

@johnpryan I know you want a new maintainer, but do you think it makes sense to follow up to this?

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[escamoteur]

Would like to see this disucssed

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[escamoteur]

any chance that we talk about this again @johnpryan

[ibrierley]

Couple of questions, as I'm not an expert on the package, but not many replies!

1. Would any changes make previous code and plugins etc incompatible (not nec a breaker, but it does need some very careful thought if so).
2. I think with streams you can update just one layer now already (I may be wrong though) ?
3. I look at the example and to me it's not any clearer, but the original isn't either :D. Maybe if you provided a complete example code (eg one thats in the examples folder that uses layers... maybe the scalebar layer plugin?)
4. Does it make it more difficult for either the maintainers, or people using the package.
5. What will users be able to do after that they couldn't before, in simple terms.

Probably a separate side point, I think you can use your own coordinate system as it currently, but I may be wrong ?

I think just if there's rearchitecturing things, the gains must be very obvious.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.