[PM-27092] Changing screen capture flow from event based to state based on Authenticator #6062
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6062 +/- ##
==========================================
+ Coverage 84.74% 84.78% +0.04%
==========================================
Files 737 720 -17
Lines 55439 55060 -379
Branches 7655 7654 -1
==========================================
- Hits 46983 46684 -299
+ Misses 5775 5695 -80
Partials 2681 2681 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
| setupEdgeToEdge(appThemeFlow = mainViewModel.stateFlow.map { it.theme }) | ||
| setContent { | ||
| val state by mainViewModel.stateFlow.collectAsStateWithLifecycle() | ||
| handleScreenCaptureSettingChange(isScreenCaptureAllowed = state.isScreenCaptureAllowed) |
There was a problem hiding this comment.
Can we rename this function, the handle moniker is reserved for actions and events.
I think we went with updateScreenCapture in the PW app.
| .isScreenCaptureAllowedStateFlow | ||
| .onEach { isAllowed -> | ||
| sendEvent(MainEvent.ScreenCaptureSettingChange(isAllowed)) | ||
| mutableStateFlow.update { it.copy(isScreenCaptureAllowed = isAllowed) } |
There was a problem hiding this comment.
You cannot update the state from within a coroutine scope. You should be sending an internal action to do this.
The docs on this can be found here.
Improved method naming on MainActivity
| sendEvent(MainEvent.ScreenCaptureSettingChange(isAllowed)) | ||
| } | ||
| .map { MainAction.Internal.ScreenCaptureUpdate(it) } | ||
| .onEach(::trySendAction) |
There was a problem hiding this comment.
Can we just use ::sendAction
| MainAction.OpenDebugMenu -> handleOpenDebugMenu() | ||
|
|
||
| is MainAction.Internal.ScreenCaptureUpdate -> handleScreenCaptureUpdate( | ||
| isAllowed = action.isScreenCaptureEnabled, |
There was a problem hiding this comment.
You should pass the whole action through
…nshot-flag # Conflicts: # authenticator/src/main/kotlin/com/bitwarden/authenticator/MainViewModel.kt # authenticator/src/test/kotlin/com/bitwarden/authenticator/MainViewModelTest.kt
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27092
📔 Objective
Changing orientation is cleaning the flags, changing
isScreenCaptureAllowedto state makes this work properly and consistent with the password manager implementation.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes