[PM-26909] Implement screen capture toggle authenticator #6033
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6033 +/- ##
==========================================
+ Coverage 84.72% 84.81% +0.09%
==========================================
Files 722 720 -2
Lines 54717 54619 -98
Branches 7583 7583
==========================================
- Hits 46358 46326 -32
+ Misses 5706 5637 -69
- Partials 2653 2656 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
| /** | ||
| * A set of Bitwarden account IDs that have previously been synced. | ||
| * Whether or not screen capture is allowed for the current user. | ||
| */ | ||
| var previouslySyncedBitwardenAccountIds: Set<String> | ||
| val isScreenCaptureAllowedStateFlow: StateFlow<Boolean> | ||
|
|
||
| /** | ||
| * Whether or not screen capture is allowed for the current user. | ||
| * A set of Bitwarden account IDs that have previously been synced. | ||
| */ | ||
| val isScreenCaptureAllowedStateFlow: StateFlow<Boolean> |
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
yes, I moved isScreenCaptureAllowedStateFlow to be right after the isScreenCaptureAllowed.
There was previouslySyncedBitwardenAccountIds between them, very minor but made me not see it immediately
| BitwardenTwoButtonDialog( | ||
| title = stringResource(BitwardenString.allow_screen_capture), | ||
| message = stringResource( | ||
| BitwardenString.are_you_sure_you_want_to_enable_screen_capture, |
There was a problem hiding this comment.
Can we add the name here
| ) | ||
| } | ||
|
|
||
| ScreenCaptureRow( |
There was a problem hiding this comment.
You're missing the 8dp spacer before this item
There was a problem hiding this comment.
Unless the intention is to make this a single card?
|
|
||
| is SettingsAction.SecurityClick.AllowScreenCaptureToggle -> { | ||
| settingsRepository.isScreenCaptureAllowed = action.enabled | ||
| mutableStateFlow.update { it.copy(allowScreenCapture = action.enabled) } |
There was a problem hiding this comment.
This should be in a handleAllowScreenCaptureToggle() function
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
This suppression is not needed
| @Composable | ||
| private fun ScreenCaptureRow( | ||
| currentValue: Boolean, | ||
| hasBiometrics: Boolean, |
There was a problem hiding this comment.
Can we pass in the cardStyle instead, there is no reason this composable needs to know about biometrics
| is SettingsAction.SecurityClick.AllowScreenCaptureToggle -> { | ||
| settingsRepository.isScreenCaptureAllowed = action.enabled | ||
| mutableStateFlow.update { it.copy(allowScreenCapture = action.enabled) } | ||
| handleAllowScreenCaptureToggle(action) |
There was a problem hiding this comment.
Thanks for that! 😄
aj-rosado
deleted the
PM-26909/implement-screen-capture-toggle-authenticator
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26909
📔 Objective
Add screen capture toggle for Authenticator
⏰ Reminders before review
📸 Screenshots
🦮 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