[FEATURE] Implement better method to organize the position of layers

[JaffaKetchup]

^{Originally discussed in #1553, converted to issue to allow tracking in v6 Release Planning.}

In flutter_map, all nonRotatedChildren always appear on top of all children. This can cause issues for some users, who need to be able to control exactly where each layer is positioned within the layer stack, and also need to control whether the layer rotates.

As such, it may be desirable in the long term to find a solution to this issue. Three proposals discussed somewhat in #1551 (#1551 (comment), #1551 (comment), #1551 (comment), #1551 (comment)) are listed below, but all FM users are welcome to add their ideas or opinions!

The builder proposal

An initial suggestion by @rorystephenson, started in #1551, was to introduce (or potentially replace the current children/nonRotatedChildren arguments with) a builder method. The user would create a Stack widget themselves, using a new widget FlutterMapTransform.rotate to enable rotation on specific layers.

I (@JaffaKetchup) think that having the user handle the stack and opt-in to rotation on layers:

• increases the time required to implement the map first time round: even though only by maybe half a minute, converting users from other libraries needs to be as quick and easy as possible
  • means the user requires knowledge about the difference between a rotated and non-rotated layer before they can implement their first layer
• is perhaps a little 'backwards' considering most layers are rotatable - surely the non-rotatable layers are the odd-ones out, and so should explicitly state that they don't support rotation
• causes friction when adding new/multiple rotatable layers, as another sub-Stack is required, or another FlutterMapTransform wrapper is required
• makes code less readable as it's hard to see what the map widget is doing immediately, and somewhat breaks the general convention of interactive maps being formed from layers: layers are now descendants of the user's custom widget tree, not FlutterMap or a related widget
• requires migration across all users (a later proposal made the builder optional via an alternative constructor)

The wrapper widget and mixin proposal

I (@JaffaKetchup) proposed an alternative that aimed to solve most of these. Create a widget NonRotatedLayer that did nothing more than direct it's builder to build the child argument, meaning it essentially acts like a tag. nonRotatedChildren still appear above all other children, and don't require an explicit NonRotatedLayer, however children can use a NonRotatedLayer just to force non-rotation to its direct descendant, as FM internals would recognise the tag.

To avoid redundant wrapping, this could (also) be implemented via a mixin (as proposed by @rorystephenson), and be allowed to be applied to all widgets. This would allow maximum flexibility.

However, as pointed out by @rorystephenson, this will not work if the layer is not directly below the children. If it is a descendant of, lets say a FutureBuilder, the effect will not apply.
I'm (@JaffaKetchup) not sure this is an issue, as either method does not restrict it's child type, so the order of the FutureBuilder and mixin/NonRotatedLayer could be swapped to restore functionality.

[rorystephenson]

I've been focusing on the refactors recently so I haven't had a change to respond until now.

---

For me the main issue with NonRotatedLayer is I feel like it breaks with expected Flutter behaviour. Image you have a widget, say CustomOverlay, which is wrapped with NonRotatedLayer. If you want to prevent it from receiving gestures a common way to do so would be to wrap it in IgnorePointer. If you wrap NonRotatedLayer with IgnorePointer suddenly the overlay would start rotating, whereas if you wrap NonRotatedLayer's child, CustomOverlay with the IgnorePointer everything would work as you expected. For a user this would be a very weird behaviour and eventually you would figure it out but in such a scenario it would be hard to understand what is going on. It is a leaky abstraction because the NonRotatedLayer is only a non-rotating layer if you place it as a direct child of FlutterMap.

One way we could improve this would be to figure out a way to throw an exception to warn the user when NonRotatedLayer is not a direct child of FlutterMap, kind of like a Positioned widget complains if it is not a child of a Stack... although a Positioned doesn't have to be a direct child of a Stack, only a descendant, so it's not really the same. Maybe it's worth having a peek at the implementation of Stack to see if we can achieve something similar... i.e. that the NonRotatedLayer could work without being a direct descendant.

[JaffaKetchup]

Sure, one thing at a time. Just trying to plan ahead a little - and I'm happy to implement this one - because I've got 10 weeks of holiday (perks of finishing major exams), so would like to aim for a release by the end of that at the latest.

---

Ok, I can kinda see what you mean about this approach being potentially confusing to users. As you've probably realized 😉 I'm a fan of documentation and think behaviour like that can be dealt with that way.

However, it would be nice to do what you suggested and throw an error/warning if the widget is used incorrectly. That should certainly prevent people from asking questions which have answers already in the docs, as the error and resolution would be right there for them: less questions for us to answer :).

What if we used something like visitAncestorElements? It sounds like it would do what we need it to do: namely, access the parent Element, then check its widget property's type.
Would need to do some testing to see whether that would be FlutterMap or one of the internals like Stack.
If it was the first one, that would be easy, and only one visit would be needed (keeping the performance cost down).
I suspect it may be the second one (based on 0 real info though), in which case we need to visit twice and wrap the Stack (or whatever) in an internal detector widget, like _NonRotatedLookupAncestor or something).
In whatever case, not finding the appropriate ancestor would mean throwing an error in the console.

How does that sound to you? Not sure if there are any other ways to check the parent widget.

[Robbendebiene]

Just stumbled across this and wanted to add my two cents.

I feel like the whole confusion/problem just arise by the name nonRotatedChildren. How about just renaming it to overlaidChildren, since that's what they basically are. I think it is important that they are separated from children and not thrown together into some kind of builder or unified children list, since they serve entirely different purposes. One is for map layers the other for UI layers.

[JaffaKetchup]

@Robbendebiene I can't imagine why, but some people may want to put a standard child above a non-rotated child, which is what this is trying to allow for, in which case, overlaidChildren may not be a suitable name, as not all are overlaid. Additionally, we do not recommend putting unrelated widgets in non-rotated children: the idea of it is for things like attribution that may need to access the map state.
The second proposal would not remove the separate children list.

But you have made a good point in that I can't really think why anyone would need this. @rorystephenson Did you have a use case?

[Robbendebiene]

I can't imagine why, but some people may want to put a standard child above a non-rotated child, which is what this is trying to allow for, in which case, overlaidChildren may not be a suitable name, as not all are overlaid.

One could argue that this is a very special use case that shouldn't be handled by the package itself and instead be moved to a plugin (the problem probably is that a plugin currently doesn't have the power to "unrotate" itself?). Still for a developer it might be possible to get the desired result by creating multiple FlutterMap instances, even though this could be painful.

Additionally, we do not recommend putting unrelated widgets in non-rotated children: the idea of it is for things like attribution that may need to access the map state.

I have to admit that's how I like to use it (putting a Stack or Align widget inside it is just very convenient and flutter like). If it is not intended by the package it should restrict to some common "LayerWidget" type in my opinion. But this is a different topic.

The only reason for me as a developer to not just simply wrap a Stack around FlutterMap in my personal code is that nonRotatedChildren has the effect that gestures/map interactions still work on the widgets supplied to it. Sometimes this is desired, sometimes not, which is why I for example make use of both ways (I wrap FlutterMap in a Stack to show a "bottom sheet" above it, but put things like attribution and zoom buttons into nonRotatedChildren). Just as another perspective of how one can think about the current nonRotatedChildren property.

[JaffaKetchup]

One could argue that this is a very special use case that shouldn't be handled by the package itself and instead be moved to a plugin (the problem probably is that a plugin currently doesn't have the power to "unrotate" itself?). Still for a developer it might be possible to get the desired result by creating multiple FlutterMap instances, even though this could be painful.

Yeah, there's no good way for a plugin to un-rotate. Theoretically, it could apply an inverse rotation transformation to itself, but I'm not sure whether Flutter is smart enough to optimise that, and without optimization, that may be not very performant.

If it is not intended by the package it should restrict to some common "LayerWidget" type in my opinion. But this is a different topic.

We have thought about this before, but it turns out it is very useful to allow something like a StreamBuilder or FutureBuilder around a layer.

but put things like attribution and zoom buttons into nonRotatedChildren

Yep, that is it's intended use case. These things need access to map state or a controller, so it makes sense to not need a controller, and just allow it to use state directly.
I meant like, it shouldn't be used as a shortcut for a Stack for unrelated widgets.

[JaffaKetchup]

@rorystephenson

I've found a way to detect whether it is a top level widget. It requires at most two iterations of context.visitAncestorElements per build.

map/non_rotated_layer.dart (new)

/// Provide a detection point for [NonRotatedLayerMixin]
///
/// Although any other internal-only widget could be used as the detection point,
/// this is provided to reduce the number of iterations
/// `context.visitAncestorElements` requires to ascertain whether
/// [NonRotatedLayerMixin] is being used correctly.
class _NRLDetectorAncestor extends StatelessWidget {
  const _NRLDetectorAncestor({required this.child});

