CB-13685 android: Adaptive Icon Support #448

erisu · 2018-06-08T10:28:40Z

Platforms affected

Android

What does this PR do?

This PR adds support for Adaptive Icon.

Changes to Default Template Project

Updated AndroidManifest to use adaptive icons configuration by default.
Added adaptive icons and ic_launcher.xml for Android platform 26 and higher.

Added JavaScript Unit Testing.

config.xml for Adaptive Icons with Images

<platform name="android">
  <icon background="res/icon/android/ldpi-background.png" density="ldpi" foreground="res/icon/android/ldpi-foreground.png" />
  <icon background="res/icon/android/mdpi-background.png" density="mdpi" foreground="res/icon/android/mdpi-foreground.png" />
  <icon background="res/icon/android/hdpi-background.png" density="hdpi" foreground="res/icon/android/hdpi-foreground.png" />
  <icon background="res/icon/android/xhdpi-background.png" density="xhdpi" foreground="res/icon/android/xhdpi-foreground.png" />
  <icon background="res/icon/android/xxhdpi-background.png" density="xxhdpi" foreground="res/icon/android/xxhdpi-foreground.png" />
  <icon background="res/icon/android/xxxhdpi-background.png" density="xxxhdpi" foreground="res/icon/android/xxxhdpi-foreground.png" />
</platform>

In this case, the foreground image will automatically be applied to the src attribute value. This is to support non-adaptive icon supported Android devices. If the user supplies their own src value, foreground will not be copied over.

config.xml for Adaptive Icons with Vectors

<platform name="android">
  <icon background="res/icon/android/ldpi-background.xml” density="ldpi" foreground="res/icon/android/ldpi-foreground.xml” src="res/icon/android/ldpi-icon.png" />
  <icon background="res/icon/android/mdpi-background.xml” density="mdpi" foreground="res/icon/android/mdpi-foreground.xml” src="res/icon/android/mdpi-icon.png" />
  <icon background="res/icon/android/hdpi-background.xml” density="hdpi" foreground="res/icon/android/hdpi-foreground.xml” src="res/icon/android/hdpi-icon.png" />
  <icon background="res/icon/android/xhdpi-background.xml” density="xhdpi" foreground="res/icon/android/xhdpi-foreground.xml” src="res/icon/android/xdpi-icon.png" />
  <icon background="res/icon/android/xxhdpi-background.xml” density="xxhdpi" foreground="res/icon/android/xxhdpi-foreground.xml” src="res/icon/android/xxhdpi-icon.png" />
  <icon background="res/icon/android/xxxhdpi-background.xml” density="xxxhdpi" foreground="res/icon/android/xxxhdpi-foreground.xml” src="res/icon/android/xxxhdpi-icon.png" />
</platform>

In this example, the adaptive icon supports vectors and colors for the foreground and background. If the foreground is a vector or color, the src attribute must also be defined with a PNG for Android devices that does not support adaptive icon.

config.xml for Adaptive Icons with Colors for Background
First: create colors.xml file with content

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <color name="background">#FF0000</color>
</resources>

Second: update config.xml with…

<platform name="android">
  <resource-file src="/res/values/colors.xml" target="/app/src/main/res/values/colors.xml" />

  <icon background=“@color/background” density="ldpi" foreground="res/icon/android/ldpi-foreground.png" />
  <icon background="@color/background" density="mdpi" foreground="res/icon/android/mdpi-foreground.png" />
  <icon background="@color/background" density="hdpi" foreground="res/icon/android/hdpi-foreground.png" />
  <icon background="@color/background" density="xhdpi" foreground="res/icon/android/xhdpi-foreground.png" />
  <icon background="@color/background" density="xxhdpi" foreground="res/icon/android/xxhdpi-foreground.png" />
  <icon background="@color/background" density="xxxhdpi" foreground="res/icon/android/xxxhdpi-foreground.png" />
</platform>

The naming convention for the two new attributes, background and foreground, was decided to match the element names that is used in the ic_launcher.xml for consistency. https://developer.android.com/guide/practices/ui_guidelines/icon_design_adaptive#creating_adaptive_icons_in_xml

 Error Example: Foreground contains vector and missing src attribute.

<platform name="android">
  <icon background="res/icon/android/mdpi-background.png" foreground="res/icon/android/mdpi-foreground.xml” density=“mdpi" />
</platform>

What testing has been done on this change?

