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

Use enums for requesting permissions #100330

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j20001970
Copy link
Contributor

Use enumeration for requesting permission instead of strings.

This is breaking changes, as the method signatures are different.

The default path of requestPermission(String permissionName, Activity activity) in PermissionUtil.java for Android will become unreachable.

doc/classes/OS.xml Outdated Show resolved Hide resolved
@j20001970 j20001970 force-pushed the os-permission-enum branch 2 times, most recently from a3524f6 to 177e474 Compare December 12, 2024 17:46
doc/classes/OS.xml Outdated Show resolved Hide resolved
@j20001970 j20001970 force-pushed the os-permission-enum branch 2 times, most recently from 127d4ec to 06112a7 Compare December 12, 2024 18:59
@j20001970 j20001970 requested a review from a team as a code owner December 12, 2024 18:59
<description>
On Android devices: Returns the list of dangerous permissions that have been granted.
On macOS: Returns the list of user selected folders accessible to the application (sandboxed applications only). Use the native file dialog to request folder access permission.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning accessible folders from a permission related method on macOS doesn't feel quite match up, so I removed it. Is such functionality still relevant?

Copy link
Member

Choose a reason for hiding this comment

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

It is related to permissions, it is a list of folders access permissions were granted for. It's definitely should not be removed, since it's the only way to know if you still have permission to access the folder (or need to ask the user to grant it again).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having different behaviours on the same method feels quite strange...

Will moving it to other class like DirAccess help?

@bruvzg
Copy link
Member

bruvzg commented Dec 12, 2024

In case of Android, it seems like you can use this functions to requires any arbitrary permission, not just one from the list, so change it to enum is likely completely wrong.

@j20001970
Copy link
Contributor Author

In case of Android, it seems like you can use this functions to requires any arbitrary permission, not just one from the list, so change it to enum is likely completely wrong.

The change was made with bringing support to other OSes in future in mind.

It is indeed that the capability of requesting arbitrary permission on Android will be lost, but it has limited use if Godot cannot access what it request permissions for without a plugin (bluetooth, location comes to the mind in the case of Android), and said plugin should already have ways to request permissions for what it is trying to access, without relying on Godot code in the first place.

Of course there might be other ways to achieve cross-platform permission requests without breaking the ability of doing platform-specific (like string constants from what I can think of, but haven't found other classes doing that).

@j20001970 j20001970 marked this pull request as draft December 13, 2024 03:11
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
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.

3 participants