Reworked layers mechanism

[JaffaKetchup]

There are four issues with the current way of defining layers:

1. One has already been extensively discussed in [FEATURE] Implement better method to organize the position of layers #1564, where 'non rotated layers' must always come on top
2. One has been spotted as a point at which people often get stuck (eg. [FEATURE] Allow gesture handling on multiple layers #1616), where they put a layer over an interactive MarkerLayer, meaning it cannot receive gesture events
3. The name 'non rotated layer' is not ideal, the layers also don't move - something like 'anchored layer' might be preferable
4. For extremely simple maps, the concept of layers might be a bit too much

I've decided to make the following changes:

• (1, 3, 4): Reworked the way layers and non-rotated layers are defined and used
  • Deprecated FlutterMap.nonRotatedChildren
  • Transferred responsibility for whether to be 'mobile' or 'static' to the layer itself, via the new MobileLayerTransformer widget which can be included in a layer's build method
• (2): Added TranslucentPointer widgets around all layers (optionally, on by default)
• (4): Added a new FlutterMap.simple constructor that just supports one very basic TileLayer and an attribution layer

The changes I've made are extremely intrusive, so open to suggestion! Doubtless someone'll spot a better way to do something.

---

Also includes some refactoring and cleanup that I couldn't resist doing. More to come in another PR!

[mootw]

i am not a huge fan of these specific changes on first reading. i feel uneasy about the layers stack changes specifically.

i really like the concept of removing non-rotated children. the end user should not be able to place a map layer somewhere where it can go, but will function incorrectly and might not be obvious (as in they should not be concerned about whether a layer rotates; the layer should decide if it wants to rotate and how).

i think the implementation of the layer stack is sub-optimal though. it is quite convoluted and the concept of an Anchored Layer is introduced. as a developer i would expect these behaviors from fm

1. i can place any widget inside of the flutter map children and they will display as a stack in the map. (like i can place an Align bottom right with child text widget to create an attribution for example).
2. i can place a tile layer in (or create one as a FM developer), and Flutter Map will serve 2 primary goals. Create a view port that my widget renders in, provide bounds and projection helpers and the camera position, zoom, and rotation. Secondly, FM will capture touch/input events that handle navigation around the map. (i think it is ideal if FM avoids messing with gestures as much as possible to avoid creating wrappers around the input system).

I think this philosophy also applies to the layers_stack implementation. I don't think it is necessary to extend Widget to create an AnchoredLayer widget to isolate layers that need rotating and then picking those out of the stack, its convoluted in my opinion and gets further away from the everything is a widget philosophy (and might even cause flutter optimization issues).

My immediate thoughts are this. Treat all layers as non-rotated children, then have a FM provided widget that is literally just rotates the map layer just like how all other layers are already (basically returning the old behavior); we can wrap all of our internal layers that want that rotation in it, and any external developers who are creating layers can easily just wrap their widgets in our "old behavior" widget to return the old behavior. this makes migration trivial.

I think the benefits are obvious with this implementation:

• mixing and matching rotated and non rotated children is trivial
• we remove non-rotated children (App developers should not need to figure out where to put the layers, they should all just go in a stack top to bottom; layer widget developers decide how the widget behaves)
• the less we add "on top" of flutter behavior the less risk of performance issues or things we need to maintain.

these are my immediate thoughts, i haven't looked deeply at the translucent_pointer, but i have a feeling both of these things can get mitigated by a slightly easier design. we shouldn't need to be injecting touch behavior, if someone wants to modify a layers touch properties they should do it by wrapping the widget themselves (in my opinion; we should avoid modifying "default" touch/input capturing behavior when possible).

i can hopefully take a closer look at this project soon, ive been incredibly busy. but i think this is worth looking at because we should solve the gesture and layer rotation problem. it is quite strange that we are just rotating all of the widgets in my opinion when they can easily handle it internally with a single widget. i think the "flutter way" is that it is okay to have a widget handle behavior change. not everything needs to be a parameter, especially not things related to gesture handling, we can provide the widgets to handle these changes, just like flutter already has IgnorePointer and others.

[JaffaKetchup]

@mootw

My immediate thoughts are this. Treat all layers as non-rotated children, then have a FM provided widget that is literally just rotates the map layer just like how all other layers are already (basically returning the old behavior); we can wrap all of our internal layers that want that rotation in it, and any external developers who are creating layers can easily just wrap their widgets in our "old behavior" widget to return the old behavior. this makes migration trivial.

Ok, I think what you're saying is the inverse of what it currently is. However, I think there are still problems with this approach:

1. As we want to still allow any type of Widget to be used as a layer, some sort of detection mechanism would be required to ensure/check that the 'rotator widget' is being used at the top level, in order to allow FM to apply the necessary transformation effects to it. This is exactly what this PR implements, but with the opposite instead.

2. This is potentially less performant that opting-out of rotation, as is current. In most maps, there will be more rotated widgets than non-rotated widgets - therefore, the detection mechanism will be run more times. The detection mechanism is not cheap, as it requires an ancestor lookup.

3. It is not necessarily performant to wrap the layers' internal widget trees with a 'rotator widget'. The detection mechanism will have to lookup more ancestors (2+): _DetectorAncestor <- TileLayer <- RotatorWidget. Therefore, a mixin would be more appropriate, as it removes the extra widget - as is implemented and recommended in this PR.

Essentially, with those 3 points, you come to this PRs implementation. However, because the detection will need to be applied more frequently, and for longer each execution (unless you use mixins), I would say it would be less performant.

4. There is an alternative, which is perhaps closer to what you were suggesting: forcing all children to be of type Layer, where Layer is sealed and extended by StandardLayer and AnchoredLayer. This would mean that the detection logic could be removed, but it might impact the developer experience having to wrap all their custom widgets in one of those. I guess I'm open to this approach, it could possibly be applied to this PR as well/instead, but it also feels just a bit 'off' somehow, like a step backwards.

5. To me, it seems counter-intuitive. Maps are mostly made out of moving, non-anchored layers. Why should these layers have to opt-in, instead of the special (anchored) layers opting-out?

(App developers should not need to figure out where to put the layers, they should all just go in a stack top to bottom; layer widget developers decide how the widget behaves)

6. This is not true. Some apps want markers on top of polygons, others might want it the other way around. The ordering is still an issue.

i haven't looked deeply at the translucent_pointer, but i have a feeling both of these things can get mitigated by a slightly easier design. we shouldn't need to be injecting touch behavior, if someone wants to modify a layers touch properties they should do it by wrapping the widget themselves (in my opinion; we should avoid modifying "default" touch/input capturing behavior when possible).

7. Again, I'm not sure this is true. The purpose of applying translucency to all layers is to allow each layer to be able to handle gestures, as you proposed. Without it, only the topmost layer can receive gestures, if it encompasses the entire screen (as, let's say the CircleLayer does). The reason I've hidden this away by default, is because I can't think of many use-cases where you wouldn't want all the layers to be able to handle gestures, and because there's many issues with people missing this point. Having users wrap every layer in the TranslucentPointer widget is still possible in this PR, but FM does it automatically by default. EDIT: [FEATURE] Allow gesture handling on multiple layers #1616 just filed, exactly this point.

