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

Request Android record permission when needed #26756

Merged
merged 1 commit into from Mar 8, 2019

Conversation

marcelofg55
Copy link
Contributor

Fixes #26506

@marcelofg55 marcelofg55 requested a review from reduz as a code owner March 7, 2019 15:14
@akien-mga akien-mga modified the milestones: 3.1, 3.2 Mar 7, 2019
@marcelofg55 marcelofg55 force-pushed the android_rec_perm branch 2 times, most recently from 571629b to 281310e Compare March 8, 2019 13:09
@akien-mga akien-mga changed the title [WIP] Request Android record permission when needed Request Android record permission when needed Mar 8, 2019
@akien-mga akien-mga modified the milestones: 3.2, 3.1 Mar 8, 2019
@akien-mga
Copy link
Member

Works perfectly on my phones (Xiaomi Pocophone F1 with Android 9 and Samsung Galaxy S3 with Android 7.1.2) with the Mic Record demo.

Starting the app triggers this permission request as expected:
screenshot_2019-03-08-15-16-06-064_com google android packageinstaller

Then the recording feature works. Subsequent runs don't re-require the permission.

@marcelofg55
Copy link
Contributor Author

Awesome, thanks for the testing!

@@ -937,6 +945,18 @@ public void setPaymentsManager(PaymentsManager mPaymentsManager) {
*/

// Audio
Copy link
Member

Choose a reason for hiding this comment

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

By chance this comment is correct now, but I guess in the future we might want to add support for more permissions in the same method, so maybe it should be removed/updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it

@@ -937,6 +945,18 @@ public void setPaymentsManager(PaymentsManager mPaymentsManager) {
*/

// Audio
public boolean requestPermission(String p_name) {
if (p_name.equals("RECORD_AUDIO")) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above comment, will this check also be relevant for other types of permissions? If so, it should probably be done the other way around?

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
	if (p_name.equals("RECORD_AUDIO")) {

or maybe even:

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
	 // Not necessary, asked on install already
	return true;
}

if (p_name.equals("RECORD_AUDIO")) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound good, I'll change it

@@ -1265,6 +1270,8 @@ void _OS::_bind_methods() {
ClassDB::bind_method(D_METHOD("get_power_seconds_left"), &_OS::get_power_seconds_left);
ClassDB::bind_method(D_METHOD("get_power_percent_left"), &_OS::get_power_percent_left);

ClassDB::bind_method(D_METHOD("request_permission", "name"), &_OS::request_permission);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add documentation for this method too since it's bound? Currently users might try to request all kinds of permissions using it, to find out that it doesn't work as they think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright :D

@akien-mga
Copy link
Member

@BastiaanOlij Should be relevant for your work :)

@akien-mga akien-mga merged commit 60d910b into godotengine:master Mar 8, 2019
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij
Copy link
Contributor

Beautiful! I'm going to redo the permissions logic for ARCore so it requests the camera permissions through this. I'll probably do a separate PR for just the camera permissions once I get my head around it.

@BastiaanOlij
Copy link
Contributor

@marcelofg55, I'm trying to duplicate this for the camera, if i understand the changes correctly, you now call request permissions from the audio driver.
If permissions were previously given this returns true and the audio driver initialises.
If no permissions where previously given the dialog opens but since code continues we return false.
When the user accepts the permissions we get a signal from the OS which is handled in onRequestPermissionsResult but what I'm missing is how you now trigger the audio driver to initialize now that it can?

@marcelofg55
Copy link
Contributor Author

@BastiaanOlij check this: https://github.com/godotengine/godot/blob/master/platform/android/java_glue.cpp#L1586
Basically onRequestPermissionsResult calls that function on java_glue.cpp and capture_start is called to initialize the audio input.

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.

None yet

3 participants