Built default app with no modifications to config.xml
Built default app with legacy icon set defined
Built default app with adaptive icon set with images.
Built default app with adaptive icon set with vectors.
Built default app with adaptive icon set with colors.
Converted from adaptive to legacy icon set and rebuilt.
Converted from legacy to adaptive icon set and rebuild.

Checklist

Reported an issue in the JIRA database
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.
Added automated test coverage as appropriate for this change.

erisu · 2018-06-08T13:49:43Z

I also want to point out that this PR will be required.

apache/cordova-common#26

This PR contains the updates to the ConfigParser to include foreground and background when getting static resources.

dpogue · 2018-06-09T07:31:12Z

You can ignore the node 4 Appveyor failure due to let/const, we're in the process of dropping support for node 4.

erisu · 2018-06-09T07:49:35Z

@dpogue I have also noticed this.

There is the CI improvement PR, #442, which deprecates node 4, adds node 10 support, and other changes.

When I tested my PR with the CI improvement PR, other issues appeared.
https://ci.appveyor.com/project/erisu/cordova-android/build/1.0.14

If we just remove node 4 and add node 10, with no additional changes, then my PR passed.
https://ci.appveyor.com/project/erisu/cordova-android/build/1.0.17

raphinesse · 2018-06-12T22:01:12Z

@erisu I rebased #442 onto latest master and your changes on top. CI tests pass on Travis as well as AppVeyor. I guess the failures were fixed by #450.

Note that in your "passing" CI tests with only Node 4 removed, the Java tests actually never ran, due to a bug that is fixed by #442.

codecov-io · 2018-06-18T01:57:43Z

Codecov Report

Merging #448 into master will decrease coverage by 19.06%.
The diff coverage is 66.92%.

@@             Coverage Diff             @@
##           master     #448       +/-   ##
===========================================
- Coverage    62.9%   43.84%   -19.07%     
===========================================
  Files          15       18        +3     
  Lines        1650     2023      +373     
  Branches      308      383       +75     
===========================================
- Hits         1038      887      -151     
- Misses        612     1136      +524

Impacted Files	Coverage Δ
bin/templates/cordova/lib/AndroidManifest.js	`35.44% <40%> (-64.56%)`	⬇️
bin/templates/cordova/lib/prepare.js	`41.66% <68%> (ø)`
bin/templates/cordova/lib/retry.js	`15.38% <0%> (-84.62%)`	⬇️
bin/templates/cordova/lib/device.js	`22.44% <0%> (-77.56%)`	⬇️
bin/templates/cordova/lib/run.js	`26.98% <0%> (-73.02%)`	⬇️
bin/templates/cordova/lib/Adb.js	`34.14% <0%> (-65.86%)`	⬇️
bin/templates/cordova/lib/builders/builders.js	`37.5% <0%> (-62.5%)`	⬇️
bin/templates/cordova/lib/emulator.js	`48.64% <0%> (-40.97%)`	⬇️
...n/templates/cordova/lib/builders/ProjectBuilder.js
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebbd91f...821038e. Read the comment docs.

erisu · 2018-06-19T07:08:48Z

@raphinesse I rebased with the latest master now that the #442 PR has been merged. Do you think this is ready to merge?

raphinesse · 2018-06-19T08:10:42Z

Sorry, I know very little about the Android build, so I can't really review this. Maybe @dpogue or @infiloop.

raphinesse

Just my two cents

raphinesse · 2018-06-19T08:16:54Z

spec/unit/prepare.spec.js

+                height: undefined,
+                background: 'res/icon/android/mdpi-background.png',
+                foreground: undefined
+            }];


You have something similar to this in almost every test. Might be nice to factor this out e.g as a function that fills in missing props as undefined. This could help to slim down this 1000 lines spec.

raphinesse · 2018-06-19T08:19:19Z

spec/unit/prepare.spec.js

+            new CordovaError('One of the following attributes are set but missing the other for the density type: mdpi. Please ensure that all require attributes are defined.')
+        );
+
+        done();


Are these tests asynchronous? If not, we don't need this. If so, I suggest returning promises to Jasmine instead of using the done callback.

erisu · 2018-06-19T11:44:35Z

@raphinesse Thank you for pointing out improvements to my test spec. I have updated the test and reduced the line count. I also removed the done() calls as you pointed out because they were not needed.

Menardi · 2018-06-21T00:56:55Z

