PR for review only - Feature/generic cache manager #6

FastNinja · 2017-12-28T00:07:44Z

No description provided.

FastNinja · 2017-12-28T00:09:43Z

lib/cached_network_image.dart

@@ -71,6 +71,10 @@ class _CachedNetworkImageState extends State<CachedNetworkImage> {
 }

 class CacheManager {
+  static Duration inbetweenCleans = new Duration(days: 7);
+  static Duration maxAgeCacheObject = new Duration(days: 30);
+  static int maxNrOfCacheObjects = 200;


How would developers be able to change these config parameters . The inbetweenCleans for example might be too big?
Same with maxNrOfCacheObjects. if objects are small I might opt in to store more..?

These are public statics, so they can be set when you start the app.

FastNinja · 2017-12-28T00:11:45Z

lib/cached_network_image.dart


    //get saved cache data from shared prefs
-    var jsonCacheString = prefs.getString("lib_cached_image_data");
-    cacheData = new Map();
+    var jsonCacheString = _prefs.getString("lib_cached_image_data");


should CacheManage know that it caches images? lib_cached_image_data implies it knows.

The key is lib_cached_image_data just because I called it that when I made this library. If we change it we should also add some migration and I didn't think that was worth it.

FastNinja · 2017-12-28T00:13:16Z

lib/cached_network_image.dart

      });
    }
+
+    // Get data about when the last clean action has been performed
+    var cleanMillis = _prefs.getInt("lib_cached_image_data_last_clean");


instead of putting comment you could extract method with the same name
lastCacheClean = _getLastCleanTimestampFromPreferences();

Yes, I probably have to refactor a bit. I also want to extract the classes in multiple files.

FastNinja · 2017-12-28T00:17:01Z

lib/cached_network_image.dart

