fleaflet / flutter_map

A versatile mapping package for Flutter. Simple and easy to learn, yet completely customizable and configurable, it's the best choice for mapping in your Flutter app.
https://pub.dev/packages/flutter_map
BSD 3-Clause "New" or "Revised" License
2.68k stars 848 forks source link

[BUG] External `MapController` lifecycle not synced with widget when destroyed and rebuilt #1892

Closed Zverik closed 5 days ago

Zverik commented 1 month ago

What is the bug?

I have a FlutterMap in a ListView. It displays fine. When I scroll down so the map is not visible (and the state is lost), and then scroll back up, the map is broken (at least in the debug build), and there is an assertion in the console: "Should not update options unless they change", originating from map_controller_impl.dart:342.

I guess the widget gets deleted when not in view, then built again, and the same mapController gets reattached. Is that correct?

How can we reproduce it?

Sorry I don't have a short code at hand, my laptop acts up. I encountered this when running Every Door: find a POI and tap on it, and then scroll down, opening "more fields", to the bottom. And then up. Here is how the object created.

Do you have a potential solution?

Remove the assertion?..

Platforms

Android 14 (Sony Xperia 10 IV).

Severity

Erroneous: Prevents normal functioning and causes errors in the console

JaffaKetchup commented 1 month ago

Thanks for reporting. This is very similar to #1760. However, it is occuring here under new conditions.

Zverik commented 1 month ago

Well it worked with version 6.1.0, but broke with 7.0.0.

Tested with a production build: the map disappears, and there is an exception in the log:

[07:35:32] Flutter: LateInitializationError: Field '_interactiveViewerState@832162146' has already been initialized.
[07:35:32] #0      LateError._throwFieldAlreadyInitialized (dart:_internal-patch/internal_patch.dart:185)
#1      MapControllerImpl._interactiveViewerState= (package:flutter_map/src/map/controller/map_controller_impl.dart)
#2      MapControllerImpl.interactiveViewerState= (package:flutter_map/src/map/controller/map_controller_impl.dart:47)
#3      MapInteractiveViewerState.initState (package:flutter_map/src/gestures/map_interactive_viewer.dart:103)
#4      StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:5618)
#5      ComponentElement.mount (package:flutter/src/widgets/framework.dart:5463)
...
JaffaKetchup commented 1 month ago

That is something we weren't expecting to see. I wasn't aware that anything had changed in that area of the code.

JaffaKetchup commented 1 month ago

@Zverik I noticed you're initialising your map controller within the build method: https://github.com/Zverik/every_door/blob/6026f164baf2b66c067898be24f7205a1d05fd74/lib/screens/editor.dart#L547. This could be causing issues. Can you try moving it out? (https://docs.fleaflet.dev/usage/programmatic-interaction/external-custom-controllers)

Zverik commented 1 month ago

Alas no, it's declared outside any of methods :( Would be an easy fix!

JaffaKetchup commented 1 month ago

Ah, misread the indentation. The only way this can occur is when the initState method is run on the MapInteractiveViewer multiple times, and the map controller remains the same.

JaffaKetchup commented 1 month ago

I'm thinking this was introduced in #1738. It made the most changes to this area between v6 and v7.

Zverik commented 1 month ago

So I took the editor.dart and shortened it piece by piece until I got a minimal example for the bug: https://gist.github.com/Zverik/d056509e22eeecffdefb3a42a587325f

When I comment out the mapController assignment, it works.

Zverik commented 1 month ago

I frankly do not understand the nature of changes in lib/src/map/widget.dart, but I feel like options should be checked for change before reassignment. The widget was disposed, then re-created, but the options (and the map controller) are the same, since the state preserved them.

JaffaKetchup commented 1 month ago

@Zverik As a short term workaround, try setting keepAlive to true in MapOptions. That should fix the detachment issue.

Zverik commented 1 month ago

Yes, that works, thank you Luka!

Zverik commented 1 month ago

Similar issue was reported on Discord, but with tabs instead of list items.

JaffaKetchup commented 4 weeks ago

I've run into a similar issue that cannot be resolved by keepAlive. EDIT: Turns out this is because I was inadvertently fully rebuilding the map (moving it in and out of a subtree (disabling/enabling some wrappers) when a switch was flipped) - interestingly, the map loaded pretty much instantly - maybe this is the image cache?

josxha commented 4 weeks ago

@JaffaKetchup What is the expected behavior here?

At the moment a MapController is not meant to be a global map state and can only get attached to a map widget once. This behavior should be the same in v6 and in v7. This means that the MapController is only meant to be kept as long as the map widget exists. A ListView does only keep widgets that are on the screen (+ maybe a buffer). This means that when a new map widget gets created, a new MapController should get used. This makes me think that something prevented the map widget from being disposed in v6?

Zverik commented 4 weeks ago

Note that a map controller can be created in a containing stateful widget, being a part of its state.

JaffaKetchup commented 3 weeks ago

This means that when a new map widget gets created, a new MapController should get used.

As @Zverik said, this isn't always easy and/or possible. You'd need to create a layer to export the map controller on every build from the inherited state I guess.