This PR looks good to me. I need adaptive icons for one of my own apps, so I'm eager to see this get merged soon. Without support for this, icons for apps using cordova-android@7 (targeting API 26+) look a bit rubbish on newer devices.

I noticed the main cordova-android maintainer, Joe, has announced on the mailing list that he is stepping down. So, I expect that getting this thoroughly reviewed is now more difficult. If there is anything I can do to get this over the line, please let me know.

dpogue · 2018-06-21T03:47:35Z

I will make sure this gets merged into the next major

janpio · 2018-08-10T17:02:22Z

@raphinesse Could you have a look again and redo your review please?

- Update default project template's icons to be adaptive. - Added backwards support for non-adaptive icon supported devices.

Requested changes have been made

raphinesse · 2018-08-10T17:20:52Z

@janpio Unfortunately I am on a very tight schedule right now. The only thing I had reviewed were the specs. I glanced over them again and they do look much better. I still saw a few things that could be improved, but I don't have time to tackle that right now.

Since it's "only the specs", I would not want to block this PR because of this. However, I can neither give my approval for the rest of this PR since I know too little of the subject at hand.

janpio · 2018-08-10T17:39:42Z

Thanks @raphinesse! Same for me... hope someone with actual Android experience turns up.

erisu · 2018-08-10T17:43:25Z

@janpio I rebased with the latest master. I also did the same for cordova-common's PR that is associated with this PR.

The downside, both cordova-common and cordova-android have jasmine@^3.1.0 as the dep. Both will update to 3.2.0, and It seems the cordova-common tests already failed for what I believe is the same side-effect of the latest Jasmine's release.

apache/cordova-common#26

I am expecting these tests may show similar side-effects. We might need to update both repo's with the temp fix...

dpogue · 2018-08-10T17:46:18Z

The changes in this PR look fine to me. I had held off on merging due to some of the questions on the cordova-common side about where the Android-specific handling code should live, especially with @erisu's proposed refactoring.

wisespira · 2018-08-30T12:21:34Z

Do I need to install anything on top of cordova to get this to work? I'm currently getting a Unquoted attribute value error

janpio · 2018-08-30T12:27:57Z

Are you using cordova-android master in your app? This hasn't really been released yet.

wisespira · 2018-08-30T12:33:04Z

Oops thanks I'll again with the master branch

erisu · 2018-08-30T12:36:40Z

Though the PR is merged into master, the package.json still references cordova-common@^2.2.0. There is a cordova-common PR that is also necessary for this change to work. The cordova-common PR has also been merged into master which will be released on cordova-common@3.0.0.

Using cordova-android@master alone will not work unless you link cordova-common@master. Or wait for release.

LarryBJohnson · 2018-09-04T14:54:49Z

How would one go about switching from standered npm cordova to this master branch?

janpio · 2018-09-04T14:56:12Z

What does slandered npm cordova mean?

LarryBJohnson · 2018-09-04T15:36:13Z

If I where to go in my command line and download cordova using node js I would type npm install -g cordova would this give me a version of cordova that would let me use adaptive icons ?

janpio · 2018-09-04T15:38:15Z

No.

raphinesse · 2018-09-04T15:39:47Z

No. npm i -g cordova@nightly and npm i cordova-android@nightly in your project

LarryBJohnson · 2018-09-04T15:41:02Z

so does adding @nightly switch it to this branch ? allowing me to create adaptive icons with cordova ?

raphinesse · 2018-09-04T15:44:31Z

No. It uses the latest nightly build. Should be easier to setup than using master. Try an earlier nightly build if npm throws errors at you during installation. A full list of available versions can be found via npm.

nprail · 2018-11-27T20:39:22Z

Has this been released yet? If not, when will it be?

dpogue · 2018-11-27T20:54:11Z

It is not released yet, it will be included in the next major version. We can't/won't commit to any particular timeline for that next major, but we are working on it.

janpio mentioned this pull request Jun 8, 2018

CB-13685 android: Added adaptive icon docs apache/cordova-docs#830

Merged

2 tasks

erisu force-pushed the CB-13685 branch from 9c05f3d to 656e9c6 Compare June 18, 2018 01:46

raphinesse previously requested changes Jun 19, 2018

View reviewed changes

erisu force-pushed the CB-13685 branch 4 times, most recently from a6fe2df to d4cd4c6 Compare June 19, 2018 11:39

erisu force-pushed the CB-13685 branch 3 times, most recently from 889efe7 to 22bb844 Compare June 21, 2018 08:07

CB-13685 android: Adaptive Icon Support

821038e

- Update default project template's icons to be adaptive. - Added backwards support for non-adaptive icon supported devices.

erisu force-pushed the CB-13685 branch from 22bb844 to 821038e Compare August 10, 2018 17:17

imhoffd mentioned this pull request Aug 23, 2018

feat: make ionic cordova resources comply with Android O adaptive icons ionic-team/ionic-cli#2132

Closed

dpogue merged commit 8f2a4c7 into apache:master Aug 29, 2018

erisu deleted the CB-13685 branch April 4, 2019 06:16

DavidWiesner mentioned this pull request Apr 12, 2019

feature: Add support for adaptive icons in Android ionic-team/capacitor-assets#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-13685 android: Adaptive Icon Support #448

CB-13685 android: Adaptive Icon Support #448

erisu commented Jun 8, 2018

erisu commented Jun 8, 2018

dpogue commented Jun 9, 2018

erisu commented Jun 9, 2018 •

edited

Loading

raphinesse commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 18, 2018 •

edited

Loading

erisu commented Jun 19, 2018

raphinesse commented Jun 19, 2018

raphinesse left a comment

raphinesse Jun 19, 2018

raphinesse Jun 19, 2018

erisu commented Jun 19, 2018

Menardi commented Jun 21, 2018

dpogue commented Jun 21, 2018

janpio commented Aug 10, 2018

raphinesse commented Aug 10, 2018 •

edited

Loading

janpio commented Aug 10, 2018

erisu commented Aug 10, 2018 •

edited

Loading

dpogue commented Aug 10, 2018

wisespira commented Aug 30, 2018

janpio commented Aug 30, 2018

wisespira commented Aug 30, 2018

erisu commented Aug 30, 2018 •

edited

Loading

LarryBJohnson commented Sep 4, 2018 •

edited

Loading

janpio commented Sep 4, 2018

LarryBJohnson commented Sep 4, 2018

janpio commented Sep 4, 2018

raphinesse commented Sep 4, 2018

LarryBJohnson commented Sep 4, 2018 •

edited

Loading

raphinesse commented Sep 4, 2018

nprail commented Nov 27, 2018

dpogue commented Nov 27, 2018

CB-13685 android: Adaptive Icon Support #448

CB-13685 android: Adaptive Icon Support #448

Conversation

erisu commented Jun 8, 2018

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

erisu commented Jun 8, 2018

dpogue commented Jun 9, 2018

erisu commented Jun 9, 2018 • edited Loading

raphinesse commented Jun 12, 2018 • edited Loading

codecov-io commented Jun 18, 2018 • edited Loading

Codecov Report

erisu commented Jun 19, 2018

raphinesse commented Jun 19, 2018

raphinesse left a comment

Choose a reason for hiding this comment

raphinesse Jun 19, 2018

Choose a reason for hiding this comment

raphinesse Jun 19, 2018

Choose a reason for hiding this comment

erisu commented Jun 19, 2018

Menardi commented Jun 21, 2018

dpogue commented Jun 21, 2018

janpio commented Aug 10, 2018

raphinesse commented Aug 10, 2018 • edited Loading

janpio commented Aug 10, 2018

erisu commented Aug 10, 2018 • edited Loading

dpogue commented Aug 10, 2018

wisespira commented Aug 30, 2018

janpio commented Aug 30, 2018

wisespira commented Aug 30, 2018

erisu commented Aug 30, 2018 • edited Loading

LarryBJohnson commented Sep 4, 2018 • edited Loading

janpio commented Sep 4, 2018

LarryBJohnson commented Sep 4, 2018

janpio commented Sep 4, 2018

raphinesse commented Sep 4, 2018

LarryBJohnson commented Sep 4, 2018 • edited Loading

raphinesse commented Sep 4, 2018

nprail commented Nov 27, 2018

dpogue commented Nov 27, 2018

erisu commented Jun 9, 2018 •

edited

Loading

raphinesse commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 18, 2018 •

edited

Loading

raphinesse commented Aug 10, 2018 •

edited

Loading

erisu commented Aug 10, 2018 •

edited

Loading

erisu commented Aug 30, 2018 •

edited

Loading

LarryBJohnson commented Sep 4, 2018 •

edited

Loading

LarryBJohnson commented Sep 4, 2018 •

edited

Loading