i think the "flutter way" is that it is okay to have a widget handle behavior change

8. I would usually agree with this. However, the current detection implementation is based off of https://api.flutter.dev/flutter/widgets/AutomaticKeepAliveClientMixin-mixin.html, which definitely changes the behaviour of widgets it is applied to. In this case, we want to manipulate as few widgets as possible (for performance), so mixins seems like a good idea.

One thing I would agree with is that the current _LayersStack implementation may be sub-optimal in terms of performance. I've tried to optimize, but it's very possible there's better ways to get less rebuilds on fewer layers.

It would be great if @TesteurManiak & @ibrierley could also join in, and maybe @Robbendebiene as well, as you seemed to be interested in this.

[ibrierley]

I'm not sure I have anything useful to add, I think I'm 50/50 on everything (without looking at the code in depth) but a couple of thoughts..

If one of the main incentives for any decision is a performance reason, I'd probably want to verify that is the case. Flutter often does a few optimisations anyway.

I'm not convinced either way on the non-rotated argument. My gut instinct is that a non-rotated widget is clear what happens to anyone new looking at code (but I don't really like the name), and makes them think about it (rather than having to think about every individual layer differently and know about it). I was wondering about the notion of only having a StaticLayer or a MovingLayer, so there's clarity, but really I'm not sure I like that either :). I feel a bit like an "anchored" layer is a bit confusing (as we use anchors elsewhere a bit differently?)

