[FEATURE] FlutterMapState over-notifies dependent widgets via updateShouldNotify

[rorystephenson]

What is the bug?

FlutterMapState over-notifies dependent widgets. InheritedWidgets should only notify their dependents when it is necessary and right now FlutterMapState will always notify dependents. This was causing me confusion when debugging one of the plugins I maintain and leads to unnecessary builds.

How can we reproduce it?

See the diff in this branch: https://github.com/fleaflet/flutter_map/compare/master...rorystephenson:flutter_map:did-change-dependencies-bug?expand=1

If you run hot reload you will see that didChangeDependencies is called even though nothing changed. I haven't tested it but I presume didChangeDependencies will also get triggered when changing parent widgets of FlutterMap even if those changes don't affect FlutterMap itself.

Do you have a potential solution?

I'm sharing this here because it's probably a symptom of a bigger issue.

As far as I can see we would need to create a real implementation for _MapStateInheritedWidget.updateShouldNotify. I don't see how we can do so without making FlutterMapState immutable and emitting a new one whenever it changes. We can't currently compare with the oldWidget's mapState because they are the same object which is mutated when changes occur.

So the proposal would be for FlutterMapState to just become an immutable object that only contains final variables that describe the current map state (center, size, zoom...) and for it to implement == so that we can determine if the map state has changed and give a proper implementation for _MapStateInheritedWidget.updateShouldNotify. The rest of the logic in FlutterMapState would move to appropriate classes.

Obviously this is a huuuge change so I want to kick off a discussion about it and see if anyone has different ideas.

Platforms

All

Severity

Obtrusive: Prevents normal functioning but causes no errors in the console

[JaffaKetchup]

Just to note, this was the comment before the recent reorganization:

[JaffaKetchup]

My position is, if something needs a rewrite to improve efficiency/performance, I'm up for it. Something like this should also not be a breaking change, which means it can be released without so much debate/waiting!

However, I'm not sure actually how much the performance is damaged by this 'wrong' implementation? Is there a way it can be quantified?

[ibrierley]

Reminds me a little of #1408 but not sure if that's a redherring, but may be worth a quick scan.

My own take I think is that it makes sense. I've also pondering before about the idea of a state that could even be serialized (for debug at least), where you could effectively record a state, replay it etc, but that's probably a bit of a distraction as well.

Out of interest, why is it a huuuge change, doesn't most stuff just go via state.dart? (not saying it isn't, trying to just get my head around it :D I typically struggle at first with this stuff)

Also out of interest, what's the main issue you've been looking at, may give some perspective also to know.

[rorystephenson]

Out of interest, why is it a huuuge change, doesn't most stuff just go via state.dart? (not saying it isn't, trying to just get my head around it :D I typically struggle at first with this stuff)

It depends how far we want to take it. If we want the FlutterMapState to only represent state then it would be split in to:

1. FlutterMapStateContainer: This would do most of what FlutterMapState does right now, including being an actual stateful widget which contains the InheritedWidget.
2. FlutterMapState: This would be an immutable object containing just variables that define the state of the map like center/zoom/size.

In an idealistic world the FlutterMapState would no longer provide methods for controlling the map, this would be done exclusively through MapController. This would mean we would need to provide a different mechanism for child widgets (e.g. plugins) to control the map. One option that comes to mind is using the notifier pattern (see ScrollNotifier and NotificationListener classes). Another option would be that the MapController needs to be explicitly passed to child widgets by the user. I'm sure there are more possibilities here.

---

Also out of interest, what's the main issue you've been looking at, may give some perspective also to know.

So when working on SuperclusterLayer I wanted to add an assertion that the maximum clustering zoom is not higher than the FlutterMap maxZoom (if it is set). Here is a shortened version of the example:

FlutterMap(
  options: MapOptions(
    maxZoom: 15,
  ),
  children: [
    SuperclusterLayer(
      maxClusterZoom: 16, // Higher than FlutterMap maxZoom, triggers assetion.
    ),
  ],
)

The problem I was having was that if I set maxClusterZoom to a value higher than the maxZoom the assertion is correctly thrown when hot-rebuilding but then when I set it back to a valid value and hot-rebuild again it still throws. If I hot-rebuild one more time without changing anything the assertion no longer fires.

The issue, it seems, is that FlutterMapState always notifies its dependencies even if it did not change. It would trigger an initial rebuild with the maxClusterZoom still at the old value before another rebuild is fired with an updated maxClusterZoom.

I have tried listening to the FlutterMapState in two different ways in SuperclusterLayer without any success:

1. Pass it in from a parent widget and then place the assertion in didUpdateWidget.
2. Use FlutterMapState.of(context) in didChangeDependencies.

[rorystephenson]

I'm having a play with what this could look like and I think there is potential to significantly reduce complexity by moving to an immutable MapState and also tidy up gesture code. The changes are extensive and it's not in a final state but if you're interested here's the branch I am working on: https://github.com/rorystephenson/flutter_map/tree/flutter-map-state-refactor

I want to see if I can clean up the gesture code. I've started by moving gestures in to a dedicated widget, InteractionDetector, and I have a few more ideas. Flutter's InteractiveViewer is worth looking at for some inspiration and it might be worth revisiting our usage of PoisitionedTapDetector2 (which was added to our source) to see if it would be cleaner to integrate it in InteractionDetector.

[JaffaKetchup]

I want to see if I can clean up the gesture code

I've been wanting to do that for ages, and even got to the point of starting on it, but I have no time for a rewrite as large as that atm! With Dart 3, there's even more possibilities, and one of the features I was thinking of adding was the ability for users to register their own event and handle something themselves if they needed to. However, I appreciate you may not be up for this!

InteractiveViewer is worth looking at for some inspiration

I've also previously looked at this, but it doesn't support rotation unfortunately (flutter/flutter#57698 and flutter/flutter#73358).

---

I'll keep an eye on your branch, but if you could open a draft PR here, that would be great!

[JaffaKetchup]

(Just to note, I've relabeled this as an enhancement, as it currently isn't causing issues, and is part of improvement works).

[rorystephenson]

Thar she blows: #1551