Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add monochrome app icon support #1550

Merged
merged 16 commits into from Apr 9, 2023

Conversation

liyamahendra
Copy link
Contributor

@liyamahendra liyamahendra commented Jan 24, 2023

Platforms affected

  • android

Motivation and Context

Android 13 added support for Themed Icons. Since themed icons aren't yet supported on Cordova Android, adding support for Themed icons.

closes #1450

Description

I'm considering the below two use cases:

  • A new Android Cordova project, should by default have support for themed icons.
  • A project which specifies the monochrome icon in the config.xml for android platform

Testing

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@liyamahendra
Copy link
Contributor Author

liyamahendra commented Jan 24, 2023

Here is the screenshot showing the themed icon for a starter project

themed-icon-for-started-project

Icon credits: @Birowsky

@liyamahendra liyamahendra force-pushed the themed-icons branch 2 times, most recently from caea0bc to 95d4cdb Compare January 24, 2023 10:22
@breautek breautek added this to the 12.0.0 milestone Jan 24, 2023
@breautek
Copy link
Contributor

Looks like a great start.

I wonder if we can update the updateIcons function so that we can optionally specify a monochrome icon via the <icon> directives.

A full example may look like:

<icon
    density="xxxhdpi" 
    background="adaptive_bg.png"
    foreground="adaptive_fg.png"
    monochrome="monochrome.png"
    src="xxxhdpi.png"
/>

Traditionally we always supported both rasterized images as well as vectors, but if monochrome doesn't support a rasterized image then we will just need to make sure we note that when we document the feature.

The android docs does mention that adaptive icons are required to be declared if using the monochrome, so if the monochrome attribute is present, then we can forcefully require the background and foreground attributes on our icon directive.

To support this feature, your app must provide both an adaptive icon and a monochromatic app icon

Hopefully the existing code is readable enough that the pattern can be followed, e.g. copying the icon resource to the appropriate drawable folder by density, then writing the changes into the adaptive icon xml.

@liyamahendra
Copy link
Contributor Author

liyamahendra commented Jan 24, 2023

Thank you for the feedback @breautek !

I wonder if we can update the updateIcons function

I was aiming to solve the first use case as documented in the description of the PR. In this scenario, updateIcons returns because icons.length is Zero. I assume we need to provide some default assets, is that correct? Is providing a default monochrome icon done correctly?

I did read through prepare.js where we define a icLauncherTemplate with content of adaptive-icon. I'll add the monochrome statement to the template.

I'll add a note to my TODO to make sure we update the docs as required.

@liyamahendra
Copy link
Contributor Author

@breautek need your inputs please.

I'm updating the updateIcons function for working with custom icons provided in the project. The cordovaProject.projectConfig contains the monochrome attribute - see below:

