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

Support Activity Result API #448

Open
bubelov opened this issue Apr 17, 2022 · 3 comments
Open

Support Activity Result API #448

bubelov opened this issue Apr 17, 2022 · 3 comments

Comments

@bubelov
Copy link

bubelov commented Apr 17, 2022

Here is how I integrate NC SSO:

override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) {
    super.onActivityResult(requestCode, resultCode, data)

    when (resultCode) {
        AppCompatActivity.RESULT_CANCELED -> setButtonsEnabled(true)

        else -> {
            AccountImporter.onActivityResult(
                requestCode,
                resultCode,
                data,
                this,
            ) { onNextcloudAccountAccessGranted(it) }
        }
    }
}

It works fine, except for showing deprecation warnings:

'onActivityResult(Int, Int, Intent?): Unit' is deprecated. Deprecated in Java

This method has been deprecated in favor of using the Activity Result API which brings increased type safety via an ActivityResultContract and the prebuilt contracts for common intents available in androidx.activity.result.contract.ActivityResultContracts, provides hooks for testing, and allow receiving results in separate, testable classes independent from your fragment. Use registerForActivityResult(ActivityResultContract, ActivityResultCallback) with the appropriate ActivityResultContract and handling the result in the callback.

It looks like the modern way to get a SingleSignOnAccount is to create an ActivityResultContract<Nothing, SingleSignOnAccount> or something like that and to ship it with the library. Are there any reasons for not supporting Activity Result API except for the lack of dev time?

@stefan-niedermann
Copy link
Member

Are there any reasons for not supporting Activity Result API except for the lack of dev time?

In short: Nope. Maybe you want to start a Pull Request? I will happily review it 🙂

@bubelov
Copy link
Author

bubelov commented Apr 17, 2022

@stefan-niedermann I don't have a NC instance at the moment, so it would be hard to test the code. Do you know any reliable hosted services? I tried a couple from the official website, one of them was freezing for hours and another one wasn't able to send me an email confirmation for some reason =)

@stefan-niedermann
Copy link
Member

Do you know any reliable hosted services?

For testing around just use one of those free hosters.

So, I played a bit around with the idea of ActivityResultContracts. I think, we need three different ActivityResultContracts:

  1. CHOOSE_ACCOUNT_SSOChooseAccountContract
  2. REQUEST_AUTH_TOKEN_SSORequestAuthTokenContract
  3. REQUEST_GET_ACCOUNTS_PERMISSIONRequestGetAccountsPermissionContract

Further thoughts:

  1. Do 3rd party apps really need to control the import aspect this detailed? Wouldn't it be better to provide just one ImportSingleSignOnAccountContract which handles the rest of the stuff under the hood?
  2. While implementing ActivityResultContract I stumbled upon the problem, that one needs to override createIntent and parseResult - both methods do not throw any exception according their signature. This leads to the problem, that we can't check and throw NextcloudFilesAppNotInstalledException for example without wrapping it into a RuntimeException or extending SSOException RuntimeException - I don't like any of those approaches. Another idea is to do the check within a constructor, but this again leads to an ugly static initialization from the 3rd party apps perspective:
private final ActivityResultLauncher<Void> launcher;

{
  try {
      launcher = registerForActivityResult(
              new ImportAccountContract(this),
              (ActivityResultCallback<SingleSignOnAccount>) this::onActivityResult
      );
  } catch (NextcloudFilesAppNotInstalledException e) {
      throw new RuntimeException(e);
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants