From acc9a39bb15a90eeb623988444cde8e8f59ca063 Mon Sep 17 00:00:00 2001 From: Ida Delphine Date: Mon, 19 Oct 2026 01:16:06 -0700 Subject: [PATCH] add download tests and fix bugs --- .../platform/AndroidDownloadStorageActions.kt | 4 - .../app/composable/SwitchPreference.kt | 29 +++---- .../screen/settings/DownloadSettingsScreen.kt | 13 ++- .../settings/DownloadSettingsViewModel.kt | 46 +++++++--- .../app/composable/SwitchPreferenceTest.kt | 18 ++++ .../settings/DebugSettingsScreenTest.kt | 6 +- .../settings/DownloadSettingsScreenTest.kt | 84 +++++++++++++++++++ 7 files changed, 165 insertions(+), 35 deletions(-) create mode 100644 shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreenTest.kt diff --git a/shared/src/androidMain/kotlin/net/newpipe/app/platform/AndroidDownloadStorageActions.kt b/shared/src/androidMain/kotlin/net/newpipe/app/platform/AndroidDownloadStorageActions.kt index b7b59d476..3b9a12ed5 100644 --- a/shared/src/androidMain/kotlin/net/newpipe/app/platform/AndroidDownloadStorageActions.kt +++ b/shared/src/androidMain/kotlin/net/newpipe/app/platform/AndroidDownloadStorageActions.kt @@ -47,7 +47,6 @@ class AndroidDownloadStorageActions(context: Context) : DownloadStorageActions { if (folder == Uri.EMPTY) { return null } - // Plain file paths read better without their scheme; SAF trees have no readable path. return when (folder.scheme) { ContentResolver.SCHEME_FILE -> folder.path else -> folder.toString() @@ -56,13 +55,10 @@ class AndroidDownloadStorageActions(context: Context) : DownloadStorageActions { @Composable override fun rememberFolderPicker(onResult: (FolderPickResult) -> Unit): () -> Unit { - // The screen passes a fresh lambda on every recomposition, so read it through a state - // holder instead of capturing the first one inside remember below. val currentOnResult by rememberUpdatedState(onResult) val launcher = rememberLauncherForActivityResult(StartActivityForResult()) { result -> if (result.resultCode != Activity.RESULT_OK) { - // The user backed out of the picker; the legacy screen stays silent too. return@rememberLauncherForActivityResult } val folder = result.data?.data diff --git a/shared/src/commonMain/kotlin/net/newpipe/app/composable/SwitchPreference.kt b/shared/src/commonMain/kotlin/net/newpipe/app/composable/SwitchPreference.kt index f252567f3..d420ed94a 100644 --- a/shared/src/commonMain/kotlin/net/newpipe/app/composable/SwitchPreference.kt +++ b/shared/src/commonMain/kotlin/net/newpipe/app/composable/SwitchPreference.kt @@ -16,14 +16,14 @@ import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.alpha import androidx.compose.ui.semantics.Role -import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.semantics.clearAndSetSemantics import androidx.compose.ui.tooling.preview.PreviewLightDark import androidx.compose.ui.tooling.preview.PreviewWrapper import androidx.compose.ui.unit.dp import net.newpipe.app.preview.ThemePreviewProvider private const val DISABLED_ALPHA = 0.38f -private val TRAILING_CONTENT_END_PADDING = 16.dp +private val SWITCH_END_PADDING = 16.dp /** * A preference row with a trailing switch, mirroring AndroidX SwitchPreferenceCompat. @@ -45,26 +45,27 @@ fun SwitchPreference( enabled: Boolean = true ) { Row( - modifier = modifier - .alpha(if (enabled) 1f else DISABLED_ALPHA) - .toggleable( - value = checked, - enabled = enabled, - role = Role.Switch, - onValueChange = onCheckedChange - ) - .semantics(mergeDescendants = true) {}, + modifier = modifier.alpha(if (enabled) 1f else DISABLED_ALPHA), verticalAlignment = Alignment.CenterVertically ) { ListItem( - modifier = Modifier.weight(1f), + modifier = Modifier + .weight(1f) + .toggleable( + value = checked, + enabled = enabled, + role = Role.Switch, + onValueChange = onCheckedChange + ), headlineContent = { Text(text = title) }, supportingContent = summary?.let { text -> { Text(text = text) } } ) Switch( - modifier = Modifier.padding(end = TRAILING_CONTENT_END_PADDING), + modifier = Modifier + .padding(end = SWITCH_END_PADDING) + .clearAndSetSemantics {}, checked = checked, - onCheckedChange = null, + onCheckedChange = onCheckedChange, enabled = enabled ) } diff --git a/shared/src/commonMain/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreen.kt b/shared/src/commonMain/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreen.kt index 5d75693f4..f88bcb03e 100644 --- a/shared/src/commonMain/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreen.kt +++ b/shared/src/commonMain/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreen.kt @@ -46,6 +46,7 @@ import net.newpipe.app.preferences.DownloadPreferences.DEFAULT_PAUSE_ON_METERED_ import net.newpipe.app.preferences.DownloadPreferences.DEFAULT_REPLACEMENT_CHARACTER import net.newpipe.app.preferences.DownloadPreferences.DEFAULT_STORAGE_ASK import net.newpipe.app.preferences.DownloadPreferences.DEFAULT_STORAGE_USE_SAF +import net.newpipe.app.preferences.DownloadPreferences.MAXIMUM_RETRIES_NONE import net.newpipe.app.preferences.DownloadPreferences.MAXIMUM_RETRY_COUNTS import net.newpipe.app.preview.ThemePreviewProvider import net.newpipe.app.viewmodel.settings.DownloadSettingsViewModel @@ -71,11 +72,11 @@ import newpipe.shared.generated.resources.general_error import newpipe.shared.generated.resources.invalid_directory import newpipe.shared.generated.resources.max_retry_desc import newpipe.shared.generated.resources.max_retry_msg -import newpipe.shared.generated.resources.minimize_on_exit_none_description import newpipe.shared.generated.resources.no_app_to_open_intent import newpipe.shared.generated.resources.no_appropriate_file_manager_message import newpipe.shared.generated.resources.no_appropriate_file_manager_message_android_10 import newpipe.shared.generated.resources.no_available_dir +import newpipe.shared.generated.resources.none import newpipe.shared.generated.resources.ok import newpipe.shared.generated.resources.pause_downloads_on_mobile import newpipe.shared.generated.resources.pause_downloads_on_mobile_desc @@ -187,9 +188,13 @@ fun DownloadSettingsScreenContent( ) ) - val noRetriesLabel = stringResource(Res.string.minimize_on_exit_none_description) - val maximumRetryEntries = listOf(ListPreferenceEntry(noRetriesLabel, noRetriesLabel)) + - MAXIMUM_RETRY_COUNTS.map { count -> ListPreferenceEntry(count, count) } + val noRetriesLabel = stringResource(Res.string.none) + val maximumRetryEntries = MAXIMUM_RETRY_COUNTS.map { count -> + ListPreferenceEntry( + value = count, + title = if (count == MAXIMUM_RETRIES_NONE) noRetriesLabel else count + ) + } val storageAskSummary = when { storageAccessFrameworkConfigurable -> diff --git a/shared/src/commonMain/kotlin/net/newpipe/app/viewmodel/settings/DownloadSettingsViewModel.kt b/shared/src/commonMain/kotlin/net/newpipe/app/viewmodel/settings/DownloadSettingsViewModel.kt index abd1955ba..dbe6555d5 100644 --- a/shared/src/commonMain/kotlin/net/newpipe/app/viewmodel/settings/DownloadSettingsViewModel.kt +++ b/shared/src/commonMain/kotlin/net/newpipe/app/viewmodel/settings/DownloadSettingsViewModel.kt @@ -26,6 +26,7 @@ import net.newpipe.app.preferences.DownloadPreferences.KEY_PAUSE_ON_METERED_NETW import net.newpipe.app.preferences.DownloadPreferences.KEY_REPLACEMENT_CHARACTER import net.newpipe.app.preferences.DownloadPreferences.KEY_STORAGE_ASK import net.newpipe.app.preferences.DownloadPreferences.KEY_STORAGE_USE_SAF +import net.newpipe.app.preferences.DownloadPreferences.normalizedMaximumRetries import org.koin.core.annotation.KoinViewModel @KoinViewModel @@ -53,15 +54,19 @@ class DownloadSettingsViewModel( field = stringFlow(KEY_REPLACEMENT_CHARACTER, DEFAULT_REPLACEMENT_CHARACTER) val maximumRetries: StateFlow - field = stringFlow(KEY_MAXIMUM_RETRIES, DEFAULT_MAXIMUM_RETRIES) + field = MutableStateFlow(repairedMaximumRetries()) val pauseOnMeteredNetwork: StateFlow - field = booleanFlow(KEY_PAUSE_ON_METERED_NETWORK, DEFAULT_PAUSE_ON_METERED_NETWORK) + field = booleanFlow( + KEY_PAUSE_ON_METERED_NETWORK, + DEFAULT_PAUSE_ON_METERED_NETWORK + ) val limitQueue: StateFlow field = booleanFlow(KEY_LIMIT_QUEUE, DEFAULT_LIMIT_QUEUE) - fun setStorageAsk(value: Boolean) = storageAsk.persist(KEY_STORAGE_ASK, value) + fun setStorageAsk(value: Boolean) = storageAsk + .persist(KEY_STORAGE_ASK, value) fun setUseStorageAccessFramework(value: Boolean) { useStorageAccessFramework.persist(KEY_STORAGE_USE_SAF, value) @@ -81,23 +86,33 @@ class DownloadSettingsViewModel( } } - fun setVideoDownloadFolder(value: String) = videoDownloadFolder.persistFolder(KEY_DOWNLOAD_FOLDER_VIDEO, value) + fun setVideoDownloadFolder(value: String) = videoDownloadFolder + .persistFolder(KEY_DOWNLOAD_FOLDER_VIDEO, value) - fun setAudioDownloadFolder(value: String) = audioDownloadFolder.persistFolder(KEY_DOWNLOAD_FOLDER_AUDIO, value) + fun setAudioDownloadFolder(value: String) = audioDownloadFolder + .persistFolder(KEY_DOWNLOAD_FOLDER_AUDIO, value) - fun setFileCharset(value: String) = fileCharset.persist(KEY_FILE_CHARSET, value) + fun setFileCharset(value: String) = fileCharset + .persist(KEY_FILE_CHARSET, value) - fun setReplacementCharacter(value: String) = replacementCharacter.persist(KEY_REPLACEMENT_CHARACTER, value) + fun setReplacementCharacter(value: String) = replacementCharacter + .persist(KEY_REPLACEMENT_CHARACTER, value) - fun setMaximumRetries(value: String) = maximumRetries.persist(KEY_MAXIMUM_RETRIES, value) + fun setMaximumRetries(value: String) = maximumRetries + .persist(KEY_MAXIMUM_RETRIES, value) - fun setPauseOnMeteredNetwork(value: Boolean) = pauseOnMeteredNetwork.persist(KEY_PAUSE_ON_METERED_NETWORK, value) + fun setPauseOnMeteredNetwork(value: Boolean) = pauseOnMeteredNetwork + .persist(KEY_PAUSE_ON_METERED_NETWORK, value) fun setLimitQueue(value: Boolean) = limitQueue.persist(KEY_LIMIT_QUEUE, value) - private fun stringFlow(key: String, defaultValue: String) = MutableStateFlow(settings.getString(key, defaultValue)) + private fun stringFlow(key: String, defaultValue: String) = MutableStateFlow( + settings.getString(key, defaultValue) + ) - private fun booleanFlow(key: String, defaultValue: Boolean) = MutableStateFlow(settings.getBoolean(key, defaultValue)) + private fun booleanFlow(key: String, defaultValue: Boolean) = MutableStateFlow( + settings.getBoolean(key, defaultValue) + ) private fun MutableStateFlow.persist(key: String, newValue: String) { settings.putString(key, newValue) @@ -117,4 +132,13 @@ class DownloadSettingsViewModel( } value = newValue } + + private fun repairedMaximumRetries(): String { + val stored = settings.getString(KEY_MAXIMUM_RETRIES, DEFAULT_MAXIMUM_RETRIES) + val repaired = normalizedMaximumRetries(stored) + if (repaired != stored) { + settings.putString(KEY_MAXIMUM_RETRIES, repaired) + } + return repaired + } } diff --git a/shared/src/commonTest/kotlin/net/newpipe/app/composable/SwitchPreferenceTest.kt b/shared/src/commonTest/kotlin/net/newpipe/app/composable/SwitchPreferenceTest.kt index 00ea451c4..ceb9bbeb1 100644 --- a/shared/src/commonTest/kotlin/net/newpipe/app/composable/SwitchPreferenceTest.kt +++ b/shared/src/commonTest/kotlin/net/newpipe/app/composable/SwitchPreferenceTest.kt @@ -6,6 +6,7 @@ package net.newpipe.app.composable import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsNotEnabled import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.v2.runComposeUiTest @@ -44,4 +45,21 @@ class SwitchPreferenceTest { onNodeWithText(title).performClick() assertFalse(fired) } + + @Test + fun disabledRowReportsItsStateOnTheTitle() = runComposeUiTest { + val title = "Enable feature" + setContent { + SwitchPreference( + title = title, + summary = "Some explanation", + checked = false, + onCheckedChange = {}, + enabled = false + ) + } + + // The title and the switch state must be one node + onNodeWithText(title).assertIsNotEnabled() + } } diff --git a/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DebugSettingsScreenTest.kt b/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DebugSettingsScreenTest.kt index 5cf313f79..16e98e68b 100644 --- a/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DebugSettingsScreenTest.kt +++ b/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DebugSettingsScreenTest.kt @@ -6,7 +6,9 @@ package net.newpipe.app.screen.settings import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertCountEquals import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo @@ -53,8 +55,8 @@ class DebugSettingsScreenTest { modules = listOf(emptySettings), content = { DebugSettingsScreenContent(leakCanaryAvailable = false) }, onContent = { - onNodeWithText(getString(Res.string.leak_canary_not_available)) - .assertIsDisplayed() + onAllNodesWithText(getString(Res.string.leak_canary_not_available)) + .assertCountEquals(2) onNodeWithText(getString(Res.string.enable_leak_canary_summary)) .assertDoesNotExist() } diff --git a/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreenTest.kt b/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreenTest.kt new file mode 100644 index 000000000..846c623ac --- /dev/null +++ b/shared/src/commonTest/kotlin/net/newpipe/app/screen/settings/DownloadSettingsScreenTest.kt @@ -0,0 +1,84 @@ +/* +* SPDX-FileCopyrightText: 2026 NewPipe e.V. +* SPDX-License-Identifier: GPL-3.0-or-later +*/ + +package net.newpipe.app.screen.settings + +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertIsNotEnabled +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.v2.runComposeUiTest +import com.russhwolf.settings.MapSettings +import com.russhwolf.settings.Settings +import kotlin.test.Test +import net.newpipe.app.extensions.withKoin +import newpipe.shared.generated.resources.Res +import newpipe.shared.generated.resources.download_path_summary +import newpipe.shared.generated.resources.download_path_title +import newpipe.shared.generated.resources.downloads_storage_use_saf_summary_api_29 +import newpipe.shared.generated.resources.downloads_storage_use_saf_title +import newpipe.shared.generated.resources.max_retry_desc +import newpipe.shared.generated.resources.settings_category_downloads_title +import org.jetbrains.compose.resources.getString +import org.koin.dsl.module + +@OptIn(ExperimentalTestApi::class) +class DownloadSettingsScreenTest { + + private val emptySettings = module { + single { MapSettings() } + } + + @Test + fun rendersTitleAndUnsetDownloadFolder() = runComposeUiTest { + withKoin( + modules = listOf(emptySettings), + content = { DownloadSettingsScreenContent() }, + onContent = { + onNodeWithText(getString(Res.string.settings_category_downloads_title)) + .assertIsDisplayed() + onNodeWithText(getString(Res.string.download_path_summary)).assertIsDisplayed() + } + ) + } + + @Test + fun downloadFolderIsDisabledWhileEveryDownloadAsks() = runComposeUiTest { + withKoin( + modules = listOf(emptySettings), + content = { DownloadSettingsScreenContent(storageAsk = true) }, + onContent = { + onNodeWithText(getString(Res.string.download_path_title)).assertIsNotEnabled() + } + ) + } + + @Test + fun storageAccessFrameworkIsLockedWhenNotConfigurable() = runComposeUiTest { + withKoin( + modules = listOf(emptySettings), + content = { + DownloadSettingsScreenContent(storageAccessFrameworkConfigurable = false) + }, + onContent = { + onNodeWithText(getString(Res.string.downloads_storage_use_saf_summary_api_29)) + .assertIsDisplayed() + onNodeWithText(getString(Res.string.downloads_storage_use_saf_title)) + .assertIsNotEnabled() + } + ) + } + + @Test + fun maximumRetriesKeepsItsStaticSummary() = runComposeUiTest { + withKoin( + modules = listOf(emptySettings), + content = { DownloadSettingsScreenContent() }, + onContent = { + onNodeWithText(getString(Res.string.max_retry_desc)).assertIsDisplayed() + } + ) + } +}