Refactor connect login account select location into feature modules#9844
Refactor connect login account select location into feature modules#9844
Conversation
Rawa
left a comment
There was a problem hiding this comment.
@Rawa partially reviewed 78 files and all commit messages, and made 8 comments.
Reviewable status: 78 of 133 files reviewed, 8 unresolved discussions (waiting on @Pururun).
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/HomeTransition.kt line 17 at r1 (raw file):
AnimatedContentTransitionScope<NavBackStackEntry>.() -> EnterTransition = { fadeIn()
Accidental? I think this should not be here?
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/ConnectScreen.kt line 180 at r1 (raw file):
animatedVisibilityScope: AnimatedVisibilityScope, // TODO Restore this when we can navigate to select location // selectLocationResultRecipient: ResultRecipient<SelectLocationMockDestination, Boolean>,
Should we include select location in this PR as well? I think it might be easier than rebase this one ontop of select location?
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/button/SwitchLocationButton.kt line 103 at r1 (raw file):
), modifier = Modifier.Companion.constrainAs(connectionButton) {
Android Studio refactoring 😠
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 95 at r1 (raw file):
SelectionContainer( modifier = Modifier.Companion.constrainAs(inAddr) {
ditto
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 137 at r1 (raw file):
Box( modifier = Modifier.Companion.constrainAs(outAddrV4) {
ditto
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 188 at r1 (raw file):
Box( modifier = Modifier.Companion.constrainAs(outAddrV6) {
ditto
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/notificationbanner/InAppNotificationController.kt line 29 at r1 (raw file):
) } .stateIn(scope, SharingStarted.Companion.Eagerly, emptyList())
ditto
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MullvadApp.kt line 86 at r1 (raw file):
val LocalNavAnimatedVisibilityScope = compositionLocalOf<AnimatedVisibilityScope?> { null } @OptIn(ExperimentalSharedTransitionApi::class) val LocalSharedTransitionScope = compositionLocalOf<SharedTransitionScope?> { null }
Did we accidentally remove these? I think they are needed for shared animations
69f8c67 to
a9a169c
Compare
Rawa
left a comment
There was a problem hiding this comment.
@Rawa partially reviewed 61 files and all commit messages, and made 5 comments.
Reviewable status: 129 of 135 files reviewed, 13 unresolved discussions (waiting on @Pururun).
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/outoftime/OutOfTimeViewModelTest.kt line 94 at r1 (raw file):
fun `when clicking on site payment then open website account view`() = runTest { // Arrange val mockToken = WebsiteAuthToken.Companion.fromString("154c4cc94810fddac78398662b7fa0c7")
remove
android/lib/feature/login/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/login/impl/LoginScreen.kt line 343 at r1 (raw file):
imeAction = if (state.loginButtonEnabled) ImeAction.Done else ImeAction.None, keyboardType = KeyboardType.Companion.accountNumberKeyboardType(LocalContext.current),
Remove
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/devicerevoked/DeviceRevokedViewModelTest.kt line 72 at r1 (raw file):
// Act, Assert viewModel.uiState.test { Assertions.assertEquals(DeviceRevokedUiState.UNKNOWN, awaitItem())
Wrong import? Should we not use kotlin.test?
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/welcome/WelcomeViewModelTest.kt line 93 at r1 (raw file):
fun `on onSitePaymentClick call uiSideEffect should emit OpenAccountView`() = runTest { // Arrange val mockToken = WebsiteAuthToken.Companion.fromString("154c4cc94810fddac78398662b7fa0c7")
remove
android/lib/push-notification/src/main/kotlin/net/mullvad/mullvadvpn/lib/pushnotification/receiver/NotificationAlarmReceiver.kt line 21 at r2 (raw file):
// It is not possible to bind to a service from a notification alarm receiver so we will use // a worker instead. Logger.Companion.d("Account expiry alarm triggered")
Remove
Rawa
left a comment
There was a problem hiding this comment.
@Rawa reviewed 6 files.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Pururun).
Pururun
left a comment
There was a problem hiding this comment.
@Pururun made 13 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Rawa).
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MullvadApp.kt line 86 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Did we accidentally remove these? I think they are needed for shared animations
It was moved to the compose module due to feature modules needing to access them.
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/HomeTransition.kt line 17 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Accidental? I think this should not be here?
Done
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/ConnectScreen.kt line 180 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Should we include select location in this PR as well? I think it might be easier than rebase this one ontop of select location?
Added
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/button/SwitchLocationButton.kt line 103 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Android Studio refactoring 😠
Done.
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 95 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
ditto
Done.
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 137 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
ditto
Done.
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/connectioninfo/ConnectionDetailPanel.kt line 188 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
ditto
Done.
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/notificationbanner/InAppNotificationController.kt line 29 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
ditto
Done.
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/devicerevoked/DeviceRevokedViewModelTest.kt line 72 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Wrong import? Should we not use
kotlin.test?
Done.
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/outoftime/OutOfTimeViewModelTest.kt line 94 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
remove
Done.
android/lib/feature/home/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/home/impl/welcome/WelcomeViewModelTest.kt line 93 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
remove
Done.
android/lib/feature/login/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/login/impl/LoginScreen.kt line 343 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Remove
Done.
android/lib/push-notification/src/main/kotlin/net/mullvad/mullvadvpn/lib/pushnotification/receiver/NotificationAlarmReceiver.kt line 21 at r2 (raw file):
Previously, Rawa (David Göransson) wrote…
Remove
Done.
Rawa
left a comment
There was a problem hiding this comment.
@Rawa partially reviewed 47 files and all commit messages, made 4 comments, and resolved 13 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun).
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MullvadApp.kt line 86 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
It was moved to the compose module due to feature modules needing to access them.
Sounds good to me
android/lib/feature/home/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/home/impl/connect/ConnectScreen.kt line 180 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Added
🚀 🦸
android/lib/feature/location/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/location/impl/list/SelectLocationListViewModelTest.kt line 95 at r3 (raw file):
// Assert Assertions.assertEquals(Lce.Loading(Unit), viewModel.uiState.value)
Import from kotlin.test
android/lib/feature/login/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/login/impl/LoginScreen.kt line 140 at r3 (raw file):
// TODO Remove this code when migrating to nav3 LaunchedEffect(accountNumber) { if (accountNumber != null && accountNumber.toLongOrNull() != null) {
Wouldn't this break if we get {accountNumber} ? Login would fail right?
Pururun
left a comment
There was a problem hiding this comment.
@Pururun made 2 comments.
Reviewable status: 158 of 165 files reviewed, 2 unresolved discussions (waiting on @Rawa).
android/lib/feature/location/impl/src/test/kotlin/net/mullvad/mullvadvpn/feature/location/impl/list/SelectLocationListViewModelTest.kt line 95 at r3 (raw file):
Previously, Rawa (David Göransson) wrote…
Import from
kotlin.test
Done.
android/lib/feature/login/impl/src/main/kotlin/net/mullvad/mullvadvpn/feature/login/impl/LoginScreen.kt line 140 at r3 (raw file):
Previously, Rawa (David Göransson) wrote…
Wouldn't this break if we get
{accountNumber}? Login would fail right?
Fixed
Rawa
left a comment
There was a problem hiding this comment.
@Rawa partially reviewed 7 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
6dd6b4b to
c17fd90
Compare
Rawa
left a comment
There was a problem hiding this comment.
@Rawa reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
Connect/Home not yet done
This change is