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

Picture cache for slow connectivity / offline #413

Open
Tracked by #863 ...
monsieurtanuki opened this issue Jun 27, 2021 · 14 comments
Open
Tracked by #863 ...

Picture cache for slow connectivity / offline #413

monsieurtanuki opened this issue Jun 27, 2021 · 14 comments
Labels
fixed ? This bug might already be fixed. If so, close it. local cache Offline - Browsing Offline 🎯 P1
Milestone

Comments

@monsieurtanuki
Copy link
Contributor

monsieurtanuki commented Jun 27, 2021

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

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 M123-dev added this to the V0.9 - Soft launch milestone Jun 27, 2021
@M123-dev M123-dev assigned M123-dev and unassigned M123-dev Jun 27, 2021
@M123-dev
Copy link
Member

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
Copy link
Contributor Author

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

@M123-dev
Copy link
Member

Then I can take care of it 👍🏻

@M123-dev M123-dev self-assigned this Jun 27, 2021
@monsieurtanuki
Copy link
Contributor Author

Yes you can!

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

@M123-dev M123-dev removed their assignment Jun 30, 2021
@monsieurtanuki
Copy link
Contributor Author

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

@renefloor
Copy link

@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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

bhattabhi013 commented Mar 21, 2022

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

@monsieurtanuki
Copy link
Contributor Author

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 teolemon modified the milestones: V0.9 - Soft launch, V1.1 Apr 11, 2022
@monsieurtanuki monsieurtanuki removed their assignment May 11, 2022
monsieurtanuki pushed a commit that referenced this issue May 13, 2022
…t also for #413 (short living) (#1600)

* Files cache

* Support for both long & short living caches

* Update packages/smooth_app/lib/cache/files/files_cache.dart

Co-authored-by: Marvin M <39344769+M123-dev@users.noreply.github.com>

* Change for the name of the cache

* Non nullable cache name

* Introduce some APIs changes as suggested by monsieurtanuki

* Change of the subdirectory name

* Only two caches are available in the app: temporary & persistent

* Some changes

* A few more doc

* Fix wrong import in tests

Co-authored-by: Marvin M <39344769+M123-dev@users.noreply.github.com>
@teolemon teolemon modified the milestones: V1.1, Offline Scan Jun 30, 2022
@teolemon teolemon added the fixed ? This bug might already be fixed. If so, close it. label Apr 29, 2023
@teolemon
Copy link
Member

teolemon commented May 9, 2023

@monsieurtanuki I believe we can close this one ?

@monsieurtanuki
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed ? This bug might already be fixed. If so, close it. local cache Offline - Browsing Offline 🎯 P1
Projects
Status: 📋 Todo (ready 2 dev)
Development

No branches or pull requests

5 participants