Skip to content

Update OS.request_permission (and related) descriptions to reflect reality. #105364

Open
The-Cyber-Captain wants to merge 2 commits intogodotengine:masterfrom
The-Cyber-Captain:patch-1
Open

Update OS.request_permission (and related) descriptions to reflect reality. #105364
The-Cyber-Captain wants to merge 2 commits intogodotengine:masterfrom
The-Cyber-Captain:patch-1

Conversation

@The-Cyber-Captain
Copy link
Copy Markdown

From a peek through the code, probably just some Legacy goop from the ever-evolving challenges Android likes to throw us. :-D

Contrary to what its previous description claimed, functionally, request_permission returns true not just when the permission has been granted. Pass it some junk, a 'normal' permission (even one without an entry in AndroidManifest!), or a typo, and - as request_permission is in essence actually a "!dispatched to Android" method - it also returns true! Quite misleading.

... fine for the plural method, request_permissions, because there's no room for bad parameters to get passed in there. Though this, in turn, suggests the original note about checking 'exports' for that method could use some elaboration for what that does; now offered.

Finally, it was originally unclear when, exactly, the on_request_permissions_result signal / signals might fire where the plural method initiated the requests; per permission, or all at once? It's all at once - phew! - but with the caveat discovered, however, that any singular request_permission made whilst the OS dialogue is open is apparently just cast aside... even though our method returns the false value suggesting it was dispatched. :-/ Popped a warning on the request_permission description not to do something daft like that, then.

@The-Cyber-Captain
Copy link
Copy Markdown
Author

Hmmmmm. Not sure if I'm reading this right?.... and not had to update any class refs in the doc before, so might be something I missed on my end.

codespell................................................................Failed
- hook id: codespell
- files were modified by this hook

FIXED: doc/classes/OS.xml

But has 'style checks via pre-commit' attempted to automagically swap "dialogue" for "dialog" (fair call, I guess), and then somehow errored out and failed after the fix anyway? :-/

Anything I can - or need - to do my end to proceed?

@Mickeon Mickeon requested a review from bruvzg April 14, 2025 22:50
@The-Cyber-Captain
Copy link
Copy Markdown
Author

The-Cyber-Captain commented Apr 22, 2025

Okay, I tried activating the cloned Actions workflow on my fork to pre-check things (like spelling prefs) before PR... but that didn't seem to give much / any joy. As I'd assumed previously, that workflow appears to want to go build the engine across multiple platforms, etc. (And fails in my github env for sure on multiple fronts and platforms I don't understand, or want to understand. LOL.)

So, despite the prior error hinting the official Godot repo Actions were going to automatically swap out "dialogue" for "dialog" here, I instead took a punt, did it by hand and the magical background checks now all pass. (y)

Been doing all this via web - as I didn't come at things as a Dev task initially, just a docs update / housekeeping [Being also, typically, off-grid or at best in VR. Yay for needless complexity on my part! //facepalm]. And AFAIK, there's no way to rebase or amend from the github UI itself. Boooo! (But, also, Yay... fair call in most cases).

Currently have the opportunity today to fetch my fork local and attempt a rebase on the commits now, which - fingers crossed - will trickle through here and give a nice clean PR to approve? Stay tuned.

Fix request_permission and request_permissions descriptions.

Update MainLoop.xml

Elaborate upon on_request_permissions_result behaviour under OS.request_permissions().
@Calinou
Copy link
Copy Markdown
Member

Calinou commented Apr 24, 2025

You need to fix indentation in OS.xml by replacing spaces with tabs on the line you added.

@The-Cyber-Captain
Copy link
Copy Markdown
Author

Bah! An artefact of trying to fix today's new merge conflict in-browser again, I suspect. What are the chances, eh? FML. :-D
... working on it. Ta.

Update OS.xml
Fix request_permission and request_permissions descriptions.
Update MainLoop.xml
Elaborate upon on_request_permissions_result behaviour under OS.request_permissions().
Copy link
Copy Markdown
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.

Thanks for the attempt at cleaning up the documentation, although this highlights to me that we need to clean up the API in order to provide more info to callers as to what actions the system is taking when the permissions is being requested.

We may need to deprecate the current permissions APIs and replace them with properly designed new ones.

cc @syntaxerror247 @Alex2782

@The-Cyber-Captain
Copy link
Copy Markdown
Author

The-Cyber-Captain commented Apr 28, 2025

Thanks @m4gr3d. No worries on the former; having spent some time scratching my head and looking into it, just looking to save anyone else - in the engine userbase - the same grief and / or time. Which, hopefully the PR would achieve?

On the latter? Indeed-indeed! I'm in the back of my mind still contemplating a hack plugin for my project to access Manifest via android.content.pm.PackageManager or similar, in order to align with the more modern Android permissions approach, and - very much more so - meet the recommended, elegant, user-experience of asking for any missing permissions in context, as required.*

Very much not my current wheelhouse, though. LOL. Another 'sidequest' of learning a new skillset to distract or derail my actual progress. O:-) (And all, absolutely, as a stop-gap for any improved API arriving to swap across to)

Also pretty clear from the merge conflict which cropped up that we've got other parties actively updating the OS permissions API for aspects of Mac use, too. I don't know much about the inner workings, or legacy, of the current code... and some of it is mighty old! But getting Deprecation and a reworked API on the propositions board sounds like a grand idea to me!

*Again, my personal primary focus being XR - cross-platform with a preference / enhanced features for standalone devices - the user-experience is extra important. But I recognise you (Fredia) from discussions and godot_openxr_vendors commits... so I'm sure you know exactly what I mean and where I'm coming from! "VR". I love it, but it's such a nuisance. :-D

@The-Cyber-Captain
Copy link
Copy Markdown
Author

PS: Yep, these are my regular feature-set permission-faves!
com.oculus.permission.USE_ANCHOR_API
com.oculus.permission.IMPORT_EXPORT_IOT_MAP_DATA
com.oculus.permission.USE_SCENE

android.permission.ACCESS_COARSE_LOCATION
android.permission.ACCESS_FINE_LOCATION
android.permission.INTERNET
com.oculus.permission.BODY_TRACKING

But I'm well-aware Camera Access etc will join high on the general permissions wish-list soon. Would be mighty fine not to have users coming to either XR or Godot fresh (to do XR) scared away by the OS permissions API.

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.

4 participants