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.

Sign up for GitHub

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

Jump to bottom

Presentation Activity in Wallet #459

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Presentation Activity in Wallet #459

merged 1 commit into from
Feb 7, 2024

Conversation

suzannajiwani
Copy link
Contributor

Adding PresentationActivity to wallet, which will appear on-screen with related presentation UI once the NfcEngagementHandler starts the activity. This addition allows for in-person presentation with NFC engagement.

Tested manually with 0/1/2 active credentials in wallet.

@suzannajiwani suzannajiwani force-pushed the wallet-nfc-presentation branch 2 times, most recently from a414116 to a0496fd Compare February 5, 2024 22:00
davidz25
davidz25 requested changes Feb 6, 2024
View reviewed changes
wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt Outdated Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt Outdated Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/NfcEngagementHandler.kt Outdated Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/MainActivity.kt Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/MainActivity.kt Outdated Show resolved Hide resolved
wallet/src/main/java/com/android/identity_credential/wallet/NfcEngagementHandler.kt Outdated Show resolved Hide resolved
@suzannajiwani suzannajiwani force-pushed the wallet-nfc-presentation branch 2 times, most recently from 4a1cf1c to 836e437 Compare February 6, 2024 21:31
davidz25
davidz25 reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@davidz25 davidz25 left a comment

Choose a reason for hiding this comment

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

Looks good for me except nits that are easily addressed. I'm not sure about the PermissionTracker changes so added @sorotokin to review that and I'll let him approve the PR.

wallet/src/main/java/com/android/identity_credential/wallet/MainActivity.kt Outdated

private val permissionTracker = PermissionTracker(this, mapOf(
Manifest.permission.CAMERA to "This application requires camera permission to scan",
Manifest.permission.NFC to "NFC permission is required to operate"
))

private val blePermissionTracker: PermissionTracker = if (Build.VERSION.SDK_INT >= 31) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call it inPersonPresentationPermissionTracker since we'd also include Wifi Aware permissions here in the (unlikely) case we'd want to support Wifi Aware as well as BLE.

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

sharedPreferences = PreferenceManager.getDefaultSharedPreferences(applicationContext)

setContent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use primitives from commonUI.kt which @sorotokin landed yesterday.

wallet/src/main/java/com/android/identity_credential/wallet/PermissionTracker.kt
Text(text = reasoningTxt, modifier = Modifier.fillMaxWidth())
Button(onClick = {requestPermission(permission)}) {
Text("I understand")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @sorotokin to review the changes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why

} else {
    SinglePermissionRequest(permission = permissionsToRequest[0])
}

is needed? It should give you a list of all the missing permission with a button to request each of them. Is it not working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, when I used it all the composables in the loop were being rendered at once, so all three ble msgs were on top of each other

@davidz25 davidz25 requested a review from sorotokin February 7, 2024 16:52
@suzannajiwani suzannajiwani force-pushed the wallet-nfc-presentation branch from 836e437 to 2fc6a36 Compare February 7, 2024 17:58
sorotokin
sorotokin reviewed Feb 7, 2024
View reviewed changes
wallet/src/main/java/com/android/identity_credential/wallet/PermissionTracker.kt
Text(text = reasoningTxt, modifier = Modifier.fillMaxWidth())
Button(onClick = {requestPermission(permission)}) {
Text("I understand")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why

} else {
    SinglePermissionRequest(permission = permissionsToRequest[0])
}

is needed? It should give you a list of all the missing permission with a button to request each of them. Is it not working?

wallet/src/main/java/com/android/identity_credential/wallet/MainActivity.kt Outdated
@@ -108,50 +125,78 @@ class MainActivity : ComponentActivity() {

private val provisioningViewModel: ProvisioningViewModel by viewModels()
private val credentialInformationViewModel: CredentialInformationViewModel by viewModels()
private lateinit var sharedPreferences: SharedPreferences

private val permissionTracker = PermissionTracker(this, mapOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just create a single permission tracker for all permissions. Map may require a builder function.

Copy link
Contributor Author

@suzannajiwani suzannajiwani Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see what you're saying, will do

wallet/src/main/java/com/android/identity_credential/wallet/MainActivity.kt Outdated

permissionTracker.updatePermissions()
inPersonPresentationPermissionTracker.updatePermissions()
val requiredPermissions: List<String> = if (Build.VERSION.SDK_INT >= 31) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be more descriptive name than just requiredPermissions (at least if declared on top level rather than locally next to its use). This set is for a particular screen or workflow, right?

@suzannajiwani
Presentation Activity
41003f8 
Adding PresentationActivity to wallet, which will appear on-screen
with related presentation UI once the NfcEngagementHandler starts the
activity. This addition allows for in-person presentation with NFC
engagement.

Tested manually with 0/1/2 active credentials in wallet.

Signed-off-by: Suzanna Jiwani <[email protected]>
@suzannajiwani suzannajiwani force-pushed the wallet-nfc-presentation branch from 2fc6a36 to 41003f8 Compare February 7, 2024 19:35
@davidz25 davidz25 merged commit 4bf50bf into openwallet-foundation-labs:main Feb 7, 2024
2 checks passed
@suzannajiwani suzannajiwani deleted the wallet-nfc-presentation branch February 8, 2024 18:55
sorotokin pushed a commit that referenced this pull request Feb 14, 2024
@suzannajiwani @sorotokin
Presentation Activity (#459)
406e896 
Adding PresentationActivity to wallet, which will appear on-screen
with related presentation UI once the NfcEngagementHandler starts the
activity. This addition allows for in-person presentation with NFC
engagement.

Tested manually with 0/1/2 active credentials in wallet.

Signed-off-by: Suzanna Jiwani <[email protected]>
Signed-off-by: Peter Sorotokin <[email protected]>
sorotokin pushed a commit that referenced this pull request Feb 16, 2024
@suzannajiwani @sorotokin
Presentation Activity (#459)
e06d834 
Adding PresentationActivity to wallet, which will appear on-screen
with related presentation UI once the NfcEngagementHandler starts the
activity. This addition allows for in-person presentation with NFC
engagement.

Tested manually with 0/1/2 active credentials in wallet.

Signed-off-by: Suzanna Jiwani <[email protected]>
Signed-off-by: Peter Sorotokin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorotokin sorotokin sorotokin left review comments

@davidz25 davidz25 davidz25 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@suzannajiwani @davidz25 @sorotokin