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

[TIMOB-13642] Android: Fixed bug where "relative" image paths won't load #10304

Merged
merged 9 commits into from Oct 2, 2018

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Aug 31, 2018

JIRA:
https://jira.appcelerator.org/browse/TIMOB-13642

Summary:

  • Relative image path was broken due to extra slash added to path, which Android asset manager could not handle.
    • Solved by trimming extra slashes.
    • Not an issue with external files since filesystem can handle extra slashes.
  • Fixed resolution of paths containing "../" when loading an image from a JS file in a subdirectory.
  • Added support for "content://" image URLs. (Used to only work via blobs.)
  • Added support for "android.resource://" image URLs. (Used to only work via blobs.)
    • Allows Ti.Filesystem.getResRawDirectory() + "MyImage" to work.
    • Allows "android.resource://" + Ti.App.id + "/drawable/MyImage" to work.

Note:
Relative paths are currently handled differently between platforms.

  • On Android, paths are relative to the JavaScript file.
  • On iOS and Windows, paths are relative to the app's "Resources" directory.

I have not changed this behavior since that would be a breaking-change.

Test:

  1. Create a "Classic App" project.
  2. Replace the project's "app.js" with the code below. (This code assumes the project has a "tab1.png".)
  3. Build and run the app on Android.
  4. Scroll through the app, verifying that every black box has loaded an image.
function createImageGroupFor(path) {
	var container = Ti.UI.createView({
		layout: "vertical",
		top: "5dp",
		left: "5dp",
		right: "5dp",
		backgroundColor: "#222222",
		width: Ti.UI.FILL,
		height: Ti.UI.SIZE,
	});
	container.add(Ti.UI.createLabel({
		text: path,
		color: "#CCCCCC",
		top: "5dp",
		left: "5dp",
		right: "5dp",
	}));
	container.add(Ti.UI.createImageView({
		image: path,
		preventDefaultImage: true,
		bottom: "5dp",
		width: "80dp",
		height: "80dp",
	}));
	return container;
}

var window = Ti.UI.createWindow({ fullscreen: true });
var scrollView = Ti.UI.createScrollView({
	layout: "vertical",
	scrollType: "vertical",
	showHorizontalScrollIndicator: false,
	shorVerticalScrollIndicator: true,
	backgroundColor: "gray",
});
scrollView.add(createImageGroupFor("/assets/images/tab1.png"));
scrollView.add(createImageGroupFor("assets/images/tab1.png"));
scrollView.add(createImageGroupFor("./assets/images/tab1.png"));
scrollView.add(createImageGroupFor("././assets/images/.././images/tab1.png"));
scrollView.add(createImageGroupFor(Ti.Filesystem.getFile("assets/images/tab1.png").nativePath));
if (Ti.Platform.name === "android") {
	var externalFilePath = Ti.Filesystem.applicationDataDirectory + "copy.png";
	Ti.Filesystem.getFile(Ti.Filesystem.resourcesDirectory, "assets/images/tab1.png").copy(externalFilePath);
	scrollView.add(createImageGroupFor(externalFilePath));
	externalFilePath = Ti.Filesystem.getFile(externalFilePath).nativePath;
	scrollView.add(createImageGroupFor(externalFilePath));
	externalFilePath = externalFilePath.substring("file:///".length);
	scrollView.add(createImageGroupFor("content://" + Ti.App.id + ".tifileprovider/filesystem/" + externalFilePath));
	scrollView.add(createImageGroupFor(Ti.Filesystem.resourcesDirectory + "assets/images/tab1.png"));
	scrollView.add(createImageGroupFor("file:///android_asset/Resources/assets/images/tab1.png"));
	scrollView.add(createImageGroupFor("android.resource://" + Ti.App.id + "/drawable/appicon"));
}
scrollView.add(Ti.UI.createView({ width: Ti.UI.FILL, height: "5dp" }));
window.add(scrollView);
window.open();

- Relative image path handling was broken due to extra slash added to path, which Android asset manager could not handle.
  * Solved by trimming extra slashes.
  * Trimming not needed for filesystem paths. Was not an issue for Ti.Filesystem.getFile() API.
- Note that relative paths worked for Ti.Filesystem.getFile(), but not for image loading. Android required a leading slash.
- Fixed resolution of paths containing "../" when loading an image from a JS file in a subdirectory.
- Added support for "content://" image URLs. (Used to only work via blobs.)
- Added support for "android.resource://" image URLs. (Used to only work via blobs.)
* Returns the same string reference if no characters were trimmed.
* Returns null if given a null string.
*/
private static String trimFront(String sourceString, char trimCharacter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to simplify this a little:

private static String trimFront(String sourceString, char trimCharacter)
{
	if (sourceString != null) {
		for (int i = 0; i < sourceString.length(); i++) {
			if (sourceString.charAt(i) != trimCharacter) {
				sourceString = sourceString.substring(i);
				break;
			}
		}
	}
	return sourceString;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't handle the case where all characters in the string are trimmable. That's why I do the substring handling outside of the loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that a for is a little easier to read than a while in this case:

private static String trimFront(String sourceString, char trimCharacter)
{
  if (sourceString != null) {
    int i = 0;
    for (; i < sourceString.length(); i++) {
      if (sourceString.charAt(i) != trimCharacter) {
        sourceString = sourceString.substring(i);
        break;
      }
    }
    if (i == sourceString.length()) {
      sourceString = "";
    }
  }
  return sourceString;
}

But they both work, so I'll let you decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote it with a for like this...

for (; (index < sourceString.length()) && (sourceString.charAt(index) == trimCharacter); index++);

But our lint step didn't like it. (shrugs)

// Note: A "file:///" URL needs 3 slashes to reference localhost.
if (defaultScheme != null) {
combined = defaultScheme + PATH_SEPARATOR;
if (defaultScheme.equals("file:")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use TiFileFactory.FILE_URL_SCHEME here? Or even FILE_URL_PREFIX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we have several "file:" string literals in the "TiURL.java" code.
I also don't like how "TiURL.java" expects a scheme string to end with a trailing colon ':' either. That's not part of the scheme. That's the character which separates the scheme from the path.

I didn't feel like mucking with this part and left it alone.

@lokeshchdhry
Copy link
Contributor

FR Passed.

Relative path images load successfully.

Studio Ver: 5.1.1.201809051655
SDK Ver: 7.5.0 local build
OS Ver: 10.13.5
Xcode Ver: Xcode 9.4.1
Appc NPM: 4.2.13
Appc CLI: 7.0.6
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.2
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ samsung SM-G955U1 (Android 8.0.0)

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Sep 14, 2018

Waiting for CR to pass.

@build
Copy link
Contributor

build commented Oct 2, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR: PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants