@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.89% +0.65%
- Complexity 2825 2874 +49
============================================
Files 216 218 +2
Lines 16982 17064 +82
Branches 2418 2423 +5
============================================
+ Hits 10740 10903 +163
+ Misses 5073 4990 -83
- Partials 1169 1171 +2
🚀 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
|
Code coverage is 96.60%. There are a handful of Jetpack Compose function call sites that are beyond the ability of JaCoCo to probe and that's expected from the research I've done. They are tested by the new tests though and I used tooling to validate that. The tests I created are comprehensive and should provide reasonable assertions for the behavior migrated from Java. There are a few places where I introduced new ways of tidying the code so it provides better coverage numbers and generates less noise in the code coverage reports. |
|
Here're screenshots of the English LTR and Arabic RTL I tested with. |
| @@ -1,68 +0,0 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
|
|
|||
| <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
I did a side-by-side comparison of this XML with the new Compose logic plus compared screenshots of the two versions for parity.
| <?xml version="1.0" encoding="utf-8"?> | ||
|
|
||
| <resources> | ||
| <!-- Dimensions used in sf__screen_lock.xml --> |
There was a problem hiding this comment.
I believe these were the only orphaned dimensions after removing the XML layout.
| <item name="android:windowLightStatusBar">false</item> | ||
| </style> | ||
|
|
||
| <style name="SalesforceSDK.ScreenLock.ErrorMessage"> |
There was a problem hiding this comment.
This style was also orphaned when the XML layout for screen lock was removed.
| import com.salesforce.androidsdk.ui.components.ScreenLockView | ||
| import com.salesforce.androidsdk.util.test.ExcludeFromJacocoGeneratedReport | ||
|
|
||
| @ExcludeFromJacocoGeneratedReport |
There was a problem hiding this comment.
At a suggestion from our tooling, I moved the preview to a separate source. There were some proposed benefits, though I later realized those didn't manifest. That said, it really did look nice having this non-release code extracted so I left it that way.
To exclude this from code coverage, one suggestion was to move previews to the debug source root. However, I then realized we're building and generating coverage from the debug variant anyways. So, that became a moot point. It was interesting to note that the ExcludeFromJacocoGeneratedReport annotation only applies to the method and not to the default parameter values. That was when I started looking for a way to exclude the entire source.
For now, this seems fine. Perhaps we revisit the variants and directory layout in the future.
| innerPadding = PaddingValues.Absolute(0.dp), | ||
| appIcon = painterResource(id = R.drawable.sf__salesforce_logo), | ||
| viewModel = ScreenLockViewModel(), | ||
| logoutAction = ::noOp, |
There was a problem hiding this comment.
This was logout = {} for a time, but of course JaCoCo flagged the anonymous function as uncovered. So, I created a utility noOp function that is trivially tested elsewhere and eliminates the coverage noise. It seemed like an easy win.
JohnsonEricAtSalesforce
force-pushed
the
feature/w-19368426_msdk-rewrite-android-screen-lock_a.k.a-passcode_for-rtl-ui
branch
from
44c5b58 to
70ae413
Compare
| */ | ||
| @Composable | ||
| internal fun ScreenLockView( | ||
| appIcon: Painter, |
There was a problem hiding this comment.
The Painter reference here is a little interesting, but it was a way to pass in the programmatically derived drawable instance for the app icon which doesn't actually have a resource id.
| appName: String, | ||
| innerPadding: PaddingValues, | ||
| logoutAction: () -> Unit, | ||
| viewModel: ScreenLockViewModel, |
There was a problem hiding this comment.
Yes, trailing commas everywhere to reduce future LoC diff.
| // Log out button. | ||
| Button( | ||
| colors = ButtonColors( | ||
| containerColor = Transparent, |
There was a problem hiding this comment.
I based all the colors, fonts and padding from other recently added Compose layouts.
| modifier = Modifier.padding(PADDING_SIZE.dp), | ||
| onClick = logoutAction, | ||
| shape = RoundedCornerShape(CORNER_RADIUS.dp), | ||
| ) { Text(color = colorScheme.primary, fontWeight = Normal, fontSize = 17.sp, text = stringResource(sf__logout)) } |
There was a problem hiding this comment.
For Compose function call sites that JaCoCo doesn't probe well, I did make them a single line to reduce the impact noise on the repo's stats.
| 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.
The setup message and setup button label became the best justification for keeping the view model, in the end. They're dynamic.
| @@ -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.
|
|
||
| /** | ||
| * 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?
| /** | ||
| * 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?
| * 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.
| 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
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.