From 48d9e80e2059b5e33d4aa8efeb623a1877c99667 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 21:34:32 +0000 Subject: [PATCH] refactor: address self-audit on notification settings - Promote the duplicate `SwitchTile` from SecurityFiltersScreen and NotificationSettingsScreen into a shared `SettingsSwitchTile` in SettingsSectionCard so all settings switches share one implementation. - Render BatteryOptimizationBanner outside the in-app section card instead of nesting a Card inside a Card and splitting the section's divider away from the row it explains. - Refresh the battery-optimization exemption on LifecycleResumeEffect so the banner disappears after the user returns from the system settings page; drop the racy post-button re-read. - Demote `HasPushNotificationProvider` to a plain `hasPushNotificationProvider` since it returns a per-flavor constant and reads no Compose state. --- .../components/SelectNotificationProvider.kt | 3 +- .../settings/NotificationSettingsScreen.kt | 142 +++++++----------- .../settings/SecurityFiltersScreen.kt | 27 +--- .../loggedIn/settings/SettingsSectionCard.kt | 20 +++ .../components/SelectNotificationProvider.kt | 3 +- 5 files changed, 81 insertions(+), 114 deletions(-) diff --git a/amethyst/src/fdroid/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt b/amethyst/src/fdroid/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt index ee8b2139af..e61c08b852 100644 --- a/amethyst/src/fdroid/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt +++ b/amethyst/src/fdroid/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt @@ -207,8 +207,7 @@ fun LoadDistributors(onInner: @Composable (String, ImmutableList, Immuta ) } -@Composable -fun HasPushNotificationProvider(): Boolean = true +fun hasPushNotificationProvider(): Boolean = true @Composable fun PushNotificationProviderTile(sharedPrefs: UiSettingsFlow) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NotificationSettingsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NotificationSettingsScreen.kt index 8b1c463713..5c2450d9c7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NotificationSettingsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/NotificationSettingsScreen.kt @@ -20,7 +20,6 @@ */ package com.vitorpamplona.amethyst.ui.screen.loggedIn.settings -import androidx.annotation.StringRes import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxWidth @@ -32,7 +31,6 @@ import androidx.compose.material3.Card import androidx.compose.material3.CardDefaults import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Scaffold -import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue @@ -44,13 +42,13 @@ import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp +import androidx.lifecycle.compose.LifecycleResumeEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R -import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbol import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.service.notifications.BatteryOptimizationHelper -import com.vitorpamplona.amethyst.ui.components.HasPushNotificationProvider import com.vitorpamplona.amethyst.ui.components.PushNotificationProviderTile +import com.vitorpamplona.amethyst.ui.components.hasPushNotificationProvider import com.vitorpamplona.amethyst.ui.navigation.navs.EmptyNav import com.vitorpamplona.amethyst.ui.navigation.navs.INav import com.vitorpamplona.amethyst.ui.navigation.topbars.TopBarWithBackButton @@ -75,109 +73,81 @@ fun NotificationSettingsScreen( .padding(horizontal = 16.dp, vertical = 12.dp), verticalArrangement = Arrangement.spacedBy(20.dp), ) { - if (HasPushNotificationProvider()) { + if (hasPushNotificationProvider()) { SettingsSection(R.string.notification_settings_section_push) { PushNotificationProviderTile(accountViewModel.settings.uiSettingsFlow) } } + val alwaysOn by accountViewModel.account.settings.alwaysOnNotificationService + .collectAsStateWithLifecycle() + val splitByFollows by accountViewModel.account.settings.splitNotificationsEnabled + .collectAsStateWithLifecycle() + SettingsSection(R.string.notification_settings_section_in_app) { - AlwaysOnServiceTile(accountViewModel) + SettingsSwitchTile( + icon = MaterialSymbols.Notifications, + title = R.string.always_on_notif_setting_title, + description = R.string.always_on_notif_setting_description, + checked = alwaysOn, + onCheckedChange = { accountViewModel.account.settings.toggleAlwaysOnNotificationService() }, + ) SettingsDivider() - SplitByFollowsTile(accountViewModel) + SettingsSwitchTile( + icon = MaterialSymbols.Forum, + title = R.string.split_notifications_setting_title, + description = R.string.split_notifications_setting_description, + checked = splitByFollows, + onCheckedChange = { accountViewModel.account.settings.toggleSplitNotificationsEnabled() }, + ) + } + + if (alwaysOn) { + BatteryOptimizationBanner() } } } } -@Composable -private fun AlwaysOnServiceTile(accountViewModel: AccountViewModel) { - val enabled by accountViewModel.account.settings.alwaysOnNotificationService - .collectAsStateWithLifecycle() - - SwitchTile( - icon = MaterialSymbols.Notifications, - title = R.string.always_on_notif_setting_title, - description = R.string.always_on_notif_setting_description, - checked = enabled, - onCheckedChange = { accountViewModel.account.settings.toggleAlwaysOnNotificationService() }, - ) - - if (enabled) { - BatteryOptimizationBanner() - } -} - -@Composable -private fun SplitByFollowsTile(accountViewModel: AccountViewModel) { - val enabled by accountViewModel.account.settings.splitNotificationsEnabled - .collectAsStateWithLifecycle() - - SwitchTile( - icon = MaterialSymbols.Forum, - title = R.string.split_notifications_setting_title, - description = R.string.split_notifications_setting_description, - checked = enabled, - onCheckedChange = { accountViewModel.account.settings.toggleSplitNotificationsEnabled() }, - ) -} - -@Composable -private fun SwitchTile( - icon: MaterialSymbol, - @StringRes title: Int, - @StringRes description: Int, - checked: Boolean, - onCheckedChange: (Boolean) -> Unit, -) { - SettingsControlRow( - icon = icon, - title = stringRes(title), - description = stringRes(description), - onClick = { onCheckedChange(!checked) }, - ) { - Switch(checked = checked, onCheckedChange = onCheckedChange) - } -} - @Composable private fun BatteryOptimizationBanner() { val context = LocalContext.current var isExempt by remember { mutableStateOf(BatteryOptimizationHelper.isIgnoringBatteryOptimizations(context)) } + LifecycleResumeEffect(Unit) { + isExempt = BatteryOptimizationHelper.isIgnoringBatteryOptimizations(context) + onPauseOrDispose {} + } - if (!isExempt) { - Card( - modifier = Modifier.fillMaxWidth().padding(horizontal = 16.dp, vertical = 8.dp), - colors = - CardDefaults.cardColors( - containerColor = MaterialTheme.colorScheme.errorContainer, - ), + if (isExempt) return + + Card( + modifier = Modifier.fillMaxWidth(), + colors = + CardDefaults.cardColors( + containerColor = MaterialTheme.colorScheme.errorContainer, + ), + ) { + Column( + modifier = Modifier.padding(16.dp), + verticalArrangement = Arrangement.spacedBy(8.dp), ) { - Column( - modifier = Modifier.padding(16.dp), - verticalArrangement = Arrangement.spacedBy(8.dp), + Text( + text = stringRes(R.string.battery_optimization_title), + style = MaterialTheme.typography.bodyMedium, + fontWeight = FontWeight.Bold, + color = MaterialTheme.colorScheme.onErrorContainer, + ) + Text( + text = stringRes(R.string.battery_optimization_description), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onErrorContainer, + ) + Button( + onClick = { BatteryOptimizationHelper.requestBatteryOptimizationExemption(context) }, ) { - Text( - text = stringRes(R.string.battery_optimization_title), - style = MaterialTheme.typography.bodyMedium, - fontWeight = FontWeight.Bold, - color = MaterialTheme.colorScheme.onErrorContainer, - ) - Text( - text = stringRes(R.string.battery_optimization_description), - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onErrorContainer, - ) - Button( - onClick = { - BatteryOptimizationHelper.requestBatteryOptimizationExemption(context) - isExempt = BatteryOptimizationHelper.isIgnoringBatteryOptimizations(context) - }, - ) { - Text(stringRes(R.string.battery_optimization_fix_now)) - } + Text(stringRes(R.string.battery_optimization_fix_now)) } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SecurityFiltersScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SecurityFiltersScreen.kt index 11faac4803..d3f9905105 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SecurityFiltersScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SecurityFiltersScreen.kt @@ -20,7 +20,6 @@ */ package com.vitorpamplona.amethyst.ui.screen.loggedIn.settings -import androidx.annotation.StringRes import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxWidth @@ -31,7 +30,6 @@ import androidx.compose.material3.Scaffold import androidx.compose.material3.SegmentedButton import androidx.compose.material3.SegmentedButtonDefaults import androidx.compose.material3.SingleChoiceSegmentedButtonRow -import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue @@ -40,7 +38,6 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R -import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbol import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.model.WarningType import com.vitorpamplona.amethyst.model.parseWarningType @@ -122,7 +119,7 @@ private fun FilterSpamTile(accountViewModel: AccountViewModel) { .filterSpamFromStrangers .collectAsStateWithLifecycle() - SwitchTile( + SettingsSwitchTile( icon = MaterialSymbols.FilterAlt, title = R.string.filter_spam_from_strangers_title, description = R.string.filter_spam_from_strangers_explainer, @@ -136,7 +133,7 @@ private fun HideCommunityViolationsTile(accountViewModel: AccountViewModel) { val hideViolations by accountViewModel.account.settings.hideCommunityRulesViolations .collectAsStateWithLifecycle() - SwitchTile( + SettingsSwitchTile( icon = MaterialSymbols.Shield, title = R.string.hide_community_rules_violations_title, description = R.string.hide_community_rules_violations_explainer, @@ -151,7 +148,7 @@ private fun WarnReportsTile(accountViewModel: AccountViewModel) { val warnReports by security.warnAboutPostsWithReports.collectAsStateWithLifecycle() val threshold by security.reportWarningThreshold.collectAsStateWithLifecycle() - SwitchTile( + SettingsSwitchTile( icon = MaterialSymbols.Report, title = R.string.warn_when_posts_have_reports_from_your_follows_title, description = R.string.warn_when_posts_have_reports_from_your_follows_explainer, @@ -194,24 +191,6 @@ private fun MaxHashtagsTile(accountViewModel: AccountViewModel) { } } -@Composable -private fun SwitchTile( - icon: MaterialSymbol, - @StringRes title: Int, - @StringRes description: Int, - checked: Boolean, - onCheckedChange: (Boolean) -> Unit, -) { - SettingsControlRow( - icon = icon, - title = stringRes(title), - description = stringRes(description), - onClick = { onCheckedChange(!checked) }, - ) { - Switch(checked = checked, onCheckedChange = onCheckedChange) - } -} - @Composable private fun BlockedContentSection( accountViewModel: AccountViewModel, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsSectionCard.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsSectionCard.kt index d190d18803..a188839456 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsSectionCard.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsSectionCard.kt @@ -38,6 +38,7 @@ import androidx.compose.material3.CardDefaults import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Switch import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.ui.Alignment @@ -248,6 +249,25 @@ internal fun SettingsControlRow( } } +/** A [SettingsControlRow] whose trailing control is a [Switch]; tapping anywhere toggles. */ +@Composable +internal fun SettingsSwitchTile( + icon: MaterialSymbol, + @StringRes title: Int, + @StringRes description: Int, + checked: Boolean, + onCheckedChange: (Boolean) -> Unit, +) { + SettingsControlRow( + icon = icon, + title = stringRes(title), + description = stringRes(description), + onClick = { onCheckedChange(!checked) }, + ) { + Switch(checked = checked, onCheckedChange = onCheckedChange) + } +} + /** * Sub-row variant of [SettingsControlRow]: indented in place of a leading icon, * used for controls hierarchically grouped under the row above (e.g. a threshold diff --git a/amethyst/src/play/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt b/amethyst/src/play/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt index 2f086def3f..dc32830cc4 100644 --- a/amethyst/src/play/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt +++ b/amethyst/src/play/java/com/vitorpamplona/amethyst/ui/components/SelectNotificationProvider.kt @@ -54,5 +54,4 @@ fun SelectNotificationProvider(sharedPrefs: UiSettingsFlow) { @Composable fun PushNotificationProviderTile(sharedPrefs: UiSettingsFlow) {} -@Composable -fun HasPushNotificationProvider(): Boolean = false +fun hasPushNotificationProvider(): Boolean = false