+    _prefs.setString("lib_cached_image_data", JSON.encode(json));
+
+    if(await _shouldSaveAgain()){
+      await _saveDataInPrefs();


looks like _saveDataInPrefs calls _saveDataInPrefs again and uses number of special flags to control that behavior and avoid infinite loop.
what is the reason we need to do Save and then again Save?

The logic I use here is the following.
Imagine you download 5 files simultaneously. You would end up in 'Save' 5 times. If the first one is still busy saving the other 4 just indicate that something has changed and return. The first one will save one more time in the end, resulting in 2 saves instead of 5. It also only cleans the cache once.

FastNinja · 2017-12-28T00:18:34Z

lib/cached_network_image.dart

+      await synchronized(_lock, () async {
+        var oldestDateAllowed = new DateTime.now().subtract(maxAgeCacheObject);
+
+        //Remove old objects


can be function _removeTooOldToKeepObjects (i.e. comment will not be necessary)

Again, I'll refactor it a bit.

FastNinja · 2017-12-28T00:24:46Z

lib/cached_network_image.dart

+        if (_cacheData.length > maxNrOfCacheObjects) {
+          var allValues = _cacheData.values.toList();
+          allValues.sort((c1, c2) => c1.touched.compareTo(c2.touched));
+          for (var i = allValues.length; i > maxNrOfCacheObjects; i--) {


you could simplify and use take(count) to get oldest objects. something like that:

allValues.sort((c1, c2) => c2.touched.compareTo(c1.touched)); // sort OLDEST first allValues.take( _cacheData.length - maxNrOfCacheObjects); // get them allValues.forEach( (item) {await _removeFile(lastItem)} ) //remove them

Good idea, I am still learning dart so didn't think about this.

FastNinja · 2017-12-28T00:27:32Z

lib/cached_network_image.dart

    if (response != null) {
      if (response.statusCode == 200) {
        await newCache.setDataFromHeaders(response.headers);
-        var folder =  new File(newCache.filePath).parent;
+        var folder = new File(newCache.filePath).parent;
        if (!(await folder.exists())) {
          folder.createSync(recursive: true);
        }


looks like sole folders are being created. I am not sure if we delete any of these folders..
what is the reason to have these folders? I thought that files will have just GUIDS as names so could stored flat?

Yes they are practically stored flat. It only creates one folder so all cache files are stored in a folder and cannot conflict with random other files in de cache folder.

FastNinja · 2017-12-28T00:29:22Z

lib/cached_network_image.dart

  setDataFromHeaders(Map<String, String> headers) async {
+    //Without a cache-control header we keep the file for a week
+    var ageDuration = new Duration(days: 7);


interesting. that contradicts earlier Duraton of 30 days (static Duration maxAgeCacheObject = new Duration(days: 30);)
I think this is still CacheManager configuration - it belongs to CacheManager

I didn't really now how to name this variable, but the functionality is different.
The CacheManager configuration is about when to delete a file and checks when the file is last used by the app.
This 'ageDuration' indicates for how long the file is valid and should normally be set by the cache-control. When the user wants the file but it is not valid anymore it has to pull it from the server again whether the file is still in the cache or not. I didn't want to make it infinite as that might result in problems when a file is changed without the url changing.

FastNinja · 2017-12-28T00:31:11Z

lib/cached_network_image.dart

+    touched = new DateTime.now();
+    _map["touched"] = touched.millisecondsSinceEpoch;
+  }
+
  setDataFromHeaders(Map<String, String> headers) async {


setDataFromHeaders is very coupled with HttpHeaders. Cache Objects is not support to know about headers - it knows the object, knows when it was touched etc...
parsing HttpHeaders sounds like an infrastructure concern? Currently Cache Manager knows that files are being downloaded from Http so it shall parse HttpHeaders (or delegate it to some parser etc..)

That is probably a good idea.

FastNinja · 2017-12-28T00:32:20Z

lib/cached_network_image.dart

+      Directory directory = await getTemporaryDirectory();
+      var folder = new Directory("${directory.path}/cache");
+      if (!(await folder.exists())) {
+      folder.createSync();


another place where we create folders ...?

Same folder as earlier. It is probably better to only do it in one place. I did this because otherwise it broke when the file was gone but the filepath was already determined in the cache manager.

FastNinja · 2017-12-28T00:35:24Z

lib/cached_network_image.dart

      //If we have never downloaded this file, do download
      if (cacheObject.filePath == null) {
-        cacheData[url] = await downloadFile(url);
+        _cacheData[url] = await downloadFile(url);
        return;
      }
      //If file is removed from the cache storage, download again


I am not sure why If we have never downloaded this file, do download has to be different to If file is removed from the cache storage, download again.
It can be exactly same flow - High level logic:

if(!exsitsInCache){ fetchFromNetwork(); } return fromCache;

True, but you would get new File(null).exists(), is that allowed? Otherwise you still have to do some checks whether path == null and I don't directly see how this can be made cleaner.

FastNinja · 2017-12-28T00:39:19Z

lib/cached_network_image.dart

@@ -170,11 +270,11 @@ class CacheManager {
    var response;
    try {
      response = await http.get(url, headers: headers);
-    }catch(e){}
+    } catch (e) {}


what is the default timeout on .get

how we can let developers know that get failed with certain error.

how developers can configure/get this error and decide what to do (present another image?)

Didn't think about it yet. I do agree that it is a good idea improve the error handling.

FastNinja · 2017-12-28T00:40:17Z

lib/cached_network_image.dart

+        return true;
+      }
+      _isStoringData = false;
+      return false;


_shouldStoreDataAgain and _isStoringData two very puzzling fields...

See my other comment about saving. It is meant that you don't save more than necessary.

What do you think about the serialization of this data? It was a quick way, but I am not sure if it is the best way. I do think that sqlite is a bit overkill though.

Moved cache manager to flutter_cache_manager Only support ImageProvider from now on

renefloor added 3 commits December 27, 2017 18:23

Added functionality to clean the cache

e1711d0

Made cache manager more generic

2d0343c

Improved synchronization

f421a39

FastNinja commented Dec 28, 2017

View reviewed changes

FastNinja mentioned this pull request Dec 28, 2017

Cache Manager - separate concern #5

Closed

renefloor added 2 commits December 28, 2017 17:16

Moved cache manager to own repo

63679d4

Moved cache manager to flutter_cache_manager Only support ImageProvider from now on

Updated changelog and readme

0123d7f

renefloor closed this Dec 29, 2017

renefloor deleted the feature/generic_cache_manager branch January 8, 2018 17:57

KingIdee mentioned this pull request Mar 27, 2018

Failed assertion: line 362: '_codec != null': is not true #26

Closed

volgin mentioned this pull request Apr 10, 2018

Cached images stop working after cache expiration time #29

Closed

mig35 mentioned this pull request Apr 20, 2018

Images are not loaded after couple of restarts #40

Closed

virenvvarasadiya mentioned this pull request May 31, 2018

[ERROR:topaz/lib/tonic/logging/dart_error.cc(16)] Unhandled exception: #35

Closed

ryanhz mentioned this pull request Sep 12, 2018

Crash on iPhone 4S: FormatException: Invalid radix-16 number #75

Closed

alexJetski mentioned this pull request Oct 20, 2018

Invalid radix-16 number error #87

Closed

mayurprajapati mentioned this pull request Mar 3, 2019

Error when passing the invalid argument #137

Closed

anjrew mentioned this pull request Mar 11, 2019

Unhandled Exception: HttpException: No valid statuscode. Statuscode was 404 #144

Closed

xiaoxiaocainiao mentioned this pull request Mar 14, 2019

Unhandled Exception: SocketException: OS Error: Connection reset by peer, errno = 104, address = www.wangyan.im, port = 38400 #145

Closed

duanyuGit mentioned this pull request May 24, 2019

android.database.sqlite.SQLiteDatabaseLockedException: database is locked #184

Closed

prasant10050 mentioned this pull request Oct 15, 2019

Library is incompatible with latest Flutter master #261

Closed

cgowthamanmca mentioned this pull request Oct 17, 2019

This issue occured #269

Closed

xiaodeng2014 mentioned this pull request Oct 18, 2019

Error: The method 'CachedNetworkImageProvider.load' has fewer positional arguments than those of overridden method 'ImageProvider.load' #270

Closed

lmanliang mentioned this pull request Mar 9, 2020

Can't Load Img with Special symbol #347

Closed

yaofuyu mentioned this pull request Apr 1, 2020

Flutter to App Native the image load faild #357

Closed

2 tasks

cgowthamanmca mentioned this pull request May 6, 2020

Could not instantiate image codec. #379

Closed

expilu mentioned this pull request Jul 23, 2020

Tests throw an exception #432

Open

2 tasks

wijesijp mentioned this pull request Aug 13, 2020

Does not work on Android 10 device release build #441

Closed

DominicOrga mentioned this pull request Dec 1, 2020

Images not rendering after updating to 2.4.0 #514

Closed

sandeep9607 mentioned this pull request May 9, 2021

CacheManager: Failed to download file from url #336

Open

bxxasn mentioned this pull request Nov 3, 2022

Exception: Invalid image data #797

Open

ProvMitraDora mentioned this pull request Jan 6, 2023

Facing issue when running flutter app as module in android. #813

Open

sudha-anecure mentioned this pull request Jan 10, 2023

Various Database Exceptions #815

Open

2 tasks

kirill-21 mentioned this pull request Sep 27, 2023

Bad state: Future already completed #880

Open

imijanur mentioned this pull request Sep 28, 2023

Failed to decode image #837

Open

2 tasks

tanmaysuryawanshi mentioned this pull request Dec 22, 2023

Unable to use cached_network_image with widget_mask #905

Open

Khondwani mentioned this pull request Mar 16, 2024

Invalid argument(s): No host specified in URI #931

Open

karbanr mentioned this pull request Jun 18, 2024

ImageLoader.loadImageAsync.<fn> FlutterError - Exception: Invalid image data. Error thrown . #953

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR for review only - Feature/generic cache manager #6

PR for review only - Feature/generic cache manager #6

FastNinja commented Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017 •

edited

Loading

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

FastNinja Dec 28, 2017

renefloor Dec 28, 2017

PR for review only - Feature/generic cache manager #6

PR for review only - Feature/generic cache manager #6

Conversation

FastNinja commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FastNinja Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FastNinja Dec 28, 2017 •

edited

Loading