Extends glide disk cache factories

[Tolriq]

Add new DefaultDiskCacheFactory and ExternalCacheDiskCacheFactory
Refactor InternalCacheDiskCacheFactory to use new DefaultDiskCacheFactory

[TWiStErRob]

Nice :)

I have one concern: DefaultDiskCacheFactory gets the path in the constructor, but what if I need to do I/O (e.g. File.exists()) to determine the folder I want to use. build() is called in the background and could call a getDiskCacheFolder() method and the child classes can then override that.
At the same time if you leave the diskCacheFolder field a default implementation of getDiskCacheFolder() can use that field for simple cases.

This way it's possible to use DefaultDCF with a folder in constructor, you can extend the class and override a method. You may also let InternalDCF/ExternalDCF's static methods override that one and they become extensible too.

[TWiStErRob]

Hmm, actually getExternalCacheDir() already does I/O to determine the result:
android.app.ContextImpl#ensureDirsExistOrFilter

[Tolriq]

Well this was my first approach but I found this simpler for newcomers.

No problem to switch to the complex one if @sjudd prefer this one.

The main problem with asynchronous folder get is that if user want to do something else with the folder for any reason it's hard to know when the folder will be created, I guess most users that will use the fully manual way will have created and ensured that the folder exist before.

[TWiStErRob]

I foresee a few issues created for StrictMode.ThreadPolicy.detectDiskReads() where we'll have to change to background anyway.
I agree though I was too trying to get the folder Glide's using for cache after the cache has been created (for debugging), and I had to store it myself because there's no getter for it.

I also just noticed that you changed the default cache behaviour:
before: Glide.getPhotoCacheDir(context), after: context.getCacheDir()
getPhotoCacheDir(context) also appends Glide.DEFAULT_DISK_CACHE_DIR.

Just nitpicking: but what's the point of creating a File then getAbsolutePath and then create the File again? Usually you already have a File object, but the cache interface forces you to convert it to String.

[Tolriq]

Glide.DEFAULT_DISK_CACHE_DIR is private and Glide.getPhotoCacheDir(context) does IO so not wanted in constructor.

I'll let @sjudd decide what is best.

For the String interface, it's because I suppose 90% of users that will need this one will have the file stored as a string either in DB or in preferences. And Android default getCache returns File and can be null.

But I can add a File way too is it's wanted.

[TWiStErRob]

ContextImpl.getCacheDir and ContextImpl.getExternalCacheDir does the same I/O if not more.

[sjudd]

as @TWiStErRob mentioned, this is unfortunately not safe because getCacheDir and getExternalCacheDir both cause strict mode violations.

[sjudd]

One way to structure this so that everything happens on the background thread would be to implement the a default factory with a private constructor and a boolean flag indicating whether the cache should prefer the internal or external cache directory directory. You could then use two static getters to make it clear which one users were going to get, for example:

DiskCache.Factory DefaultFactory.getInternal(Context context) {
    return new DefaultFactory(context, false /*preferExternal*/);
}

DiskCache.Factory DefaultFactory.getExternal(Context context) {
    return new DefaultFactory(context, true /*preferExternal*/);
}

It doesn't help users wanting custom cache locations, but it would let you share a lot of code between the internal and external cache factories and still make it simple for users to pick one.

As @TWiStErRob pointed out, I think most custom locations are going to require doing some kind of I/O to verify the directory either exists or can be created. The problem with passing in a String or a File in the constructor of the factory is that it encourages users to do these checks prior on the main thread. The goal of the factory class is to give users a place to do those checks off of the main thread.

[Tolriq]

Well the primary need is to allow custom cache at least for me :)

What about the 1st solution I envisaged and @TWiStErRob proposed back ? (I'd prefer doing it with a specific constructor that accept an interface to get the folder file from the build ?)

[TWiStErRob]

Are you proposing something like (for a totally custom directory):

glide.setDiskCache(new DefaultDiskCacheFactory(context, new DirectoryFinder(){
     public File findDir(Context context) { return ...; }
}));

because I agree that in separation of concerns it would be the good OO design, but that's just one too many new objects, and it's easier to just override a method if you need to create a new class anyway for finding the dir.

[Tolriq]

Yes this is the idea as it's clean.

I'll take my case as a generality but I do have all needed things to properly prepare the client folder and store it in a preference.

I'm sure most users with a custom directory need will do the same.

So having to create a custom factory to override a function just to return a string is not worth it and have 0 benefit over : https://github.com/bumptech/glide/wiki/Configuration#location

[TWiStErRob]

I think the purpose of a DefaultDiskCacheFactory (or BaseDCF as I see it) is to hide the logic needed to ensure a directory and create the Lru wrapper. In the link you gave there's 0 error handling :) Go ahead, and update the PR, I just wanted to weigh in, that if you create a new class (anonymous in the above example I gave) and create a new object (or 2), you might as well let those thing merge a simplify when a library user uses it.

[sjudd]

I think either approach is reasonable (either abstract base class or an interface), there are pros and cons of both. It might make sense to rename it to DiskLruCacheFactory just to be explicit that the main purpose is to create a DiskLruCache based instance in the given directory.

Either way we should provide default implementations for the internal and external cache directories, since my guess is that those are going to be the most common.

Backwards compatibility is also an issue, but we can address that when we're happy with everything else.

[Tolriq]

PR updated with interface way and full backward compatibility.

[TWiStErRob]

I just thought of something: what if someone wants to give int diskCacheSize = or 30% of available space, then they still need to do I/O on the main thread (in module setup) to read that value. I'm not sure how viable this use case is, I'm happy with the default 250MB.

[TWiStErRob]

Ah never mind :) Advanced users can implement DiskCache.Factory, this PR is about providing an easily movable folder API.

[sjudd]

Looking good, just the two small comments. Thanks for putting this together!

[Tolriq]

Let's hope this is the good one :)

[TWiStErRob]

It's missing from the diff, but this has been added to the only place where build is called from in #374.

[TWiStErRob]

I added comments, but I'm happy with it. Thanks for your patience!
Just out of curiosity: what was the findbugs error in the very first version? (travis is not so verbose about it)

[Tolriq]

@TWiStErRob to be honest you start be really be near the end of my patience ;)

I agree with @sjudd on second comment and 1st comment is handled by previous @sjudd commit if this is what you are talking about.

[TWiStErRob]

I agree with Sam too, that's not a breaking change if added later. Only the interfaces have to be nailed, since they can't really be changed, possibly for a few years.

[sjudd]

Thanks!

[eicm]

A semi related question: based on what factors the library decides to clear
the cached images on Disk

On Tue, Apr 21, 2015 at 2:20 PM, Sam [email protected] wrote:

Thanks!

—
Reply to this email directly or view it on GitHub
#430 (comment).

[TWiStErRob]

@eicm see 3rd paragraph of https://github.com/JakeWharton/DiskLruCache:

This cache limits the number of bytes that it will store on the filesystem. When the number of stored bytes exceeds the limit, the cache will remove entries in the background until the limit is satisfied.

Glide uses 250MB limit by default and creates DiskLruCache with DiskLruCacheWrapper.get.

[eicm]

Thanks

[TWiStErRob]

@sjudd, is Glide.getPhotoCacheDir() deprecated? Because the library doesn't use it any more.