[v2.1.0] Fix LateInitializationError with MapController

[Zverik]

There is a setter for a final error, which calls for an exception. And it came!

Fixes #1288.

[JaffaKetchup]

Will try to review at some point soon, quite busy right now. Thanks!

[JaffaKetchup]

@Zverik

I seem to get another error now when I try to interact with the map:

...so not sure this is fixed.

I just used the code in the body of #1288 on the latest version.

Any ideas?

[Zverik]

Yes, the fix is partial, and it highlight the greater issue. For example, we're calling mapController.dispose() in the FlutterMapState.dispose() method, which is a bad practice: controllers should be disposed by their owning objects. Which in case of MapControllerImpl() is actually the FlutterMap, which adds to the problem. I guess the better way would be creating the ...Impl() in the state's initState()...

[Zverik]

Yes, I did a test, and indeed initState() is called with the exactly same controller object (Impl) that was disposed in dispose().

[Zverik]

Okay, I fixed this. Could everybody please test that nothing (including examples) has broken? :)

I used _TextFieldState with its controller handling for a template.

[LeoCreno]

Your branch worked for me !

[JaffaKetchup]

@Zverik Will test this in a minute. Preparing the CHANGELOG for v2.0.0, so hoping to merge this in-time.

[JaffaKetchup]

@Zverik Sorry, wasn't able to test in time for v2 :(. I'll try to test sooner rather than later for v2.1.0.

[JaffaKetchup]

Hi there @Zverik,

I've been testing the effect of this fixed, and have noticed that I have had this problem multiple times before in the past, except in PageViews.
In order to fix it, I've just wrapped the FlutterMap() with the custom widget just below. This ensures that it and its contents are not disposed of when the PageView (equivalent to ListView in this situation) makes it go out of sight, and it's always worked well for me.

class KeepAlive extends StatefulWidget {
  const KeepAlive({
    Key? key,
    required this.child,
  }) : super(key: key);

  final Widget child; // Pass in the `FlutterMap()` here as normal

  @override
  State<KeepAlive> createState() => _KeepAliveState();
}

class _KeepAliveState extends State<KeepAlive>
    with AutomaticKeepAliveClientMixin {
  @override
  Widget build(BuildContext context) {
    super.build(context);
    return widget.child;
  }

  @override
  bool get wantKeepAlive => true;
}

When testing with your fixes, and without the above widget setup, I noticed that the map returns to the initial center (as defined in the MapOptions) when it scrolls back into view, leading me to believe that it may be being rebuilt.
Indeed, I think this is what this PR is about: ensuring the controller remains safe, even when the map gets rebuilt. Please correct me if I'm wrong. This PR successfully fixes this issue, and I would be happy to approve.

However, it also exposes the underlying problem of the map getting rebuilt. Now, another undesirable situation has arisen, which may cause surprise amongst users.
Therefore, I think it is a good idea to make an accompanying change of one of the following (I'd also like @ibrierley's input):

1. Document this behaviour, making it clear why it exists, and doing nothing else
2. Integrate the AutomaticKeepAliveClientMixin mixin into FlutterMapState
   I've played around with this - essentially just incorporating the custom widget above into it - and it works as expected. There are two sub-options for this:
   1. Always enable this functionality by setting wantKeepAlive to true
   2. Create an option to enable/disable this feature - documenting its existence in the process
3. Add a new widget and expose it publicly, with exactly the same functionality as the widget above and just a different name - documenting its existence in the process

Personally, I'm in favour of 2(ii).

In terms of this PR, as I said before, I'm happy to approve this as LGTM once we've made a decision about the above issue.

Thanks for your cooperation, and sorry we're taking so long!

[JaffaKetchup]

See #1293 (comment).

[Zverik]

Thanks for looking into this! I observe the same behaviour in my testing app, both with an implied and an explicit controllers.

To me, the existence of KeepAlive widget and AutomaticKeepAliveClientMixin is a sign that resetting a widget in a list is an expected behaviour of anything put into lists. Any widget you put there resets to its initial state, not just the map. Hence I don't consider this an issue.

I mostly use the map in a static context, and it's to be expected in a list: if you want interactivity, you would pin the map somewhere on the screen, and not allow it to scroll out of view. Or at least store its state somewhere safe.

So as you see, 3 has already been done by the Flutter team, and 2 would actually limit the usage, not fix anything: we either introduce complexity around an existing feature, or remove an option of having a stateless map in a list. Thus it seems to me 1 with a short note on losing state in a list would do.

[JaffaKetchup]

No problem, and thanks for the heads up about that KeepAlive widget - never seen it before!

Ok, I'm happy to go with option 1. I'll add something to the docs about this potential problem, and mention the usage of the KeepAlive widget. Not sure where I'll put this, but I'll find a suitable place.

I will have to do this tomorrow now - about 12 hours (@ibrierley if you have an objection, just let me know before then). Note that a pub.dev release likely won't be made just yet: there's a few other PRs I/we are looking at to merge before v2.1.0.

Thanks for your contribution!

[ibrierley]

Heya, I'm not getting a lot of time to look at things in depth just atm...

However, I'm a bit unclear where its at. Are we saying that with this PR, if the widget goes out of the widget tree and returns, it isn't at its last postion, but the initial one ? This doesn't fee right to me if so...(however, people can use the widget above I assume, or an Offstage widget to get around it, and we're in a better position that currently?).

[JaffaKetchup]

@ibrierley Yeah, you're exactly right.

To save memory and increase performance, Flutter disposes of some widgets when they scroll out of view.

This PR fixes the issues arising from the map controller becoming 'detached' from the map, and then being unable to 'reattach'.

However, it does not prevent the widget getting rebuilt: not everyone wants or needs this, and it's fighting against Flutter to implement this unnecessarily. This is what the above comments were about. When the map gets rebuilt, it loses its state, and so returns to the initial position.

This can be easily fixed with the KeepAlive widget (or similar). If you would prefer another option from the 5 above, let me know.

[ibrierley]

I don't think this is right as is (as a full solution). I think the PR fixes the main issue which is great, but I don't think the user would expect the map to get reset (and user shouldn't really care too much about underlying code), unless their own code would decide that.

To me it feels like @JaffaKetchup's 2(ii) would make most sense still as an addition (I don't care if it's only there as an option, and whether the option enabled is default though...?), I don't think I quite understood the objection there, so may need a bit of clarity!

Would all of this work fine also in a navigator context, where someone navigates away and returns as well (as opposed to a listview) ?

[JaffaKetchup]

@ibrierley I'm onboard with 2(ii). I'd leave the option disabled by default, but it can be implemented in under 10 additional lines. I'll make a PR.

I know that it would work with a PageView and tabbed setup, but I doubt it would work with navigating away from a page - although I have no real basis for this. I can test it easily.

[JaffaKetchup]

@ibrierley Can confirm that the keep alive functionality has no effect on navigation.

[aytunch]

would RestorationMixin help keep the state of the flutter_map instances?

[JaffaKetchup]

That looks way too confusing and high effort, I think the automatic keep alive mixin is good for now.

[JaffaKetchup]

Thanks for your contributions!