Reworked retina mode behaviour

[bramp]

Adds a new retinaMethod member to TileLayer that takes the following values, auto, server, and simulate. As well as updated the Examples page to help test/debug.

To avoid the issue with needing the BuildContext to get the device's density I left the boolean retinaMode to toggle the feature on and off. The idea is users can do this:

TileLayer(
    retinaMode: MediaQuery.of(context).devicePixelRatio > 1.0,
),

and it'll use the best method available (server, then simulate). However, they can override the method by setting TileLayer.retinaMethod.

Now, I think more work is needed with the min/maxZoom. It seems the intention is if the server supports 19, we should not allow zooming past 18 when simulating. Since at 18, the zoomOffset is actually fetching 19s. However, it seems to me that maxNativeZoom should be changed, not maxZoom. But before I go down that I wanted to get early feedback on the approach.

FYI, on my Mac (which has a 2x) you can very clearly see the improvement:

and finally, it seems desktops typically stick at 1x, but Android/iOS devices are all 3-4x these days.

thanks

[JaffaKetchup]

I've reworked the initial implementation a bit, let me know if you agree with what I've done.

retinaContext now takes BuildContext?. If it's null (or represents a low density display), and retinaMethod is in auto, then retina mode is disabled.
Otherwise, auto just means preferServer. preferServer falls back to simulating it. forceSimulation is now pretty much for testing only, and there's an annotation on it to make it clear.

[bramp]

Storing the BuildContext makes me uneasy, because that's not a pattern I see other widgets do, and atleast stackoverflow discourages it. If we accept it in the constructor we should quickly use and discard.

[JaffaKetchup]

@bramp Yes, you're quite right that it's bad practise to store a BuildContext, as it leads to potential mistakes when it becomes unmounted. However, I also wanted to reduce the resistance for the user to enable retina functionality.

I've made some larger changes again, as I've realised some of what we did was not necessary.
Here's how to achieve each mode:

• Disabled (default)
  • retinaContext: null (default)
  • forceRetinaMode: false (default)
• Automatic, including disabled
  • retinaContext: passed
  • forceRetinaMode: false (default)
• Automatic, excluding disabled
  • retinaContext: null (default)
  • forceRetinaMode: true

For other combinations, "@2x" and "nothing" can still be used in the URL to achieve the desired result (ie. forcing simulation can be done just by omitting '{r}').

[JaffaKetchup]

You mentioned about maxZoom vs nativeMaxZoom when it comes to simulating retina mode. I agree that this needs to be fixed, maybe just by also decreasing nativeMaxZoom in addition to maxZoom?

[bramp]

Thanks, ok it seems to be converging. Three general thoughts:

1. Now we have useServerRetina and useSimulatedRetina what happens if they are both true? That's ambigious, thus a enum may be preferred with the three states, None, Server, Simulated.

2. The API from previous version to this version is now broken, we've dropped the bool retinaMode, and almost replaced it with forceRetinaMode. I wonder if there is a way to keep retinaMode working.

3. Instead of accepting a context, I wonder if we can provide a new function/helper that ends up just passing a bool into the TileLayer, for example.

   TileLayer(
     retina: RetinaMode.isHighResolution(context)
   )

   I'm generally concerned that TileLayer may be too complex. In fact, is there a long term plan to refactor it? It seems it has lots of different concerns, that can be refactored out into base class, or sub classes.

[bramp]

I don't really understand the purpose of maxZoom. At a certain level the TileLayer just hides itself. That seems a great feature if I'm intentionally displaying a overlap that I want to hide. But if this is the base layer, it seems a bad user experience to make the map just disappear.

But yes, at a minimum I think maxNativeZoom needs to be changed.

[JaffaKetchup]

Now we have useServerRetina and useSimulatedRetina what happens if they are both true? That's ambigious, thus a enum may be preferred with the three states, None, Server, Simulated.

They are never true at the same time. Check the logic in the TileLayer constructor.

The API from previous version to this version is now broken, we've dropped the bool retinaMode, and almost replaced it with forceRetinaMode. I wonder if there is a way to keep retinaMode working.

Whilst we could add a deprecation, I'm not sure it's in the best interest of users. Previously, it referred to simulating retina mode, but, as you found out, this was not very clear. This is such a different way of thinking of things that it might be more confusing to keep it in.

Instead of accepting a context, I wonder if we can provide a new function/helper that ends up just passing a bool into the TileLayer, for example.

If we were to do that, then that might allow the collapsing of the two options as we have now. I'll see what I can figure out.

I'm generally concerned that TileLayer may be too complex. In fact, is there a long term plan to refactor it? It seems it has lots of different concerns, that can be refactored out into base class, or sub classes.

At the moment, there is no long term plan to refactor it - there are parts that are in more need of this kind of attention (for example, the gesture handling code). I'm also not sure which parts could be refactored out - they all directly relate to the display of tiles. Although it looks complex when looking at the source, most users will only use two or three properties.

I don't really understand the purpose of maxZoom. At a certain level the TileLayer just hides itself. That seems a great feature if I'm intentionally displaying a overlap that I want to hide. But if this is the base layer, it seems a bad user experience to make the map just disappear.

That's why we're trying to dissuade it's usage from v6 onwards. There is indeed no good reason to use it on the base layer, however, there is no distinction between the base layer and other tile layers.

[bramp]

They are never true at the same time. Check the logic in the TileLayer constructor.

At the moment, but in future code changes could change that invariant. Just defensive coding.

Whilst we could add a deprecation, I'm not sure it's in the best interest of users. Previously, it referred to simulating retina mode, but, as you found out, this was not very clear.

That's fair. I guess I read it as "use retina" but you are right, it was "simulate retina". So yes perhaps it would be a breaking change if we change it from simulate to use best method. But maybe that's a change for good? But if property is renamed, I do think the old one should stay there with a deprecation warning / assert failure, to help folks as they upgrade.

I'm also not sure which parts could be refactored out

I was referring to the mix of WMS, and Network (subdomains, urlTemplate/fallbackUrl, etc). Asset Provider don't use url, but they do need paths (so the naming is a little inconsistent).... But you point is taken, there are perhaps better places.

Ok as for this change. I raised my concerns, but happy for you to submit what you feel is best. One last change is to remove the API key from the Example.

[JaffaKetchup]

Let me know what you think of the latest commit, we've now massively simplified this, and I can't understand why it needed to be so complicated - my fault!

In terms of deprecation, we're now using it again, so there's no opportunity for deprecation. I'll add the changes to the migration instructions.

[JaffaKetchup]

At the moment, but in future code changes could change that invariant. Just defensive coding.

This is now resolved as well, re-using the RetinaMode wrapper.

[bramp]

Cool cool. So what's lost now, is allow folks to set which method they prefer (which make testing easier)? I guess that's ok because a user can control that with the {r}.

[JaffaKetchup]

So what's lost now, is allow folks to set which method they prefer (which make testing easier)? I guess that's ok because a user can control that with the {r}.

Yep, I factored that in. No need to complicate retinaMode with that (which is what was happening in revision 1 and 2).

[JaffaKetchup]

I know this was a lot of my commits, so I probably shouldn't be self approving this, but we need to get this moving to get v6 released at some point, so I'm bypassing normal processes :)!

Happy to make changes if any of the maintainers would like that.