[FEATURE] Re-expose MapControllerImpl after v5.0.0

[JosefWN]

What is the bug?

Since v5.0.0, the FlutterMapControllerImpl does not export its constructors. This makes it hard to use a custom map controller, which I'm doing, and which also @TesteurManiak is doing: https://github.com/TesteurManiak/flutter_map_animations/blob/main/lib/src/animated_map_controller.dart#L21-L25

The member 'MapControllerImpl' can only be used within its package.dart[invalid_use_of_internal_member](https://dart.dev/diagnostics/invalid_use_of_internal_member)

FYI, also ran into this issue with the tests when debugging, but it seems to be something on flutters end: flutter/flutter#127571

How can we reproduce it?

Try to implement a custom map controller.

Do you have a potential solution?

Export the constructor.

Platforms

All

Severity

Fatal: Causes the application to crash

[TesteurManiak]

To me it's not that big of a deal, I'm currently updating flutter_map_animations so it can take an instance of MapController (or instantiate it internally) but it will cause a few breaking changes for my package.

[JosefWN]

Okay, my map controller works a bit differently so it might be tricker for me 🥹

[JaffaKetchup]

Hi @JosefWN,
Not sure why the constructor needs to be exposed? MapController's constructor redirects to it, and it doesn't take any arguments itself. But I'm probably misunderstanding something, so an example would be helpful.

[TesteurManiak]

@JaffaKetchup to give you an example here's my implementation of AnimatedMapController:

Before flutter_map v5

class AnimatedMapController extends MapControllerImpl {
  // ...
}

Which allowed to completely replace MapController:

late final mapController = AnimatedMapController(vsync: this);

@override
void dispose() {
  mapController.dispose();
  super.dispose();
}

@override
Widget build(BuildContext context) {
  return FlutterMap(
    mapController: mapController,
  );
}

After flutter_map v5

Now it acts more like a wrapping of MapController:

class AnimatedMapController {
  AnimatedMapController({
    required this.vsync,
    MapController? mapController,
   // ...
  })  : mapController = mapController ?? MapController(),
        // used to know if the controller should be disposed internally
        _internal = mapController == null;

  // ...
}

late final animatedMapController = AnimatedMapController(vsync: this);

@override
void dispose() {
  animatedMapController.dispose();
  super.dispose();
}

@override
Widget build(BuildContext context) {
  return FlutterMap(
    mapController: animatedMapController.mapController,
  );
}

[JaffaKetchup]

Thanks @TesteurManiak, I understand now. Indeed, this should probably be changed back.

[TesteurManiak]

With Dart 3 MapControllerImpl could use the base class if you want to only allow extension 🤔

[JosefWN]

With Dart 3 MapControllerImpl could use the base class if you want to only allow extension 🤔

I think exposing an interface and requiring adherence just to that interface is a bit cleaner though, and I suspect it provides greater flexibility for the person implementing the map controller? Haven't dug into what the actual difference would be to me though.

[JaffaKetchup]

Side note, if you guys haven't seen this yet, it's very helpful:

[rorystephenson]

Just dropping in quickly here to say that I don't think it's a good idea to expose MapControllerImpl as that class exists to hide internal methods etc that we don't want accessed from outside of the package (in part because that then makes them something we should support and keep stable, part of our API).

I think it's worth weighing up the alternatives (including wrapping the controller as @TesteurManiak does).

[JaffaKetchup]

@rorystephenson Thanks for the input. We've been discussing this over at #1548. If you could pop over there and see if anything discussed there sticks out to you, that would be great!

[JaffaKetchup]

See #1548 for closure reasons.