cordovaProject.projectConfig:  {
    "path": "/Users/mahendraliya/Documents/dev/HelloWorld/config.xml",
    "doc": {
        "_root": {
            "_id": 0,
            "tag": "widget",
            "attrib": {
                "id": "io.cordova.hellocordova",
                "version": "1.0.0",
                "xmlns": "http://www.w3.org/ns/widgets",
                "xmlns:cdv": "http://cordova.apache.org/ns/1.0"
            },
            "text": "\n    ",
            "tail": null,
            "_children": [
                {
                    "_id": 1,
                    "tag": "name",
                    "attrib": {},
                    "text": "HelloCordova",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 2,
                    "tag": "description",
                    "attrib": {},
                    "text": "Sample Apache Cordova App",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 3,
                    "tag": "author",
                    "attrib": {
                        "email": "dev@cordova.apache.org",
                        "href": "https://cordova.apache.org"
                    },
                    "text": "\n        Apache Cordova Team\n    ",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 4,
                    "tag": "content",
                    "attrib": {
                        "src": "index.html"
                    },
                    "text": "",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 5,
                    "tag": "allow-intent",
                    "attrib": {
                        "href": "http://*/*"
                    },
                    "text": "",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 6,
                    "tag": "allow-intent",
                    "attrib": {
                        "href": "https://*/*"
                    },
                    "text": "",
                    "tail": "\n    ",
                    "_children": []
                },
                {
                    "_id": 7,
                    "tag": "platform",
                    "attrib": {
                        "name": "android"
                    },
                    "text": "\n        ",
                    "tail": "\n",
                    "_children": [
                        {
                            "_id": 8,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/ldpi-background.png",
                                "density": "ldpi",
                                "foreground": "resources/android/icon/ldpi-foreground.png",
                                "monochrome": "resources/android/icon/ldpi-monochrome.png",
                                "src": "resources/android/icon/ldpi.png"
                            },
                            "text": "",
                            "tail": "\n        ",
                            "_children": []
                        },
                        {
                            "_id": 9,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/mdpi-background.png",
                                "density": "mdpi",
                                "foreground": "resources/android/icon/mdpi-foreground.png",
                                "monochrome": "resources/android/icon/mdpi-monochrome.png",
                                "src": "resources/android/icon/mdpi.png"
                            },
                            "text": "",
                            "tail": "\n        ",
                            "_children": []
                        },
                        {
                            "_id": 10,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/hdpi-background.png",
                                "density": "hdpi",
                                "foreground": "resources/android/icon/hdpi-foreground.png",
                                "monochrome": "resources/android/icon/hdpi-monochrome.png",
                                "src": "resources/android/icon/hdpi.png"
                            },
                            "text": "",
                            "tail": "\n        ",
                            "_children": []
                        },
                        {
                            "_id": 11,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/xhdpi-background.png",
                                "density": "xhdpi",
                                "foreground": "resources/android/icon/xhdpi-foreground.png",
                                "monochrome": "resources/android/icon/xhdpi-monochrome.png",
                                "src": "resources/android/icon/xhdpi.png"
                            },
                            "text": "",
                            "tail": "\n        ",
                            "_children": []
                        },
                        {
                            "_id": 12,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/xxhdpi-background.png",
                                "density": "xxhdpi",
                                "foreground": "resources/android/icon/xxhdpi-foreground.png",
                                "monochrome": "resources/android/icon/xxhdpi-monochrome.png",
                                "src": "resources/android/icon/xxhdpi.png"
                            },
                            "text": "",
                            "tail": "\n        ",
                            "_children": []
                        },
                        {
                            "_id": 13,
                            "tag": "icon",
                            "attrib": {
                                "background": "resources/android/icon/xxxhdpi-background.png",
                                "density": "xxxhdpi",
                                "foreground": "resources/android/icon/xxxhdpi-foreground.png",
                                "monochrome": "resources/android/icon/xxxhdpi-monochrome.png",
                                "src": "resources/android/icon/xxxhdpi.png"
                            },
                            "text": "",
                            "tail": "\n    ",
                            "_children": []
                        }
                    ]
                }
            ]
        }
    },
    "cdvNamespacePrefix": "cdv"
}

but it isn't present in the icons returned from cordovaProject.projectConfig.getIcons function call - see below:

[
    {
        "platform": "android",
        "src": "resources/android/icon/ldpi.png",
        "density": "ldpi",
        "background": "resources/android/icon/ldpi-background.png",
        "foreground": "resources/android/icon/ldpi-foreground.png"
    },
    {
        "platform": "android",
        "src": "resources/android/icon/mdpi.png",
        "density": "mdpi",
        "background": "resources/android/icon/mdpi-background.png",
        "foreground": "resources/android/icon/mdpi-foreground.png"
    },
    {
        "platform": "android",
        "src": "resources/android/icon/hdpi.png",
        "density": "hdpi",
        "background": "resources/android/icon/hdpi-background.png",
        "foreground": "resources/android/icon/hdpi-foreground.png"
    },
    {
        "platform": "android",
        "src": "resources/android/icon/xhdpi.png",
        "density": "xhdpi",
        "background": "resources/android/icon/xhdpi-background.png",
        "foreground": "resources/android/icon/xhdpi-foreground.png"
    },
    {
        "platform": "android",
        "src": "resources/android/icon/xxhdpi.png",
        "density": "xxhdpi",
        "background": "resources/android/icon/xxhdpi-background.png",
        "foreground": "resources/android/icon/xxhdpi-foreground.png"
    },
    {
        "platform": "android",
        "src": "resources/android/icon/xxxhdpi.png",
        "density": "xxxhdpi",
        "background": "resources/android/icon/xxxhdpi-background.png",
        "foreground": "resources/android/icon/xxxhdpi-foreground.png"
    }
]

I tried to do grep search to see the definition of getIcons function but couldn't find it anywhere in the cordova-android project.

Would you advise where can I find the getIcons function please?

@liyamahendra
Copy link
Contributor Author

I was able to figure this out. This function calls

return this.getStaticResources(platform, 'icon');

FYI.

@liyamahendra
Copy link
Contributor Author

@breautek I think I've finished the implementation for support for monochrome icons. Kindly review the code. Apart from changes in this project, I had to do a small change in cordova-common, the details of which can be seen here.

So far, I've covered two use cases - (i) Cordova Starter project created with cordova create <ProjectName> and (ii) An existing project where the config.xml file has icons defined.

A sample Cordova project was created to test use case 2, which can be found here.

If everything looks good, we'll have to update the docs.

Please let me know your comments.