  final Widget child;

  @override
  Widget build(BuildContext context) => child;
}

/// Prevent a widget's rotation with the map, when built within the standard
/// [FlutterMap.children] list instead of the [FlutterMap.nonRotatedChildren]
/// list (where this is not required)
///
/// The widget mixing this in must always be a top level widget in
/// [FlutterMap.children], ie. it must not be a child of another widget. Failure
/// to do this will throw an error, as the behaviour will not be correct.
///
/// Always call `super.build(context)` from within the widget's `build` method.
/// Ignore its result, and build children as normal.
///
/// Prefer using [NonRotatedLayer], which mixes this in automatically.
mixin NonRotatedLayerMixin on StatelessWidget {
  @override
  @mustCallSuper
  Widget build(BuildContext context) {
    // `context.visitAncestorElements` can be expensive, so set a maximum number
    // of iterations
    int i = 0;
    context.visitAncestorElements((e) {
      i++;

      if (i != 2 || e.widget is _NRLDetectorAncestor) return true;

      throw FlutterError(
        'The widget with `NonRotatedLayerMixin` (or `NonRotatedLayer`) must only '
        'be used as a top level widget in `FlutterMap.children`\n'
        'Failure to do so will mean that the layer still rotates as normal.\n'
        'To resolve this, and restore functionality, swap the parent\'s of this '
        'widget with its `child`.\n',
      );
    });

    // The user shouldn't build the output of this method
    return Builder(
      builder: (_) => throw FlutterError(
        'Widgets that mixin `NonRotatedLayerMixin` must call '
        '`super.build(context)`, but must also ignore the return value',
      ),
    );
  }
}

/// Prevent a widget's rotation with the map, when built within the standard
/// [FlutterMap.children] list instead of the [FlutterMap.nonRotatedChildren]
/// list (where this is not required)
///
/// This widget must always be a top level widget in [FlutterMap.children], ie.
/// it must not be a child of another widget. Failure to do this will throw an
/// error, as the behaviour will not be correct.
class NonRotatedLayer extends StatelessWidget with NonRotatedLayerMixin {
  const NonRotatedLayer({
    super.key,
    required this.child,
  });

  final Widget child;

  @override
  Widget build(BuildContext context) {
    super.build(context);
    return child;
  }
}

map/state.dart

  Iterable<Widget> _prepareChildren(List<Widget> children) sync* {
    final stackChildren = <Widget>[];

    Widget prepareRotateStack() {
      final box = OverflowBox(
        minWidth: size.x,
        maxWidth: size.x,
        minHeight: size.y,
        maxHeight: size.y,
        child: Transform.rotate(
          angle: rotationRad,
          child: Stack(children: List.from(stackChildren)),
        ),
      );
      stackChildren.clear();
      return box;
    }

    for (final Widget child in children) {
      if (child is NonRotatedLayerMixin) {
        if (stackChildren.isNotEmpty) yield prepareRotateStack();
        yield child;
      } else {
        stackChildren.add(child);
      }
    }
    if (stackChildren.isNotEmpty) yield prepareRotateStack();
  }

// To build the children

RawGestureDetector(
  gestures: gestures,
  child: ClipRect(
    child: _NRLDetectorAncestor(
      child: Stack(
        children: [
          ..._prepareChildren(widget.children),
          ...widget.nonRotatedChildren,
        ],
      ),
    ),
  ),
),

[rorystephenson]

Congrats on finishing your exams @JaffaKetchup , hope they went well!

Both of your comments made me revisit why I proposed a change in the first place and there are two main reasons:

1. It's an unusual pattern in Flutter for a widget to have two sets of children, one that is affected by the parent widget and one that is not.
2. I remember noticing the limitation a long time ago that you can't interleave rotated/non-rotated children which again seemed like a very weird limitation for a Flutter widget. I have a vague recollection of someone raising an issue about not being able to interleave but after a quick search I couldn't find it.

@Robbendebiene I can't imagine why, but some people may want to put a standard child above a non-rotated child, which is what this is trying to allow for, in which case, overlaidChildren may not be a suitable name, as not all are overlaid.

I think you're probably right that if such a use case exists it is probably, as @Robbendebiene said, a very special use case. So if we consider that unsupported behaviour then renaming to overlaidChildren (or just overlays) would make it much clearer what it is intended for.

