Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added memory caching support to FlutterMapNetworkImageProvider #1629

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented Aug 29, 2023

Fixes #1628.

Implemented a simple equality operator that just compares the URL. If a fallback URL is specified, caching is disabled to avoid potential issues.

There's no point comparing the HTTP BaseClient, because they are never equal unless they are identical, which they are not. Deeply comparing the headers map could be expensive, and usually headers don't change within the same session anyway.

@Robbendebiene
Copy link
Contributor

Now looking at the code, I feel like adding the equality check doesn't quite do the trick. This is what I would expect now:

The fallback image will be cached if the original isn't available.
However when requesting a new tile that fails to load as well, the fallback image will be fetched again for this tile since its key depends on the tile url AND fallback url.

Additionally the provider will not try to re-fetch the true tile image once the fallback image is in cache, which feels wrong.

@JaffaKetchup
Copy link
Member Author

That's complicating things quite a bit, and getting to the point of needed a full caching logic, which is out of scope. Personally, I think that fallbackUrl is used infrequently enough for this not to be a concern.

the fallback image will be fetched again for this tile since its key depends on the tile url AND fallback url.

Isn't that what we want. We want the fallback URL to be fetched whenever the normal URL doesn't work. The fallback URL might refer to a different image every time (for example, using Mapbox's tile server, but falling back to OpenStreetMaps should that be necessary).

Additionally the provider will not try to re-fetch the true tile image once the fallback image is in cache, which feels wrong.

If a tile fetch fails, the most likely issue is that the server is broken or unreachable. If broken, the chance of it being repaired in the same session is tiny. If it's unreachable, then it might change, but oh well I guess ;)

@Robbendebiene
Copy link
Contributor

Isn't that what we want. We want the fallback URL to be fetched whenever the normal URL doesn't work. The fallback URL might refer to a different image every time (for example, using Mapbox's tile server, but falling back to OpenStreetMaps should that be necessary).

Okay I thought the fallback URL is used to show an image with some text like "no tiles can be loaded", but that's what errorImage is for.

If a tile fetch fails, the most likely issue is that the server is broken or unreachable. If broken, the chance of it being repaired in the same session is tiny. If it's unreachable, then it might change, but oh well I guess ;)

Let's say a server discards a response because of high load or too many tile requests then the fallback tile will be loaded. If they provide the same image then everything is fine. But if it is a different tileset then one might end up with a map of mixed tiles. To me this smells like this will raise issues 😅

If the TileLayer itself would decide to either load the primary url or the fallback url the problem I've described couldn't occur. It would also speed up fallback tile loading (since it doesn't have to try the primary url for each tile again) and FlutterMapNetworkImageProvider would no longer be necessary since NetworkImage could be used then.

I can certainly understand that this may be out of scope or undesirable from a coding perspective. Perhaps fallback url should have never been added in the first place and instead something like the ImageErrorWidgetBuilder should have been used. 🙈

@JaffaKetchup
Copy link
Member Author

Let's say a server discards a response because of high load or too many tile requests then the fallback tile will be loaded. If they provide the same image then everything is fine. But if it is a different tileset then one might end up with a map of mixed tiles. To me this smells like this will raise issues 😅

That's a good point. I guess that means we can either close this PR and leave it how it is, or merge this and accept that it may occasionally cause issues. I can't think of anything in-between. Only using the primary URL in the equality won't help, as the fallback URL should usually match one and only one primary URL. I'll leave this up to someone else to decide.

As an afterthought, perhaps one solution would be to disable the caching if a fallback URL is in use, but that has its own technical difficulties, and also breaks the expected contract of equality: outside state should not affect it.

If the TileLayer itself would decide to either load the primary url or the fallback url the problem I've described couldn't occur.

I'm not sure TileLayer can reliably test whether the primary URL is accessible without sending requests. If we tried to make it send an initial request in initState, then that's very complicated, but also wouldn't catch where the primary URL goes 'offline', due to let's say a rate limit. This is also very out of scope of TileLayer: its job is not to send requests.

@Robbendebiene
Copy link
Contributor

As an afterthought, perhaps one solution would be to disable the caching if a fallback URL is in use, but that has its own technical difficulties, and also breaks the expected contract of equality: outside state should not affect it.

Since they key doesn't have to be the image provider itself (see for example) I think this would be a valid solution. To me it all depends on how the fallback url is supposed to be used. If it is meant to serve the exact same tiles just from another server then they should simply be cached like the current PR does. Still in combination with the retry client I would expect the fallback tile loading to be slow since it probably first does a number of retries till it loads the tile from the fallback url (and that for every single tile).

I'm not sure TileLayer can reliably test whether the primary URL is accessible without sending requests. If we tried to make it send an initial request in initState, then that's very complicated, but also wouldn't catch where the primary URL goes 'offline', due to let's say a rate limit. This is also very out of scope of TileLayer: its job is not to send requests.

I think there is room for some solutions. For example even the TileLayer itself has a chance to detect tile loading errors. So either it could internally switch to the fallback url itself or provide a callback method that allows users to decide what should happen on loading errors.

There could also be something like a FallbackImageProvider that takes another normal image provider (similar to Flutter's ResizeImage) and the fallback url. Its key is then based on the wrapped image provider or a new (network) image provider with the fallback url supplied as the url.

But at the end these are just ideas. Unfortunately I don't have the time to test them.

@JaffaKetchup
Copy link
Member Author

@Robbendebiene Ok, I've changed this to prevent equality (and caching) if fallbackUrl is non-null (unless they are identical, which is necessary to avoid null-check errors inside Flutter's ImageCache logic itself). I've also updated the documentation to reflect this.

@Robbendebiene
Copy link
Contributor

Given the good documentation, that's fine with me :)

Copy link
Collaborator

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Network images should be cached by URL
3 participants