lib/prepare.js Outdated Show resolved Hide resolved
lib/prepare.js Outdated Show resolved Hide resolved
lib/prepare.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/prepare.js Outdated Show resolved Hide resolved
@liyamahendra
Copy link
Contributor Author

@breautek can you please advise the next steps? One aspect I know of is updating the docs. Any pointers regarding this would be helpful.

@erisu
Copy link
Member

erisu commented Jan 27, 2023

@liyamahendra The file that you would need to edit is here: https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/config_ref/images.md

This page contains the current documentation for Adaptive Icons.

@breautek
Copy link
Contributor

In addition to what Erisu said, please have a look at the unit tests, there are a few lint checks that are failing.

Error:   741:13  error  'monochromeVal' is never reassigned. Use 'const' instead  prefer-const
Error:   780:9   error  Expected space(s) after "if"                              keyword-spacing
Error:   793:34  error  Strings must use singlequote                              quotes
Error:   794:9   error  Expected space(s) after "if"                              keyword-spacing
Error:   849:9   error  Expected space(s) after "if"                              keyword-spacing

I think the overall test will fail until we have the cordova-common changes merged and released, but we should be able to correct the lint errors in the meantime.

These are all trivial errors that eslint should be able to fix automatically using npm run lint:fix.
The npm run lint command can also be ran to run the linter locally.

@erisu erisu changed the title Added the monochrome version for Cordova's icon feat: add monochrome app icon support Feb 1, 2023
@breautek
Copy link
Contributor

breautek commented Feb 1, 2023

I think the changes look good, at this point I think we just need to wait for a cordova-common@5 release for apache/cordova-common#197

I'll do a more proper review when all the dependent pieces are all in place.

@liyamahendra
Copy link
Contributor Author

@breautek have created a PR for updating the docs.

package.json Outdated
@@ -26,7 +26,7 @@
"license": "Apache-2.0",
"dependencies": {
"android-versions": "^1.7.0",
"cordova-common": "^4.0.2",
"cordova-common": "^4.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

@liyamahendra Can you revert this accidental change?

Also rebase with master after. I have released cordova-common@5.0.0, bumped the package, and merged into master.

#1566

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erisu please review it again, when you can. Thanks

Copy link
Member

@erisu erisu Mar 9, 2023

Choose a reason for hiding this comment

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

Some reason the rebase didn't look correct.

If it is OK, and if I can push, can I try and rebase your branch to fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Please feel free to push and rebase my branch as required. Thank you for asking!

Copy link
Member

Choose a reason for hiding this comment

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

@liyamahendra sorry for the delayed response.

I had reverted and rebased the branch correctly. It no longer shows changes that doesnt belong in this PR.

You will most likely need to force pull the changes since it was a rebase.

I will review the changes again.

Copy link
Member

@erisu erisu Mar 28, 2023

Choose a reason for hiding this comment

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

If you can also review the commit changes one time to confirm nothing was lost, that will be great.

So far everything looks great on my end.

There is one test failure,

✗ Test#010 : Should detech adaptive icon with vector foreground and throws error for missing backwards compatability settings. (0.013 sec)
- Expected function to throw CordovaError: For the following icons with the density of: mdpi, adaptive foreground with a defined color or vector can not be used as a standard fallback icon for older Android devices. To support older Android environments, please provide a value for the src attribute., but it threw Error: ENOENT: no such file or directory, open '/home/runner/work/cordova-android/cordova-android/platforms/android/app/src/main/res/mipmap-mdpi-v26/ic_launcher.xml'.

I believe it is related to this change:

https://github.com/apache/cordova-android/pull/1550/files#diff-c6eb45ef369fbd224eff9d2927942c4732c0e8ffe2779a74e05aee9f15b5bdb2L308-R314

It seems the .xml file extension was changed to .png while the test is related to vector foreground. I think this is an incorrect change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erisu you're right - changing the extension was causing the test to fail. Corrected that change.

Please have a look and let me know if there is anything else.

liyamahendra and others added 12 commits March 28, 2023 14:55
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Co-authored-by: エリス <erisu@users.noreply.github.com>
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Tested and I think it looks good!

Available for API 33 once you enable Themed Icons. Tested using an API 33 emulator.

To enable Themed Icons, go to

  1. Settings
  2. Wallpaper & style
  3. Toggle enable Themed icons

Screenshot from 2023-04-08 12-59-01

@breautek
Copy link
Contributor

breautek commented Apr 8, 2023

Final call for reviews before merging.

@breautek breautek merged commit 0160185 into apache:master Apr 9, 2023
6 checks passed
@breautek
Copy link
Contributor

breautek commented Apr 9, 2023

Thank you @liyamahendra for helping us provide the monochrome theme icon feature.

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

Successfully merging this pull request may close these issues.

Supporting themed app icons for Android 13
3 participants