Re-expose MapControllerImpl

[JosefWN]

Simple proposal for #1544.

In line with v4.0.0 behavior. Tried and it works with my custom map controller.

[TesteurManiak]

LGTM! @JaffaKetchup was there any specific reason to make this constructor private in v5?

[JaffaKetchup]

@TesteurManiak No reason other than cleanup of external APIs.

Btw, doesn't it seem redundant to even have this separation anyway (between impl and standard), if they're always exposed together? If they're not merged, we need to copy the docs across to ensure better docs coverage.

[TesteurManiak]

Yeah the separation between MapController and MapControllerImpl is redundant to me too. I don't see why they couldn't be merged into one MapController class, that would probably make more sens.

[JaffaKetchup]

I think that should be done then.

[JosefWN]

Not sure about the abstract class as a whole, but this part doesn't make much sense:

/// Factory constructor redirects to underlying implementation's constructor.
factory MapController() = MapControllerImpl;

Normally only an implementation needs to know about its abstract class in order to fulfill the constraints imposed by it, whereas the abstract class should assume no knowledge of the classes implementing it. This assumption has several upsides: it makes the intent expressed in the code more clear (MapController() creates an arbitrary map controller vs MapControllerImpl() creates just that), and it makes the implementation itself simpler, since the package maintainers are likely not aware of when to choose which map controller, or even which map controller implementations there are "out in the wild".

Perhaps it's overkill to have an interface (abstract class), if we anticipate that large parts of the controller will remain the same regardless of implementation. Another potential solution could be to remove the factory constructor in the abstract class. That way you would be able to choose between extending the existing MapControllerImpl or implementing a new MapController without the potentially unrelated excess that comes with MapControllerImpl.

[JaffaKetchup]

@JosefWN I'm not 100% sure what you mean by that I'm afraid!
I don't see why one non-abstract class wouldn't be suitable, which can be extended/implemented by other libraries.

[JosefWN]

I don't see why one non-abstract class wouldn't be suitable, which can be extended/implemented by other libraries.

Abstract classes/interfaces can be implemented and base classes can be extended, but that is typically not the same thing.

For one, if you look at the abstract MapController, it does not contain any form of variables/state, it just specifies methods that need to be implemented. This gives full flexibility to the person implementing the MapController with regards to the internal state. This is not the case if you extend MapControllerImpl, then you will have to live with whatever variables it happens to have, whether you like/need them or not.

For another, the MapControllerImpl (or any other implementation for that matter), may evolve without affecting the MapController interface. The interface would just serve as a least common denominator for all map controllers, with a minimal set of methods required for internal use by flutter_map, rather than being a placeholder for every method of the MapControllerImpl.

I suppose it boils down to whether we think that MapControllerImpl will grow larger over time. If we think it will, then it might be a good idea to keep MapController and MapControllerImpl separate, because merging them would impose needlessly strict and potentially "clunky" constraints on any extension of MapControllerImpl vs an implementation of MapController.

[JaffaKetchup]

I suppose it boils down to whether we think that MapControllerImpl will grow larger over time.

Given that #1549 was only recently opened to add new functionality, I think we have to assume it might change.

because merging them would impose needlessly strict and potentially "clunky" constraints on any extension of MapControllerImpl vs an implementation of MapController.

However, I'm not sure it's needless. Anyway, if we move the implementation methods into the current MapController, and make it a base (abstract?) class, extenders still don't have to implement all methods - they can just override ones important to them, or add new ones? They can also override all getters and variables.

Perhaps I'm missing something. But I do understand that extension/implementation is different.

I would also like @mootw and @ibrierley s' inputs to this!

[ibrierley]

I think I'm kind of indifferent on this one, as I never quite understood why it was designed in the way it was originally, so don't feel that aware to spot any issues.

[rorystephenson]

My 2 cents (sorry it's late here and I don't have time to read this all now):

I prefer to keep the API size as small as possible by only exposing what we want to which in turn makes refactoring much easier. For that reason I am for keeping MapControllerImpl and not exporting it to hides methods/variables we don't want to expose (like the stream sink or the setter for the map state for example). If there is another way to achieve the same thing I'm all for it.

Again I haven't had time to read this or even check the PR changes and it's laaate here so I this may already be addressed.

EDIT:

OK I've had a re-read and I see that the PR is exporting MapControllerImpl. In the refactoring PR I am working on the FlutterMapState becomes immutable and MapControllerImpl is given direct access to another internal controller which manipulates the map state. This is an internal API and exporting the MapControllerImpl would mean opening up this internal API which is definitely a bad idea. In fact I would go in the other direction, making MapController a sealed class. This would mean the only option is to wrap it like @TesteurManiak has done and I think that's the best option.

[JaffaKetchup]

Converting to a draft for now. Let's see how #1551 plays out first!

[JaffaKetchup]

@rorystephenson @JosefWN With #1551 now merged, what needs to happen here (if anything)?

[rorystephenson]

My vote is to close this and leave things as-is and close this. If we can find a more elegant way to allow extending MapController without exposing MapControllerImpl that would be great but for now it doesn't look like it.

[JaffaKetchup]

Maybe we can export impl through the plugins API import? Doesn't make much of a difference, but at least it would be something.

If not, I'm happy to close this.

[rorystephenson]

We don't want the MapControllerImpl to be available even to plugins. It gives access to methods which should only be used by flutter_map internals.

I've upgraded the latest version of flutter_map_animations which wraps the MapController and I think it's an elegant solution. IMO we can close this PR.

[JaffaKetchup]

Ok. We'll close for now, but if anyone has any objections, just ask for this to be reopened.