Picture cache for slow connectivity / offline

monsieurtanuki commented 3 years ago

What

Product pictures should be cache-able, in order to deal with the slow connectivity and offline mode.
Probably it should be in the user settings, something like "Never store pictures on the device", "Keep the pictures until they reach n Mb / until they are m weeks old".

Part of

863

Screenshots

Simulator Screen Shot - iPhone 8 Plus - 2021-06-27 at 19 27 31

Capture d’écran 2021-06-27 à 19 28 16

M123-dev commented 3 years ago

Are you planning to add the pictures to the database, otherwise there is a great package for exactly that: https://pub.dev/packages/cached_network_image

monsieurtanuki commented 3 years ago

No plans for the moment, just the initial idea. But a database is not a good option for storing images like that.

M123-dev commented 3 years ago

Then I can take care of it 👍🏻

monsieurtanuki commented 3 years ago

Yes you can!

I'm a bit dubious about the package you mentioned though:

in one issue it looks like they're using sqflite, which seems to be overkill (https://github.com/Baseflow/flutter_cached_network_image/issues/607)
it looks like they're overriding another package (https://pub.dev/packages/flutter_cache_manager) - maybe we'll be better off with the parent package
remember that we need to handle the following cases - "I don't have space on my device, get lost with your cache"; "stale" pictures; LRU cache garbage collecting when we exceed a given amount of space (user settings)

monsieurtanuki commented 3 years ago

@M123-dev I'm going to work on the subject. It's also related to #41.

renefloor commented 3 years ago

@monsieurtanuki I just noticed this ticket due to the mention. I can comment on your points.

in one issue it looks like they're using sqflite, which seems to be overkill

I thought it was a good idea at first, but for Windows and Linux I implemented a json file which is completely loaded into memory, so it's way faster. However, for the other platforms I still need to switch the default and make sure the existing data is migrated. Note that sqflite is not used to store the files, but information about the files.

it looks like they're overriding another package

At first this was 1 package, but I got a lot of demands for caching other stuff than images, so I decided to extract the caching mechanism into a separate package for those users. So the cache manager is specifically made for CachedNetworkImage, not the other way around.

remember that we need to handle the following cases - "I don't have space on my device, get lost with your cache"; "stale" pictures; LRU cache garbage collecting when we exceed a given amount of space (user settings)

This package needs improvements on checking the remaining storage on the device. Currently you can only configure the number of items in the cache. The files that have not been used for the longest time are removed when the cache becomes too big. You can also set the maximum age of a cache file, for example remove files if you don't use them for a week.

You can also completely clear the cache in 1 call if you want.

So you can create your own CacheManager with custom settings and supply that to CachedNetworkImage.

monsieurtanuki commented 3 years ago

Thank you @renefloor for your answers! Today caching is not a top priority for this project.

But beyond that the two aspects that would potentially bother me are

the use of sqflite (and I do understand how painful tap-dancing with migrated-unmigrated-migrating data and code can be) (perhaps creating a "CacheRepository" interface implemented by both the JSON and the sqflite code, coding a sqflite2JSON migration method, and using the legacy version (sqflite or JSON) as default)
the possibility to set a total max size - I'm sure it wouldn't be too hard to store the size of the files and to LRU-GC them accordingly (regardless of computing the actual remaining storage)

renefloor commented 3 years ago

perhaps creating a "CacheRepository" interface implemented by both the JSON and the sqflite code, coding a sqflite2JSON migration method, and using the legacy version (sqflite or JSON) as default

Yes indeed, we already have the interface, the sqflite implementation, the json implementation and even the migrate function. So we'll only have to make sure this is somehow executed by default when it is not migrated yet.

the possibility to set a total max size - I'm sure it wouldn't be too hard to store the size of the files and to LRU-GC them accordingly (regardless of computing the actual remaining storage)

This is indeed not that hard and on my backlog, but I have to do it in my free time, so even things that are not too hard will be postponed when I'm busy with other stuff.

monsieurtanuki commented 3 years ago

The tricky part is "make sure this is somehow executed by default", right? My suggestion: don't do it "somehow" and "by default", make the choice and the migration explicit for developers, that will either ignore that and keep using the old sqflite version, or have an interest in migrating to JSON.

renefloor commented 3 years ago

Yeah true. But I also kind of want to get rid of the sqflite implementation in the code and move it to a separate package, so I want to make it as easy and clear as possible. But that is indeed the tricky part.

bhattabhi013 commented 2 years ago

Hi everyone, I've used CachedNetworkImage package for image caching purpose. Can that be used in this project too?

monsieurtanuki commented 2 years ago

Hi @bhattabhi013!

Your package looks very expensive: just one of its dependencies (flutter_cache_manager) needs clock, collection, file, flutter, http, path, path_provider, pedantic, rxdart, sqflite, uuid. Especially sqflite sounds like a 3rd "database" (after sharedpreferences and hive), that sounds too much to me.

I think it would be feasible without all that fuss, given that the product image url are kind of formatted:

https://static.openfoodfacts.org/images/products/359/671/046/2858 (for barcode 3596710462858, cf. ImageHelper.getProductImageRootUrl)
front_fr.4.100.jpg (ImageHelper.getProductImageFilename)
in our case we could put in cache a file called 3596710462858_front_fr.4.100.jpg - if there's a file with that name, it's our cache, if not it's not cached

What do you think about that huge simplification?

teolemon commented 1 year ago

@monsieurtanuki I believe we can close this one ?

monsieurtanuki commented 1 year ago

@teolemon As far as I know, no we can't.

The idea here is to cache product pictures that we display and download, so that we get a picture even with no/limited connectivity next time. This is not implemented.

What is implemented is the background upload of edited pictures, with transient local pictures.

Not the same thing.

openfoodfacts / smooth-app