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.
2026-03-13 09:01:28 +08:00 · 2023-04-23 12:32:55 +01:00
parent 4427788489
commit 0f3c1aafc4
9 changed files with 167 additions and 60 deletions
--- 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
--- 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
@@ -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<AutofillType>,
+    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)
+                }
+            }
+        }
+}
--- 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) }
+            )
    )
 }

--- 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
+                                )
+                            }
                        }
                    }
                }
--- 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()) }) {
--- 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(
--- 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
--- 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
 }
--- 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<AccessTokenRepository>()
+    private val mockAuthCaster = mockk<AuthCaster>()
+    private lateinit var testObject: AuthRefreshInterceptor
+
+    @Before
+    fun setup() {
+        clearAllMocks()
+
+        testObject = AuthRefreshInterceptor(mockRepo, mockAuthCaster)
+    }
+
    @Test
    fun `successful response just returns response`() = runBlocking {
-        val mockRepo = mockk<AccessTokenRepository>()
-        val mockAuthCaster = mockk<AuthCaster>()
        val response = mockk<Response> {
            every { isSuccessful } returns true
+            every { code() } returns 200
        }
        val mockChain = mockk<Interceptor.Chain> {
            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<AccessTokenRepository>()
-        val mockAuthCaster = mockk<AuthCaster>()
        val response = mockk<Response> {
            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<AccessTokenRepository> {
-            coEvery { refresh() } returns AccessTokenResult.CommunicationError
-        }
-        val mockAuthCaster = mockk<AuthCaster> {
-            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<Response> {
            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<Response> {
+            every { isSuccessful } returns false
+            every { code() } returns 403
+            every { request() } returns mockk()
+            every { protocol() } returns mockk()
+            every { message() } returns "message"
+        }
+        val mockChain = mockk<Interceptor.Chain> {
+            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<Response> {
+            every { isSuccessful } returns false
+            every { code() } returns 401
+            every { request() } returns mockk()
+            every { protocol() } returns mockk()
+            every { message() } returns "message"
+        }
+        val mockChain = mockk<Interceptor.Chain> {
+            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<AccessTokenRepository> {
-            coEvery { refresh() } returns AccessTokenResult.CommunicationError
-        }
-        val mockAuthCaster = mockk<AuthCaster> {
-            every { issueRefreshingToken() } just Runs
-        }
+        coEvery { mockRepo.refresh() } returns AccessTokenResult.CommunicationError
+        every { mockAuthCaster.issueRefreshingToken() } just Runs
        val response = mockk<Response> {
            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<AccessTokenRepository> {
+        mockRepo.apply {
            every { accessToken } returns "accessToken"
            coEvery { refresh() } returns AccessTokenResult.Success
        }
-        val mockAuthCaster = mockk<AuthCaster>()
        val response = mockk<Response> {
            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)