diff --git a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreen.kt index 9304ee4e19..9e20c25823 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreen.kt @@ -33,13 +33,18 @@ import androidx.compose.material3.TextField import androidx.compose.material3.TextFieldDefaults import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.focus.FocusRequester import androidx.compose.ui.focus.focusRequester +import androidx.compose.ui.focus.onFocusChanged import androidx.compose.ui.graphics.SolidColor import androidx.compose.ui.platform.LocalDensity +import androidx.compose.ui.platform.LocalFocusManager import androidx.compose.ui.platform.testTag import androidx.compose.ui.res.painterResource import androidx.compose.ui.res.stringResource @@ -334,6 +339,8 @@ private fun DiscriminatorField( ) { val textStyle = LocalTextStyle.current.copy(color = MaterialTheme.colorScheme.onSurface) val textMeasurer = rememberTextMeasurer() + val focusManager = LocalFocusManager.current + var wasFocused by remember { mutableStateOf(false) } val width = with(LocalDensity.current) { val content = textMeasurer.measure(state.discriminator, textStyle).size.width @@ -354,13 +361,21 @@ private fun DiscriminatorField( ), keyboardActions = KeyboardActions( onDone = { - if (state.isSubmittable) { + if (state.discriminator.isBlank()) { + focusManager.clearFocus() + } else if (state.isSubmittable) { onEvent(AddUsernameScreenEvents.NextClicked) } } ), modifier = Modifier .width(width) + .onFocusChanged { focusState -> + if (wasFocused && !focusState.isFocused) { + onEvent(AddUsernameScreenEvents.DiscriminatorFocusLost) + } + wasFocused = focusState.isFocused + } .testTag(TestTags.ADD_USERNAME_DISCRIMINATOR_FIELD) ) } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreenEvents.kt b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreenEvents.kt index 6d7caf3f48..f406e4099c 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreenEvents.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameScreenEvents.kt @@ -21,6 +21,9 @@ sealed class AddUsernameScreenEvents { override fun toString(): String = "DiscriminatorChanged(value=${value.censor()})" } + /** The discriminator field lost focus, settling whatever the user left in it. */ + data object DiscriminatorFocusLost : AddUsernameScreenEvents() + /** * Internal: the user paused typing long enough for the entered username to be validated and reserved. A null * [discriminator] means the service should assign one. diff --git a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameViewModel.kt index 67e81485ef..35a5275775 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/addusername/AddUsernameViewModel.kt @@ -17,7 +17,6 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.debounce -import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.receiveAsFlow @@ -70,7 +69,6 @@ class AddUsernameViewModel( .launchIn(viewModelScope) entryChanges - .distinctUntilChanged() .debounce(ENTRY_DEBOUNCE) .onEach { onEvent(it) } .launchIn(viewModelScope) @@ -90,6 +88,7 @@ class AddUsernameViewModel( when (event) { is AddUsernameScreenEvents.UsernameChanged -> applyUsernameChanged(state, event.value, stateEmitter) is AddUsernameScreenEvents.DiscriminatorChanged -> applyDiscriminatorChanged(state, event.value, stateEmitter) + is AddUsernameScreenEvents.DiscriminatorFocusLost -> applyDiscriminatorFocusLost(state) is AddUsernameScreenEvents.EntrySettled -> applyEntrySettled(state, event, stateEmitter) is AddUsernameScreenEvents.ReservationCompleted -> applyReservationCompleted(state, event, stateEmitter) is AddUsernameScreenEvents.LearnMoreClicked -> _actions.trySend(AddUsernameScreenActions.OpenLearnMoreArticle) @@ -124,8 +123,9 @@ class AddUsernameViewModel( } /** - * Non-digits are dropped as they're typed, since a discriminator can only ever be digits. A blank discriminator hands - * control back to the service, matching the behavior of clearing the field in the app's username editor. + * Non-digits are dropped as they're typed, since a discriminator can only ever be digits. + * + * Emptying the field hands control back to the service once the field loses focus (see [applyDiscriminatorFocusLost]). */ private fun applyDiscriminatorChanged(state: AddUsernameState, discriminator: String, stateEmitter: (AddUsernameState) -> Unit) { val digitsOnly = discriminator.filter { it in '0'..'9' } @@ -141,14 +141,31 @@ class AddUsernameViewModel( discriminator = digitsOnly, isDiscriminatorUserSet = isUserSet, validationError = null, - reservation = null, + reservation = if (isUserSet) null else state.reservation, isReserving = false ) stateEmitter(updated) - scheduleReservation(updated) + + if (isUserSet) { + scheduleReservation(updated) + } } + /** + * Reserves right away rather than through [scheduleReservation], since focus loss already means the user is done + * typing and there is nothing left to debounce. + */ + private fun applyDiscriminatorFocusLost(state: AddUsernameState) { + if (state.discriminator.isBlank() && state.username.isNotBlank()) { + onEvent(AddUsernameScreenEvents.EntrySettled(state.username, null)) + } + } + + /** + * Deliberately not de-duplicated: the entry can travel away from a pair and back to it, and those repeats still need + * a fresh reservation. + */ private fun scheduleReservation(state: AddUsernameState) { if (state.username.isNotBlank()) { entryChanges.tryEmit(AddUsernameScreenEvents.EntrySettled(state.username, state.requestedDiscriminator)) diff --git a/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameScreenTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameScreenTest.kt index 7d9f7df773..654b19b3af 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameScreenTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameScreenTest.kt @@ -12,10 +12,12 @@ import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performTextInput import androidx.compose.ui.test.performTextReplacement +import androidx.compose.ui.test.requestFocus import androidx.test.core.app.ApplicationProvider import assertk.assertThat import assertk.assertions.contains import assertk.assertions.containsExactly +import assertk.assertions.doesNotContain import assertk.assertions.isEqualTo import assertk.assertions.isNotNull import org.junit.Rule @@ -87,6 +89,37 @@ class AddUsernameScreenTest { assertThat(emittedEvent).isEqualTo(AddUsernameScreenEvents.DiscriminatorChanged("77")) } + @Test + fun `showing the discriminator field does not emit DiscriminatorFocusLost`() { + val emittedEvents = mutableListOf() + + composeTestRule.setContent { + SignalTheme { + AddUsernameScreen(state = reservedState, onEvent = { emittedEvents.add(it) }) + } + } + + composeTestRule.onNodeWithTag(TestTags.ADD_USERNAME_DISCRIMINATOR_FIELD).assertIsDisplayed() + + assertThat(emittedEvents).doesNotContain(AddUsernameScreenEvents.DiscriminatorFocusLost) + } + + @Test + fun `when the discriminator field loses focus, DiscriminatorFocusLost is emitted`() { + val emittedEvents = mutableListOf() + + composeTestRule.setContent { + SignalTheme { + AddUsernameScreen(state = reservedState, onEvent = { emittedEvents.add(it) }) + } + } + + composeTestRule.onNodeWithTag(TestTags.ADD_USERNAME_DISCRIMINATOR_FIELD).requestFocus() + composeTestRule.onNodeWithTag(TestTags.ADD_USERNAME_FIELD).requestFocus() + + assertThat(emittedEvents).contains(AddUsernameScreenEvents.DiscriminatorFocusLost) + } + @Test fun `when typing in the username field, UsernameChanged is emitted`() { var emittedEvent: AddUsernameScreenEvents? = null diff --git a/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameViewModelTest.kt index f684429846..068b715f63 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/addusername/AddUsernameViewModelTest.kt @@ -14,6 +14,7 @@ import assertk.assertions.isFalse import assertk.assertions.isNull import assertk.assertions.isTrue import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.mockk import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -138,6 +139,8 @@ class AddUsernameViewModelTest { advanceUntilIdle() viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorChanged("")) advanceUntilIdle() + viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorFocusLost) + advanceUntilIdle() assertThat(viewModel.state.value.isDiscriminatorUserSet).isFalse() assertThat(viewModel.state.value.discriminator).isEqualTo("45") @@ -175,8 +178,8 @@ class AddUsernameViewModelTest { } @Test - fun `clearing a service-assigned discriminator empties the field`() = runTest(testDispatcher) { - coEvery { mockRepository.reserveUsername("maya") } returns RequestResult.Success(Username("maya.45")) + fun `emptying the discriminator while still focused does not reserve`() = runTest(testDispatcher) { + coEvery { mockRepository.reserveUsername("maya") } returns RequestResult.Success(Username("maya.45")) andThen RequestResult.Success(Username("maya.99")) viewModel.onEvent(AddUsernameScreenEvents.UsernameChanged("maya")) advanceUntilIdle() @@ -185,8 +188,51 @@ class AddUsernameViewModelTest { assertThat(viewModel.state.value.discriminator).isEmpty() assertThat(viewModel.state.value.isDiscriminatorUserSet).isFalse() - assertThat(viewModel.state.value.requestedDiscriminator).isNull() assertThat(viewModel.state.value.validationError).isNull() + coVerify(exactly = 1) { mockRepository.reserveUsername("maya") } + } + + @Test + fun `an emptied discriminator keeps the entry submittable while the user decides`() = runTest(testDispatcher) { + coEvery { mockRepository.reserveUsername("maya") } returns RequestResult.Success(Username("maya.45")) + + viewModel.onEvent(AddUsernameScreenEvents.UsernameChanged("maya")) + advanceUntilIdle() + viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorChanged("")) + advanceUntilIdle() + + assertThat(viewModel.state.value.reservation).isEqualTo(Username("maya.45")) + assertThat(viewModel.state.value.isSubmittable).isTrue() + } + + @Test + fun `losing focus with an empty discriminator hands it back to the service`() = runTest(testDispatcher) { + coEvery { mockRepository.reserveUsername("maya") } returns RequestResult.Success(Username("maya.45")) andThen RequestResult.Success(Username("maya.99")) + + viewModel.onEvent(AddUsernameScreenEvents.UsernameChanged("maya")) + advanceUntilIdle() + viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorChanged("")) + advanceUntilIdle() + viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorFocusLost) + advanceUntilIdle() + + assertThat(viewModel.state.value.discriminator).isEqualTo("99") + assertThat(viewModel.state.value.isDiscriminatorUserSet).isFalse() + assertThat(viewModel.state.value.reservation).isEqualTo(Username("maya.99")) + assertThat(viewModel.state.value.isSubmittable).isTrue() + } + + @Test + fun `losing focus with a discriminator still in the field reserves nothing new`() = runTest(testDispatcher) { + coEvery { mockRepository.reserveUsername("maya") } returns RequestResult.Success(Username("maya.45")) + + viewModel.onEvent(AddUsernameScreenEvents.UsernameChanged("maya")) + advanceUntilIdle() + viewModel.onEvent(AddUsernameScreenEvents.DiscriminatorFocusLost) + advanceUntilIdle() + + assertThat(viewModel.state.value.discriminator).isEqualTo("45") + coVerify(exactly = 1) { mockRepository.reserveUsername("maya") } } @Test