This resolves both of my initial motivations for making this change and, if anyone ever provides a good example of why interleaving rotated/non-rotated widgets is necessary I think we can reconsider this.

---

Interesting find on visitAncestorElements @JaffaKetchup , I wasn't aware of that.

[JaffaKetchup]

Congrats on finishing your exams @JaffaKetchup , hope they went well!

Thanks, I'll have to wait and see about that ;)

It's an unusual pattern in Flutter for a widget to have two sets of children, one that is affected by the parent widget and one that is not.

I would say that both are affected by the parent. Just one is affected more than the other - the non-rotated children can still access and control the parent's state as necessary. Plus, flutter_map is kind of an unusual pattern throughout :D.

---

renaming to overlaidChildren (or just overlays) would make it much clearer what it is intended for.

This resolves both of my initial motivations for making this change and, if anyone ever provides a good example of why interleaving rotated/non-rotated widgets is necessary I think we can reconsider this.

I doubt most people will even think about the positioning of the layers - nonRotatedChildren is usually used for controls, and so should usually be on top, as it is - and therefore, I'm not sure we need to push 'overlay' that much. Instead of renaming, I'll just alter the docs.

For now, this can sit here, and I'll lower the priority. If anyone suddenly needs this functionality, the code is pretty much good to go, and can be added relatively quickly.

[ibrierley]

Just a note, it may have been me that raised a similar issue historically, but iirc this was for when we had both sets of layers as well (so children, nonrotatedchildren, layers and nonrotatedlayers I think), and that made it extra confusing because order mattered even more there.
Edit: found the one I was thinking of... #925 so if you're thinking of that one, the main issue there is redundant since the removal of layers, but you may be thinking of a different issue.

Haven't been following this too closely, apologies.
Wouldn't it be possible just to have a NonRotateLayer(child: AttributionWidget....and we do a check if the widget is that to see if widgets get grouped into a transform.rotate or not (with maybe a couple needed if there is a nonrotated inbetween the rotated ones?) (not quite sure if thats similar to the wrapper widget mentioned earlier or not).

[JaffaKetchup]

Wouldn't it be possible just to have a NonRotateLayer(child: AttributionWidget....and we do a check if the widget is that to see if widgets get grouped into a transform.rotate or not (with maybe a couple needed if there is a nonrotated inbetween the rotated ones?) (not quite sure if thats similar to the wrapper widget mentioned earlier or not).

I think what you're describing is exactly the same as the NonRotatedLayer that was discussed above.

[Robbendebiene]

It's an unusual pattern in Flutter for a widget to have two sets of children, one that is affected by the parent widget and one that is not.

@rorystephenson While it's not common there are widgets that have multiple child properties like Scaffold, AppBar, AlertDialog and others. So I would say it depends if one would expect this functionality/property from a Widget or not.

I doubt most people will even think about the positioning of the layers - nonRotatedChildren is usually used for controls, and so should usually be on top, as it is - and therefore, I'm not sure we need to push 'overlay' that much. Instead of renaming, I'll just alter the docs.

@JaffaKetchup I would vote for a deprecation of the nonRotatedChildren property and adding the overlays property as @rorystephenson suggested. At least from time to time I have to look up whether nonRotated means that it should be used for layers whose markers/children shouldn't be rotated when the map is rotated or if it is supposed to be used for overlays 🙈

Edit: I know this feels like a step backwards, but maybe renaming children to layers in combination with the overlays property would be the best. After all children suggests that it can be used for arbitrary widgets which isn't really the case. Also it's not uncommon to have widgets with "child properties" that are not named children, just lookup the widgets I mentioned above.

[ignatz]

Sorry for being late to the party.

Fundamentally, I'd think about the distribution of responsibilities. Specifically, who decides whether a layer is functionally rotating or not? It's not the user, it's always the layer itself. In other words, I can't think of a single case where a layer would meaningfully work both in a rotating and non-rotating mode and the user can choose which box it should go to.

Giving the user two properties/buckets in FlutterMap gives them at best the illusion of choice, so you might as well take it away. I liked the idea of tagging layer implementations (whether that's done through mixins, traits, or carrier pidgins). A single bucket with a defined ordering will always be easier than two buckets. (and anything is better than leaflet where some layers are being reordered by the framework)

[JaffaKetchup]

@ignatz Great to hear from you.
I agree with everything you said. Can you have a look at #1615, and see whether what I've done there makes sense to you? There's also a question in the latest comment that needs to be answered.