Is there a reason why being able to reassign wouldn't work?

(Also moving this to P2 as the workaround seems to work fine).

josxha commented 3 weeks ago

Is there a reason why being able to reassign wouldn't work?

Not really, should be doable. It just wasn't designed that way in the past. Could be that it was a problem with the cyclic dependency of the map controller but that got removed in https://github.com/fleaflet/flutter_map/pull/1738. I just can't tell much about potential side effects at the moment.

FeofanGreek commented 3 weeks ago

We have the same problem when using the global map controller. But we are forced to do this because third-party application functions access the controller to obtain the current state or even simply convert coordinates into pixels. The assignment is done by: return FlutterMap( mapController: controller, How can this situation be stopped?

ibrierley commented 3 weeks ago

I'd probably take a look at an Offstage widget to put the map in (and possibly move it offstage from the listview when outside the displayed widgets if it doesn't keep its state at that point still).

josxha commented 3 weeks ago

But we are forced to do this because third-party application functions access the controller to obtain the current state or even simply convert coordinates into pixels.

@FeofanGreek Could you give more information about the 3rd party application you are using? When do you want to convert coordinates into pixels while the map widget does not exist?

FeofanGreek commented 3 weeks ago

I didn’t express myself correctly about the “Application”, of course we use functions inside our application, but launched in the background. However, the functions themselves require simple controller-based transformations. And it is not always possible to gain access through the instance of the screen on which the map is displayed. Example: controller.camera.visibleBounds.west or controller.move(controller.camera.center, UserConfig.instance.data.startZoom) or controller.camera.pointToLatLng(Point(width, 0)) or controller.camera.latLngToScreenPoint(b )

But we are forced to do this because third-party application functions access the controller to obtain the current state or even simply convert coordinates into pixels.

@FeofanGreek Could you give more information about the 3rd party application you are using? When do you want to convert coordinates into pixels while the map widget does not exist?

MobileDev500 commented 3 weeks ago

I have implemented the following function to recreate MapController and MapOptions each time FlutterMap is rerendered, but it still throws the error in described in OP:

MapController getMapController() {
    mapController?.dispose();
    mapController = MapController();
    return mapController!;
  }

  MapOptions getMapOptions() {
    return MapOptions(
        initialCenter: initialCenter,
        initialZoom: 15.0,
        minZoom: Random().nextDouble(),
  }

Even though minZoom field of MapOptions is randomized (eg. changes every time the function is called) Flutter still does not recognize that options have changed. How is this even possible?

josxha commented 3 weeks ago

You can try to use the pull request https://github.com/fleaflet/flutter_map/pull/1915 to see if works with it.

dependency_overrides:
  flutter_map:
    git:
        url: https://github.com/josxha/flutter_map
        ref: feat/assign-controller-to-map-widget-multiple-times
FeofanGreek commented 3 weeks ago

Hooray! Everything worked

MobileDev500 commented 3 weeks ago

The fix by @josxha works.

If anyone encounters the issue of Polygons not updating, the fix is to give new key value to PolygonLayer constructor whenever you need to update your Polygons.

JaffaKetchup commented 3 weeks ago

If anyone encounters the issue of Polygons not updating, the fix is to give new key value to PolygonLayer constructor whenever you need to update your Polygons.

This shouldn't be necessary as #1904 should've fixed this. Not sure, maybe @josxha's PR doesn't include thix somehow?

josxha commented 3 weeks ago

Not sure, maybe @josxha's PR doesn't include thix somehow?

Not really, the pr shouldn't affect it.

JaffaKetchup commented 3 weeks ago

Yeah that's why I was confused.

JaffaKetchup commented 2 weeks ago

Please also double check https://docs.fleaflet.dev/usage/programmatic-interaction/external-custom-controllers#usage-within-a-state-system-model if using external state. It may be that some of these issues occur when this is handled improperly. This won't apply to situations where the map controller is entirely with a stateful widget directly, but instead to solutions such as Riverpod/Provider.

pogorv12 commented 1 week ago

I have the similar issue.

I have the stateful widget with the map and refresh buttom that suppose to bring map to initial position with tha mapController defined on the state widget level.

After updating 6.0.1 -> 7.0.1 I'm getting an error when that button is pressed and the setState initiated.

The following LateError was thrown building LayoutBuilder:
LateInitializationError: Field '_interactiveViewerState' has already been
initialized.

When the exception was thrown, the stack was:
http://localhost:52041/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 296:3       throw_
http://localhost:52041/packages/flutter_map/src/map/controller/map_controller_impl.dart 19:40            set [_interactiveViewerState]
http://localhost:52041/packages/flutter_map/src/map/controller/map_controller_impl.dart 47:7             set interactiveViewerState
http://localhost:52041/packages/flutter_map/src/gestures/map_interactive_viewer.dart 103:12              initState

To eliminate the issue I've removed [final] from _interactiveViewerState declaration. This solved the error.

But, comparing to 6.0.1, 7.0.1 widget dont automaticaly catch the new initial LatLon during redrawing, so I have to manually add controller move after each location change

_mapController.move(LatLng(_location.latitude, _location.longitude), _mapController.camera.zoom);