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-19295] iOS9: Titanium CLI automatically converts existing images to assets catalog #7231

Merged
merged 1 commit into from Sep 30, 2015

Conversation

feons
Copy link
Contributor

@feons feons commented Sep 24, 2015

@@ -4002,6 +4003,7 @@ iOSBuilder.prototype.copyResources = function copyResources(next) {
// fall through to default case

default:
imageAssets[relPath] = info;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. Images are going to be copied into the app twice: once as they do now and again into the asset catalog.

You should add a 'jpg' and 'png' case above it and track imageAssets separately. This case should be BEFORE 'html' as 'html' is designed to fall through.

@cb1kenobi
Copy link
Contributor

No where are you deleting these images from this.buildDirFiles. If you don't delete the entry from this.buildDirFiles, then the build will clean up all files marked in this.buildDirFiles and since you didn't unmark them, you're probably going to run into issues with subsequent builds.

// update image file's destination
imageAssets[file].dest = path.join(imageSetAbsPath, imageName + '.' + imageExt);

if (!(imageSetRelPath in imageSets)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally the in operator is frowned down upon. You can generally say !imageSets[imageSetRelPath], but sometimes you need to do !imageSets.hasOwnProperty(imageSetRelPath).

@feons feons force-pushed the TIMOB-19295 branch 2 times, most recently from d165b1a to 51cee5e Compare September 25, 2015 04:55
name: imageSetName
}

fs.existsSync(imageSetAbsPath) || wrench.mkdirSyncRecursive(imageSetAbsPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is unnecessary.

@feons feons force-pushed the TIMOB-19295 branch 5 times, most recently from 80c9192 to 397a762 Compare September 25, 2015 06:30
@cb1kenobi
Copy link
Contributor

CR'd and FR'd. Looks grrrrrrrreat! APPROVED

cb1kenobi added a commit that referenced this pull request Sep 30, 2015
[TIMOB-19295] iOS9: Titanium CLI automatically converts existing images to assets catalog
@cb1kenobi cb1kenobi merged commit 7eda074 into tidev:master Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants