Fix modal contents overlapping screen lock pin #2692 #2874

ganfra · 2024-05-17T16:42:41Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Extract PinUnlock to a separate Activity so we are sure it's always rendered on top of everything else.

Motivation and context

Closes #2692

Screenshots / GIFs

Tests

Setup a pin
Open a modal content (dialog, bottomsheet)
Put the app in background
Open the app
You should see only the Pin unlock screen

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

codecov · 2024-05-17T17:09:06Z

Codecov Report

Attention: Patch coverage is 36.95652% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 74.28%. Comparing base (4919cd6) to head (5c4326e).
Report is 13 commits behind head on develop.

Files	Patch %	Lines
...ckscreen/impl/unlock/activity/PinUnlockActivity.kt	0.00%	18 Missing ⚠️
...c/main/kotlin/io/element/android/x/MainActivity.kt	0.00%	9 Missing ⚠️
...res/lockscreen/impl/DefaultLockScreenEntryPoint.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2874      +/-   ##
===========================================
- Coverage    74.30%   74.28%   -0.03%     
===========================================
  Files         1530     1532       +2     
  Lines        36524    36550      +26     
  Branches      7054     7058       +4     
===========================================
+ Hits         27140    27150      +10     
- Misses        5698     5714      +16     
  Partials      3686     3686

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-17T21:56:37Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/pSpG9A

bmarty

Nice work, thanks! Some remarks / questions

bmarty · 2024-05-17T21:49:34Z

app/src/main/AndroidManifest.xml

+        <activity
+            android:name="io.element.android.features.lockscreen.impl.unlock.activity.PinUnlockActivity"
+            android:configChanges="screenSize|screenLayout|orientation|keyboardHidden|keyboard|navigation|uiMode"/>
+


Can this be moved to an new Android Manifest in the lockscreen:impl module?

bmarty · 2024-05-17T21:49:55Z

app/src/main/kotlin/io/element/android/x/MainActivity.kt

-                        .fillMaxSize()
-                        .background(MaterialTheme.colorScheme.background),
+                            .fillMaxSize()
+                            .background(MaterialTheme.colorScheme.background),


We need to find a solution to avoid such change

bmarty · 2024-05-17T21:51:42Z

app/src/main/kotlin/io/element/android/x/MainActivity.kt

+                lockScreenService.lockState.collect { state ->
+                    if (state == LockScreenLockState.Locked) {
+                        startActivity(PinUnlockActivity.newIntent(this@MainActivity))
+                    }


This is working, but can we try to use only things from the api module?
Maybe expose a getIntent(): Intent? api and do nothing when null is returned?

It's ok for me to use impl in the app module as it's the glue, but I can still expose something in the EntryPoint if u prefer.

bmarty · 2024-05-17T21:52:53Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

-                lockScreenEntryPoint.nodeBuilder(this, buildContext)
-                    .target(LockScreenEntryPoint.Target.Unlock)
-                    .build()
-            }


I think that Target.Unlock can be removed, since it's not used anymore.
Same for LockScreenFlowNode.NavTarget.Unlock and PinUnlockNode.Inputs since isInAppUnlockwill always be false.

bmarty · 2024-05-17T22:00:06Z

...main/kotlin/io/element/android/features/lockscreen/impl/unlock/activity/PinUnlockActivity.kt

+        setContent {
+            ElementTheme {
+                val state = presenter.present()
+                PinUnlockView(state = state, isInAppUnlock = false)


Shouldn't isInAppUnlock be set to true here? The parameter is always false in production code (i.e. not counting preview, can you double check please?

No, isInAppUnlock is true when used for accessing the pin settings here

OK, thanks. Strange that I missed on it during the first review.

features/lockscreen/impl/src/main/AndroidManifest.xml

sonarcloud · 2024-05-21T09:16:45Z

Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarCloud

bmarty

Thanks, tested OK right now!

ganfra · 2024-05-21T12:05:20Z

@bmarty should we handle the SonarCloud failure?

Fix modal contents overlapping screen lock pin #2692

b0c369e

ganfra marked this pull request as ready for review May 17, 2024 16:42

ganfra requested a review from a team as a code owner May 17, 2024 16:42

ganfra requested review from bmarty and removed request for a team May 17, 2024 16:42

bmarty reviewed May 17, 2024

View reviewed changes

ganfra requested a review from bmarty May 20, 2024 13:57

ganfra added 2 commits May 20, 2024 15:57

Merge branch 'develop' into feature/fga/fix_2692

efc0a36

Pin : clean up after PR review #2692

73905fa

ganfra force-pushed the feature/fga/fix_2692 branch from a19f69a to 73905fa Compare May 20, 2024 14:35

bmarty reviewed May 21, 2024

View reviewed changes

features/lockscreen/impl/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved

Correct date for Copyright

5c4326e

bmarty approved these changes May 21, 2024

View reviewed changes

ganfra added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

ganfra merged commit ac123bd into develop May 21, 2024
22 of 25 checks passed

ganfra deleted the feature/fga/fix_2692 branch May 21, 2024 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix modal contents overlapping screen lock pin #2692 #2874

Fix modal contents overlapping screen lock pin #2692 #2874

ganfra commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

bmarty left a comment

bmarty May 17, 2024

bmarty May 17, 2024

bmarty May 17, 2024

ganfra May 20, 2024

bmarty May 21, 2024

bmarty May 17, 2024

bmarty May 17, 2024

ganfra May 20, 2024

bmarty May 21, 2024

sonarcloud bot commented May 21, 2024

bmarty left a comment

ganfra commented May 21, 2024

Fix modal contents overlapping screen lock pin #2692 #2874

Fix modal contents overlapping screen lock pin #2692 #2874

Conversation

ganfra commented May 17, 2024

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

codecov bot commented May 17, 2024 • edited

Codecov Report

github-actions bot commented May 17, 2024 • edited

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 21, 2024

Quality Gate failed

bmarty left a comment

Choose a reason for hiding this comment

ganfra commented May 21, 2024

codecov bot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited