Replace the flutter_image package with RetryClient from the http package

[josxha]

Since flutter_image seems to have a very low priority from the flutter development team, I followed this tip and replaced the package with the RetryClient from the http package. I tested my pull request with the example app and it worked fine.

The PR would also make flutter_map independent of flutter_image in the future.

[kengu]

@johnpryan I think this is a good approach to the lack of maintenance of flutter_image. Removing a dependency is good practice when the usage is limited to a single use-case (here NetworkImageWithRetry). It increases the code footprint slightly, but not to a degree that is concerning.

It would introduce a breaking change that need to be noted in the change log @josxha

[josxha]

@kengu Yes you are right. I haven't touched the change log because the package version hasn't been raised and no new section exists in the change log yet.
I can look over the pull requests #870 and #851 and start collecting release notes.

[kengu]

@josxha That would be great! Establishing an entry for version 0.13.0 and adding this change would suffice for me to approve the change (if the community agree with it).

[josxha]

@kengu How should I proceed with the mentions? Are only committers counted or also code reviewers?
The mentions in the last release notes:
Thanks to gr4yscale, maRci002, MooNag, tlserver, 6y

[kengu]

@kengu How should I proceed with the mentions? Are only committers counted or also code reviewers?
The mentions in the last release notes:
Thanks to gr4yscale, maRci002, MooNag, tlserver, 6y

Both collaborators and code authors I guess.

[josxha]

I hope these are all the changes.
The release date is currently blank and has to be set when the time comes.

[kengu]

LGTM

[mootw]

I think this is a good change. Removing dependencies that are use-case specific is important (a feature that has a hanging dependency). I think this is a good middle ground for this since most things use http we are simplifying the dependency chain.

[kengu]

@josxha / @moonag I've done some testing with the example app and there are significant difference in behaviour between NetworkImageWithRetry implemented here and in flutter_image. More specific, caching is much poorer in the new implementation. See below.

Behaviour without flutter_image
https://user-images.githubusercontent.com/866528/118354562-eac77380-b56b-11eb-9329-e2d1399a7e2f.mp4

Behaviour with flutter_image
https://user-images.githubusercontent.com/866528/118354582-fca91680-b56b-11eb-9e61-22a8a9e32a48.mp4

I think we should stick to flutter_image now that it is migrated to null-safety. We still need to wait for them to release a new version before we can release a new version of flutter_map.

[mootw]

@kengu caching should be handled by your own implementation and have nothing to do with the image provider. Check the Readme section of flutter map for how to implement caching. Ping me if you would like me to help you (I handled that section) or make a sample project with and without flutter_image. If that does not make the caching consistent then ping me and I will look into it because then it would be a bug that I need to fix.

[kengu]

@moonag I'm not able to do any changes to this PR, OP @josxha needs to do that.

My point it that changing to this new implementation of NetworkImageWithRetry regress to a less usable behaviour compared to NetworkImageWithRetry implemented in flutter_image (play and compare the two videos above). The issue is not about caching per se, it's about a less usable solution compared to what we are replacing.

Now that flutter_image is part of the core plugins maintained by the Flutter Team, I don't see the need to replace it any longer.

[ThexXTURBOXx]

Nice to see that my suggestion has been implemented here already ^^

I just wanted to do that myself, so thanks @josxha for doing this yourself and sparing my time here :)

[ThexXTURBOXx]

I updated the codebase of this branch to the one from the official repo. See here: josxha#1
As soon as @josxha merges this into his branch, this PR can be accepted and merged into the official one (along with some other minor fixes)

[josxha]

I find @kengu's cache test very interesting. flutter_image clearly offers the better "out of the box" solution here.
However, I also find @ThexXTURBOXx's view plausible that the user should be given complete freedom over caching.
I would therefore find it difficult to choose between the two.

I have merged the pull request josxha#1 so that a potential release is not stuck with me. It looks good to me.

[ThexXTURBOXx]

I also find the cache test indeed very interesting. I also think, this should really be handled by the user. For this very reason, there already exist some libraries that cover this behavior. See https://github.com/JaffaKetchup/flutter_map_tile_caching for example.

[ibrierley]

I think whatever is chosen, it's very key to make sure there's some documentation about it in the readme (and ideally suggest the alternatives).

[kengu]

@ThexXTURBOXx My main reservation it that this change results in unexpected behaviour for current users of NetworkTileProvider. Even with proper warnings in changelog and readme, it is highly probable that someone is going to file a bug.

[mootw]

I just wanted to chime in because I am the one to ping about caching.

For this package caching is supposed to be handled by the user because every person needs a different solution. I have documented in the Readme turn-key disk/memory caching solutions.

I suspect that the flutter_image package is providing memory caching. I think that is the package I use in the Readme as the memory caching solution.

When using this pr please use one of the caching solutions in the Readme (which involves creating your own tile provider) to see if the issue persists. If it does persist it is an issue with this pr.

It is likely people will see that as a bug and report it. If they do tell them to read the Readme. We can alleviate it by including the breaking change in the change log instructing people to read the caching section of the Readme.

[ThexXTURBOXx]

@moonag Actually, after some research, I found out that the flutter_image does not explicitly cache stuff. The problem is that the http package's client does not cache, unlike the io package's client, which does cache. There are already several issues opened for http that suppose a cache manager or similar to enable caching by default, but for io it seems to be even undocumented behavior, which is unwanted in many cases (according to many issue reports).

Since it is undocumented behavior, it would be even better to move away from this old behavior to implementing this caching by ourselves using flutter_cache_manager for example, such that this behavior isn't fixed in a future release of Flutter and then people will complain.

[JaffaKetchup]

Hi all,
If you want to remove flutter_image, but still need good caching, please check out the plugin: https://github.com/JaffaKetchup/flutter_map_tile_caching

[ThexXTURBOXx]

@JaffaKetchup I already mentioned your plugin here :)

[mootw]

That plug-in implements an even more turn key solution. It came as a result of us deciding any caching is going to be handled outside of flutter_map. That plugin was created and I added documentation to the Readme. To think of it I should also add mention to that plug-in in the Readme.

Removal of flutter_image from flutter_map is the right decision. The only value it provides is NetworkImageWithRetry and it also exhibits unexpected caching behavior as it is caching where it maybe shouldn't be. Users of flutter_map should use a proper caching image provider. The change in behavior will probably break some people's projects but it will be for the better; and they will know about the breaking change in a change log entry with an explanation of the change and how to fix it by reading the Readme and or using a flutter_map plugin or using a caching image provider they create in house or on pub.dev

[JaffaKetchup]

@ThexXTURBOXx It seems like you did indeed! GitHub was hiding some comments from me, so I couldn't see it without clicking to show more. :)

@moonag I made a pull request (#869) a few weeks back adding my plugin and removing the information about self-implementation of caching that you added in (in PR #841) because it seemed less useful with the new plugin. If you want to add your guide back in, just let me I know and I can open a new PR if you don't want to do it.

However, I also agree that http is a better package choice than flutter_image for exactly the reason @moonag said (#894 (comment)).

[mootw]

@JaffaKetchup I think we should keep the docs for self-implemented caching and the little paragraph I had explaining how the chain of ImageProvider->TileProvider-> what gets displayed on the map works to make sure that caching makes sense to people and isn't just a black box. Or if people want to use flutter_image, or some other image provider they are already using, have made in house, or prefer more than the black box plugin. It is just a few lines of code and it will still work as long even if the caching plugin for flutter_map isn't maintained anymore. My personal preference for dependencies is less is more. Hanging features and things that are user dependent or already have a huge ecosystem (like caching ImageProviders in flutter) CAN have a black box solution for flutter_map but it should be clearly documented that you can cache without an extra black_box dependency or use an image provider you already are using.

I think options are good but just telling people to use a black box solution to something like caching is dangerous when it is can be easily handled by 5 lines of code and an existing well maintained image provider.

[josxha]

@moonag Since it is undocumented behavior, it would be even better to move away from this old behavior to implementing this caching by ourselves using flutter_cache_manager for example, such that this behavior isn't fixed in a future release of Flutter and then people will complain.

As far as I know flutter_cache_manager has been a dependency in earlier versions of flutter_map.

[0.11.0] - 01/29/2021
This version removes various tile providers that depend on plugins. This helps simplify the flutter_map release process. Tile providers can be implemented in your app or in a separate package.
remove mbtiles tile provider + sqlflite dependency (#787)

Glad to hear about flutter_map_tile_caching! 😀

[kengu]

@ThexXTURBOXx Nothing more than switching between the implementations.

@moonag If we are going this route, the broader question then becomes what the real value of having two network providers? The naming of NoCachingNetworkTileProvider which indicates that NetworkTileProvider is caching implicitly.

I think the new implementation of NetworkTileProvider could be renamed to something like NoCachingNetworkWithRetryTileProvider to clarify and maybe help the community to not expect it to cache (anymore)?

[mootw]

@moonag Since it is undocumented behavior, it would be even better to move away from this old behavior to implementing this caching by ourselves using flutter_cache_manager for example, such that this behavior isn't fixed in a future release of Flutter and then people will complain.

As far as I know flutter_cache_manager has been a dependency in earlier versions of flutter_map.

[0.11.0] - 01/29/2021
This version removes various tile providers that depend on plugins. This helps simplify the flutter_map release process. Tile providers can be implemented in your app or in a separate package.
remove mbtiles tile provider + sqlflite dependency (#787)

Glad to hear about flutter_map_tile_caching! 😀

Yeah we removed it because caching is such a user specific thing we wanted to allow for the user to choose how or if they cache their image tiles. Also that dependency was heavy and relied on a lot of other dependencies. Removing it gave a choice of how people want to cache and made flutter_map significantly more lightweight. I added the docs on how to create a tile provider with whatever image provider (that caches or doesn't cache) you want at that time to document the change.

[mootw]

@kengu The expectation should be that NO built in tile providers provide caching. Some might still provide memory caching. There is a built in flutter image provider that might be used in one of the tile providers that does memory caching.

[kengu]

@kengu The expectation should be that NO built in tile providers provide caching. Some might still provide memory caching. There is a built in flutter image provider that might be used in one of the tile providers that does memory caching.

I don't disagree with that, less is more here, and one less dependency is a good thing. But I want to make sure we help the community to understand this through consistent naming of code, not readme alone (which many do not read that carefully).

[JaffaKetchup]

@moonag, did you want me to add back your instructions then, perhaps as a 'if you don't want another package in your project, then you can do this...'? If so, I'll open a new PR at some point soon.
And I think the naming is important, it needs to be clear what caches and what doesn't.

[mootw]

@JaffaKetchup yeah let's open a PR to add the docs back and also mention that there are no built in tile providers that provide caching and also look at modifying this PR to include changes to the built in tile providers naming to make it not sound like they cache.

[JaffaKetchup]

Ok, I'm making a new PR now.

[JaffaKetchup]

#913 is the new PR for what @moonag, @kengu, and I have just been talking about.

[JaffaKetchup]

I have further changes my PR to reflect the new information I have found looking through the source code.
Please see #870 (comment) for my reasoning behind this.