The gesture stuff, I'm even less sure of. Sorry, not being much help here, but will give any thoughts if they pop up.

[Robbendebiene]

Just quickly read through this so apologies for any potential misconceptions.
I really like the concept of @mootw at least the way I understood it wihch seems to differ from @JaffaKetchup

I don't really understand why detection would still be needed in this case.
Currently all layers are rotated together via a bundled transform widget. If instead every layer has to rotate itself then there shouldn't be any detection needed. While this may sound more verbose, at the end it makes the layers more flexible and the verbosity can be solved by adding a mixin, super class or "rotation widget" that can be used by layers who want to rotate with the map.
I also think this perfectly fits into the concept that every widget below flutter map has access to the controller/camera and can act accordingly to state changes. With the suggested changes the flutter map widget would just be a wrapped stack with some inherited widgets and gesture detection stuff on top, wherefore any sort of widget can simply be thrown into it.

[JaffaKetchup]

Ah, I think I've got you now.

Of course, I forgot that layers can access the map's state :d. I was going along that the state would need to be injected into the layers. Apologies for my stupidity.

I do wonder whether transforming more, shorter depth, widgets is less performant than transforming fewer, stacked/longer depth, widgets. But as @ibrierley said, we should probably test the performance somehow.

In regards to the translucent gesture stuff, that's almost definitely necessary IMO. There's no other way to allow all layers to handle their gestures independently.

I'll see if I can put something together...

[JaffaKetchup]

@mootw @Robbendebiene @ibrierley @TesteurManiak Ok, I have implemented that in 48c0ec6. Let me know what you think.

However, one thing to note is that a widget which uses neither the new MobileLayerTransformer, nor a static positioning widget (such as Align), will not demonstrate the correct behaviour.
We can either leave this as it is, or implement a StaticLayerTransformer, which uses Align and/or SizedBox.expand internally, then make them implement a sealed LayerTransformer interface and only accept them as children. At least the second option would make it more obvious what each widget type is, as I do feel like it's not 100% clear at the moment, but maybe you disagree.

On a different note, I also have not tested performance yet.

[ignatz]

I did have some issues where it mispositioned itself, and one where it did some weird things when rotating. I can't remember exactly what I did to get these behaviours unfortunately.

I've forgotten the exact implementation of the zoom buttons, but I believe the issue might arise when there are uncertain sizes, hence why SizedBox.expand also works sometimes?

What happens with just using a ColoredBox with no other widgets? As far as I can remember, FM sized it a little strangely?

The zoom buttons are implemented as a column of FloatingActionButtons. I removed the alignment and used the Column's mainAxisAlignment + a row to push 'em back to the bottom right. The Row and Column should fully expand into the parent's constraints.

I also added a marker layer (with the MobileTransformer) to make sure to have both in the stack and added a semi-transparent colored container with a margin:

Rotating seems fine. Did you have the issues with this revision of your code? I'm a bit baffled because just by reading the Stack's documentation I would expect issues but I'm not having any. Should I be excited? 🤔 Did you maybe have the Stack within the OverflowBox at some point?

[JaffaKetchup]

I can't seem to reproduce the issue now either. Maybe I managed to reproduce it then made some changes afterward that fixed it without me realising.
I guess that means it can be left how it is (other than renaming 'mobile' to 'dynamic') :D

[mootw]

looks good

[mootw]

renaming anchorPose to alignment is breaking but a better name. just need to make sure documentation exists for that change

[JaffaKetchup]

Yep, it was changed because anchor was a poor name that could be considered to mean the opposite of what it actually did.

Migration info is already written (check the v6 docs).