refactor(android): Ti.Filesystem external storage handling

android/app/src/main/AndroidManifest.xml

@@ -4,7 +4,7 @@
 	<uses-permission android:name="android.permission.INTERNET"/>
 	<uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
 	<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
-	<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
+	<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="28"/>
 
 	<!-- Permissions needed to test Ti.Geolocation module. -->
 	<uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION"/>

android/cli/commands/_build.js

@@ -3476,10 +3476,11 @@ AndroidBuilder.prototype.fetchNeededManifestSettings = function fetchNeededManif
 
 	// Define Android <uses-permission/> names needed by our core Titanium APIs.
 	const calendarPermissions = [ 'android.permission.READ_CALENDAR', 'android.permission.WRITE_CALENDAR' ];
-	const cameraPermissions = [ 'android.permission.CAMERA' ];
+	const cameraPermissions = [ 'android.permission.CAMERA', 'android.permission.WRITE_EXTERNAL_STORAGE' ];
 	const contactsPermissions = [ 'android.permission.READ_CONTACTS', 'android.permission.WRITE_CONTACTS' ];
 	const contactsReadPermissions = [ 'android.permission.READ_CONTACTS' ];
 	const geoPermissions = [ 'android.permission.ACCESS_COARSE_LOCATION', 'android.permission.ACCESS_FINE_LOCATION' ];
+	const storagePermissions = [ 'android.permission.WRITE_EXTERNAL_STORAGE' ];
 	const vibratePermissions = [ 'android.permission.VIBRATE' ];
 	const wallpaperPermissions = [ 'android.permission.SET_WALLPAPER' ];
 
@@ -3504,7 +3505,10 @@ AndroidBuilder.prototype.fetchNeededManifestSettings = function fetchNeededManif
 		'Contacts.getAllPeople': contactsReadPermissions,
 		'Contacts.getAllGroups': contactsReadPermissions,
 
+		'Filesystem.requestStoragePermissions': storagePermissions,
+
 		'Media.Android.setSystemWallpaper': wallpaperPermissions,
+		'Media.saveToPhotoGallery': storagePermissions,
 		'Media.showCamera': cameraPermissions,
 		'Media.vibrate': vibratePermissions,
 	};
@@ -3517,9 +3521,12 @@ AndroidBuilder.prototype.fetchNeededManifestSettings = function fetchNeededManif
 		neededPermissionDictionary['android.permission.INTERNET'] = true;
 		neededPermissionDictionary['android.permission.ACCESS_WIFI_STATE'] = true;
 		neededPermissionDictionary['android.permission.ACCESS_NETWORK_STATE'] = true;
-		neededPermissionDictionary['android.permission.WRITE_EXTERNAL_STORAGE'] = true;
 	}
 
+	// Set the max API Level the "WRITE_EXTERNAL_STORAGE" permission should use.
+	// Android 10 and higher doesn't need this permission unless requestStoragePermissions() method is used.
+	let storagePermissionMaxSdkVersion = 28;
+
 	// Define JavaScript methods that need manifest <queries> entries.
 	// The value strings are used as boolean property names in our "AndroidManifest.xml" EJS template.
 	const tiMethodQueries = {
@@ -3579,6 +3586,9 @@ AndroidBuilder.prototype.fetchNeededManifestSettings = function fetchNeededManif
 					for (const permission of permissionArray) {
 						neededPermissionDictionary[permission] = true;
 					}
+					if (symbol === 'Filesystem.requestStoragePermissions') {
+						storagePermissionMaxSdkVersion = undefined;
+					}
 				}
 			}
 
@@ -3592,8 +3602,9 @@ AndroidBuilder.prototype.fetchNeededManifestSettings = function fetchNeededManif
 
 	// Return the entries needed to be injected into the generated "AndroidManifest.xml" file.
 	const neededSettings = {
-		usesPermissions: Object.keys(neededPermissionDictionary),
-		queries: neededQueriesDictionary
+		queries: neededQueriesDictionary,
+		storagePermissionMaxSdkVersion: storagePermissionMaxSdkVersion,
+		usesPermissions: Object.keys(neededPermissionDictionary)
 	};
 	return neededSettings;
 };
@@ -3688,6 +3699,7 @@ AndroidBuilder.prototype.generateAndroidManifest = async function generateAndroi
 		appLabel: this.tiapp.name,
 		appTheme: `@style/${this.defaultAppThemeName}`,
 		classname: this.classname,
+		storagePermissionMaxSdkVersion: neededManifestSettings.storagePermissionMaxSdkVersion,
 		packageName: this.appid,
 		queries: neededManifestSettings.queries,
 		usesPermissions: neededManifestSettings.usesPermissions

android/cli/lib/android-manifest.js

@@ -434,6 +434,23 @@ class AndroidManifest {
 		// Apply "tools:replace" attribute to <manifest/> element. (Must be done after setting namespace above.)
 		applyToolsReplaceToElement(manifestElement);
 
+		// Apply 'tools:node="replace"' to WRITE_EXTERNAL_STORAGE permission if no other tools attribute is set.
+		// Titanium adds "maxSdkVersion" attribute to this permission by default. This removes that attribute.
+		const permissionElement = getFirstChildElementByTagAndAndroidName(
+			manifestElement, 'uses-permission', 'android.permission.WRITE_EXTERNAL_STORAGE');
+		if (permissionElement) {
+			let hasToolsAttribute = false;
+			for (let index = 0; index < permissionElement.attributes.length; index++) {
+				if (permissionElement.attributes.item(index).name.startsWith('tools:')) {
+					hasToolsAttribute = true;
+					break;
+				}
+			}
+			if (!hasToolsAttribute) {
+				permissionElement.setAttribute('tools:node', 'replace');
+			}
+		}
+
 		// Fetch the <application/> element.
 		const appElement = getFirstChildElementByTagName(manifestElement, 'application');
 		if (!appElement) {

android/modules/filesystem/src/java/ti/modules/titanium/filesystem/FilesystemModule.java

@@ -8,7 +8,6 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.net.MalformedURLException;
 
 import android.Manifest;
 import android.app.Activity;
@@ -21,6 +20,7 @@
 import org.appcelerator.kroll.KrollModule;
 import org.appcelerator.kroll.annotations.Kroll;
 import org.appcelerator.kroll.common.Log;
+import org.appcelerator.titanium.io.TiFileFactory;
 import org.appcelerator.titanium.TiApplication;
 import org.appcelerator.titanium.TiBaseActivity;
 import org.appcelerator.titanium.TiC;
@@ -40,8 +40,6 @@ public class FilesystemModule extends KrollModule
 	@Kroll.constant
 	public static final int MODE_APPEND = 2;
 
-	private static String[] RESOURCES_DIR = { "app://" };
-
 	// Methods
 	public FilesystemModule()
 	{
@@ -145,7 +143,7 @@ public FileProxy getApplicationDirectory()
 	@Kroll.getProperty
 	public String getApplicationDataDirectory()
 	{
-		return "appdata-private://";
+		return TiFileFactory.APPDATA_PRIVATE_URL_SCHEME + "://";
 	}
 
 	@Kroll.method
@@ -155,39 +153,31 @@ public String getResRawDirectory()
 		return "android.resource://" + TiApplication.getInstance().getPackageName() + "/raw/";
 	}
 
-	@SuppressWarnings("deprecation")
 	@Kroll.method
 	@Kroll.getProperty
 	public String getApplicationCacheDirectory()
 	{
-		TiApplication app = TiApplication.getInstance();
-		if (app == null) {
-			return null;
-		}
-
-		File cacheDir = app.getCacheDir();
-
-		try {
-			return cacheDir.toURL().toString();
-
-		} catch (MalformedURLException e) {
-			Log.e(TAG, "Exception converting cache directory to URL", e);
-			return null;
-		}
+		return "file://" + TiApplication.getInstance().getCacheDir().getAbsolutePath();
 	}
 
 	@Kroll.method
 	@Kroll.getProperty
 	public String getResourcesDirectory()
 	{
-		return "app://";
+		return TiC.URL_APP_PREFIX;
+	}
+
+	@Kroll.getProperty
+	public String getExternalCacheDirectory()
+	{
+		return TiFileFactory.APPCACHE_EXTERNAL_URL_SCHEME + "://";
 	}
 
 	@Kroll.method
 	@Kroll.getProperty
 	public String getExternalStorageDirectory()
 	{
-		return "appdata://";
+		return TiFileFactory.APPDATA_URL_SCHEME + "://";
 	}
 
 	@Kroll.method

android/templates/build/AndroidManifest.xml

@@ -5,7 +5,11 @@
 	android:versionCode="1">
 
 <% if (usesPermissions) { for (const nextPermission of usesPermissions) { %>
+<% if ((nextPermission === 'android.permission.WRITE_EXTERNAL_STORAGE') && storagePermissionMaxSdkVersion) { %>
+	<uses-permission android:name="<%- nextPermission %>" android:maxSdkVersion="<%- storagePermissionMaxSdkVersion %>"/>
+<% } else { %>
 	<uses-permission android:name="<%- nextPermission %>"/>
+<% } %>
 <% } } %>
 
 <% if (queries && (Object.keys(queries).length > 0)) { %>

android/titanium/src/java/org/appcelerator/titanium/io/TiFileFactory.java

@@ -26,13 +26,10 @@ public class TiFileFactory
 {
 	private static final String TAG = "TiFileFactory";
 	private static final String ANDROID_RESOURCE_URL_SCHEME = ContentResolver.SCHEME_ANDROID_RESOURCE;
-	private static final String ANDROID_RESOURCE_URL_PREFIX = ANDROID_RESOURCE_URL_SCHEME + "://";
-	private static final String APPDATA_URL_SCHEME = "appdata";
-	private static final String APPDATA_URL_PREFIX = APPDATA_URL_SCHEME + "://";
-	private static final String APPDATA_PRIVATE_URL_SCHEME = "appdata-private";
-	private static final String APPDATA_PRIVATE_URL_PREFIX = APPDATA_PRIVATE_URL_SCHEME + "://";
+	public static final String APPCACHE_EXTERNAL_URL_SCHEME = "appcache-external";
+	public static final String APPDATA_URL_SCHEME = "appdata";
+	public static final String APPDATA_PRIVATE_URL_SCHEME = "appdata-private";
 	private static final String CONTENT_URL_SCHEME = ContentResolver.SCHEME_CONTENT;
-	private static final String CONTENT_URL_PREFIX = CONTENT_URL_SCHEME + "://";
 	private static final String FILE_URL_SCHEME = ContentResolver.SCHEME_FILE;
 	private static final String FILE_URL_PREFIX = FILE_URL_SCHEME + "://";
 	// strip file:// prefix off the special URL we need to handle like app:
@@ -45,6 +42,7 @@ public class TiFileFactory
 	{
 		localSchemeSet = new HashSet<String>();
 		localSchemeSet.add(TiC.URL_APP_SCHEME.toLowerCase());
+		localSchemeSet.add(APPCACHE_EXTERNAL_URL_SCHEME.toLowerCase());
 		localSchemeSet.add(APPDATA_URL_SCHEME.toLowerCase());
 		localSchemeSet.add(APPDATA_PRIVATE_URL_SCHEME.toLowerCase());
 		localSchemeSet.add(FILE_URL_SCHEME.toLowerCase());
@@ -76,8 +74,9 @@ public static TiBaseFile createTitaniumFile(String path, boolean stream)
 	 */
 	public static TiBaseFile createTitaniumFile(String[] parts, boolean stream)
 	{
+		// Parse for the URL scheme and file path.
 		String possibleURI = joinPathSegments(parts);
-		String scheme = null;
+		String scheme = "";
 		String path = "";
 		int colonIndex = possibleURI.indexOf(':');
 		if (colonIndex != -1) {
@@ -107,45 +106,59 @@ public static TiBaseFile createTitaniumFile(String[] parts, boolean stream)
 			}
 		}
 
-		if (TiC.URL_APP_SCHEME.equals(scheme)) {
-			return new TiResourceFile(trimFront(path, '/'));
-		}
-
-		if (APPDATA_PRIVATE_URL_SCHEME.equals(scheme)) {
-			File f = new File(getDataDirectory(true), path);
-			return new TiFile(f, possibleURI, stream);
-		}
-
-		if (APPDATA_URL_SCHEME.equals(scheme)) {
-			File f = new File(getDataDirectory(false), path);
-			return new TiFile(f, possibleURI, stream);
-		}
-
-		if (CONTENT_URL_SCHEME.equals(scheme) || ANDROID_RESOURCE_URL_SCHEME.equals(scheme)) {
-			return new TiContentFile(possibleURI); // TODO: Forward along the actual URI instance?
-		}
-
-		if (FILE_URL_SCHEME.equals(scheme)) {
-			// check for fake "file:///android_asset/Resources/" URL, treat like app:
-			if (path.startsWith(ANDROID_ASSET_RESOURCES)) {
-				// Strip this fake base path
-				path = path.substring(ANDROID_ASSET_RESOURCES.length());
-				return new TiResourceFile(trimFront(path, '/')); // remove leading '/' characters
+		// Create a Titanium file object capable of accessing the file referenced by given URL.
+		TiBaseFile tiFile = null;
+		switch (scheme) {
+			case TiC.URL_APP_SCHEME: {
+				tiFile = new TiResourceFile(trimFront(path, '/'));
+				break;
+			}
+			case APPCACHE_EXTERNAL_URL_SCHEME: {
+				File cacheDir = TiApplication.getInstance().getExternalCacheDir();
+				if (cacheDir != null) {
+					File file = new File(cacheDir, path);
+					tiFile = new TiFile(file, possibleURI, stream);
+				}
+				break;
+			}
+			case APPDATA_URL_SCHEME:            // Use external storage.
+			case APPDATA_PRIVATE_URL_SCHEME:    // Use internal storage.
+			case TI_URL_SCHEME: {               // Use internal storage.
+				boolean isInternal = !scheme.equals(APPDATA_URL_SCHEME);
+				File dataDir = getDataDirectory(isInternal);
+				if (dataDir != null) {
+					File file = new File(dataDir, path);
+					tiFile = new TiFile(file, possibleURI, stream);
+				}
+				break;
+			}
+			case CONTENT_URL_SCHEME:
+			case ANDROID_RESOURCE_URL_SCHEME: {
+				tiFile = new TiContentFile(possibleURI);
+				break;
+			}
+			case FILE_URL_SCHEME: {
+				if (path.startsWith(ANDROID_ASSET_RESOURCES)) {
+					// This is a "file:///android_asset/Resources/" referencing an APK "assets" file.
+					path = path.substring(ANDROID_ASSET_RESOURCES.length());
+					tiFile = new TiResourceFile(trimFront(path, '/'));
+				} else {
+					// This is a normal file system path.
+					tiFile = new TiFile(new File(path), possibleURI, stream);
+				}
+				break;
 			}
-
-			// Normal file
-			return new TiFile(new File(path), possibleURI, stream);
 		}
 
-		if (TI_URL_SCHEME.equals(scheme)) { // treat like appdata-private
-			// TODO: Do we need to trim leading '/'?
-			File f = new File(getDataDirectory(true), path);
-			return new TiFile(f, possibleURI, stream);
+		// Create a mock file object whose methods no-op if we failed to handle given path. Can happen if:
+		// - Given an invalid/unknown URL scheme.
+		// - Unable to access destination, such as external storage not being available.
+		if (tiFile == null) {
+			tiFile = new TiMockFile(possibleURI);
 		}
 
-		// TODO: Throw an exception? Ideally this shouldn't ever happen, but could if an unhandled scheme URI came in here
-		// i.e. http:, ftp:, https:, mailto:, etc.
-		return null;
+		// Return a TiBaseFile wrapping the given path. Will never be null.
+		return tiFile;
 	}
 
 	private static String joinPathSegments(String[] parts)
@@ -204,8 +217,7 @@ private static String trimFront(String sourceString, char trimCharacter)
 	 */
 	public static File getDataDirectory(boolean privateStorage)
 	{
-		TiFileHelper tfh = new TiFileHelper(TiApplication.getInstance());
-		return tfh.getDataDirectory(privateStorage);
+		return TiFileHelper.getInstance().getDataDirectory(privateStorage);
 	}
 
 	public static boolean isLocalScheme(String url)