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

Android: Refactor icon logic and make monochrome icon optional #99378

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

syntaxerror247
Copy link
Contributor

@syntaxerror247 syntaxerror247 commented Nov 18, 2024

Continuation of #97517

Monochrome icons are optional in Android, but our current implementation requires them to be included. This PR addresses that issue by disabling the monochrome icon if it is not provided in the export settings, giving developers greater flexibility and control.

Showcase video

First, I installed apk without providing monochrome icon. Second apk is with monochrome icon.

showcase_video.mp4

@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch 3 times, most recently from b6b13ec to 97b1c4d Compare November 18, 2024 02:56
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 18, 2024
@AThousandShips AThousandShips added cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed bug labels Nov 18, 2024
@AThousandShips AThousandShips modified the milestones: 4.4, 4.x Nov 18, 2024
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch from 97b1c4d to 69a1eac Compare November 19, 2024 15:51
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch from 69a1eac to c8aca35 Compare November 19, 2024 16:00
@syntaxerror247 syntaxerror247 marked this pull request as ready for review November 19, 2024 16:03
@syntaxerror247 syntaxerror247 requested a review from a team as a code owner November 19, 2024 16:03
@syntaxerror247 syntaxerror247 changed the title Android: Make monochrome icon optional Android: Refactor icon logic and make monochrome icon optional Nov 19, 2024
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

@syntaxerror247 Unfortunately the approach will not work either for the gradle build or for the legacy build.

In both scenarios, you have to keep in mind that the adaptive icon is only used for api 26 and above; for apis lower than 26 (our min is 21), the apk uses the png mipmap resources and with your proposed fix, the manifest declaration will no longer match those png resources.

Here's how I think we can resolve this issue..

For the gradle build, you'll need to update icon.xml to add / remove the monochrome tag based on whether the user specified a monochrome icon or not.

For the legacy build, themed_icon.xml will need to be in the apk as well, while the default remains icon.xml (which does not contain a monochrome tag):

  • If a monochrome icon is not specified, nothing to do
  • If a monochrome icon is specified, then themed_icon.xml data should be retrieved from the apk, and used to overwrite icon.xml data similarly to how we overwrite the png resource data in
    if (file.ends_with(".png") && file.contains("mipmap")) {

@syntaxerror247
Copy link
Contributor Author

In both scenarios, you have to keep in mind that the adaptive icon is only used for api 26 and above; for apis lower than 26 (our min is 21), the apk uses the png mipmap resources and with your proposed fix, the manifest declaration will no longer match those png resources.

Sorry I don't understand it, isn't our current implementation similar. I will admit I haven't tested with api 26 and older devices but I don't anticipate any issues.

android:icon="@mipmap/icon"

For the gradle build, you'll need to update icon.xml to add / remove the monochrome tag based on whether the user specified a monochrome icon or not.

This approach aligns with what I was previously doing before resorting to directly modifying the file in the Manifest. I’ll switch to using this method instead.

If a monochrome icon is specified, then themed_icon.xml data should be retrieved from the apk, and used to overwrite icon.xml data similarly to how we overwrite the png resource data in

This is a great idea, I can just overwrite it :D. I will update the PR to use this method.

@m4gr3d
Copy link
Contributor

m4gr3d commented Dec 2, 2024

In both scenarios, you have to keep in mind that the adaptive icon is only used for api 26 and above; for apis lower than 26 (our min is 21), the apk uses the png mipmap resources and with your proposed fix, the manifest declaration will no longer match those png resources.

Sorry I don't understand it, isn't our current implementation similar. I will admit I haven't tested with api 26 and older devices but I don't anticipate any issues.

In this version of the PR, the presence of a monochrome icon changes the android:icon declaration in both the gradle & legacy builds to android:icon="@mipmap/themed_icon".
The issue with that is that @mipmap/themed_icon does not match any of the png resources in the mipmap folders, and so on devices running api less than 26, the icon resource will not be resolvable.

@syntaxerror247
Copy link
Contributor Author

In both scenarios, you have to keep in mind that the adaptive icon is only used for api 26 and above; for apis lower than 26 (our min is 21), the apk uses the png mipmap resources and with your proposed fix, the manifest declaration will no longer match those png resources.

Sorry I don't understand it, isn't our current implementation similar. I will admit I haven't tested with api 26 and older devices but I don't anticipate any issues.

In this version of the PR, the presence of a monochrome icon changes the android:icon declaration in both the gradle & legacy builds to android:icon="@mipmap/themed_icon". The issue with that is that @mipmap/themed_icon does not match any of the png resources in the mipmap folders, and so on devices running api less than 26, the icon resource will not be resolvable.

Now I understand, it would be an issue if a user enables monochrome and then uses the app on an older device and due to absence of icon App will falback to default Android icon. Thanks for clarifying!

@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch from c8aca35 to 29f9e4d Compare December 2, 2024 06:26
@syntaxerror247 syntaxerror247 marked this pull request as draft December 2, 2024 06:27
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch 2 times, most recently from 94c107d to 5c7ad28 Compare December 2, 2024 07:48
@syntaxerror247 syntaxerror247 marked this pull request as ready for review December 2, 2024 07:58
@syntaxerror247 syntaxerror247 requested a review from m4gr3d December 2, 2024 07:58
@syntaxerror247
Copy link
Contributor Author

syntaxerror247 commented Dec 2, 2024

Completely revamped the PR to address feedback and resolve issues with API 26 below devices. Also included a showcase video in the PR description.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great! I've added some comments to polish the PR, then it should be good to go!

Btw, can you take a look at #98622 and check that it would not interfere with this fix.

platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch 3 times, most recently from 185be86 to 18885d0 Compare December 2, 2024 15:37
platform/android/java/lib/res/mipmap-anydpi-v26/icon.xml Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch 3 times, most recently from 0d30f3b to 59d970c Compare December 2, 2024 16:08
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks great!
Added a couple nitpick comments.

platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@syntaxerror247 syntaxerror247 force-pushed the optional_monochrome_icon branch from 59d970c to f767cf0 Compare December 2, 2024 16:34
@m4gr3d m4gr3d modified the milestones: 4.x, 4.4 Dec 2, 2024
@Repiteo Repiteo merged commit 6e8c0a4 into godotengine:master Dec 3, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 3, 2024

Thanks!

@syntaxerror247 syntaxerror247 deleted the optional_monochrome_icon branch December 4, 2024 00:52
@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants