@W-19368426: [MSDK] Rewrite Android Screen Lock (a.k.a Passcode) for RTL UI #2835
Conversation
…RTL UI Rename .java to .kt
…RTL UI (Migrate To Activity Result API)
…RTL UI (Resolve Accessibility Event Deprecation)
…RTL UI (Resolve 'onBackPressed' Deprecation)
Generated by 🚫 Danger |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -22,6 +22,7 @@ dependencies { | |||
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | |||
There was a problem hiding this comment.
| A newer version of androidx.browser:browser than 1.8.0 is available: 1.9.0 |
| @@ -22,6 +22,7 @@ dependencies { | |||
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | |||
| api("androidx.work:work-runtime-ktx:2.10.3") | |||
There was a problem hiding this comment.
| A newer version of androidx.work:work-runtime-ktx than 2.10.3 is available: 2.11.1 |
Generated by 🚫 Danger |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2835 +/- ##
============================================
+ Coverage 63.24% 63.90% +0.66%
- Complexity 2825 2875 +50
============================================
Files 216 219 +3
Lines 16982 17064 +82
Branches 2418 2423 +5
============================================
+ Hits 10740 10905 +165
+ Misses 5073 4990 -83
Partials 1169 1169
🚀 New features to boost your workflow:
|
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
2 times, most recently
from
bcf7c35 to
282337b
Compare
brandonpage
left a comment
brandonpage
left a comment
There was a problem hiding this comment.
Just a couple high level comments while you are in draft. 👍
| import com.salesforce.androidsdk.R.string.sf__server_remove_content_description | ||
| import com.salesforce.androidsdk.R.string.sf__server_url_delete | ||
| import com.salesforce.androidsdk.config.LoginServerManager.LoginServer | ||
| import com.salesforce.androidsdk.ui.PADDING_SIZE |
There was a problem hiding this comment.
The local PADDING_SIZE is not removed.
There was a problem hiding this comment.
When I was basing the new ScreenLockActivity off other recent Compose implementations, I noticed this PADDING_SIZE and CORNER_RADIUS where used in a lot of disparate sources though still defined in LoginView.kt. Doesn't it seem they should be in a common location? I created UserInterfaceConstants.kt but we could do something different. There may be other values as well.
| * An activity that locks the app behind the operating system's provided | ||
| * authentication. | ||
| */ | ||
| internal class ScreenLockActivity : FragmentActivity() { |
There was a problem hiding this comment.
Technically for Semver this should be open class ScreenLockActivity (public). It wasn't possible for us to use a subclass so it shouldn't matter. I don't think this was a bad decision, just throwing it out there for the sake of completeness and conversation.
There was a problem hiding this comment.
I went ahead and made it public again as I'm getting ready to mark this ready for review soon ✅
| internal class ScreenLockViewModel( | ||
| build: Int = SDK_INT, | ||
| // TODO: Test coverage needed. ECJ20260205 | ||
| val appName: String = getInstance().appName ?: "App", | ||
| ) : ViewModel() { |
There was a problem hiding this comment.
A ViewModel seems like overkill IMO. Is there a reason it is needed?
Throwing that out for consideration before you spend a ton of time writing tests.
There was a problem hiding this comment.
I was wondering as well since at first the view model was relatively light and much of the logic related to screen lock is at the activity/view level. That said, there are some observables and they will benefit from the life cycle management the view model offers. It will also be ready should this grow in complexity. So, I'm leaning towards keeping the view model unless we feel a compelling reason not to.
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
2 times, most recently
from
a69d97c to
a29609b
Compare
Job Summary for GradlePull Request :: test-android
|
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
15 times, most recently
from
692a6f3 to
954c6b2
Compare
| @@ -1,3 +1,29 @@ | |||
| /* | |||
There was a problem hiding this comment.
I added the copyright header here since it was missing. We do want those everywhere, correct?
| EdgeToEdge.enable(this); | ||
|
|
||
| // Protect against screenshots. | ||
| getWindow().setFlags(WindowManager.LayoutParams.FLAG_SECURE, |
There was a problem hiding this comment.
I carefully migrated all the logic from the previous activity line-by-line and method-by-method to the new activity in a best attempt to avoid missing anything.
| @Override | ||
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| create() |
There was a problem hiding this comment.
I really wanted to be able to pull the data levers on the create logic for top-notch onCreate unit tests and coverage, but being a lifecycle method there's no opening to it. So, I moved all the logic to a private/VisibleForTest method with parameters. That really worked like a charm, I thought.
There was a problem hiding this comment.
ActivityScenario is used in most of our other test to exercise onCreate.
There was a problem hiding this comment.
True, though all the tests I'd used on LoginActivity were able to manipulate intent extras or data to exercise logic in the activity instance. In this case, ScreenLockActivity doesn't have any external API to accomplish that.
|
|
||
| /** | ||
| * Implements the creation of the activity. | ||
| * @param build The Android SDK build. This parameter is intended for |
There was a problem hiding this comment.
The commentary is verbose, yes, but being professional open source I'm really hoping it helps others know the intent and not abuse the contract of the code. I have often felt it is difficult to interpret the idea behind much of the code in the library, so this could help.
| @SuppressLint("NewApi") | ||
| @VisibleForTesting | ||
| internal fun create( | ||
| build: Int = SDK_INT, |
There was a problem hiding this comment.
This activity has a lot of SDK-dependent logic, so I added testable parameters where needed. The bad thing is that it also requires a "suppress". Used with discipline, it seems alright and gets that path automatically tested. I'd be open to hear other ideas on that, though.
| internal fun create( | ||
| build: Int = SDK_INT, | ||
| packageManager: PackageManager = this.packageManager, | ||
| sdkManager: SalesforceSDKManager = getInstance(), |
There was a problem hiding this comment.
It's too big a topic for a pull request thread, but I'll go on a limb and wonder if this level of testability might require a dependency injection tool like Hilt. I used it quite a bit and it was a love 'n hate experience. It automates this type of test set up a lot, though. It could a great retro topic after this pull request.
| build: Int = SDK_INT, | ||
| ) { | ||
| when (biometricManager.canAuthenticate(viewModel.biometricAuthenticators())) { | ||
| BIOMETRIC_ERROR_NO_HARDWARE, BIOMETRIC_ERROR_SECURITY_UPDATE_REQUIRED, BIOMETRIC_ERROR_UNSUPPORTED, BIOMETRIC_STATUS_UNKNOWN -> { |
There was a problem hiding this comment.
These codes are not a type safe enumeration. Should we just make this an else so it cannot fall through? @brandonpage, thoughts?
There was a problem hiding this comment.
Looking at BiometricManger.java the return type has an @IntDef annotation:
Denotes that the annotated element of integer type, represents a logical type and that its value should be one of the explicitly named constants. If the IntDef#flag() attribute is set to true, multiple constants can be combined.
So if a new Android API level added a new value we would get an compile time error when we updated to target it. So what you have is fine as is.
There was a problem hiding this comment.
Perfect-O. Thanks for looking at that.
| /** | ||
| * An empty function. | ||
| */ | ||
| internal fun noOp() {} |
There was a problem hiding this comment.
Here's a dandy, code-covered no-op function (which Kotlin doesn't provide) so we can reduce JaCoCo noise. Perhaps this could live in the new UserInterfaceConstants.kt or elsewhere?
There was a problem hiding this comment.
Having this in a centralized place sounds good to me.
| * SalesforceSDKManager instance | ||
| * @return The displayed name of the app | ||
| */ | ||
| fun appName( |
There was a problem hiding this comment.
This became more testable as a function than a computed property.
| * @param build The Android SDK build. This parameter is intended for | ||
| * testing purposes only. Defaults to the current Android SDK build | ||
| */ | ||
| fun biometricAuthenticators( |
There was a problem hiding this comment.
This also became more testable as a function than a computed property.
| <uses-permission android:name="android.permission.USE_BIOMETRIC" /> | ||
|
|
||
| <application> | ||
| <application android:supportsRtl="true"> |
There was a problem hiding this comment.
Wow, so after the Jetpack Compose migration this was actually the only task left. Note, without this the app gets into a strange state where it will use the RTL text but used the LTR layout. It looks really odd.
I reckon this will resource-merge through to the applications and in this case that means all application modules that depend on SalesforceSDK. Is that acceptable? Would we want to leave this off here and let the app modules take responsibility for adding it? Is there a case where an app module could be negatively affected? They could override as needed. I always consider that when updating a library module such as this.
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | ||
| api("androidx.work:work-runtime-ktx:2.10.3") | ||
|
|
||
| implementation("com.google.accompanist:accompanist-drawablepainter:0.37.3") |
There was a problem hiding this comment.
Here's some Google code that lets us "remember" the programmatically-derived Drawable for the app icon.
There was a problem hiding this comment.
Do we need a 3PP for this? If it is from Google maybe not?
There was a problem hiding this comment.
We should check. Can you direct message any instructions you have on how I can review or request that?
There was a problem hiding this comment.
That 3PP is approved ✅
| assertTrue(sendAccessibilityCapturingSlot.captured.contains(activity.getString(sf__screen_lock_auth_failed))) | ||
| } | ||
|
|
||
| private fun Drawable.renderToBitmap(): Bitmap { |
There was a problem hiding this comment.
For asserting the content of the app icon, here's a utility method that helps.
…RTL UI (Self-Review Cleanup)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
from
70ae413 to
33fc0b1
Compare
| import com.salesforce.androidsdk.ui.components.ScreenLockView | ||
| import com.salesforce.androidsdk.util.test.ExcludeFromJacocoGeneratedReport | ||
|
|
||
| @ExcludeFromJacocoGeneratedReport |
There was a problem hiding this comment.
If the Previews aren't in the same file as the view code then why even have them? The whole point is that you can see the changes you are making live as you edit. You could have ScreenLockView and ScreenLockViewPreview open side by side but I don't understand the point.
| appName: String, | ||
| innerPadding: PaddingValues, | ||
| logoutAction: () -> Unit, | ||
| viewModel: ScreenLockViewModel, |
There was a problem hiding this comment.
Passing ViewModels is not recommended. If it is necessary for testing we should have a viewModel(factory = ViewModelFactory) default value so it isn't passed in production.
There was a problem hiding this comment.
We do already have lifecycle-viewmodel-compose-android as a dependency and screen lock only has a top-level composable. If this parameter defaulted to viewModel() from that dependency, it would seem to match Thinking in Compose better.
I'd interpreted the style guide to mean a "screen level" composable was allowed access to the view model while "component level" composables called from other composables were not in order to preserve the "skip as much as possible" guideline.
I'll push a quick commit so we can review what that looks like.
| Text( | ||
| fontSize = 14.sp, | ||
| modifier = Modifier.align(CenterHorizontally), | ||
| text = viewModel.setupMessageText.value ?: stringResource(sf__screen_lock_setup_required, appName), |
There was a problem hiding this comment.
Huh? ScreenLockViewModel is internal and setupMessageText never set (outside of tests).
| viewModel.setupButtonAction.value() | ||
| }, | ||
| shape = RoundedCornerShape(CORNER_RADIUS.dp), | ||
| ) { Text(color = colorScheme.onPrimary, fontSize = 14.sp, fontWeight = Normal, text = viewModel.setupButtonLabel.value ?: stringResource(sf__screen_lock_setup_button)) } |
There was a problem hiding this comment.
NIT: fontSize (3 times in this file) and icon size should probably reference a common const value.
| @Override | ||
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| create() |
There was a problem hiding this comment.
ActivityScenario is used in most of our other test to exercise onCreate.
| enableEdgeToEdge() | ||
|
|
||
| // Protect against screenshots. | ||
| window.setFlags(FLAG_SECURE, FLAG_SECURE) |
There was a problem hiding this comment.
I would not mind this being in a if (!SalesforceSDKManager.getInstance().isDebugBuild). The OS prompts are blocked regardless so it might not be needed at all.
There was a problem hiding this comment.
I'd kept this exactly as it was. Should we keep the functionality the same for a point release such as 13.2?
| build: Int = SDK_INT, | ||
| ) { | ||
| when (biometricManager.canAuthenticate(viewModel.biometricAuthenticators())) { | ||
| BIOMETRIC_ERROR_NO_HARDWARE, BIOMETRIC_ERROR_SECURITY_UPDATE_REQUIRED, BIOMETRIC_ERROR_UNSUPPORTED, BIOMETRIC_STATUS_UNKNOWN -> { |
There was a problem hiding this comment.
Looking at BiometricManger.java the return type has an @IntDef annotation:
Denotes that the annotated element of integer type, represents a logical type and that its value should be one of the explicitly named constants. If the IntDef#flag() attribute is set to true, multiple constants can be combined.
So if a new Android API level added a new value we would get an compile time error when we updated to target it. So what you have is fine as is.
| /** | ||
| * An empty function. | ||
| */ | ||
| internal fun noOp() {} |
There was a problem hiding this comment.
Having this in a centralized place sounds good to me.
| @VisibleForTesting | ||
| internal inner class BiometricAuthenticationCallback( | ||
| private val activity: ScreenLockActivity = this@ScreenLockActivity, | ||
| ) : AuthenticationCallback() { |
There was a problem hiding this comment.
Could this just be an inline lambda now? If that makes testing a lot harder than nbd.
There was a problem hiding this comment.
You're correct that it's an extracted class for testing ✅
| api("androidx.browser:browser:1.8.0") // Update requires API 36 compileSdk | ||
| api("androidx.work:work-runtime-ktx:2.10.3") | ||
|
|
||
| implementation("com.google.accompanist:accompanist-drawablepainter:0.37.3") |
There was a problem hiding this comment.
Do we need a 3PP for this? If it is from Google maybe not?
| // Disable back navigation. | ||
| if (build >= TIRAMISU) { | ||
| getOnBackInvokedDispatcher().registerOnBackInvokedCallback( | ||
| PRIORITY_DEFAULT, ::noOp |
There was a problem hiding this comment.
Field requires API level 33 (current min is 28): android.window.OnBackInvokedDispatcher#PRIORITY_DEFAULT |
| // Prompts the user to setup the operating system screen lock and biometrics. | ||
| if (build >= R) { // TODO: Remove when min API > 29. | ||
| viewModel.setupButtonAction.value = { | ||
| biometricSetupActivityResultLauncher.launch(Intent(ACTION_BIOMETRIC_ENROLL).apply { |
There was a problem hiding this comment.
Field requires API level 30 (current min is 28): android.provider.Settings#ACTION_BIOMETRIC_ENROLL |
| if (build >= R) { // TODO: Remove when min API > 29. | ||
| viewModel.setupButtonAction.value = { | ||
| biometricSetupActivityResultLauncher.launch(Intent(ACTION_BIOMETRIC_ENROLL).apply { | ||
| putExtra(EXTRA_BIOMETRIC_AUTHENTICATORS_ALLOWED, viewModel.biometricAuthenticators()) |
There was a problem hiding this comment.
Field requires API level 30 (current min is 28): android.provider.Settings#EXTRA_BIOMETRIC_AUTHENTICATORS_ALLOWED |
| packageManager: PackageManager = this.packageManager, | ||
| ): PromptInfo { | ||
| val hasFaceUnlock = if (build >= Q) { // TODO: Remove when min API > 28. | ||
| packageManager.hasSystemFeature(FEATURE_FACE) || (packageManager.hasSystemFeature(FEATURE_IRIS)) |
There was a problem hiding this comment.
Field requires API level 29 (current min is 28): android.content.pm.PackageManager#FEATURE_FACE |
| packageManager: PackageManager = this.packageManager, | ||
| ): PromptInfo { | ||
| val hasFaceUnlock = if (build >= Q) { // TODO: Remove when min API > 28. | ||
| packageManager.hasSystemFeature(FEATURE_FACE) || (packageManager.hasSystemFeature(FEATURE_IRIS)) |
There was a problem hiding this comment.
Field requires API level 29 (current min is 28): android.content.pm.PackageManager#FEATURE_IRIS |
| // TODO: Remove this when min API > 33 | ||
| // Disable back navigation. | ||
| if (build >= TIRAMISU) { | ||
| getOnBackInvokedDispatcher().registerOnBackInvokedCallback( |
There was a problem hiding this comment.
| 🚫 | Call requires API level 33 (current min is 28): android.app.Activity#getOnBackInvokedDispatcher |
| // TODO: Remove this when min API > 33 | ||
| // Disable back navigation. | ||
| if (build >= TIRAMISU) { | ||
| getOnBackInvokedDispatcher().registerOnBackInvokedCallback( |
There was a problem hiding this comment.
| 🚫 | Call requires API level 33 (current min is 28): android.window.OnBackInvokedDispatcher#registerOnBackInvokedCallback |
| if (accessibilityManager.isEnabled) { | ||
| accessibilityManager.sendAccessibilityEvent( | ||
| if (build >= R) { | ||
| AccessibilityEvent() |
There was a problem hiding this comment.
| 🚫 | Call requires API level 30 (current min is 28): android.view.accessibility.AccessibilityEvent() |
| }.apply { | ||
| setEventType(TYPE_WINDOW_STATE_CHANGED) | ||
| setClassName(this@ScreenLockActivity.javaClass.getName()) | ||
| setPackageName(this@ScreenLockActivity::javaClass.get().packageName) |
There was a problem hiding this comment.
| 🚫 | Call requires API level 31 (current min is 28): java.lang.Class#getPackageName |
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
from
f9deadb to
b8c78ad
Compare
Job Summary for GradlePull Request :: test-android
|
…RTL UI (Peer-Review Cleanup)
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
from
b8c78ad to
bfefa75
Compare
2 participants
🎸 Ready For Review 🥁
This updates the screen lock setup activity to support right-to-left text layouts along with a much heftier effort to migrate from Java to Kotlin, layout using Jetpack Compose and provide comprehensive unit tests.
All deprecations are resolved as well and the sources should have no inspector warnings.
To test, provide the strings used by the activity in an RTL language such as Arabic. Then, set the system language to that language.