mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-08 23:54:39 +00:00
fix(desktop): auto-enable master notif switch when OS permission already granted
Follow-up to e9475dd079. That commit fixed the case where the user
clicked the "Enable OS notifications" button on a fresh install
(permission NotRequested \u2192 Granted) but the master toggle stayed off.
It missed the two closely-related cases the user was still hitting on
v1.13.1:
1. Permission was already granted from a previous session or install
(e.g. an earlier v1.13.0 build, or the user allowed it via
System Settings \u2192 Notifications directly). In this state,
permissionState == Granted, so the "Enable OS notifications"
button never renders \u2014 the button label promised the whole
handshake but the code path that flipped the master switch only
ran under NotRequested.
2. On Windows/Linux `permissionState` defaults to `NotApplicable`
from the moment the app starts. The master switch is off by
default (first-launch UX choice) and nothing ever flips it, so
the auto-dispatcher stayed muted forever unless the user found
the switch manually.
Fix:
- Add `NotificationSettings.wasExplicitlyDisabled()` so the Settings
screen can distinguish "master switch is off because it defaults
off on first launch" (auto-enable is fine) from "master switch is
off because the user turned it off" (leave alone). Backed by a
new java.util.prefs key `explicitly_disabled` that flips true on
`setEnabled(false)` and gets cleared on `setEnabled(true)`.
- In `NotificationSettingsScreen`, a `LaunchedEffect(permissionState,
enabled)` observes when the OS permission is Granted OR NotApplicable
and the master switch is off. If the user has never explicitly
turned it off, it auto-flips on \u2014 matching the "Enable OS
notifications" contract for the paths the previous fix missed.
- Also render a "Turn on desktop notifications" button in the
Granted branch when the user has explicitly turned notifications
off. That's the recovery path for users who deliberately opted
out and later want to opt back in without hunting for the
master switch two rows away.
Behaviour on the fresh-install macOS path (permission NotRequested)
is unchanged \u2014 that path still runs the `requestPermission()`
flow inside the button's onClick, and the auto-enable happens via
the same LaunchedEffect once permissionState flips to Granted.
Tests (jvmTest, hermetic \u2014 UUID-scoped prefs nodes so tests never
share state or pollute real user prefs):
PreferencesNotificationSettingsExplicitDisableTest:
- fresh install defaults to not-explicitly-disabled
- turning off marks explicitly disabled
- turning on clears the explicit-disable flag
- flag persists across new instances on the same prefs node
\ud83e\udd16 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
+10
@@ -81,6 +81,16 @@ interface NotificationSettings {
|
||||
|
||||
fun setEnabled(v: Boolean)
|
||||
|
||||
/**
|
||||
* True iff the user has taken an explicit action to disable
|
||||
* notifications (i.e. flipped the master switch OFF at some point).
|
||||
* Used by the Settings screen to distinguish "master switch is off
|
||||
* because it defaults to off on first launch" from "master switch
|
||||
* is off because the user asked for it to be off". Only the former
|
||||
* gets auto-enabled when the OS permission check passes.
|
||||
*/
|
||||
fun wasExplicitlyDisabled(): Boolean
|
||||
|
||||
fun setKindToggle(
|
||||
kind: NotifKind,
|
||||
v: Boolean,
|
||||
|
||||
+13
@@ -53,8 +53,20 @@ class PreferencesNotificationSettings(
|
||||
override fun setEnabled(v: Boolean) {
|
||||
_enabled.value = v
|
||||
prefs.putBoolean(KEY_ENABLED, v)
|
||||
// Track explicit user intent so the Settings screen can auto-enable
|
||||
// on next visit for users who never touched the switch, while
|
||||
// respecting users who deliberately turned it off. Only false
|
||||
// → "explicit disable"; going from off to on clears the flag so
|
||||
// subsequent auto-enable heuristics work normally.
|
||||
if (v) {
|
||||
prefs.remove(KEY_EXPLICITLY_DISABLED)
|
||||
} else {
|
||||
prefs.putBoolean(KEY_EXPLICITLY_DISABLED, true)
|
||||
}
|
||||
}
|
||||
|
||||
override fun wasExplicitlyDisabled(): Boolean = prefs.getBoolean(KEY_EXPLICITLY_DISABLED, false)
|
||||
|
||||
override fun setKindToggle(
|
||||
kind: NotifKind,
|
||||
v: Boolean,
|
||||
@@ -92,6 +104,7 @@ class PreferencesNotificationSettings(
|
||||
companion object {
|
||||
const val NODE = "com/vitorpamplona/amethyst/notifications"
|
||||
private const val KEY_ENABLED = "enabled"
|
||||
private const val KEY_EXPLICITLY_DISABLED = "explicitly_disabled"
|
||||
private const val KEY_DND_UNTIL = "dnd_until"
|
||||
private const val KEY_PREVIEW = "preview_in_toast"
|
||||
|
||||
|
||||
+86
@@ -0,0 +1,86 @@
|
||||
/*
|
||||
* Copyright (c) 2025 Vitor Pamplona
|
||||
*
|
||||
* Permission is hereby granted, free of charge, to any person obtaining a copy of
|
||||
* this software and associated documentation files (the "Software"), to deal in
|
||||
* the Software without restriction, including without limitation the rights to use,
|
||||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
|
||||
* Software, and to permit persons to whom the Software is furnished to do so,
|
||||
* subject to the following conditions:
|
||||
*
|
||||
* The above copyright notice and this permission notice shall be included in all
|
||||
* copies or substantial portions of the Software.
|
||||
*
|
||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
||||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
|
||||
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
|
||||
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
|
||||
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
|
||||
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.commons.moderation.notifications
|
||||
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
import java.util.UUID
|
||||
import java.util.prefs.Preferences
|
||||
|
||||
/**
|
||||
* Pins the semantics of the new [NotificationSettings.wasExplicitlyDisabled]
|
||||
* flag added to unblock the "Enable OS notifications button doesn't work on
|
||||
* desktop" bug's second failure mode.
|
||||
*
|
||||
* The Settings screen auto-enables the master notifications switch when it
|
||||
* detects that the OS permission is fine and the switch is off. That's the
|
||||
* common path for users who click the "Enable OS notifications" button and
|
||||
* expect it to fully take effect (button label promise). But it MUST NOT
|
||||
* override users who deliberately turned notifications off. The
|
||||
* [wasExplicitlyDisabled] flag is how we distinguish the two.
|
||||
*/
|
||||
class PreferencesNotificationSettingsExplicitDisableTest {
|
||||
private fun freshNode(): Preferences {
|
||||
// Use a UUID-scoped node so tests never share state and never
|
||||
// pollute the real user prefs on the machine running CI/dev builds.
|
||||
return Preferences.userRoot().node("amethyst-test-" + UUID.randomUUID())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fresh install defaults to not-explicitly-disabled`() {
|
||||
val settings = PreferencesNotificationSettings(freshNode())
|
||||
assertFalse(
|
||||
"First launch must not look like a deliberate opt-out; otherwise auto-enable stays off forever",
|
||||
settings.wasExplicitlyDisabled(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `turning off marks explicitly disabled`() {
|
||||
val prefs = freshNode()
|
||||
val settings = PreferencesNotificationSettings(prefs)
|
||||
settings.setEnabled(false)
|
||||
assertTrue(settings.wasExplicitlyDisabled())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `turning on clears the explicit-disable flag`() {
|
||||
val prefs = freshNode()
|
||||
val settings = PreferencesNotificationSettings(prefs)
|
||||
settings.setEnabled(false)
|
||||
assertTrue(settings.wasExplicitlyDisabled())
|
||||
settings.setEnabled(true)
|
||||
assertFalse(
|
||||
"Toggling back on must clear the flag so subsequent OFF->auto-enable cycles work",
|
||||
settings.wasExplicitlyDisabled(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `flag persists across new instances on the same prefs node`() {
|
||||
val prefs = freshNode()
|
||||
PreferencesNotificationSettings(prefs).setEnabled(false)
|
||||
// Second instance opens the same node \u2014 flag must survive process restart.
|
||||
val reopened = PreferencesNotificationSettings(prefs)
|
||||
assertTrue(reopened.wasExplicitlyDisabled())
|
||||
}
|
||||
}
|
||||
+32
@@ -132,6 +132,27 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) {
|
||||
}
|
||||
}
|
||||
|
||||
// Handle the still-broken case that the previous fix missed:
|
||||
// the user granted OS permission in a prior session (either via
|
||||
// the older "Enable OS notifications" button whose auto-enable
|
||||
// guard I initially forgot, via System Settings directly, or on
|
||||
// Windows/Linux where permissionState defaults to NotApplicable).
|
||||
// When they come back to Settings, permissionState == Granted so
|
||||
// the "Enable OS notifications" button doesn't render, the master
|
||||
// switch is still OFF from first-launch defaults, and there is no
|
||||
// affordance that both tells them what's wrong and fixes it in
|
||||
// one click. Auto-enable once per screen entry when we detect
|
||||
// "permission is fine, but master switch is off and the user
|
||||
// has never explicitly disabled it". PreferencesNotificationSettings
|
||||
// exposes [wasExplicitlyDisabled] so we don't overrule a deliberate
|
||||
// opt-out.
|
||||
androidx.compose.runtime.LaunchedEffect(permissionState, enabled) {
|
||||
val allowed = permissionState == PermissionState.Granted || permissionState == PermissionState.NotApplicable
|
||||
if (allowed && !enabled && !settings.wasExplicitlyDisabled()) {
|
||||
settings.setEnabled(true)
|
||||
}
|
||||
}
|
||||
|
||||
PlatformStatusCard(
|
||||
host = host,
|
||||
nativeAvailable = nativeAvailable,
|
||||
@@ -219,6 +240,17 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) {
|
||||
}
|
||||
}
|
||||
PermissionState.Granted, PermissionState.NotApplicable -> {
|
||||
// Turn-on button: renders only when master switch is
|
||||
// off *and* the user explicitly disabled it before.
|
||||
// The LaunchedEffect above auto-enables the switch
|
||||
// for the common "never touched it" path; this button
|
||||
// is the recovery for the deliberate-opt-out path.
|
||||
if (!enabled) {
|
||||
OutlinedButton(
|
||||
onClick = { settings.setEnabled(true) },
|
||||
enabled = true,
|
||||
) { Text("Turn on desktop notifications") }
|
||||
}
|
||||
OutlinedButton(
|
||||
onClick = {
|
||||
if (sendingTest) return@OutlinedButton
|
||||
|
||||
Reference in New Issue
Block a user