diff --git a/CHANGELOG.md b/CHANGELOG.md index d37d6c2d7fd..1700060c557 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ 8.18 ----- +* New Features + * Improve feedback when adding episodes to playlists + ([#5632](https://github.com/Automattic/pocket-casts-android/pull/5632)) * Bug Fixes * Allow copying description and show notes links with a long press ([#5628](https://github.com/Automattic/pocket-casts-android/pull/5628)) diff --git a/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistFragment.kt b/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistFragment.kt index fb565e68026..bc70c10daf0 100644 --- a/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistFragment.kt +++ b/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistFragment.kt @@ -26,8 +26,8 @@ import au.com.shiftyjelly.pocketcasts.compose.CallOnce import au.com.shiftyjelly.pocketcasts.compose.components.AnimatedNonNullVisibility import au.com.shiftyjelly.pocketcasts.compose.components.ThemedSnackbarHost import au.com.shiftyjelly.pocketcasts.models.to.EpisodeUuidPair -import au.com.shiftyjelly.pocketcasts.models.to.PlaylistPreviewForEpisode import au.com.shiftyjelly.pocketcasts.playlists.PlaylistFragment +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeFeedback import au.com.shiftyjelly.pocketcasts.repositories.playlist.Playlist import au.com.shiftyjelly.pocketcasts.ui.helper.FragmentHostListener import au.com.shiftyjelly.pocketcasts.ui.theme.Theme @@ -105,7 +105,10 @@ internal class AddToPlaylistFragment : BaseDialogFragment() { viewModel.removeFromPlaylist(playlist.uuid) } else { viewModel.trackEpisodeAddTapped(playlist, isPlaylistFull = false) - viewModel.addToPlaylist(playlist.uuid) + viewModel.addToPlaylist( + playlistUuid = playlist.uuid, + playlistTitle = playlist.title, + ) } } else { viewModel.trackEpisodeAddTapped(playlist, isPlaylistFull = true) @@ -160,27 +163,31 @@ internal class AddToPlaylistFragment : BaseDialogFragment() { override fun onDismiss(dialog: DialogInterface) { super.onDismiss(dialog) if (!requireActivity().isChangingConfigurations && isDoneTapped) { - showDoneSnackbar(viewModel.getPlaylistsAddedTo()) + showDoneSnackbar(viewModel.getPlaylistChangeFeedback()) } } - private fun showDoneSnackbar(playlistsAddedTo: Set) { + private fun showDoneSnackbar(feedback: PlaylistChangeFeedback) { val hostListener = requireActivity() as FragmentHostListener val snackbarView = hostListener.snackBarView() - val snackbar = when (val size = playlistsAddedTo.size) { - 0 -> return - - 1 -> { - val playlist = playlistsAddedTo.first() - val message = getString(LR.string.added_to_playlist_single, playlist.title) + val snackbar = when (feedback) { + is PlaylistChangeFeedback.SinglePlaylistAddition -> { + val playlist = feedback.playlist + val message = getString(feedback.resourceId, playlist.title) Snackbar.make(snackbarView, message, Snackbar.LENGTH_LONG) .setAction(LR.string.view) { hostListener.openManualPlaylist(playlist.uuid) } } - else -> { - val message = resources.getQuantityString(LR.plurals.added_to_playlist_single_multiple, size, size) + is PlaylistChangeFeedback.StringResource -> { + Snackbar.make(snackbarView, getString(feedback.resourceId), Snackbar.LENGTH_LONG) + } + + is PlaylistChangeFeedback.PluralResource -> { + val message = resources.getQuantityString(feedback.resourceId, feedback.quantity, feedback.quantity) Snackbar.make(snackbarView, message, Snackbar.LENGTH_LONG) } + + PlaylistChangeFeedback.None -> return } snackbar.show() } diff --git a/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModel.kt b/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModel.kt index 7feb220266e..78957863a55 100644 --- a/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModel.kt +++ b/modules/features/filters/src/main/java/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModel.kt @@ -1,5 +1,7 @@ package au.com.shiftyjelly.pocketcasts.playlists.manual +import androidx.annotation.PluralsRes +import androidx.annotation.StringRes import androidx.compose.foundation.text.input.TextFieldState import androidx.compose.ui.text.TextRange import androidx.lifecycle.ViewModel @@ -44,6 +46,7 @@ import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import au.com.shiftyjelly.pocketcasts.localization.R as LR @OptIn(ExperimentalCoroutinesApi::class) @HiltViewModel(assistedFactory = AddToPlaylistViewModel.Factory::class) @@ -93,14 +96,22 @@ class AddToPlaylistViewModel @AssistedInject constructor( emitAll(uiStates) }.stateIn(viewModelScope, SharingStarted.Eagerly, initialValue = null) - private val playlistsChanges = mutableMapOf() + private val playlistsChanges = mutableMapOf() - private fun cachePlaylistChange(uuid: String, shouldAdd: Boolean) { + private fun cachePlaylistChange( + uuid: String, + shouldAdd: Boolean, + playlistTitle: String? = null, + ) { + val change = PlaylistChange( + shouldAdd = shouldAdd, + playlistTitle = playlistTitle, + ) // If the change is not present add it. - playlistsChanges.merge(uuid, shouldAdd) { isCurrentlyAdded, _ -> - if (isCurrentlyAdded == shouldAdd) { + playlistsChanges.merge(uuid, change) { currentChange, _ -> + if (currentChange.shouldAdd == shouldAdd) { // If the change is already stored keep it. - isCurrentlyAdded + currentChange } else { // If playlist was added but will be removed or vice versa remove the change value. null @@ -108,10 +119,27 @@ class AddToPlaylistViewModel @AssistedInject constructor( } } - fun getPlaylistsAddedTo(): Set { - val playlists = uiState.value?.playlistPreviews.orEmpty() - val uuidsAddedTo = playlistsChanges.filterValues { it }.keys - return playlists.filterTo(mutableSetOf()) { playlist -> playlist.uuid in uuidsAddedTo } + fun getPlaylistChangeSummary(): PlaylistChangeSummary { + return PlaylistChangeSummary( + addedCount = playlistsChanges.count { it.value.shouldAdd }, + removedCount = playlistsChanges.count { !it.value.shouldAdd }, + ) + } + + fun getPlaylistChangeFeedback(): PlaylistChangeFeedback { + val singleAddedPlaylist = playlistsChanges + .entries + .singleOrNull { it.value.shouldAdd } + ?.let { (uuid, change) -> + AddedPlaylist( + uuid = uuid, + title = requireNotNull(change.playlistTitle), + ) + } + return PlaylistChangeFeedback.from( + summary = getPlaylistChangeSummary(), + singleAddedPlaylist = singleAddedPlaylist, + ) } fun getArtworkUuidsFlow(playlistUuid: String): StateFlow?> { @@ -141,8 +169,15 @@ class AddToPlaylistViewModel @AssistedInject constructor( } } - fun addToPlaylist(playlistUuid: String) { - cachePlaylistChange(playlistUuid, shouldAdd = true) + fun addToPlaylist( + playlistUuid: String, + playlistTitle: String, + ) { + cachePlaylistChange( + uuid = playlistUuid, + shouldAdd = true, + playlistTitle = playlistTitle, + ) viewModelScope.launch(Dispatchers.Default) { previewsFlow.update { previews -> @@ -179,8 +214,8 @@ class AddToPlaylistViewModel @AssistedInject constructor( viewModelScope.launch { withContext(NonCancellable) { val uuids = episodeUuids.map(EpisodeUuidPair::episodeUuid) - for ((playlistUuid, isAdded) in playlistsChanges) { - if (isAdded) { + for ((playlistUuid, change) in playlistsChanges) { + if (change.shouldAdd) { playlistManager.addManualEpisodes(playlistUuid, uuids) } else { playlistManager.deleteManualEpisodes(playlistUuid, uuids) @@ -291,6 +326,80 @@ class AddToPlaylistViewModel @AssistedInject constructor( val title: String, ) + data class PlaylistChangeSummary( + val addedCount: Int, + val removedCount: Int, + ) + + data class AddedPlaylist( + val uuid: String, + val title: String, + ) + + private data class PlaylistChange( + val shouldAdd: Boolean, + val playlistTitle: String?, + ) + + sealed interface PlaylistChangeFeedback { + data class SinglePlaylistAddition( + @StringRes val resourceId: Int, + val playlist: AddedPlaylist, + ) : PlaylistChangeFeedback + + data class StringResource( + @StringRes val resourceId: Int, + ) : PlaylistChangeFeedback + + data class PluralResource( + @PluralsRes val resourceId: Int, + val quantity: Int, + ) : PlaylistChangeFeedback + + data object None : PlaylistChangeFeedback + + companion object { + fun from( + summary: PlaylistChangeSummary, + singleAddedPlaylist: AddedPlaylist?, + ): PlaylistChangeFeedback { + return when { + summary.addedCount > 0 && summary.removedCount > 0 -> { + PluralResource( + resourceId = LR.plurals.changed_playlists, + quantity = summary.addedCount + summary.removedCount, + ) + } + + summary.addedCount == 1 -> { + SinglePlaylistAddition( + resourceId = LR.string.added_to_playlist_single, + playlist = requireNotNull(singleAddedPlaylist), + ) + } + + summary.addedCount > 1 -> { + PluralResource( + resourceId = LR.plurals.added_to_playlist_single_multiple, + quantity = summary.addedCount, + ) + } + + summary.removedCount == 1 -> StringResource(LR.string.removed_from_playlist_feedback) + + summary.removedCount > 1 -> { + PluralResource( + resourceId = LR.plurals.removed_from_playlists, + quantity = summary.removedCount, + ) + } + + else -> None + } + } + } + } + @AssistedFactory interface Factory { fun create( diff --git a/modules/features/filters/src/test/kotlin/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModelTest.kt b/modules/features/filters/src/test/kotlin/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModelTest.kt index 8fd99e82cfa..c86532ca6ac 100644 --- a/modules/features/filters/src/test/kotlin/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModelTest.kt +++ b/modules/features/filters/src/test/kotlin/au/com/shiftyjelly/pocketcasts/playlists/manual/AddToPlaylistViewModelTest.kt @@ -3,6 +3,12 @@ package au.com.shiftyjelly.pocketcasts.playlists.manual import au.com.shiftyjelly.pocketcasts.analytics.testing.TestEventSink import au.com.shiftyjelly.pocketcasts.models.to.EpisodeUuidPair import au.com.shiftyjelly.pocketcasts.playlists.create.FakePlaylistManager +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.AddedPlaylist +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeFeedback +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeFeedback.PluralResource +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeFeedback.SinglePlaylistAddition +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeFeedback.StringResource +import au.com.shiftyjelly.pocketcasts.playlists.manual.AddToPlaylistViewModel.PlaylistChangeSummary import au.com.shiftyjelly.pocketcasts.sharedtest.MainCoroutineRule import au.com.shiftyjelly.pocketcasts.views.swipe.AddToPlaylistFragmentFactory import com.automattic.eventhorizon.EventHorizon @@ -11,6 +17,7 @@ import org.junit.Assert.assertEquals import org.junit.Before import org.junit.Rule import org.junit.Test +import au.com.shiftyjelly.pocketcasts.localization.R as LR class AddToPlaylistViewModelTest { @get:Rule @@ -33,16 +40,119 @@ class AddToPlaylistViewModelTest { ) } + @Test + fun `playlist change summary reports net added and removed playlist counts`() = runTest(coroutineRule.testDispatcher) { + assertEquals( + PlaylistChangeSummary(addedCount = 0, removedCount = 0), + viewModel.getPlaylistChangeSummary(), + ) + + viewModel.addToPlaylist("playlist-uuid-1", "Playlist 1") + viewModel.addToPlaylist("playlist-uuid-2", "Playlist 2") + viewModel.removeFromPlaylist("playlist-uuid-3") + + assertEquals( + PlaylistChangeSummary(addedCount = 2, removedCount = 1), + viewModel.getPlaylistChangeSummary(), + ) + + viewModel.removeFromPlaylist("playlist-uuid-2") + viewModel.addToPlaylist("playlist-uuid-3", "Playlist 3") + + assertEquals( + PlaylistChangeSummary(addedCount = 1, removedCount = 0), + viewModel.getPlaylistChangeSummary(), + ) + } + + @Test + fun `playlist change feedback selects the correct message for every outcome`() = runTest(coroutineRule.testDispatcher) { + val addedPlaylist = AddedPlaylist(uuid = "playlist-uuid", title = "Playlist title") + val cases = listOf( + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 0, removedCount = 0), + singleAddedPlaylist = null, + expectedFeedback = PlaylistChangeFeedback.None, + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 1, removedCount = 0), + singleAddedPlaylist = addedPlaylist, + expectedFeedback = SinglePlaylistAddition( + resourceId = LR.string.added_to_playlist_single, + playlist = addedPlaylist, + ), + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 2, removedCount = 0), + singleAddedPlaylist = null, + expectedFeedback = PluralResource(LR.plurals.added_to_playlist_single_multiple, quantity = 2), + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 0, removedCount = 1), + singleAddedPlaylist = null, + expectedFeedback = StringResource(LR.string.removed_from_playlist_feedback), + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 0, removedCount = 2), + singleAddedPlaylist = null, + expectedFeedback = PluralResource(LR.plurals.removed_from_playlists, quantity = 2), + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 1, removedCount = 1), + singleAddedPlaylist = addedPlaylist, + expectedFeedback = PluralResource(LR.plurals.changed_playlists, quantity = 2), + ), + FeedbackCase( + summary = PlaylistChangeSummary(addedCount = 2, removedCount = 1), + singleAddedPlaylist = null, + expectedFeedback = PluralResource(LR.plurals.changed_playlists, quantity = 3), + ), + ) + + cases.forEach { case -> + assertEquals( + case.expectedFeedback, + PlaylistChangeFeedback.from(case.summary, case.singleAddedPlaylist), + ) + } + } + + @Test + fun `single playlist addition feedback keeps the cached playlist title and uuid`() = runTest( + coroutineRule.testDispatcher, + ) { + viewModel.addToPlaylist( + playlistUuid = "playlist-uuid", + playlistTitle = "Playlist title", + ) + viewModel.addToPlaylist( + playlistUuid = "other-playlist-uuid", + playlistTitle = "Other playlist", + ) + viewModel.removeFromPlaylist("other-playlist-uuid") + + assertEquals( + SinglePlaylistAddition( + resourceId = LR.string.added_to_playlist_single, + playlist = AddedPlaylist( + uuid = "playlist-uuid", + title = "Playlist title", + ), + ), + viewModel.getPlaylistChangeFeedback(), + ) + } + @Test fun `submit playlist changes only when committing`() = runTest(coroutineRule.testDispatcher) { - viewModel.addToPlaylist("playlist-uuid-1") + viewModel.addToPlaylist("playlist-uuid-1", "Playlist 1") viewModel.removeFromPlaylist("playlist-uuid-1") - viewModel.addToPlaylist("playlist-uuid-1") + viewModel.addToPlaylist("playlist-uuid-1", "Playlist 1") - viewModel.addToPlaylist("playlist-uuid-2") + viewModel.addToPlaylist("playlist-uuid-2", "Playlist 2") viewModel.removeFromPlaylist("playlist-uuid-2") - viewModel.addToPlaylist("playlist-uuid-3") + viewModel.addToPlaylist("playlist-uuid-3", "Playlist 3") viewModel.removeFromPlaylist("playlist-uuid-4") @@ -71,4 +181,10 @@ class AddToPlaylistViewModelTest { playlistManager.addManualEpisodeTurbine.expectNoEvents() playlistManager.deleteManualEpisodeTurbine.expectNoEvents() } + + private data class FeedbackCase( + val summary: PlaylistChangeSummary, + val singleAddedPlaylist: AddedPlaylist?, + val expectedFeedback: PlaylistChangeFeedback, + ) } diff --git a/modules/services/localization/src/main/res/values/strings.xml b/modules/services/localization/src/main/res/values/strings.xml index 88c2dca9da3..83dde496a09 100644 --- a/modules/services/localization/src/main/res/values/strings.xml +++ b/modules/services/localization/src/main/res/values/strings.xml @@ -955,10 +955,22 @@ This playlist is full. Remove a few episodes or start a new one. Added to %1$s + Added to %d playlist Added to %d playlists + Removed from playlist + + + Removed from %d playlist + Removed from %d playlists + + + + Changed %d playlist + Changed %d playlists + Create playlist Create smart playlist Decrement \"longer than\" duration