Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Planning version 0.8 #405

Closed
GregorySech opened this issue Aug 29, 2019 · 12 comments
Closed

Planning version 0.8 #405

GregorySech opened this issue Aug 29, 2019 · 12 comments

Comments

@GregorySech
Copy link
Contributor

This issue is to discuss priorities in the next version of the plugin.
The issues selected here should not preclude any PR merge, it's just a discussion to select which issues are necessary for getting at 0.8 release.

@GregorySech
Copy link
Contributor Author

GregorySech commented Aug 29, 2019

IMO the discussion should start from #174 .
Getting this right would at least get the package in the right track to address some other issues, even close some.
For sure this (probably) breaking change should be done as soon as possible, the longer the wait the harder it will get.

The one I'm seeing as related and why:

The problem is that this change needs to be well thought out as getting this wrong would be nasty.

edit: writing is hard.

@GregorySech
Copy link
Contributor Author

Should 0.8 remove isUserGesture as said here #241 ?

@castaway
Copy link

castaway commented Sep 5, 2019

#389 already removed it according to changelog

@GregorySech
Copy link
Contributor Author

You are completely right.

@escamoteur
Copy link
Contributor

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

@q869939686
Copy link

#177

@angel1st
Copy link

Hopefully #458 would be taken under consideration when 0.8 is planned.

@devmgs
Copy link

devmgs commented Nov 11, 2019

Any plans for #412

@johnpryan
Copy link
Collaborator

Thanks for the input everyone - 0.8.0 has been released (I bumped the minor version since Flutter 1.12 was just released).

I would like everyone to keep in mind that flutter_map is a community-maintained project. If you want a feature to be added, the best way is to make a pull request. The second-best way is to make a file a very clear and detailed issue, preferably with a link to the Leaflet equivalent, since this project is based on Leaflet.

If you are blocked by me, I'm happy to add you as a collaborator. Reach out to me and give me some examples of some pull requests or issues that you've made. the Kanban board is what I would like to use to track things. If you're interested in working on an issue, call it out in a comment and I will assign you.

happy flutter_mapping!

@escamoteur
Copy link
Contributor

@johnpryan I'm actually a bit disappointed that I didn't get any feedback on my proposal to improve the overall architecture from you.

@johnpryan
Copy link
Collaborator

@escamoteur I think it got a little lost in the shuffle. Maybe you could file a new issue and link to it here so that people can give feedback

@escamoteur
Copy link
Contributor

#490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants