You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by daserge <gi...@git.apache.org> on 2016/11/25 10:33:45 UTC
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
GitHub user daserge opened a pull request:
https://github.com/apache/cordova-windows/pull/212
CB-9287 Not enough Icons and Splashscreens for Windows 8.1 and Windows Phone 8.1
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist
is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
Windows
### What does this PR do?
Add JPG support, added missing splashscreen scales for projects
cordova.js - refine splashscreen extension based on config.xml
Added a warning when non-MRT splash screens have mixed extensions
Added a warning for size limit exceeding (applies to win10 project only)
Added a warning for images with unsupported format
Tests
[Jira issue CB-9287](https://issues.apache.org/jira/browse/CB-9287)
### What testing has been done on this change?
Manual and auto tests
### Checklist
- [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
- [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
- [x] Added automated test coverage as appropriate for this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/daserge/cordova-windows CB-9287
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-windows/pull/212.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #212
----
commit d4640969dc2b9b7b5322629a899c040fb9496a26
Author: daserge <v-...@microsoft.com>
Date: 2016-11-25T10:30:46Z
CB-9287 Not enough Icons and Splashscreens for Windows 8.1 and Windows Phone 8.1
Add JPG support
cordova.js - refine splashscreen extension based on config.xml
Added a warning when non-MRT splash screens have mixed extensions
Added a warning for size limit exceeding (applies to win10 project only)
Added a warning for images with unsupported format
commit bd0096b71a3d6d65169d7d4582e50a05dc48ecb0
Author: daserge <v-...@microsoft.com>
Date: 2016-11-25T10:31:03Z
CB-9287 Not enough Icons and Splashscreens for Windows 8.1 and Windows Phone 8.1
Tests
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89992916
--- Diff: template/cordova/lib/prepare.js ---
@@ -297,48 +305,80 @@ function applyNavigationWhitelist(config, manifest) {
manifest.getApplication().setAccessRules(whitelistRules);
}
-function mapImageResources(images, imagesDir) {
- var pathMap = {};
+// Platform default images
+var platformImages = [
+ {dest: 'Square150x150Logo.scale-100', width: 150, height: 150},
+ {dest: 'Square30x30Logo.scale-100', width: 30, height: 30},
+ {dest: 'StoreLogo.scale-100', width: 50, height: 50},
+ {dest: 'SplashScreen.scale-100', width: 620, height: 300, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-125', width: 775, height: 375, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-140', width: 868, height: 420, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-150', width: 930, height: 450, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-180', width: 1116, height: 540, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-200', width: 1240, height: 600, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-400', width: 2480, height: 1200, targetProject: TARGET_PROJECT_10},
+ // scaled images are specified here for backward compatibility only so we can find them by size
+ {dest: 'StoreLogo.scale-240', width: 120, height: 120},
+ {dest: 'Square44x44Logo.scale-100', width: 44, height: 44},
+ {dest: 'Square44x44Logo.scale-240', width: 106, height: 106},
+ {dest: 'Square70x70Logo.scale-100', width: 70, height: 70},
+ {dest: 'Square71x71Logo.scale-100', width: 71, height: 71},
+ {dest: 'Square71x71Logo.scale-240', width: 170, height: 170},
+ {dest: 'Square150x150Logo.scale-240', width: 360, height: 360},
+ {dest: 'Square310x310Logo.scale-100', width: 310, height: 310},
+ {dest: 'Wide310x150Logo.scale-100', width: 310, height: 150},
+ {dest: 'Wide310x150Logo.scale-240', width: 744, height: 360},
+ {dest: 'SplashScreenPhone.scale-100', width: 480, height: 800, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-140', width: 672, height: 1120, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-240', width: 1152, height: 1920, targetProject: TARGET_PROJECT_WP81}
+];
+
+function findPlatformImage(width, height) {
+ if (!width && !height){
+ // this could be default image,
+ // Windows requires specific image dimension so we can't apply it
+ return null;
+ }
+ for (var idx in platformImages){
+ var res = platformImages[idx];
+ // If only one of width or height is not specified, use another parameter for comparation
+ // If both specified, compare both.
+ if ((!width || (width == res.width)) &&
+ (!height || (height == res.height))){
+ return res;
+ }
+ }
+ return null;
+}
- // Platform default images
- var platformImages = [
- {dest: 'Square150x150Logo.scale-100.png', width: 150, height: 150},
- {dest: 'Square30x30Logo.scale-100.png', width: 30, height: 30},
- {dest: 'StoreLogo.scale-100.png', width: 50, height: 50},
- {dest: 'SplashScreen.scale-100.png', width: 620, height: 300},
- // scaled images are specified here for backward compatibility only so we can find them by size
- {dest: 'StoreLogo.scale-240.png', width: 120, height: 120},
- {dest: 'Square44x44Logo.scale-100.png', width: 44, height: 44},
- {dest: 'Square44x44Logo.scale-240.png', width: 106, height: 106},
- {dest: 'Square70x70Logo.scale-100.png', width: 70, height: 70},
- {dest: 'Square71x71Logo.scale-100.png', width: 71, height: 71},
- {dest: 'Square71x71Logo.scale-240.png', width: 170, height: 170},
- {dest: 'Square150x150Logo.scale-240.png', width: 360, height: 360},
- {dest: 'Square310x310Logo.scale-100.png', width: 310, height: 310},
- {dest: 'Wide310x150Logo.scale-100.png', width: 310, height: 150},
- {dest: 'Wide310x150Logo.scale-240.png', width: 744, height: 360},
- {dest: 'SplashScreenPhone.scale-240.png', width: 1152, height: 1920}
- ];
+/** Maps MRT splashscreen image to its target project defined in platformImages -> 8.1|WP8.1|10
+ * This assumes we have different scales used for 8.1 and 10 projects.
+ * The only intersection is scale-100, which is treated as Win10 project' splashscreen (because
+ * size limit applies to Win10 project so we'll need to check it).
+ */
+function mrtSplashScreenToTargetProject(mrtImage) {
+ var matchingSplashScreen = platformImages.filter(function (img) {
+ return img.dest.indexOf('SplashScreen') !== -1 &&
--- End diff --
There is a `MRTImage.prototype.generateFilenameFrom` method that returns a file name for MRT image, so you could just compare it with `img.dest`. Also ypu probably need to use `mrtImage.basename` instead of hardcoded `SplashScreen` string
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89792140
--- Diff: cordova-js-src/splashscreen.js ---
@@ -58,8 +58,45 @@ function readBoolFromCfg(preferenceName, defaultValue, cfg) {
}
}
+// Refine splashscreen file extension to match what is in config.xml
+function refineSplashScreenExtension(cfg) {
--- End diff --
We could go that way and add another method in confighelper like `readManifest` but:
1. I'm not sure how it will affect performance (confighelper is based on async xhr),
2. Then we'll need to manipulate the Image.src anyway as it is in different format.
The benefit of this approach is more confidence that we are using the correct (base)name - but it is not an issue as it is hardcoded now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89739874
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
+ } else {
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ matchesTarget = splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+ }
+
+ return matchesTarget;
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
+
+ if (matchesTarget === true) {
+ extensionsUsed[target] = extensionsUsed[target] || [];
+ extensionsUsed[target].push(path.extname(img.src.toLowerCase()));
+ }
+
+ return matchesTarget;
+ }
+
+ function updateSplashExtensionInManifest(manifestFileName, splashScreen) {
+ var manifest = AppxManifest.get(path.join(locations.root, manifestFileName));
+ var newExtension = path.extname(splashScreen.src);
+
+ if (manifest.getVisualElements().getSplashScreenExtension() !== newExtension) {
+ events.emit('verbose', 'Set ' + manifestFileName + ' SplashScreen image extension to "' + newExtension + '"');
+ manifest.getVisualElements().setSplashScreenExtension(newExtension);
+ manifest.write();
+ }
+ }
+
+ var splashScreens = cordovaProject.projectConfig.getSplashScreens('windows');
+
+ var desktopSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_DESKTOP_TARGET_NAME);
+ })[0];
+
+ var phoneSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_PHONE_TARGET_NAME);
+ })[0];
+
+ checkThatExtensionsAreNotMixed();
+
+ var manifestSplashScreenMap = [{
--- End diff --
Nit: IMO dictionaly is a better candidate here than array of dictionaries w/ 2 keys each
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89739598
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
+ } else {
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ matchesTarget = splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+ }
+
+ return matchesTarget;
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
+
+ if (matchesTarget === true) {
+ extensionsUsed[target] = extensionsUsed[target] || [];
+ extensionsUsed[target].push(path.extname(img.src.toLowerCase()));
+ }
+
+ return matchesTarget;
+ }
+
+ function updateSplashExtensionInManifest(manifestFileName, splashScreen) {
+ var manifest = AppxManifest.get(path.join(locations.root, manifestFileName));
+ var newExtension = path.extname(splashScreen.src);
+
+ if (manifest.getVisualElements().getSplashScreenExtension() !== newExtension) {
+ events.emit('verbose', 'Set ' + manifestFileName + ' SplashScreen image extension to "' + newExtension + '"');
+ manifest.getVisualElements().setSplashScreenExtension(newExtension);
+ manifest.write();
+ }
+ }
+
+ var splashScreens = cordovaProject.projectConfig.getSplashScreens('windows');
+
+ var desktopSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_DESKTOP_TARGET_NAME);
+ })[0];
+
+ var phoneSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_PHONE_TARGET_NAME);
+ })[0];
+
+ checkThatExtensionsAreNotMixed();
--- End diff --
Could you please explain, what's the purpose of this check? Please give a couple of examples when it would yield a warning?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89769817
--- Diff: template/CordovaApp.Phone.jsproj ---
@@ -79,7 +79,9 @@
<AppxManifest Include="package.phone.appxmanifest">
<SubType>Designer</SubType>
</AppxManifest>
- <Content Include="images\*.png" Exclude="images\*.scale-180.*" />
+ <Content Exclude="images\*SplashScreen.*" Include="images\*.png" />
--- End diff --
We are excluding images named like `SplashScreen.jpg` or `SplashScreen.scale-100.png` here for phone project (we can do this taking into account that phone and desktop projects have different hardcoded splashscreen names).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r90007607
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
+ } else {
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ matchesTarget = splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+ }
+
+ return matchesTarget;
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
+
+ if (matchesTarget === true) {
+ extensionsUsed[target] = extensionsUsed[target] || [];
+ extensionsUsed[target].push(path.extname(img.src.toLowerCase()));
+ }
+
+ return matchesTarget;
+ }
+
+ function updateSplashExtensionInManifest(manifestFileName, splashScreen) {
+ var manifest = AppxManifest.get(path.join(locations.root, manifestFileName));
+ var newExtension = path.extname(splashScreen.src);
+
+ if (manifest.getVisualElements().getSplashScreenExtension() !== newExtension) {
+ events.emit('verbose', 'Set ' + manifestFileName + ' SplashScreen image extension to "' + newExtension + '"');
+ manifest.getVisualElements().setSplashScreenExtension(newExtension);
+ manifest.write();
+ }
+ }
+
+ var splashScreens = cordovaProject.projectConfig.getSplashScreens('windows');
+
+ var desktopSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_DESKTOP_TARGET_NAME);
+ })[0];
+
+ var phoneSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_PHONE_TARGET_NAME);
+ })[0];
+
+ checkThatExtensionsAreNotMixed();
--- End diff --
In fact I've added this for a case when a developer could try to use JPG only for larger images (scale 200% and up) because they usually exceed the 200K limit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89729105
--- Diff: template/cordova/lib/prepare.js ---
@@ -353,6 +391,13 @@ function mapImageResources(images, imagesDir) {
events.emit('warn', 'No images found for target: ' + img.target);
} else {
candidates.forEach(function(mrtImage) {
+ if (img.target === SPLASH_SCREEN_DESKTOP_TARGET_NAME &&
+ mrtSplashScreenToTargetProject(mrtImage) === TARGET_PROJECT_10 &&
+ exceedsSizeLimit(mrtImage.path)) {
--- End diff --
Are you sure that we need to handle this case for the user? I'd rather the build fail to clearly indicate to user that spash image must be updated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-windows/pull/212
## [Current coverage](https://codecov.io/gh/apache/cordova-windows/pull/212?src=pr) is 76.04% (diff: 95.69%)
> Merging [#212](https://codecov.io/gh/apache/cordova-windows/pull/212?src=pr) into [master](https://codecov.io/gh/apache/cordova-windows/branch/master?src=pr) will increase coverage by **0.76%**
```diff
@@ master #212 diff @@
==========================================
Files 16 16
Lines 2120 2200 +80
Methods 401 415 +14
Messages 0 0
Branches 410 430 +20
==========================================
+ Hits 1596 1673 +77
- Misses 524 527 +3
Partials 0 0
```
> Powered by [Codecov](https://codecov.io?src=pr). Last update [09d582c...bd0096b](https://codecov.io/gh/apache/cordova-windows/compare/09d582c8705625bbdcd8fb97c30c02eea04d14b0...bd0096b71a3d6d65169d7d4582e50a05dc48ecb0?src=pr)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89770853
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
+ } else {
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ matchesTarget = splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+ }
+
+ return matchesTarget;
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
+
+ if (matchesTarget === true) {
+ extensionsUsed[target] = extensionsUsed[target] || [];
+ extensionsUsed[target].push(path.extname(img.src.toLowerCase()));
+ }
+
+ return matchesTarget;
+ }
+
+ function updateSplashExtensionInManifest(manifestFileName, splashScreen) {
+ var manifest = AppxManifest.get(path.join(locations.root, manifestFileName));
+ var newExtension = path.extname(splashScreen.src);
+
+ if (manifest.getVisualElements().getSplashScreenExtension() !== newExtension) {
+ events.emit('verbose', 'Set ' + manifestFileName + ' SplashScreen image extension to "' + newExtension + '"');
+ manifest.getVisualElements().setSplashScreenExtension(newExtension);
+ manifest.write();
+ }
+ }
+
+ var splashScreens = cordovaProject.projectConfig.getSplashScreens('windows');
+
+ var desktopSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_DESKTOP_TARGET_NAME);
+ })[0];
+
+ var phoneSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_PHONE_TARGET_NAME);
+ })[0];
+
+ checkThatExtensionsAreNotMixed();
--- End diff --
```xml
<splash src="res/screen/windows/splashscreen.png" width="620" height="300"/>
<splash src="res/screen/windows/splashscreen.scale-125.jpg" width="775" height="375"/>
```
You should not mix image extensions for non-MRT syntax inside one project group - in this case ^ the second JPG image will not be used as AppxManifest will have PNG for SplashScreen.Image' extension.
There is also a [test for this case](https://github.com/apache/cordova-windows/pull/212/files#diff-954e5996601fc5ce41f4107af4f31525R620).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89991524
--- Diff: template/cordova/lib/prepare.js ---
@@ -297,48 +305,80 @@ function applyNavigationWhitelist(config, manifest) {
manifest.getApplication().setAccessRules(whitelistRules);
}
-function mapImageResources(images, imagesDir) {
- var pathMap = {};
+// Platform default images
+var platformImages = [
+ {dest: 'Square150x150Logo.scale-100', width: 150, height: 150},
+ {dest: 'Square30x30Logo.scale-100', width: 30, height: 30},
+ {dest: 'StoreLogo.scale-100', width: 50, height: 50},
+ {dest: 'SplashScreen.scale-100', width: 620, height: 300, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-125', width: 775, height: 375, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-140', width: 868, height: 420, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-150', width: 930, height: 450, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-180', width: 1116, height: 540, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-200', width: 1240, height: 600, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-400', width: 2480, height: 1200, targetProject: TARGET_PROJECT_10},
+ // scaled images are specified here for backward compatibility only so we can find them by size
+ {dest: 'StoreLogo.scale-240', width: 120, height: 120},
+ {dest: 'Square44x44Logo.scale-100', width: 44, height: 44},
+ {dest: 'Square44x44Logo.scale-240', width: 106, height: 106},
+ {dest: 'Square70x70Logo.scale-100', width: 70, height: 70},
+ {dest: 'Square71x71Logo.scale-100', width: 71, height: 71},
+ {dest: 'Square71x71Logo.scale-240', width: 170, height: 170},
+ {dest: 'Square150x150Logo.scale-240', width: 360, height: 360},
+ {dest: 'Square310x310Logo.scale-100', width: 310, height: 310},
+ {dest: 'Wide310x150Logo.scale-100', width: 310, height: 150},
+ {dest: 'Wide310x150Logo.scale-240', width: 744, height: 360},
+ {dest: 'SplashScreenPhone.scale-100', width: 480, height: 800, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-140', width: 672, height: 1120, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-240', width: 1152, height: 1920, targetProject: TARGET_PROJECT_WP81}
+];
+
+function findPlatformImage(width, height) {
+ if (!width && !height){
+ // this could be default image,
+ // Windows requires specific image dimension so we can't apply it
+ return null;
+ }
+ for (var idx in platformImages){
+ var res = platformImages[idx];
+ // If only one of width or height is not specified, use another parameter for comparation
+ // If both specified, compare both.
+ if ((!width || (width == res.width)) &&
+ (!height || (height == res.height))){
+ return res;
+ }
+ }
+ return null;
+}
- // Platform default images
- var platformImages = [
- {dest: 'Square150x150Logo.scale-100.png', width: 150, height: 150},
- {dest: 'Square30x30Logo.scale-100.png', width: 30, height: 30},
- {dest: 'StoreLogo.scale-100.png', width: 50, height: 50},
- {dest: 'SplashScreen.scale-100.png', width: 620, height: 300},
- // scaled images are specified here for backward compatibility only so we can find them by size
- {dest: 'StoreLogo.scale-240.png', width: 120, height: 120},
- {dest: 'Square44x44Logo.scale-100.png', width: 44, height: 44},
- {dest: 'Square44x44Logo.scale-240.png', width: 106, height: 106},
- {dest: 'Square70x70Logo.scale-100.png', width: 70, height: 70},
- {dest: 'Square71x71Logo.scale-100.png', width: 71, height: 71},
- {dest: 'Square71x71Logo.scale-240.png', width: 170, height: 170},
- {dest: 'Square150x150Logo.scale-240.png', width: 360, height: 360},
- {dest: 'Square310x310Logo.scale-100.png', width: 310, height: 310},
- {dest: 'Wide310x150Logo.scale-100.png', width: 310, height: 150},
- {dest: 'Wide310x150Logo.scale-240.png', width: 744, height: 360},
- {dest: 'SplashScreenPhone.scale-240.png', width: 1152, height: 1920}
- ];
+/** Maps MRT splashscreen image to its target project defined in platformImages -> 8.1|WP8.1|10
--- End diff --
nit: another JSDoc format issue
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89727836
--- Diff: template/cordova/lib/prepare.js ---
@@ -297,48 +305,78 @@ function applyNavigationWhitelist(config, manifest) {
manifest.getApplication().setAccessRules(whitelistRules);
}
-function mapImageResources(images, imagesDir) {
- var pathMap = {};
+// Platform default images
+var platformImages = [
+ {dest: 'Square150x150Logo.scale-100', width: 150, height: 150},
+ {dest: 'Square30x30Logo.scale-100', width: 30, height: 30},
+ {dest: 'StoreLogo.scale-100', width: 50, height: 50},
+ {dest: 'SplashScreen.scale-100', width: 620, height: 300, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-125', width: 775, height: 375, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-140', width: 868, height: 420, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-150', width: 930, height: 450, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-180', width: 1116, height: 540, targetProject: TARGET_PROJECT_81},
+ {dest: 'SplashScreen.scale-200', width: 1240, height: 600, targetProject: TARGET_PROJECT_10},
+ {dest: 'SplashScreen.scale-400', width: 2480, height: 1200, targetProject: TARGET_PROJECT_10},
+ // scaled images are specified here for backward compatibility only so we can find them by size
+ {dest: 'StoreLogo.scale-240', width: 120, height: 120},
+ {dest: 'Square44x44Logo.scale-100', width: 44, height: 44},
+ {dest: 'Square44x44Logo.scale-240', width: 106, height: 106},
+ {dest: 'Square70x70Logo.scale-100', width: 70, height: 70},
+ {dest: 'Square71x71Logo.scale-100', width: 71, height: 71},
+ {dest: 'Square71x71Logo.scale-240', width: 170, height: 170},
+ {dest: 'Square150x150Logo.scale-240', width: 360, height: 360},
+ {dest: 'Square310x310Logo.scale-100', width: 310, height: 310},
+ {dest: 'Wide310x150Logo.scale-100', width: 310, height: 150},
+ {dest: 'Wide310x150Logo.scale-240', width: 744, height: 360},
+ {dest: 'SplashScreenPhone.scale-100', width: 480, height: 800, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-140', width: 672, height: 1120, targetProject: TARGET_PROJECT_WP81},
+ {dest: 'SplashScreenPhone.scale-240', width: 1152, height: 1920, targetProject: TARGET_PROJECT_WP81}
+];
+
+function findPlatformImage(width, height) {
+ if (!width && !height){
+ // this could be default image,
+ // Windows requires specific image dimension so we can't apply it
+ return null;
+ }
+ for (var idx in platformImages){
+ var res = platformImages[idx];
+ // If only one of width or height is not specified, use another parameter for comparation
+ // If both specified, compare both.
+ if ((!width || (width == res.width)) &&
+ (!height || (height == res.height))){
+ return res;
+ }
+ }
+ return null;
+}
- // Platform default images
- var platformImages = [
- {dest: 'Square150x150Logo.scale-100.png', width: 150, height: 150},
- {dest: 'Square30x30Logo.scale-100.png', width: 30, height: 30},
- {dest: 'StoreLogo.scale-100.png', width: 50, height: 50},
- {dest: 'SplashScreen.scale-100.png', width: 620, height: 300},
- // scaled images are specified here for backward compatibility only so we can find them by size
- {dest: 'StoreLogo.scale-240.png', width: 120, height: 120},
- {dest: 'Square44x44Logo.scale-100.png', width: 44, height: 44},
- {dest: 'Square44x44Logo.scale-240.png', width: 106, height: 106},
- {dest: 'Square70x70Logo.scale-100.png', width: 70, height: 70},
- {dest: 'Square71x71Logo.scale-100.png', width: 71, height: 71},
- {dest: 'Square71x71Logo.scale-240.png', width: 170, height: 170},
- {dest: 'Square150x150Logo.scale-240.png', width: 360, height: 360},
- {dest: 'Square310x310Logo.scale-100.png', width: 310, height: 310},
- {dest: 'Wide310x150Logo.scale-100.png', width: 310, height: 150},
- {dest: 'Wide310x150Logo.scale-240.png', width: 744, height: 360},
- {dest: 'SplashScreenPhone.scale-240.png', width: 1152, height: 1920}
- ];
+/** Maps MRT splashscreen image to its target project defined in platformImages -> 8.1|WP8.1|10
+ * This assumes we have different
--- End diff --
Nit: incomplete comment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89727040
--- Diff: template/CordovaApp.Phone.jsproj ---
@@ -79,7 +79,9 @@
<AppxManifest Include="package.phone.appxmanifest">
<SubType>Designer</SubType>
</AppxManifest>
- <Content Include="images\*.png" Exclude="images\*.scale-180.*" />
+ <Content Exclude="images\*SplashScreen.*" Include="images\*.png" />
--- End diff --
Could you please explain how does this work?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89729302
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
--- End diff --
Nit: this is not a valid syntax for multiline JSDoc comments, please update
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89994632
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +492,107 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/**
+ * Checks if the splash screen matches the target project
+ * @param {Object} splash SplashScreen object to check
+ * @param {String} targetName SplashScreen target name: 'SplashScreen'|'SplashScreenPhone'
+ * @returns {Boolean} True if the splash screen matches the target project
+ */
+function splashScreenTargetIs(splash, targetName) {
+ var targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ return splash.target === targetName;
+ }
+
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ return splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
--- End diff --
IMO it's not very clear, what is `splashScreenTargetIs` function for and what values it can return. I'd suggest to rework this in the following way:
1. have a function that returns a target project based on image `target` or dimensions (e.g. `getTargetForImage`)
2. do a comparison of that function's result and `target` variable here so it's be clear that we expect result to match target
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the issue:
https://github.com/apache/cordova-windows/pull/212
@vladimir-kotikov addressed, thanks! It is good to go?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89789288
--- Diff: template/cordova/lib/prepare.js ---
@@ -353,6 +391,13 @@ function mapImageResources(images, imagesDir) {
events.emit('warn', 'No images found for target: ' + img.target);
} else {
candidates.forEach(function(mrtImage) {
+ if (img.target === SPLASH_SCREEN_DESKTOP_TARGET_NAME &&
+ mrtSplashScreenToTargetProject(mrtImage) === TARGET_PROJECT_10 &&
+ exceedsSizeLimit(mrtImage.path)) {
--- End diff --
This does not result in a build failure and the issue could only be seen in Visual Studio manifest designer so I believe the warning is useful here.
Do you mean we should fail the build rather than showing this warning?
It currently just skips oversized images.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the issue:
https://github.com/apache/cordova-windows/pull/212
@vladimir-kotikov thank you for the review!
I've addressed the notes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by frederikbosch <gi...@git.apache.org>.
Github user frederikbosch commented on the issue:
https://github.com/apache/cordova-windows/pull/212
Any chance that a version with this included will be released any time soon?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89729637
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
--- End diff --
Nit: `return splash.target === targetName` is the same
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89996388
--- Diff: template/cordova/lib/prepare.js ---
@@ -439,6 +490,109 @@ function getUAPVersions(config) {
};
}
+/** Checks if a targetProject matches splashscreen target name */
+function splashScreenTargetProjectMatchesTargetName(targetProject, targetName) {
+ if (targetName === SPLASH_SCREEN_DESKTOP_TARGET_NAME) {
+ return targetProject === TARGET_PROJECT_81 || targetProject === TARGET_PROJECT_10;
+ }
+
+ return targetProject === TARGET_PROJECT_WP81;
+}
+
+/** Checks if the splash screen matches the target project
+ * @param targetName 'SplashScreen'|'SplashScreenPhone'
+ * @returns boolean */
+function splashScreenTargetIs(splash, targetName) {
+ var matchesTarget, targetImg;
+
+ if (splash.target) {
+ // MRT syntax:
+ matchesTarget = splash.target === targetName;
+ } else {
+ // Fall back on find by size for old non-MRT syntax:
+ targetImg = findPlatformImage(splash.width, splash.height);
+ matchesTarget = splashScreenTargetProjectMatchesTargetName(targetImg.targetProject, targetName);
+ }
+
+ return matchesTarget;
+}
+
+// Updates manifests to match the app splash screen image types (PNG/JPG/JPEG)
+function updateSplashScreenImageExtensions(cordovaProject, locations) {
+
+ // Saving all extensions used for targets to verify them later
+ var extensionsUsed = {};
+
+ function checkThatExtensionsAreNotMixed() {
+ for (var target in extensionsUsed) {
+ /*jshint loopfunc: true */
+ if (extensionsUsed.hasOwnProperty(target)) {
+ var extensionsUsedForTarget = extensionsUsed[target];
+
+ // Check that extensions are not mixed:
+ if (extensionsUsedForTarget.length > 1 && extensionsUsedForTarget.some(function(item) {
+ return item !== extensionsUsedForTarget[0];
+ })) {
+ events.emit('warn', '"' + target + '" splash screens have mixed file extensions which is not supported. Some of the images will not be used.');
+ }
+ }
+ }
+ }
+
+ function checkTargetMatchAndUpdateUsedExtensions(img, target) {
+ var matchesTarget = splashScreenTargetIs(img, target);
+
+ if (matchesTarget === true) {
+ extensionsUsed[target] = extensionsUsed[target] || [];
+ extensionsUsed[target].push(path.extname(img.src.toLowerCase()));
+ }
+
+ return matchesTarget;
+ }
+
+ function updateSplashExtensionInManifest(manifestFileName, splashScreen) {
+ var manifest = AppxManifest.get(path.join(locations.root, manifestFileName));
+ var newExtension = path.extname(splashScreen.src);
+
+ if (manifest.getVisualElements().getSplashScreenExtension() !== newExtension) {
+ events.emit('verbose', 'Set ' + manifestFileName + ' SplashScreen image extension to "' + newExtension + '"');
+ manifest.getVisualElements().setSplashScreenExtension(newExtension);
+ manifest.write();
+ }
+ }
+
+ var splashScreens = cordovaProject.projectConfig.getSplashScreens('windows');
+
+ var desktopSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_DESKTOP_TARGET_NAME);
+ })[0];
+
+ var phoneSplashScreen = splashScreens.filter(function(img) {
+ return checkTargetMatchAndUpdateUsedExtensions(img, SPLASH_SCREEN_PHONE_TARGET_NAME);
+ })[0];
+
+ checkThatExtensionsAreNotMixed();
--- End diff --
> You should not mix image extensions for non-MRT syntax inside one project group
I thought it's pretty obvious :)
> in this case ^ the second JPG image will not be used as AppxManifest will have PNG for SplashScreen.Image' extension
Yep. If you specify the same splascreens using MRT and 'target' attribute, `splashscreen.scale-125.jpg` also would be silently ignored because `mapImageResources` requires extensions to match. That said I don't think we need to care about this at all.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the issue:
https://github.com/apache/cordova-windows/pull/212
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89991151
--- Diff: template/cordova/lib/AppxManifest.js ---
@@ -416,6 +417,18 @@ AppxManifest.prototype.getVisualElements = function () {
}
return this;
},
+ getSplashScreenExtension: function (extension) {
--- End diff --
Instead of very specific to this issue `get/setSplashScreenExtension` can we have more generic `get/setSplashScreen` to set the filename?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89990562
--- Diff: cordova-js-src/splashscreen.js ---
@@ -58,8 +58,45 @@ function readBoolFromCfg(preferenceName, defaultValue, cfg) {
}
}
+// Refine splashscreen file extension to match what is in config.xml
+function refineSplashScreenExtension(cfg) {
--- End diff --
> Then we'll need to manipulate the Image.src anyway as it is in different format.
Why? Isn't the logic in 'prepare' supposed to update splash screen file name to actual value? The benefit IMO is that you'll have one place where you're trying to infer splashscreen real filename based on `splashscreen` preferences
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89726245
--- Diff: cordova-js-src/splashscreen.js ---
@@ -58,8 +58,45 @@ function readBoolFromCfg(preferenceName, defaultValue, cfg) {
}
}
+// Refine splashscreen file extension to match what is in config.xml
+function refineSplashScreenExtension(cfg) {
--- End diff --
Just a suggestion - is it possible to read splashscreen file name and extension directly from appxmanifest? This way you could avoid all this tricky logic for getting the right filename.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89726640
--- Diff: spec/unit/Prepare.Win10.spec.js ---
@@ -541,4 +541,165 @@ describe('copyIcons method', function () {
expect(FileUpdater.updatePaths).toHaveBeenCalledWith(expectedPathMap, { rootDir: PROJECT }, logFileOp);
});
});
+
+ it('should ignore splashScreens for Windows 10 project with size >200K and emit a warning', function () {
+ var size300K = 300 * 1024;
+ var warnSpy = jasmine.createSpy('warn');
+ events.on('warn', warnSpy);
+
+ var splashScreensFiles = [
--- End diff --
Nit: this could be calculated from `splashScreens`: `splashScreens.map(splash => path.basename(splash.src)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-windows/pull/212
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r90007897
--- Diff: template/cordova/lib/AppxManifest.js ---
@@ -416,6 +417,18 @@ AppxManifest.prototype.getVisualElements = function () {
}
return this;
},
+ getSplashScreenExtension: function (extension) {
--- End diff --
Please see https://github.com/apache/cordova-windows/pull/212#discussion_r90006200
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89993606
--- Diff: template/cordova/lib/prepare.js ---
@@ -353,6 +391,13 @@ function mapImageResources(images, imagesDir) {
events.emit('warn', 'No images found for target: ' + img.target);
} else {
candidates.forEach(function(mrtImage) {
+ if (img.target === SPLASH_SCREEN_DESKTOP_TARGET_NAME &&
+ mrtSplashScreenToTargetProject(mrtImage) === TARGET_PROJECT_10 &&
+ exceedsSizeLimit(mrtImage.path)) {
--- End diff --
Makes sense
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows issue #212: CB-9287 Not enough Icons and Splashscreens for W...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the issue:
https://github.com/apache/cordova-windows/pull/212
@vladimir-kotikov could you please take a look again?
I've also added a fix for [CB-12192 - No SplashScreen on Windows when content.src is subpage](https://issues.apache.org/jira/browse/CB-12192) - the issue occured while testing splashscreen API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89991437
--- Diff: template/cordova/lib/prepare.js ---
@@ -297,48 +305,80 @@ function applyNavigationWhitelist(config, manifest) {
manifest.getApplication().setAccessRules(whitelistRules);
}
-function mapImageResources(images, imagesDir) {
- var pathMap = {};
+// Platform default images
+var platformImages = [
--- End diff --
nit: could you please make it uppercase, so i'd be easy to understand in code that we're using some constant value
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89726251
--- Diff: cordova-js-src/confighelper.js ---
@@ -22,16 +22,17 @@
// config.xml wrapper (non-node ConfigParser analogue)
var config;
function Config(xhr) {
- function loadPreferences(xhr) {
+ function load(xhr, tagName) {
var parser = new DOMParser();
var doc = parser.parseFromString(xhr.responseText, "application/xml");
- var preferences = doc.getElementsByTagName("preference");
+ var preferences = doc.getElementsByTagName(tagName);
return Array.prototype.slice.call(preferences);
}
this.xhr = xhr;
- this.preferences = loadPreferences(this.xhr);
+ this.preferences = load(this.xhr, "preference");
+ this.splashScreens = load(this.xhr, "splash");
--- End diff --
Calling `load` second time here will trigger re-parsing of `config.xml` which, I believe, is not very efficient. Might be better to rework `load` method to reuse already parsed document
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-windows pull request #212: CB-9287 Not enough Icons and Splashscreen...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/212#discussion_r89729119
--- Diff: template/cordova/lib/prepare.js ---
@@ -362,7 +407,13 @@ function mapImageResources(images, imagesDir) {
// find target image by size
var targetImg = findPlatformImage(img.width, img.height);
if (targetImg) {
- var targetPath = path.join(imagesDir, targetImg.dest);
+ if (targetImg.targetProject === TARGET_PROJECT_10 &&
--- End diff --
Same as above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org