Improve handling of notification enabled setting #1264

mchowning · 2023-08-09T19:00:51Z

Description

This PR addresses an issue where we were not using the notifications enabled setting to determine the state of the notifications enabled toggle. Instead we were checking whether any podcasts had notifications enabled. This PR updates this and makes the behavior consistent with iOS.

This issue exists on main and is not a result of the recent Settings refactorings.

This change feels a bit tricky, so let me know if you can think of any undesireable behavior that these changes might introduce.

I am targeting the settings sync refactor branch with this change because it is easier to just include this with the other settings changes, and I anticipate that I'll be merging this branch to main pretty soon.

Testing Instructions

Go to the notification settings
Change the notification setting
Exit the screen and return to the screen
Note that the new setting value persists
Update the setting to be turned on, but with no podcasts selected
Exit the screen and return to the screen
Note that notifications are still enabled (previously, notifications would have been turned off because we were just checking if any podcasts had notifications enabled).
Turn off notifications
Tap on the podcasts tab (do not back out of the notifications screen), and open a subscribed podcast
Tap the notification bell to turn on notifications
Tap on the profile tab to return to the notifications screen
Note that notifications are now on (previously, notifications would have been turned on, but the UI would have shown that notifications were off because the UI was out of date).

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md
Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
I have considered whether it makes sense to add tests for my changes
All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
Any jetpack compose components I added or changed are covered by compose previews

This avoids the setup becoming out of date if a user changes the notification setting from another screen (like enabling notifications for a single podcast from the podcast screen).

mchowning · 2023-08-09T19:10:36Z

...tings/src/main/java/au/com/shiftyjelly/pocketcasts/settings/NotificationsSettingsFragment.kt

+
+                enabledPreference?.setOnPreferenceChangeListener { _, newValue ->
+                    val checked = newValue as Boolean
+                    settings.notifyRefreshPodcast.set(checked)


Apart from moving this entire method to be run in onResume instead of only when the fragment was created, the only change to the logic was to add this settings.notifyRefreshPodcast.set(checked) line so that we now persist the enabled setting. Apart from this, all the logic here is the same, just in a new location.

EDIT: With a542615, there are now some other changes to the logic here.

geekygecko

There is something slightly strange happening where you turn on "Notify me", it says "No podcasts selected", you exit the screen, come back, and now it says "All podcasts".

Screen.Recording.2023-08-10.at.5.22.05.pm.mov

geekygecko · 2023-08-10T07:47:10Z

...tings/src/main/java/au/com/shiftyjelly/pocketcasts/settings/NotificationsSettingsFragment.kt

@@ -445,6 +402,48 @@ class NotificationsSettingsFragment :
        }
    }

+    private fun setupEnabledNotifications() {
+        launch(Dispatchers.Default) {
+            val enabled = settings.notifyRefreshPodcast.flow.value


Do you think it's worth having a value method that just calls flow.value as we seem to call it lots? Feel free to ignore this.

My initial thought was to intentionally not add something like that because I wanted to provide a push toward reacting to the flow instead of just fetching the latest value. There are enough places where we just get the latest value though that you're probably right that it makes sense to make it more convenient. I'll make this change in a separate PR. Thanks for bringing this up.

notifications turned on

mchowning · 2023-08-10T18:17:41Z

There is something slightly strange happening where you turn on "Notify me", it says "No podcasts selected", you exit the screen, come back, and now it says "All podcasts".

Looks like it was a race condition, but I'm pretty sure you just brought this up to bait me into removing some RxJava 😛 . Should be fixed by a542615. Since this isn't a trivial change, I'll hold off on merging until you give this another look.

mchowning · 2023-08-10T18:19:14Z

...tings/src/main/java/au/com/shiftyjelly/pocketcasts/settings/NotificationsSettingsFragment.kt

+                        mapOf("enabled" to checked)
+                    )
+
+                    lifecycleScope.launch {


I was surprised to find that if I used the Fragment's CouroutineScope directly here by calling launch instead of lifecycleScope.launch the coroutine doesn't run. I'm not sure why that is 😕, so if you have any insight, let me know.

geekygecko · 2023-08-15T07:36:46Z

Thanks for those changes. It worked well for me.

mchowning added 3 commits August 9, 2023 14:44

Persist and use notifications enabled setting

b167b88

Move enabled notifications setup to onResume

218d095

This avoids the setup becoming out of date if a user changes the notification setting from another screen (like enabling notifications for a single podcast from the podcast screen).

Update changelog

3cb00af

mchowning marked this pull request as ready for review August 9, 2023 19:02

mchowning requested a review from a team as a code owner August 9, 2023 19:02

mchowning changed the title ~~Fix/persist and use notifications enabled setting~~ Improve handling of notification enabled setting Aug 9, 2023

mchowning commented Aug 9, 2023

View reviewed changes

geekygecko approved these changes Aug 10, 2023

View reviewed changes

Ensure podcasts are updated before checking which podcasts have

a542615

notifications turned on

mchowning requested a review from geekygecko August 10, 2023 18:17

mchowning commented Aug 10, 2023

View reviewed changes

mchowning mentioned this pull request Aug 10, 2023

Replace UserSetting.flow.value with UserSetting.flow #1268

Merged

5 tasks

geekygecko merged commit 335eef4 into feature/sync-settings-refactor Aug 15, 2023

geekygecko deleted the fix/persist-and-use-notifications-enabled-setting branch August 15, 2023 07:36

mchowning mentioned this pull request Aug 18, 2023

Merging UserSettings to main #1290

Merged

5 tasks

ashiagr added this to the 7.47 ❄️ milestone Sep 4, 2023

mchowning added the [Area] Sync Settings label Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of notification enabled setting #1264

Improve handling of notification enabled setting #1264

mchowning commented Aug 9, 2023

mchowning Aug 9, 2023 •

edited

Loading

geekygecko left a comment

geekygecko Aug 10, 2023

mchowning Aug 10, 2023 •

edited

Loading

mchowning commented Aug 10, 2023 •

edited

Loading

mchowning Aug 10, 2023 •

edited

Loading

geekygecko commented Aug 15, 2023

Improve handling of notification enabled setting #1264

Improve handling of notification enabled setting #1264

Conversation

mchowning commented Aug 9, 2023

Description

Testing Instructions

Checklist

mchowning Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

geekygecko left a comment

Choose a reason for hiding this comment

geekygecko Aug 10, 2023

Choose a reason for hiding this comment

mchowning Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

mchowning commented Aug 10, 2023 • edited Loading

mchowning Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

geekygecko commented Aug 15, 2023

mchowning Aug 9, 2023 •

edited

Loading

mchowning Aug 10, 2023 •

edited

Loading

mchowning commented Aug 10, 2023 •

edited

Loading

mchowning Aug 10, 2023 •

edited

Loading