wrapp-archive / WebImage-Android

Asynchronous image loading library for Android
MIT License
41 stars 5 forks source link

Enhancement: Cache directory #2

Closed ghost closed 12 years ago

ghost commented 12 years ago

In ImageCache.java you are not using the android built-in getExternalCacheDir(), to retrieve a path to save the files. Do you have a reason to do this? Maybe it is better to use getExternalCacheDir(). It has some benefits like clearing this directory on uninstall and using the packagename within the path. Maybe you should check the docs on this: http://developer.android.com/reference/android/content/ContextWrapper.html#getExternalCacheDir%28%29

ghost commented 12 years ago

After reading the docs again, maybe getExternalFilesDir() would be a better choice, because these files are kept private to your app only and are excluded from the mediascanner. If getExternalCacheDir() is used, you have to create a ".nomedia" file to exclude it from the mediascanner.

The only problem is that both getExternalCacheDir() and getExternalFilesDir() are available from api-level 8. So if you would like to support api-level 7, then setCacheDirectory() (in ImageCache.java) should check the api-level and use the supported option.

nikreiman commented 12 years ago

Yes, as you noted, getExternalCacheDir() was introduced in API 8, and we need to support Android 2.1 as long as these devices have a non-trivial marketshare (< 5%, and it's currently ~10%). However, I would like to change WebImage to use the Android-recommended storage location, which is /mnt/sdcard/Android/data/com.example.appname. Currently we use /mnt/sdcard/data/com.example.appname, which a few other apps use as well.

ghost commented 12 years ago

Thanks for your changes, but there is still something missing. There should be a subdirectory "/cache" in order to have it deleted on uninstall of the app (API > 7).

Also the packagename should be set to your real app-packagename, otherwise android will not know, this folder belongs to your app. So you should call WebImage.setCacheDirectory(this.getPackageName(), null) from your app. Another option would be to include Context to this library, and set the correct packagename. Either way, this should be made clear in the Readme.

I made some other changes too. Maybe you can have a look at the following code:


    private static boolean isExternalStorageAvailable() {
        return android.os.Environment.getExternalStorageState().equals(
                android.os.Environment.MEDIA_MOUNTED);
    }

    private static boolean isExternalStorageReadOnly() {
        return android.os.Environment.getExternalStorageState().equals(
                android.os.Environment.MEDIA_MOUNTED_READ_ONLY);
    }

    public static void setCacheDirectory(String packageName, String subdirectoryName) {
        if (isExternalStorageAvailable() && !isExternalStorageReadOnly()) {
            // Directory will be like: "/Android/data/<package_name>/cache/<subdirectory>/"
            cacheDirectory = new File(
                    android.os.Environment.getExternalStorageDirectory().getAbsolutePath(), 
                        "Android" 
                        + File.separator + "data" 
                        + File.separator + packageName != null ? packageName : ImageCache.class.getPackage().getName()
                        + File.separator + "cache" 
                        + File.separator + subdirectoryName != null ? subdirectoryName : DEFAULT_CACHE_SUBDIRECTORY_NAME
                        );
            if (!cacheDirectory.exists()) {
                if (cacheDirectory.mkdirs()) {
                    LogWrapper.logMessage("Cache directory '" + cacheDirectory + "' created");
                }
                else {
                    LogWrapper.logMessage("Cache directory '" + cacheDirectory + "' could not be created");
                }
            }
            else {
                LogWrapper.logMessage("Cache directory '" + cacheDirectory + "' is already present");
            }
        }
        else {
            // ExternalStorage is not available or writable
            // Fallback to internal storage or disable filecache or ??
        }
        if (packageName != null) {
            // WebImage 1.1.2 and earlier stored images in
            // /mnt/sdcard/data/packageName. If images are found there,
            // we should migrate them to the correct location.
            // Unfortunately, WebImage 1.1.2 and below also used
            // the location /mnt/sdcard/data/images if no packageName was
            // supplied. Since this isn't very specific,
            // we don't bother to remove those images, as they may belong to
            // other applications.
            final File oldDataDirectory = new File(android.os.Environment.getExternalStorageDirectory(), "data");
            final File oldPackageDirectory = new File(oldDataDirectory, packageName);
            final File oldCacheDirectory = new File(oldPackageDirectory, subdirectoryName);
            if (oldCacheDirectory.exists()) {
                if (cacheDirectory.delete()) {
                    if (!oldCacheDirectory.renameTo(cacheDirectory)) {
                        LogWrapper.logMessage("Could not migrate old image directory");
                    }
                }
            }
        }
    }

Would be nice if you can adapt these changes, and maybe improve if needed.

nikreiman commented 12 years ago

@OKA133, these are both good points. I've split them off into separate issues (#6 and #7 respectively). I'm on holidays for another week, but feel free to make a pull request for either one if you like.

nikreiman commented 12 years ago

BTW both of these issues are now fixed in edc6132. Next version will probably be 2.0; I need some time to consider the API changes before I make a release.