Audio ducking now supported during external notifications #1009

jamie23 · 2023-05-26T22:08:29Z

Description

Added functionality for audio ducking when external notifications come in. There was already some logic there around it hence I've labeled this a fix.

Fixes #529

Testing Instructions

Fresh install of the app.
Start the app.
Navigate to Profile, then Settings, then Notifications
Notice that there is no longer a toggle for 'Play over notifications' and it now launches a dialog
Select Never
Play a podcast then launch an external audible notification
- To launch an external audible notification, I used Google maps and asked for turn by turn directions whilst a podcast was playing.
Notice that the apps audio stops playing while the audible notification is active and resumes after the notification stops.
Select the toggle for 'Play over notifications' again.
Select 'Duck volume' and repeat step 6.
Notice that the podcast volumes dips to half volume while the external notification plays and resumes full volume after the notification ends.
Select the toggle for 'Play over notifications' again.
Select 'Always' and repeat step 6.
Notice that the podcast audio plays throughout the external notification plays at full volume

Screenshots or Screencast

Updated the logs file to show the setting chosen:

Checklist

If this is a user-facing change, I have added an entry in CHANGELOG.md
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

I have tested any UI changes...

with different themes
with a landscape orientation
with the device set to have a large display and font size
for accessibility with TalkBack

jamie23 · 2023-05-26T22:18:42Z

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

+        playOverNotificationPreference?.setOnPreferenceChangeListener { _, newValue ->
+            analyticsTracker.track(
+                AnalyticsEvent.SETTINGS_NOTIFICATIONS_PLAY_OVER_NOTIFICATIONS_TOGGLED,
+                mapOf("enabled" to (newValue != "2"))


One thing to note around this, in the old form the tracking is a boolean since the setting is a switch and is either on or off:

AnalyticsEvent.SETTINGS_NOTIFICATIONS_PLAY_OVER_NOTIFICATIONS_TOGGLED, mapOf("enabled" to newValue as Boolean)

I assume you're using this behind the scenes to do analytics and so I can't just create a new event, so to keep it useful, I kept the event the same, so true is when the setting is duck or always and false is when the setting is never.

If you want me to improve this in some way please let me know!

Thanks for calling this out. What you've done here makes sense. Let's leave that to keep the legacy tracking of the "enabled" prop since that will keep us consistent with iOS, but we can also add a new property with the form of "value" to "never"/"duck"/"always". wdyt?

we can also add a new property with the form of "value" to "never"/"duck"/"always". wdyt?

Yea that sounds good! Would you like me to add that now in this PR and you guys can add it behind the scenes when you get some time or would you rather me leave it out of this PR and you can add it in on both sides at the same time?

Let's include it in this PR.

mchowning

This is looking really good @jamie23 ! Thanks so much for the contribution! 🙇

I left a few comments, but there are no major issues. This works very well.

One thing I did notice as I was looking through the code is that it seemed like the logic around handling the different settings values was getting spread around to a lot of places (there are a lot of classes that need to know what 0, 1, and 2 mean. I really liked how you created a PlayOverNotificationSetting enum and thought we could maybe expand on that to consolidate the logic around those settings a bit and generally make the code a bit more readable.

I put together a quick POC here to show what I had in mind. Don't feel like you have to use that though, feel free to do it in your own style or decline this suggestion if you'd rather leave it as-is.

mchowning · 2023-06-19T19:40:52Z

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

+        playOverNotificationPreference?.setOnPreferenceChangeListener { _, newValue ->
+            analyticsTracker.track(
+                AnalyticsEvent.SETTINGS_NOTIFICATIONS_PLAY_OVER_NOTIFICATIONS_TOGGLED,
+                mapOf("enabled" to (newValue != "2"))


Thanks for calling this out. What you've done here makes sense. Let's leave that to keep the legacy tracking of the "enabled" prop since that will keep us consistent with iOS, but we can also add a new property with the form of "value" to "never"/"duck"/"always". wdyt?

mchowning · 2023-06-19T19:51:27Z

...ervices/preferences/src/main/java/au/com/shiftyjelly/pocketcasts/preferences/SettingsImpl.kt

+        val value = sharedPreferences.getString(
+            Settings.PREFERENCE_NOTIFICATION_AUDIO,
+            Settings.PREFERENCE_NOTIFICATION_AUDIO_DEFAULT
+        ) ?: Settings.PREFERENCE_NOTIFICATION_AUDIO_DEFAULT
+        return Integer.parseInt(value)


What would you think about checking the value of Settings.PREFERENCE_OVERRIDE_AUDIO if there is nothing saved for Settings.PREFERENCE_NOTIFICATION_AUDIO? In particular, I'm thinking that for users who have previously applied the setting to play over notification audio it would be good if we retain that setting for them when they upgrade to a version of the app with your changes.

In particular, I'm looking at these test steps:

Install previous version of the app

Update notification preference to play over audio

Install updated version of the app with the changes from this PR

❗ The app should still have the setting for playing over audio applied

wdyt?

Yea great pickup and much more user friendly than a user losing their previous preference, I made an adjustment like so:

override fun getPlayOverNotification(): PlayOverNotificationSetting { val value = sharedPreferences.getString(Settings.PREFERENCE_OVERRIDE_NOTIFICATION_AUDIO, null) ?: legacyPlayOverNotification() return PlayOverNotificationSetting.fromPreferenceString(value) } private fun legacyPlayOverNotification(): String { if (sharedPreferences.getBoolean(Settings.PREFERENCE_OVERRIDE_AUDIO_LEGACY, false)) { return PlayOverNotificationSetting.ALWAYS.preferenceInt.toString() } return PlayOverNotificationSetting.NEVER.preferenceInt.toString() }

So basically, check if they have a preference set, if not check then check if the legacy preference is set to true then return Always otherwise return Never.

I gave it a quick test on my device and it worked well for your test. Happy to modify if though if it can be improved further.

mchowning · 2023-06-19T19:52:59Z

...es/services/preferences/src/main/java/au/com/shiftyjelly/pocketcasts/preferences/Settings.kt

+        const val PREFERENCE_NOTIFICATION_AUDIO = "notificationAudio"
+        const val PREFERENCE_NOTIFICATION_AUDIO_DEFAULT = "2"


"NOTIFICATION_AUDIO" makes me think that this is selecting the sound file to play with notifications. I would probably keep something like "override" or "play over" in the names here. That is just my personal preference though. This certainly isn't a blocker, so feel free to leave it as-is if you prefer.

mchowning · 2023-06-19T20:17:15Z

...ositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/FocusManager.kt

+        return playOverNotification()
+    }
+
+    protected open fun playOverNotification() = when (settings.getPlayOverNotification()) {


I don't think this needs to be protected or open.

mchowning · 2023-06-19T20:31:34Z

modules/services/localization/src/main/res/values/strings.xml

@@ -1090,8 +1090,10 @@
    <string name="settings_notification_choose_podcasts">Choose podcasts</string>
    <string name="settings_notification_hide_on_pause">Hide playback notification on pause</string>
    <string name="settings_notification_notify_me">Notify me</string>
-    <string name="settings_notification_play_over">Play over notifications</string>
-    <string name="settings_notification_play_over_summary">Keep playing even when other apps, like notifications or navigation, play sounds.</string>
+    <string name="settings_notification_play_over">Play over external notifications</string>


I noticed that you added "external" to this string. Do you find "notifications" on it's own to be unclear? Just asking because I'm curious.

I think when I worked on it pre vacation I must have thought so, after some time away and re-reading it though I'm happy to leave "external" out of it.

mchowning · 2023-06-19T20:48:47Z

CHANGELOG.md

+    *   Added audio ducking as an option when playing over external notifications
+        ([#1009](https://github.com/Automattic/pocket-casts-android/pull/1009)).


We'll need to merge in main and move this entry to the next release before we merge this.

larena1 · 2023-07-10T10:02:35Z

@jamie23 are you still working on this? It'd be great to have this 😊

jamie23 · 2023-07-17T00:08:25Z

This is looking really good @jamie23 ! Thanks so much for the contribution! 🙇

I left a few comments, but there are no major issues. This works very well.

One thing I did notice as I was looking through the code is that it seemed like the logic around handling the different settings values was getting spread around to a lot of places (there are a lot of classes that need to know what 0, 1, and 2 mean. I really liked how you created a PlayOverNotificationSetting enum and thought we could maybe expand on that to consolidate the logic around those settings a bit and generally make the code a bit more readable.

I put together a quick POC here to show what I had in mind. Don't feel like you have to use that though, feel free to do it in your own style or decline this suggestion if you'd rather leave it as-is.

Thanks for the review! I've been on vacation for the last couple of weeks so just getting back round to looking at your comments (sorry if you've been waiting on this @larena1 !).

Yea I really like what you've done with PlayOverNotificationSetting and happy to include it, I've cherry-picked it into this branch as it does simplify things.

mchowning

This looks good! Thanks!

jamie23 requested a review from a team as a code owner May 26, 2023 22:08

jamie23 force-pushed the fix/audio-ducking branch from e768508 to b54b425 Compare May 26, 2023 22:11

jamie23 commented May 26, 2023

View reviewed changes

mchowning self-assigned this Jun 19, 2023

mchowning reviewed Jun 19, 2023

View reviewed changes

mchowning removed their assignment Jun 20, 2023

jamie23 force-pushed the fix/audio-ducking branch from b54b425 to 297c1da Compare July 17, 2023 00:07

jamie23 force-pushed the fix/audio-ducking branch from 297c1da to d72d56a Compare July 17, 2023 00:14

jamie23 force-pushed the fix/audio-ducking branch from d72d56a to a899510 Compare July 24, 2023 17:12

jamie23 and others added 2 commits July 24, 2023 18:29

Audio ducking now supported during notifications

f223138

POC using enum for notification play over setting

3fdb8f8

jamie23 force-pushed the fix/audio-ducking branch from a899510 to 3e7b7e0 Compare July 24, 2023 17:30

Audio ducking now reads legacy setting

bf20eaa

jamie23 force-pushed the fix/audio-ducking branch from 3e7b7e0 to bf20eaa Compare July 24, 2023 17:31

jamie23 requested a review from mchowning July 24, 2023 17:33

mchowning approved these changes Jul 24, 2023

View reviewed changes

Update CHANGELOG.md

9511f0a

mchowning enabled auto-merge (squash) July 24, 2023 19:53

mchowning merged commit 814ca7e into Automattic:main Jul 24, 2023

ashiagr added this to the 7.45 ❄️ milestone Aug 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio ducking now supported during external notifications #1009

Audio ducking now supported during external notifications #1009

jamie23 commented May 26, 2023 •

edited

Loading

jamie23 May 26, 2023

mchowning Jun 19, 2023

jamie23 Jul 13, 2023

mchowning Jul 14, 2023

mchowning left a comment

mchowning Jun 19, 2023

mchowning Jun 19, 2023 •

edited

Loading

jamie23 Jul 16, 2023 •

edited

Loading

mchowning Jun 19, 2023 •

edited

Loading

mchowning Jun 19, 2023

mchowning Jun 19, 2023

jamie23 Jul 17, 2023

mchowning Jun 19, 2023

larena1 commented Jul 10, 2023

jamie23 commented Jul 17, 2023 •

edited

Loading

mchowning left a comment

		const val PREFERENCE_NOTIFICATION_AUDIO = "notificationAudio"
		const val PREFERENCE_NOTIFICATION_AUDIO_DEFAULT = "2"

		* Added audio ducking as an option when playing over external notifications
		([#1009](https://github.com/Automattic/pocket-casts-android/pull/1009)).

Audio ducking now supported during external notifications #1009

Audio ducking now supported during external notifications #1009

Conversation

jamie23 commented May 26, 2023 • edited Loading

Description

Testing Instructions

Screenshots or Screencast

Checklist

I have tested any UI changes...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchowning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchowning Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

jamie23 Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

mchowning Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larena1 commented Jul 10, 2023

jamie23 commented Jul 17, 2023 • edited Loading

mchowning left a comment

Choose a reason for hiding this comment

jamie23 commented May 26, 2023 •

edited

Loading

mchowning Jun 19, 2023 •

edited

Loading

jamie23 Jul 16, 2023 •

edited

Loading

mchowning Jun 19, 2023 •

edited

Loading

jamie23 commented Jul 17, 2023 •

edited

Loading