cordova-android

cordova-android copied to clipboard

onRequestPermissionResult is marked as deprecated in favor of onRequestPermissionsResult: https://github.com/apache/cordova-android/blob/a1ed1c0af7c6267f47e580e8850295202692f4ea/framework/src/org/apache/cordova/CordovaPlugin.java#L418-L424

However, CordovaInterfaceImpl calls the deprecated method instead of the new one: https://github.com/apache/cordova-android/blob/a1ed1c0af7c6267f47e580e8850295202692f4ea/framework/src/org/apache/cordova/CordovaInterfaceImpl.java#L218-L224

Maybe should CordovaPlugin's default implementation of onRequestPermissionResult call the new onRequestPermissionsResult method until onRequestPermissionResult is removed?

They are both called by the PermissionsHelper: https://github.com/apache/cordova-android/blob/a1ed1c0af7c6267f47e580e8850295202692f4ea/framework/src/org/apache/cordova/PermissionHelper.java#L76-L88

We can't remove onRequestPermissionResult until the next major version because it will break existing plugins that haven't been updated to use onRequestPermissionsResult.

Where does callback.first.onRequestPermissionResult in CordovaInterfaceImpl call to?

CordovaInterfaceImpl is directly calling the deprecated method onRequestPermissionResult but the deprecation annotation is pushing developers to override the new method that is never executed.

Also, does it make sense for the PermissionHelper to execute both the deprecated and the new methods? That seems like a source of bugs lurking around for plugins as it isn't explicit that both will be executed and mainly because PermissionHelper is to provide back compatibility with potentially legacy code.

@dpogue

They are both called by the PermissionsHelper:

This isn't really correct though, this method is private and never called by anything that I can see.

There's essentially no way to have onRequestPermissionResults be called.

@gwynjudd I think the deliverPermissionResult method is deprecated and can be removed.

If I remember correctly, this helper class originated from Apache's cordova-plugin-compat plugin. That plugin is deprecated long ago. Its version of the helper class had and used the deliverPermissionResult method.

I believe the class was copied to this repo to make it available for other plugins and was a part of Cordova-Android 5 backward compatibility support. https://github.com/apache/cordova-android/issues/594

Removing onRequestPermissionResult may break existing plugins.

Even though Apache has added the deprecated flag and updated Apache core plugins to not use the method, many unmaintained third-party plugins works with the current version of Cordova-Android and could be calling the deprecated method. If we remove the method, then all those plugins will stop working.

There might have been some agreement that this step has to be taken either way, but when?

Hello,

It is this method in PermissionsHelper which is private

https://github.com/apache/cordova-android/blob/c9e7c5998635ad86412c5e8ef90df357a1676dcf/framework/src/org/apache/cordova/PermissionHelper.java#L76

As far as I can tell, it is the only method which ever calls CordovaPlugin#onRequestPermissionResult

From: Thibault @.> Sent: Thursday, 3 August 2023 8:49 pm To: apache/cordova-android @.> Cc: Gwyn Judd @.>; Mention @.> Subject: Re: [apache/cordova-android] onRequestPermissionResult deprecation issue (Issue #1388)

Hi @gwynjuddhttps://github.com/gwynjudd,

Small question, how this method is private? https://github.com/apache/cordova-android/blob/c9e7c5998635ad86412c5e8ef90df357a1676dcf/framework/src/org/apache/cordova/CordovaPlugin.java#L413-L426

By my understanding, both method works at the end?

Kr.

Reply to this email directly, view it on GitHubhttps://github.com/apache/cordova-android/issues/1388#issuecomment-1663558024, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AACMWTFT62NMSJAUXU2LNSDXTNQYZANCNFSM5LKYYHOQ. You are receiving this because you were mentioned.Message ID: @.@.>>

I've just spent a day adding POST_NOTIFICATIONS permission to a plugin that didn't previously need permissions, and have run across this issue.

If you override onRequestPermissionsResult (the new method) in your plugin, it does not work. That method is not called when the user picks an option on the permissions dialog (or swipes to dismiss).

To actually get called back, you have to override the deprecated method onRequestPermissionResult.

The documentation correctly tells you to use the old deprecated method name. I was surprised to find that Android Studio struck through the method name indicating that it was deprecated. I changed it to the new name, then wasted a lot of time wondering why it didn't work until I realised that the callback wasn't even called.

In my project, nothing calls PermissionsHelper.deliverPermissionResult. Indeed, since it is a private method in PermissionHelper, the only way it could be called is through reflection. It seems highly unlikely that anyone would do that, since you can simply call onRequestPermissionsResult yourself directly. The purpose of this code was to call the callback on Android versions that didn't support the requestPermissions API, but since the minimum supported Android version is now 7.0 (SDK 24), the requestPermission/s methods no longer need to implement a workaround.

Suggested fixes:

1. Revert #1047 and live with the inconsistency, OR
2. Add a call to callback.first.onRequestPermissionsResult to CordovaInterfaceImpl.onRequestPermissionResult, after the call to callback.first.onRequestPermissionResult. Update the documentation to the new method.

I also suggest that PermissionsHelper.deliverPermissionResult is removed.

As an alternative, we could make CordovaPlugin.onRequestPermissionsResult (the new method's default implementation) call CordovaPlugin.onRequestPermissionResult (the old method, which may be overridden in plugins). Then change CordovaInterfaceImpl to only call callback.first.onRequestPermissionsResult (i.e. the new method).

// CordovaPlugin.java

    /**
     * Called by the system when the user grants permissions
     *
     * @param requestCode
     * @param permissions
     * @param grantResults
     * 
     * @deprecated Use {@link #onRequestPermissionsResult} instead.
     */
    @Deprecated
    public void onRequestPermissionResult(int requestCode, String[] permissions,
                                          int[] grantResults) throws JSONException {

    }

    /**
     * Called by the system when the user grants permissions
     *
     * @param requestCode
     * @param permissions
     * @param grantResults
     */
    public void onRequestPermissionsResult(int requestCode, String[] permissions,
                                          int[] grantResults) throws JSONException {
        // Call the old method name for backward compatibility
        this.onRequestPermissionResult(requestCode, permissions, grantResults);
    }

/// CordovaInterfaceImpl.java

    /**
     * Called by the system when the user grants permissions
     *
     * @param requestCode
     * @param permissions
     * @param grantResults
     */
    public void onRequestPermissionResult(int requestCode, String[] permissions,
                                          int[] grantResults) throws JSONException {
        Pair<CordovaPlugin, Integer> callback = permissionResultCallbacks.getAndRemoveCallback(requestCode);
        if(callback != null) {
            callback.first.onRequestPermissionsResult(callback.second, permissions, grantResults);
        }
    }

It may be worth renaming the method in CordovaInterface (therefore also CordovaInterfaceImpl) for consistency.