From 0f3c1aafc4d1312688c3cce4bc3422a226f1b9f2 Mon Sep 17 00:00:00 2001 From: Troy Rijkaard Date: Sun, 23 Apr 2023 12:32:55 +0100 Subject: [PATCH] fix: ui issues in compose (#970) * fix: clickable area incorrect in settings In the settings screen the clickable area is too small, fix the order of modifiers to fix this. * fix: hide the plus one icon on completed series * fix: rating dialog not taking material colours * fix: throw auth error if we get a 401 If we get a 401 during normal operation, then attempt to refresh and throw an error if we cant. * feat: add autofill to the login screen Add a new extension method to allow autofill and use this on the credentials screen. * style: fix ktlint issues * chore: add gitignore line for deployment targets * style: fix code style difference * fix: incorrect clickable height on rating preference * fix: whole line not selectable in config Update padding to include the whole line in the config screen. * fix: rating dialog buttons in wrong location Move the dialog buttons to be end aligned like the other dialogs. --- .gitignore | 1 + .../nekome/core/compose/ModifierExtensions.kt | 39 ++++++++ .../login/credentials/ui/CredentialsScreen.kt | 20 ++-- .../series/collection/ui/CollectionScreen.kt | 32 ++++--- .../app/series/collection/ui/RatingDialog.kt | 15 +-- .../app/settings/config/ui/ConfigScreen.kt | 15 ++- libraries/datasource/auth/build.gradle | 1 + .../auth/remote/AuthRefreshInterceptor.kt | 10 +- .../remote/AuthRefreshInterceptorTests.kt | 94 ++++++++++++++----- 9 files changed, 167 insertions(+), 60 deletions(-) create mode 100644 core/compose/src/main/java/com/chesire/nekome/core/compose/ModifierExtensions.kt diff --git a/.gitignore b/.gitignore index 944cce4f..71a557a7 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,7 @@ captures/ *.iml .idea/assetWizardSettings.xml .idea/androidTestResultsUserPreferences.xml +.idea/deploymentTargetDropDown.xml .idea/workspace.xml .idea/tasks.xml .idea/gradle.xml diff --git a/core/compose/src/main/java/com/chesire/nekome/core/compose/ModifierExtensions.kt b/core/compose/src/main/java/com/chesire/nekome/core/compose/ModifierExtensions.kt new file mode 100644 index 00000000..704852e1 --- /dev/null +++ b/core/compose/src/main/java/com/chesire/nekome/core/compose/ModifierExtensions.kt @@ -0,0 +1,39 @@ +@file:OptIn(ExperimentalComposeUiApi::class) + +package com.chesire.nekome.core.compose + +import androidx.compose.ui.ExperimentalComposeUiApi +import androidx.compose.ui.Modifier +import androidx.compose.ui.autofill.AutofillNode +import androidx.compose.ui.autofill.AutofillType +import androidx.compose.ui.composed +import androidx.compose.ui.focus.onFocusChanged +import androidx.compose.ui.layout.boundsInWindow +import androidx.compose.ui.layout.onGloballyPositioned +import androidx.compose.ui.platform.LocalAutofill +import androidx.compose.ui.platform.LocalAutofillTree + +/** + * Provides autofill to a view. + * This should be removed once compose completely supports autofill nicely. + */ +fun Modifier.autofill( + autofillTypes: List, + onFill: ((String) -> Unit) +) = composed { + val autofill = LocalAutofill.current + val autofillNode = AutofillNode(onFill = onFill, autofillTypes = autofillTypes) + LocalAutofillTree.current += autofillNode + + this + .onGloballyPositioned { autofillNode.boundingBox = it.boundsInWindow() } + .onFocusChanged { focusState -> + autofill?.run { + if (focusState.isFocused) { + requestAutofillForNode(autofillNode) + } else { + cancelAutofillForNode(autofillNode) + } + } + } +} diff --git a/features/login/src/main/java/com/chesire/nekome/app/login/credentials/ui/CredentialsScreen.kt b/features/login/src/main/java/com/chesire/nekome/app/login/credentials/ui/CredentialsScreen.kt index e64115a0..2989c782 100644 --- a/features/login/src/main/java/com/chesire/nekome/app/login/credentials/ui/CredentialsScreen.kt +++ b/features/login/src/main/java/com/chesire/nekome/app/login/credentials/ui/CredentialsScreen.kt @@ -41,6 +41,7 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.ExperimentalComposeUiApi import androidx.compose.ui.Modifier +import androidx.compose.ui.autofill.AutofillType import androidx.compose.ui.focus.FocusDirection import androidx.compose.ui.layout.ContentScale import androidx.compose.ui.platform.LocalFocusManager @@ -58,6 +59,7 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.hilt.navigation.compose.hiltViewModel import com.chesire.nekome.app.login.R +import com.chesire.nekome.core.compose.autofill import com.chesire.nekome.core.compose.theme.NekomeTheme @Composable @@ -193,9 +195,12 @@ private fun UsernameInput( isError = isUsernameError, singleLine = true, label = { Text(text = stringResource(id = R.string.login_username)) }, - modifier = Modifier.semantics { - testTag = CredentialsTags.Username - } + modifier = Modifier + .semantics { testTag = CredentialsTags.Username } + .autofill( + autofillTypes = listOf(AutofillType.EmailAddress, AutofillType.Username), + onFill = { onUsernameChanged(it) } + ) ) } @@ -248,9 +253,12 @@ private fun PasswordInput( isError = isPasswordError, singleLine = true, label = { Text(text = stringResource(id = R.string.login_password)) }, - modifier = Modifier.semantics { - testTag = CredentialsTags.Password - } + modifier = Modifier + .semantics { testTag = CredentialsTags.Password } + .autofill( + autofillTypes = listOf(AutofillType.Password), + onFill = { onPasswordChanged(it) } + ) ) } diff --git a/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/CollectionScreen.kt b/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/CollectionScreen.kt index 045d9af7..9c01097a 100644 --- a/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/CollectionScreen.kt +++ b/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/CollectionScreen.kt @@ -322,21 +322,23 @@ private fun SeriesItem( style = MaterialTheme.typography.bodyMedium, modifier = Modifier.align(Alignment.CenterVertically) ) - IconButton( - modifier = Modifier - .alpha(if (model.isUpdating) 0.3f else 1f) - .align(Alignment.CenterVertically) - .semantics { testTag = SeriesCollectionTags.PlusOne }, - enabled = !model.isUpdating, - onClick = { onIncrementSeries(model) } - ) { - Icon( - imageVector = Icons.Default.PlusOne, - contentDescription = stringResource( - id = R.string.series_list_plus_one - ), - tint = MaterialTheme.colorScheme.primary - ) + if (model.showPlusOne) { + IconButton( + modifier = Modifier + .alpha(if (model.isUpdating) 0.3f else 1f) + .align(Alignment.CenterVertically) + .semantics { testTag = SeriesCollectionTags.PlusOne }, + enabled = !model.isUpdating, + onClick = { onIncrementSeries(model) } + ) { + Icon( + imageVector = Icons.Default.PlusOne, + contentDescription = stringResource( + id = R.string.series_list_plus_one + ), + tint = MaterialTheme.colorScheme.primary + ) + } } } } diff --git a/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/RatingDialog.kt b/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/RatingDialog.kt index 2ff9cc6e..5bc8de98 100644 --- a/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/RatingDialog.kt +++ b/features/series/src/main/java/com/chesire/nekome/app/series/collection/ui/RatingDialog.kt @@ -5,10 +5,10 @@ import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding -import androidx.compose.material.Card -import androidx.compose.material.Slider -import androidx.compose.material.Text -import androidx.compose.material.TextButton +import androidx.compose.material3.Card +import androidx.compose.material3.Slider +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -79,9 +79,12 @@ private fun Render( Row( modifier = Modifier.fillMaxWidth(), - horizontalArrangement = Arrangement.SpaceEvenly + horizontalArrangement = Arrangement.End ) { - TextButton(onClick = { onRatingResult(null) }) { + TextButton( + modifier = Modifier.padding(horizontal = 8.dp), + onClick = { onRatingResult(null) } + ) { Text(text = stringResource(id = R.string.series_list_rate_cancel)) } TextButton(onClick = { onRatingResult(sliderValue.roundToInt()) }) { diff --git a/features/settings/src/main/java/com/chesire/nekome/app/settings/config/ui/ConfigScreen.kt b/features/settings/src/main/java/com/chesire/nekome/app/settings/config/ui/ConfigScreen.kt index 927af938..787623c0 100644 --- a/features/settings/src/main/java/com/chesire/nekome/app/settings/config/ui/ConfigScreen.kt +++ b/features/settings/src/main/java/com/chesire/nekome/app/settings/config/ui/ConfigScreen.kt @@ -100,7 +100,7 @@ private fun Render( modifier = Modifier .padding(paddingValues) .verticalScroll(state = scrollableState) - .padding(start = 16.dp, top = 16.dp, end = 16.dp) + .padding(top = 16.dp) .fillMaxSize() ) { ProfileSection(state.value.userModel, onLogoutClicked) @@ -179,6 +179,7 @@ private fun ProfileSection( if (userModel != null) { Card( modifier = Modifier + .padding(horizontal = 16.dp) .fillMaxWidth() .wrapContentHeight() ) { @@ -259,11 +260,9 @@ private fun RateSeriesPreference( ) { Row( modifier = Modifier - .padding(vertical = 8.dp) .fillMaxWidth() - .clickable { - onRateSeriesClicked(!shouldRateSeries) - }, + .clickable { onRateSeriesClicked(!shouldRateSeries) } + .padding(horizontal = 16.dp, vertical = 8.dp), horizontalArrangement = Arrangement.SpaceBetween, verticalAlignment = Alignment.CenterVertically ) { @@ -325,7 +324,7 @@ private fun PreferenceHeading(title: String) { text = title, style = MaterialTheme.typography.titleLarge, modifier = Modifier - .padding(top = 16.dp) + .padding(start = 16.dp, top = 16.dp, end = 16.dp) .fillMaxWidth(), color = MaterialTheme.colorScheme.primary ) @@ -339,9 +338,9 @@ private fun PreferenceSection( ) { Column( modifier = Modifier - .padding(vertical = 8.dp) .fillMaxWidth() - .clickable(enabled = onClick != null) { onClick?.invoke() }, + .clickable(enabled = onClick != null) { onClick?.invoke() } + .padding(horizontal = 16.dp, vertical = 8.dp), verticalArrangement = Arrangement.Center ) { Text( diff --git a/libraries/datasource/auth/build.gradle b/libraries/datasource/auth/build.gradle index bed2b242..64cbcfc5 100644 --- a/libraries/datasource/auth/build.gradle +++ b/libraries/datasource/auth/build.gradle @@ -33,6 +33,7 @@ dependencies { implementation libs.kotlin.coroutines.core implementation libs.kotlin.result implementation libs.squareup.retrofit2 + implementation libs.timber testImplementation project(":testing") testImplementation libs.junit diff --git a/libraries/datasource/auth/src/main/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptor.kt b/libraries/datasource/auth/src/main/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptor.kt index 9dc202b4..67c12dd1 100644 --- a/libraries/datasource/auth/src/main/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptor.kt +++ b/libraries/datasource/auth/src/main/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptor.kt @@ -9,6 +9,7 @@ import javax.inject.Inject import kotlinx.coroutines.runBlocking import okhttp3.Interceptor import okhttp3.Response +import timber.log.Timber /** * Interceptor to handle refreshing access tokens if required. @@ -29,7 +30,8 @@ class AuthRefreshInterceptor @Inject constructor( val originRequest = chain.request() val response = chain.proceed(originRequest) - return if (!response.isSuccessful && response.code() == HttpURLConnection.HTTP_FORBIDDEN) { + return if (response.isAuthError) { + Timber.w("Response threw an auth error (${response.code()}), attempting to refresh") val refreshResponse = runBlocking { repo.refresh() } if (refreshResponse is AccessTokenResult.Success) { chain.proceed( @@ -39,6 +41,7 @@ class AuthRefreshInterceptor @Inject constructor( .build() ) } else { + Timber.w("Could not refresh the token, logging user out") authCaster.issueRefreshingToken() throw AuthException() } @@ -46,4 +49,9 @@ class AuthRefreshInterceptor @Inject constructor( response } } + + private val Response.isAuthError: Boolean + get() = !isSuccessful && + code() == HttpURLConnection.HTTP_FORBIDDEN || + code() == HttpURLConnection.HTTP_UNAUTHORIZED } diff --git a/libraries/datasource/auth/src/test/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptorTests.kt b/libraries/datasource/auth/src/test/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptorTests.kt index 1f68e515..ccee8f2e 100644 --- a/libraries/datasource/auth/src/test/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptorTests.kt +++ b/libraries/datasource/auth/src/test/java/com/chesire/nekome/datasource/auth/remote/AuthRefreshInterceptorTests.kt @@ -5,6 +5,7 @@ import com.chesire.nekome.datasource.auth.AccessTokenRepository import com.chesire.nekome.datasource.auth.AccessTokenResult import com.chesire.nekome.datasource.auth.AuthException import io.mockk.Runs +import io.mockk.clearAllMocks import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every @@ -17,23 +18,33 @@ import okhttp3.Response import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Assert.fail +import org.junit.Before import org.junit.Test @Suppress("SwallowedException") class AuthRefreshInterceptorTests { + private val mockRepo = mockk() + private val mockAuthCaster = mockk() + private lateinit var testObject: AuthRefreshInterceptor + + @Before + fun setup() { + clearAllMocks() + + testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) + } + @Test fun `successful response just returns response`() = runBlocking { - val mockRepo = mockk() - val mockAuthCaster = mockk() val response = mockk { every { isSuccessful } returns true + every { code() } returns 200 } val mockChain = mockk { every { request() } returns mockk() every { proceed(any()) } returns response } - val testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) val result = testObject.intercept(mockChain) @@ -43,8 +54,6 @@ class AuthRefreshInterceptorTests { @Test fun `failure response with code !403 just returns response`() = runBlocking { - val mockRepo = mockk() - val mockAuthCaster = mockk() val response = mockk { every { isSuccessful } returns false every { code() } returns 404 @@ -53,7 +62,6 @@ class AuthRefreshInterceptorTests { every { request() } returns mockk() every { proceed(any()) } returns response } - val testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) val result = testObject.intercept(mockChain) @@ -62,13 +70,9 @@ class AuthRefreshInterceptorTests { } @Test - fun `getting new auth failure, notifies authCaster issue refreshing`() = runBlocking { - val mockRepo = mockk { - coEvery { refresh() } returns AccessTokenResult.CommunicationError - } - val mockAuthCaster = mockk { - every { issueRefreshingToken() } just Runs - } + fun `getting 401 failure, attempts to refresh token`() = runBlocking { + coEvery { mockRepo.refresh() } returns AccessTokenResult.CommunicationError + every { mockAuthCaster.issueRefreshingToken() } just Runs val response = mockk { every { isSuccessful } returns false every { code() } returns 403 @@ -80,7 +84,56 @@ class AuthRefreshInterceptorTests { every { request() } returns mockk() every { proceed(any()) } returns response } - val testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) + + try { + testObject.intercept(mockChain) + } catch (ex: AuthException) { + // Ignore the crash + } + + coVerify(exactly = 1) { mockRepo.refresh() } + } + + @Test + fun `getting 403 failure, attempts to refresh token`() = runBlocking { + coEvery { mockRepo.refresh() } returns AccessTokenResult.CommunicationError + every { mockAuthCaster.issueRefreshingToken() } just Runs + val response = mockk { + every { isSuccessful } returns false + every { code() } returns 403 + every { request() } returns mockk() + every { protocol() } returns mockk() + every { message() } returns "message" + } + val mockChain = mockk { + every { request() } returns mockk() + every { proceed(any()) } returns response + } + + try { + testObject.intercept(mockChain) + } catch (ex: AuthException) { + // Ignore the crash + } + + coVerify(exactly = 1) { mockRepo.refresh() } + } + + @Test + fun `getting new auth failure, notifies authCaster issue refreshing`() = runBlocking { + coEvery { mockRepo.refresh() } returns AccessTokenResult.CommunicationError + every { mockAuthCaster.issueRefreshingToken() } just Runs + val response = mockk { + every { isSuccessful } returns false + every { code() } returns 401 + every { request() } returns mockk() + every { protocol() } returns mockk() + every { message() } returns "message" + } + val mockChain = mockk { + every { request() } returns mockk() + every { proceed(any()) } returns response + } try { testObject.intercept(mockChain) @@ -94,12 +147,8 @@ class AuthRefreshInterceptorTests { @Test(expected = AuthException::class) fun `getting new auth failure, throws AuthException`() = runBlocking { - val mockRepo = mockk { - coEvery { refresh() } returns AccessTokenResult.CommunicationError - } - val mockAuthCaster = mockk { - every { issueRefreshingToken() } just Runs - } + coEvery { mockRepo.refresh() } returns AccessTokenResult.CommunicationError + every { mockAuthCaster.issueRefreshingToken() } just Runs val response = mockk { every { isSuccessful } returns false every { code() } returns 403 @@ -111,7 +160,6 @@ class AuthRefreshInterceptorTests { every { request() } returns mockk() every { proceed(any()) } returns response } - val testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) testObject.intercept(mockChain) @@ -120,11 +168,10 @@ class AuthRefreshInterceptorTests { @Test fun `getting new auth retries previous request`() = runBlocking { - val mockRepo = mockk { + mockRepo.apply { every { accessToken } returns "accessToken" coEvery { refresh() } returns AccessTokenResult.Success } - val mockAuthCaster = mockk() val response = mockk { every { isSuccessful } returns false every { code() } returns 403 @@ -136,7 +183,6 @@ class AuthRefreshInterceptorTests { every { request() } returns mockk(relaxed = true) every { proceed(any()) } returns response } - val testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster) testObject.intercept(mockChain)