feat: introduce assets generation #3435

ivanovit · 2018-03-09T09:46:41Z

Introduce assets generate icons and assets generate splashes commands to generate assets needed for iOS and Android.

The operations are as follow:

Icons:
- For all platforms, we just resize the provided image.
Splash screens:
- For Android we generate 2 png for each dpi: blank with the background(background.png) and resized image based on the provided image(logo.png). During the app start, the logo overlays the background.
- For iOS we generate 1 png for each dpi which has resized and centered image on the background.

All resources with corresponding sizes and operations are described in image-definitions.ts.

Added image-generation-test.png in order to provide an easy way to test this functionality.

For image resizing we use https://github.com/oliver-moran/jimp.

rosen-vladimirov · 2018-03-12T19:59:12Z

lib/bootstrap.ts

@@ -144,3 +144,7 @@ $injector.requirePublic("extensibilityService", "./services/extensibility-servic

 $injector.require("nodeModulesDependenciesBuilder", "./tools/node-modules/node-modules-dependencies-builder");
 $injector.require("subscriptionService", "./services/subscription-service");
+
+$injector.requireCommand("generate-icons", "./commands/generate-assets")


We've agreed the commands to be:

resources|generate|icons resources|generate|splashes

The usage will be:

tns resources generate icons path_to_image tns resources generate splashes path_to_image --background <color>

rosen-vladimirov · 2018-03-12T19:59:29Z

lib/bootstrap.ts

+
+$injector.requireCommand("generate-icons", "./commands/generate-assets")
+$injector.requireCommand("generate-splashscreens", "./commands/generate-assets")
+$injector.requirePublic("assetsGenerationService", "./services/assets-generation/assets-generation-service");


new line at the end of file please

rosen-vladimirov · 2018-03-12T20:21:37Z

package.json

@@ -30,13 +30,15 @@
    "mobile"
  ],
  "dependencies": {
+    "@types/color": "^3.0.0",


this should be in the devDependencies section. Also, can you please use exact version.

npm i --save-dev --save-exact @types/color

rosen-vladimirov · 2018-03-12T20:22:11Z

package.json

    "bplist-parser": "0.1.0",
    "bufferpack": "0.0.6",
    "byline": "4.2.1",
    "chalk": "1.1.0",
    "chokidar": "1.7.0",
    "cli-table": "https://github.com/telerik/cli-table/tarball/v0.3.1.2",
    "clui": "0.3.1",
+    "color": "^3.0.0",


Strict version, please (you can use npm i --save --save-exact color in order to update package.json and npm-shrinkwrap.json correctly)

rosen-vladimirov · 2018-03-12T20:22:22Z

package.json

@@ -48,6 +50,7 @@
    "ios-device-lib": "0.4.10",
    "ios-mobileprovision-finder": "1.0.10",
    "ios-sim-portable": "3.3.0",
+    "jimp": "^0.2.28",


Again, strict version please

rosen-vladimirov · 2018-03-12T20:25:09Z

lib/services/assets-generation/assets-generation-service.ts

+	}
+
+	@exported("assetsGenerationService")
+	public async generateIcons(imagePath: string, resourcesPath: string): Promise<void> {


Can you use object as single argument (you'll have to introduce interface for it). This will allow us to handle future changes easier

rosen-vladimirov · 2018-03-12T20:25:15Z

lib/services/assets-generation/assets-generation-service.ts

+	}
+
+	@exported("assetsGenerationService")
+	public async generateSplashScreens(imagePath: string, resourcesPath: string, background?: string): Promise<void> {


Can you use object as single argument (you'll have to introduce interface for it). This will allow us to handle future changes easier

rosen-vladimirov · 2018-03-12T20:35:59Z

lib/services/assets-generation/assets-generation-service.ts

+
+	private generateImage(background: string, width: number, height: number, outputPath: string, overlayImage?: Jimp) {
+		//Typescript declarations for Jimp are not updated to define the constructor with backgroundColor so we workaround it by casting it to <any> for this case only.
+		let J = <any>Jimp;


this should be const

rosen-vladimirov · 2018-03-12T20:36:11Z

lib/services/assets-generation/assets-generation-service.ts

+		}
+	}
+
+	private async resize(imagePath: string, width: number, height: number) {


this method needs return type

rosen-vladimirov · 2018-03-12T20:37:08Z

lib/services/assets-generation/assets-generation-service.ts

+		return image.scaleToFit(width, height);
+	}
+
+	private generateImage(background: string, width: number, height: number, outputPath: string, overlayImage?: Jimp) {


this method needs return type. Also can you use a single object for parameter.

It's not part of the public interface and I think it will be better to leave it as it is.

petekanev · 2018-03-13T13:15:36Z

@ivanovit @rosen-vladimirov
The paths in https://github.com/NativeScript/nativescript-cli/pull/3435/files#diff-ec7f1f51cae0151d8566c9bce4534772 to the android resources directory will only work in case the internal directory structure has not been changed. If it's been updated - as a result of running tns resources update beforehand, then the resources need to go to Android/src/main/res/

ivanovit · 2018-03-14T14:52:55Z

ping @rosen-vladimirov @Pip3r4o

rosen-vladimirov

Great work! 🎉 💯 🥇

petekanev

LGTM, thanks for addressing that comment!

rosen-vladimirov · 2018-03-15T09:58:40Z

lib/services/assets-generation/assets-generation-service.ts

+	@exported("assetsGenerationService")
+	public async generateIcons(resourceGenerationData: IResourceGenerationData): Promise<void> {
+		this.$logger.info("Generating icons ...");
+		await this.generateImagesForDefinitions(resourceGenerationData.imagePath, resourceGenerationData.resourcesPath, Icons, resourceGenerationData.platform);


the platform argument should be fifth

Fatme · 2018-03-15T10:32:43Z

lib/services/assets-generation/assets-generation-service.ts

@@ -12,21 +12,23 @@ export class AssetsGenerationService implements IAssetsGenerationService {
 	@exported("assetsGenerationService")
 	public async generateIcons(resourceGenerationData: IResourceGenerationData): Promise<void> {
 		this.$logger.info("Generating icons ...");
-		await this.generateImagesForDefinitions(resourceGenerationData.imagePath, resourceGenerationData.resourcesPath, Icons);
+		await this.generateImagesForDefinitions(resourceGenerationData.imagePath, resourceGenerationData.resourcesPath, Icons, resourceGenerationData.platform);


Can we introduce an interface and pass object as param?

As it is a private method and I have some concerns about polluting the .d.fs only for this method I will leave it as it is.

Mitko-Kerezov · 2018-03-20T16:11:58Z

lib/commands/generate-assets.ts

+			super($options, $injector, $projectData, $stringParameterBuilder, $assetsGenerationService);
+	}
+
+	protected async generate(imagePath: string, background?: string): Promise<void> {


The second argument here is unnecessary and could be omitted (I think it would still conform to the signature of the Base command)

I prefer to keep it just to ensure the same signature

Mitko-Kerezov · 2018-03-20T16:12:45Z

lib/commands/generate-assets.ts

+			super($options, $injector, $projectData, $stringParameterBuilder, $assetsGenerationService);
+	}
+
+	protected async generate(imagePath: string, resourcesPath: string, background?: string): Promise<void> {


The signature of the base method differs from this one and the third argument will never be passed - I think we'd need to delete resourcesPath: string as a whole?

Mitko-Kerezov · 2018-03-20T16:14:00Z

lib/declarations.d.ts

@@ -463,6 +463,7 @@ interface IOptions extends ICommonOptions, IBundleString, IPlatformTemplate, IEm
 	liveEdit: boolean;
 	chrome: boolean;
 	inspector: boolean; // the counterpart to --chrome
+	background: string


Missing ;

Mitko-Kerezov · 2018-03-20T16:21:48Z

lib/services/assets-generation/assets-generation-service.ts

@@ -0,0 +1,122 @@
+import * as Jimp from "jimp";


Why the pascal case - this is actually an instance object and should be camelCase

It is used as a constructor, so it makes sense to be with pascal case

Mitko-Kerezov · 2018-03-20T16:25:20Z

lib/services/assets-generation/assets-generation-service.ts

+		this.$logger.info("Splash screens generation completed.");
+	}
+
+	private async generateImagesForDefinitions(data: IGenerateImagesData): Promise<void> {


You could accept data: IResourceGenerationData, propertiesToEnumerate: string[] here in order to avoid casting to IGenerateImagesData in the caller methods above.
This is purely cosmetic and not a merge-stopper at all

Mitko-Kerezov · 2018-03-20T16:58:51Z

lib/services/assets-generation/assets-generation-service.ts

+		const assetsStructure = await this.$projectDataService.getAssetsStructure({ projectDir: data.projectDir });
+
+		for (const platform in assetsStructure) {
+			if (data.platform && platform.toLowerCase() !== data.platform.toLowerCase()) {


Instead of this whole logic you can construct the required array (an array of values gotten from assetsStructure) using the lodash filter.
You could filter out all the unneeded platforms and items that do not include imageTypeKey.
You would then be able to skip all the continue statements that simulate filtering

Mitko-Kerezov · 2018-03-20T17:08:47Z

lib/services/project-data-service.ts

+
+		const imageDefinitions = this.getImageDefinitions().ios;
+
+		if (content && content.images) {


You can skip this check since you're using the lodash each method - it could just be:
_.each(content && content.images, image => {

Mitko-Kerezov · 2018-03-20T17:14:26Z

lib/services/project-data-service.ts

+		_.each(assetItems, assetItem => {
+			_.each(normalizedPaths, currentNormalizedPath => {
+				const imagePath = path.join(assetItem.directory, assetItem.filename);
+				if (currentNormalizedPath.indexOf(path.normalize(imagePath)) !== -1) {


You can use _.find to locate the value of interest instead of using _.each and then return false (e.g. break)

I've used find at the beginning, but it looks strange, so I prefer the each

Mitko-Kerezov · 2018-03-20T17:16:43Z

lib/services/project-data-service.ts

+		};
+	}
+
+	private getImageDefinitions(): any {


I believe this method returns IAssetsStructure

unfortunately it does not :(

Mitko-Kerezov · 2018-03-20T17:18:20Z

lib/services/project-data-service.ts

+					}
+				} else {
+					// Find the image size based on the hardcoded values in the image-definitions.json
+					_.each(imageDefinitions, (assetSubGroup: IAssetItem[]) => {


You shouldn't need to point the type of assetSubGroup out if imageDefinitions was strongly typed - I believe its type is actually IAssetsStructure

Introduce ``generate-icons`` and ``generate-splashscreens`` commands to generate assets needed for iOS and Android. The operations are as follow: - Icons: - For all platforms we just resize the provided image. - Splash screens: - For Android we generate 2 png for each dpi: blank with background(background.png) and resized image based on provided image(logo.png). During the app start the logo overlays the background. - For iOS we generate 1 png for each dpi which has resized and centered image on the background. All resources with corresponding sizes and operations are described in image-definitions.ts. Added image-generation-test.png in order to provide easy way to test this functionality. For image resizing we use https://github.com/oliver-moran/jimp.

Introduce new methods in `projectDataService` to get the current assets strucuture. Use a predefined .json file in the resources to get information about image sizes. For iOS read the Contents.json file in specific iOS Resource directories. For Android - enumerate the files and construct required objects. Use the new method in the assetsGenerationService. Generate icons and splashes based on the argument. Require projectDir instead of resourcesDir for assets generation in order to get all CLI's specific files info (nsconfig, package.json, project location, etc.). Fix comands for generating assets to have mandatory command parameter - previously it had a non-mandatory param. Add newly exposed methods to the tests.

ivanovit · 2018-03-21T12:57:41Z

lib/services/assets-generation/assets-generation-service.ts

+
+		for (const assetItem of assetItems) {
+			const operation = assetItem.resizeOperation || Operations.Resize;
+			const scale = assetItem.scale || 0.8;


The scale option must be respected for the Resize operation as well. The scale must be parsed to string because the value there is 1x, 2x.

ivanovit · 2018-03-21T13:03:32Z

lib/services/assets-generation/assets-generation-service.ts

+			.value();
+
+		for (const assetItem of assetItems) {
+			const operation = assetItem.resizeOperation || Operations.Resize;


How do you infer the image operation for the asset only from Content.json?

It is taken in project-data-service for Android, but I've missed to set it for iOS 😿

rosen-vladimirov mentioned this pull request Mar 12, 2018

feat: introduce resources update command #3347

Merged

rosen-vladimirov suggested changes Mar 12, 2018

View reviewed changes

rosen-vladimirov added this to the 4.0.0 milestone Mar 12, 2018

rosen-vladimirov added the feature label Mar 12, 2018

ivanovit force-pushed the IIIvanov/Assets-Generation branch 2 times, most recently from 18398b8 to 6f03ed7 Compare March 14, 2018 13:38

rosen-vladimirov approved these changes Mar 14, 2018

View reviewed changes

petekanev approved these changes Mar 14, 2018

View reviewed changes

ivanovit changed the base branch from master to release March 15, 2018 09:53

rosen-vladimirov suggested changes Mar 15, 2018

View reviewed changes

ivanovit force-pushed the IIIvanov/Assets-Generation branch from 622cef1 to 79d618f Compare March 15, 2018 10:08

Fatme reviewed Mar 15, 2018

View reviewed changes

rosen-vladimirov approved these changes Mar 15, 2018

View reviewed changes

rosen-vladimirov force-pushed the IIIvanov/Assets-Generation branch from 79d618f to 6461ac3 Compare March 19, 2018 23:27

Mitko-Kerezov approved these changes Mar 20, 2018

View reviewed changes

ivanovit and others added 9 commits March 21, 2018 10:09

Expose assets-generation-service

6143320

Fixes after review

9aabb6c

Fix the linter

5493cef

Add platform parameter

25f63c3

chore: Remove commented code and improve variables names

5a97228

docs: Add help for resources generate commands

b5e3bc9

chore: Improve code for getting assets to be generated

d5bfec2

rosen-vladimirov force-pushed the IIIvanov/Assets-Generation branch from 00ad6ba to d5bfec2 Compare March 21, 2018 08:10

docs: Add new methods to PublicAPI

dd2c26d

rosen-vladimirov merged commit d341285 into release Mar 21, 2018

rosen-vladimirov deleted the IIIvanov/Assets-Generation branch March 21, 2018 12:02

ivanovit commented Mar 21, 2018

View reviewed changes


		const imageDefinitions = this.getImageDefinitions().ios;

		if (content && content.images) {

feat: introduce assets generation #3435

feat: introduce assets generation #3435

Conversation

ivanovit commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petekanev commented Mar 13, 2018

ivanovit commented Mar 14, 2018

rosen-vladimirov left a comment

Choose a reason for hiding this comment

petekanev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovit commented Mar 9, 2018 •

edited

Loading