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

Introduce appCategory attribute of android to set category #63483

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Introduce appCategory attribute of android to set category #63483

merged 1 commit into from
Jan 3, 2023

Conversation

qianjunakasumi
Copy link
Contributor

According to android documentation (https://developer.android.com/guide/topics/manifest/application-element#isGame), android:isGame is deprecated.
To accommodate this change, use the appCategory attribute set to game.

: )

Considering that godot may also run on a lower version of android (because of minSDK), so the isGame attribute not delete.

Personally think: the change is compatible with 3.x.

@akien-mga
Copy link
Member

Exposing appCategory makes sense, but it needs more work to be done properly. Not all Godot projects are game so like isGame, it needs to be configurable in the export preset. See #50028 for how isGame support was implemented.

There are two options, either we keep the current classify_as_game preset option and use it to set both isGame and appCategory=game, or we add a new app_category preset option with the full enum from https://developer.android.com/guide/topics/manifest/application-element#appCategory so users can select any value.

@m4gr3d
Copy link
Contributor

m4gr3d commented Jul 26, 2022

There are two options, either we keep the current classify_as_game preset option and use it to set both isGame and appCategory=game, or we add a new app_category preset option with the full enum from https://developer.android.com/guide/topics/manifest/application-element#appCategory so users can select any value.

We should go with the new app_category preset. That would break compatibility so we can either only use that option for the master branch, or go forward with it for the 3.6 release.

@qianjunakasumi qianjunakasumi marked this pull request as draft July 26, 2022 15:25
@fire-forge
Copy link
Contributor

fire-forge commented Jul 26, 2022

What I think should be done is replacing the classify_as_game export option with an enum for setting appCategory with all of the available enum values, and setting isGame to true or false depending on if the category setting is "game, for backwards compatibility with older versions of Android. The default value can be "game" since most projects will be games, but then there's more flexibility for anyone who is making something other than a game.

@qianjunakasumi
Copy link
Contributor Author

Thank you for your suggestions. Now I have written most of the code and it looks good in the compiled editor. I'm testing its export product .apk and will push it for review as soon as I feel the time is right.

image

@qianjunakasumi
Copy link
Contributor Author

qianjunakasumi commented Aug 5, 2022

When trying the export type Accessibility, I found that android:appCategory cannot display the corresponding string android:appCategory: accessibility normally (android:appCategory: 8).

I have made some efforts but cannot change this problem, so I submit it and hope to get help.
Any help is appreciated!

@qianjunakasumi
Copy link
Contributor Author

After a moment of searching, there doesn't seem to be any solution to the previous problem.
To make use of this enhancement as soon as possible, I took the liberty of removing accessibility appCategory, as it doesn't seem to fit the target category of the godot engine well.

Now it works fine and could merges into master
If you have a good idea, I'd appreciate it!

@qianjunakasumi qianjunakasumi marked this pull request as ready for review September 9, 2022 04:13
@m4gr3d
Copy link
Contributor

m4gr3d commented Dec 23, 2022

When trying the export type Accessibility, I found that android:appCategory cannot display the corresponding string android:appCategory: accessibility normally (android:appCategory: 8).

I have made some efforts but cannot change this problem, so I submit it and hope to get help. Any help is appreciated!

Shouldn't the value for the accessibility category be 0 instead of 8 based on the ordering in the documentation?
Can you show the error you're running into when setting accessibility as the app category.

@qianjunakasumi
Copy link
Contributor Author

Shouldn't the value for the accessibility category be 0 instead of 8 based on the ordering in the documentation?

Actually, it's 0 in the document. However, when getting the corresponding category according to the index, founding that the serial numbers were in a jumbled order. I have no idea how it actually work, so I wrote a function that maps a fake index to a real one

cut

It is very strange that no matter how I order them, there is always an index corresponding to a category that does not display properly, guessing maybe the enumeration can only hold 8 objects? It looks good when there are only 8 objects.

@m4gr3d
Copy link
Contributor

m4gr3d commented Dec 27, 2022

I tested with 'Accessibility' using the value 8 and it seems to work as expected.
The compiled manifest doesn't show the string, but shows the value assigned to them (which can be validated using the custom build).

So it should be safe to reintroduce the Accessibility category, and assign it with a value of 8.

@qianjunakasumi
Copy link
Contributor Author

The compiled manifest doesn't show the string, but shows the value assigned to them (which can be validated using the custom build).

Like this, it's not the same as the other categories (android:appCategory="game") and maybe it's a bug ?🤔, I have no idea on how to fix it.

@m4gr3d
Copy link
Contributor

m4gr3d commented Dec 31, 2022

@qianjunakasumi Can you squash the commits.

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.

The changes look good, and the logic works as expected (based on my own testing) so this PR is good to go.

As mentioned, once you squash the commits, this should be ready to merge.

@qianjunakasumi qianjunakasumi changed the title Use appCategory attribute of android set to game Introduce appCategory attribute of android to set category Jan 1, 2023
@qianjunakasumi
Copy link
Contributor Author

@qianjunakasumi Can you squash the commits.

sure, ptal :)

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 3, 2023
@akien-mga akien-mga merged commit 6dc9629 into godotengine:master Jan 3, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement platform:android topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants