From da224ffda0142be45da4851bd1a32c7fae9301e0 Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Tue, 17 Nov 2020 11:23:14 -0700 Subject: [PATCH] Fix persistence bugs with notification Fix issues where the notification for Auxio will show up incomplete during the restore process. --- .../java/org/oxycblt/auxio/MainFragment.kt | 11 +-- .../java/org/oxycblt/auxio/music/Models.kt | 47 ++++++++-- .../auxio/playback/NotificationUtils.kt | 20 +++-- .../oxycblt/auxio/playback/PlaybackService.kt | 88 +++++++++---------- .../auxio/playback/PlaybackViewModel.kt | 19 ++-- .../playback/state/PlaybackStateManager.kt | 15 ++-- 6 files changed, 122 insertions(+), 78 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/MainFragment.kt b/app/src/main/java/org/oxycblt/auxio/MainFragment.kt index c79b50429..7ea262fb6 100644 --- a/app/src/main/java/org/oxycblt/auxio/MainFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/MainFragment.kt @@ -12,7 +12,6 @@ import androidx.fragment.app.Fragment import androidx.fragment.app.activityViewModels import androidx.navigation.NavController import androidx.navigation.NavOptions -import androidx.navigation.findNavController import androidx.navigation.fragment.NavHostFragment import androidx.navigation.fragment.findNavController import androidx.navigation.ui.NavigationUI @@ -50,8 +49,8 @@ class MainFragment : Fragment() { getInactiveAlpha(accent.first) ) - // Set up the tints for the navigation icon - val navIconTints = ColorStateList( + // Set up the tints for the navigation icons + text + val navTints = ColorStateList( arrayOf( intArrayOf(-android.R.attr.state_checked), intArrayOf(android.R.attr.state_checked) @@ -68,8 +67,8 @@ class MainFragment : Fragment() { binding.lifecycleOwner = this - binding.navBar.itemIconTintList = navIconTints - binding.navBar.itemTextColor = navIconTints + binding.navBar.itemIconTintList = navTints + binding.navBar.itemTextColor = navTints navController?.let { binding.navBar.setOnNavigationItemSelectedListener { item -> @@ -93,6 +92,8 @@ class MainFragment : Fragment() { } } + playbackModel.restorePlaybackIfNeeded(requireContext()) + Log.d(this::class.simpleName, "Fragment Created.") return binding.root diff --git a/app/src/main/java/org/oxycblt/auxio/music/Models.kt b/app/src/main/java/org/oxycblt/auxio/music/Models.kt index 9bb12f588..d0f63c3a3 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Models.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/Models.kt @@ -4,14 +4,27 @@ import android.net.Uri // --- MUSIC MODELS --- -// The base model for all music -// This is used in a lot of general functions in order to have generic utilities +/** + * The base data object for all music. + * @property id The ID that is assigned to this object + * @property name The name of this object (Such as a song title) + * @author OxygenCobalt + */ sealed class BaseModel { abstract val id: Long abstract val name: String } -// Song +/** + * The data object for a song. Inherits [BaseModel]. + * @property albumId The Song's Album ID. Never use this outside of when attaching a song to its album. + * @property track The Song's Track number + * @property duration The duration of the song, in millis. + * @property album The Song's parent album. Use this instead of [albumId]. + * @property seconds The Song's duration in seconds + * @property formattedDuration The Song's duration as a duration string. + * @author OxygenCobalt + */ data class Song( override val id: Long = -1, override var name: String, @@ -25,7 +38,17 @@ data class Song( val formattedDuration: String = seconds.toDuration() } -// Album +/** + * The data object for an album. Inherits [BaseModel]. + * @property artistId The Album's parent artist ID. Do not use this outside of attaching an album to its parent artist. + * @property coverUri The [Uri] for the album's cover. **Load this using Coil.** + * @property year The year this album was released. 0 if there is none in the metadata. + * @property artist The Album's parent [Artist]. use this instead of [artistId] + * @property songs The Album's child [Song]s. + * @property numSongs The amount of songs in an Album. + * @property totalDuration The combined duration of all of the album's child songs, formatted. + * @author OxygenCobalt + */ data class Album( override val id: Long = -1, override val name: String, @@ -46,7 +69,10 @@ data class Album( } } -// Artist +/** + * The data object for an artist. Inherits [BaseModel] + * @author OxygenCobalt + */ data class Artist( override val id: Long = -1, override var name: String @@ -71,7 +97,12 @@ data class Artist( } } -// Genre +/** + * The data object for a genre. Inherits [BaseModel] + * @property artists The list of all [Artist]s in this genre + * @property songs The list of all [Song]s in this genre. + * @author OxygenCobalt + */ data class Genre( override val id: Long = -1, override var name: String, @@ -102,7 +133,9 @@ data class Genre( } } -// Header [Used for search, nothing else] +/** + * A data object used solely for the "Header" UI element. Inherits [BaseModel]. + */ data class Header( override val id: Long = -1, override var name: String = "", diff --git a/app/src/main/java/org/oxycblt/auxio/playback/NotificationUtils.kt b/app/src/main/java/org/oxycblt/auxio/playback/NotificationUtils.kt index 9c88c4d87..925b65b80 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/NotificationUtils.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/NotificationUtils.kt @@ -19,8 +19,6 @@ import org.oxycblt.auxio.playback.state.PlaybackMode import org.oxycblt.auxio.playback.state.PlaybackStateManager object NotificationUtils { - val DO_COMPAT_SUBTEXT = Build.VERSION.SDK_INT < Build.VERSION_CODES.O - const val CHANNEL_ID = "CHANNEL_AUXIO_PLAYBACK" const val NOTIFICATION_ID = 0xA0A0 const val REQUEST_CODE = 0xA0C0 @@ -81,6 +79,13 @@ fun NotificationManager.createMediaNotification( .setVisibility(NotificationCompat.VISIBILITY_PUBLIC) } +/** + * Set the current metadata of a media notification. + * @param song The [Song] that the notification should reflect + * @param context The [Context] needed to load the cover bitmap + * @param onDone A callback for when the process is finished + * @author OxygenCobalt + */ fun NotificationCompat.Builder.setMetadata(song: Song, context: Context, onDone: () -> Unit) { setContentTitle(song.name) setContentText( @@ -89,7 +94,7 @@ fun NotificationCompat.Builder.setMetadata(song: Song, context: Context, onDone: // On older versions of android [API <26], show the song's album on the subtext instead of // the current mode, as that makes more sense for the old style of media notifications. - if (NotificationUtils.DO_COMPAT_SUBTEXT) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) { setSubText(song.album.name) } @@ -102,7 +107,7 @@ fun NotificationCompat.Builder.setMetadata(song: Song, context: Context, onDone: } } -// I have no idea how to update actions on the fly so I have to use these restricted APIs. +// I have no idea how to update a specific action on the fly so I have to use these restricted APIs @SuppressLint("RestrictedApi") fun NotificationCompat.Builder.updatePlaying(context: Context) { mActions[2] = newAction(NotificationUtils.ACTION_PLAY_PAUSE, context) @@ -114,7 +119,7 @@ fun NotificationCompat.Builder.updateLoop(context: Context) { } fun NotificationCompat.Builder.updateMode(context: Context) { - if (!NotificationUtils.DO_COMPAT_SUBTEXT) { + if (Build.VERSION.SDK_INT > Build.VERSION_CODES.O) { val playbackManager = PlaybackStateManager.getInstance() // If playing from all songs, set the subtext as that, otherwise the currently played parent. @@ -126,6 +131,11 @@ fun NotificationCompat.Builder.updateMode(context: Context) { } } +/** + * Create a new [NotificationCompat.Action]. + * @param action The action that the notification action should represent + * @param context The [Context] needed to create the action + */ private fun newAction(action: String, context: Context): NotificationCompat.Action { val playbackManager = PlaybackStateManager.getInstance() diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackService.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackService.kt index 6ff3bfb6e..78884e995 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackService.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackService.kt @@ -145,6 +145,19 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca if (playbackManager.song != null) { restorePlayer() + notification.updateLoop(this) + notification.updateMode(this) + notification.updatePlaying(this) + + playbackManager.song?.let { + notification.setMetadata(it, this) { + if (playbackManager.isPlaying) { + startForegroundOrNotify("Restore") + } else { + stopForegroundAndNotification() + } + } + } } } @@ -223,8 +236,13 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca } uploadMetadataToSession(it) - notification.setMetadata(playbackManager.song!!, this) { - startForegroundOrNotify() + + if (playbackManager.isRestored) { + notification.setMetadata(playbackManager.song!!, this) { + startForegroundOrNotify("Song") + } + } else { + notification.setMetadata(playbackManager.song!!, this) {} } return @@ -238,7 +256,7 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca override fun onModeUpdate(mode: PlaybackMode) { notification.updateMode(this) - startForegroundOrNotify() + startForegroundOrNotify("Mode") } override fun onPlayingUpdate(isPlaying: Boolean) { @@ -247,13 +265,13 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca if (isPlaying && !player.isPlaying) { player.play() notification.updatePlaying(this) - startForegroundOrNotify() + startForegroundOrNotify("Play") startPollingPosition() } else { player.pause() notification.updatePlaying(this) - startForegroundOrNotify() + startForegroundOrNotify("Pause") } } @@ -270,7 +288,7 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca } notification.updateLoop(this) - startForegroundOrNotify() + startForegroundOrNotify("Loop") } override fun onSeekConfirm(position: Long) { @@ -279,20 +297,6 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca player.seekTo(position) } - override fun onNeedContextToRestoreState() { - Log.d(this::class.simpleName, "Giving context to PlaybackStateManager") - - serviceScope.launch { - playbackManager.getStateFromDatabase(this@PlaybackService) - } - - Log.d(this::class.simpleName, "notification restore") - - // FIXME: Current position just will not show on notification when state is restored - restorePlayer() - restoreNotification() - } - // --- OTHER FUNCTIONS --- private fun restorePlayer() { @@ -301,27 +305,13 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca player.setMediaItem(item) player.prepare() player.seekTo(playbackManager.position) - - notification.setMetadata(it, this) { - startForegroundOrNotify() - } } } - private fun restoreNotification() { - playbackManager.song?.let { - uploadMetadataToSession(it) - notification.updateLoop(this) - notification.updateMode(this) - notification.updatePlaying(this) - notification.setMetadata(it, this) { - startForegroundOrNotify() - } + override fun onRestoreFinish() { + Log.d(this::class.simpleName, "Restore done") - return - } - - stopForegroundAndNotification() + restorePlayer() } private fun uploadMetadataToSession(song: Song) { @@ -356,19 +346,23 @@ class PlaybackService : Service(), Player.EventListener, PlaybackStateManager.Ca } } - private fun startForegroundOrNotify() { + private fun startForegroundOrNotify(reason: String) { // Start the service in the foreground if haven't already. - if (!isForeground) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - startForeground( - NotificationUtils.NOTIFICATION_ID, notification.build(), - ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK - ) + if (playbackManager.isRestored) { + Log.d(this::class.simpleName, "Starting foreground because of $reason") + + if (!isForeground) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + startForeground( + NotificationUtils.NOTIFICATION_ID, notification.build(), + ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK + ) + } else { + startForeground(NotificationUtils.NOTIFICATION_ID, notification.build()) + } } else { - startForeground(NotificationUtils.NOTIFICATION_ID, notification.build()) + notificationManager.notify(NotificationUtils.NOTIFICATION_ID, notification.build()) } - } else { - notificationManager.notify(NotificationUtils.NOTIFICATION_ID, notification.build()) } } diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt index 087e819de..b92afc9c5 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt @@ -1,10 +1,13 @@ package org.oxycblt.auxio.playback +import android.content.Context import android.util.Log import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.Transformations import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope +import kotlinx.coroutines.launch import org.oxycblt.auxio.music.Album import org.oxycblt.auxio.music.Artist import org.oxycblt.auxio.music.BaseModel @@ -78,14 +81,10 @@ class PlaybackViewModel : ViewModel(), PlaybackStateManager.Callback { // If the PlaybackViewModel was cleared [Signified by PlaybackStateManager still being // around & the fact that we are in the init function], then attempt to restore the - // viewmodel state. + // viewmodel state. If it isn't, then wait for MainFragment to give the command to restore + // PlaybackStateManager. if (playbackManager.isRestored) { restorePlaybackState() - } else { - // If [PlaybackStateManger] was also cleared [Due to Auxio's process dying], then - // attempt to restore it [Albeit PlaybackService will need to do the job as it - // has a context while PlaybackViewModel doesn't]. - playbackManager.needContextToRestoreState() } } @@ -249,6 +248,14 @@ class PlaybackViewModel : ViewModel(), PlaybackStateManager.Callback { mIsSeeking.value = value } + fun restorePlaybackIfNeeded(context: Context) { + if (!playbackManager.isRestored) { + viewModelScope.launch { + playbackManager.getStateFromDatabase(context) + } + } + } + // --- OVERRIDES --- override fun onCleared() { diff --git a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt index d9569f72f..7fb84ee36 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt @@ -24,7 +24,6 @@ import kotlin.random.Random * * All instantiation should be done with [PlaybackStateManager.from()]. * @author OxygenCobalt - * FIXME: Add started variable instead of relying on mSong being null */ class PlaybackStateManager private constructor() { // Playback @@ -455,10 +454,6 @@ class PlaybackStateManager private constructor() { // TODO: Persist queue edits? // FIXME: Shuffling w/o knowing the original queue edit from keepSong will cause issues - fun needContextToRestoreState() { - callbacks.forEach { it.onNeedContextToRestoreState() } - } - suspend fun saveStateToDatabase(context: Context) { Log.d(this::class.simpleName, "Saving state to DB.") @@ -483,16 +478,19 @@ class PlaybackStateManager private constructor() { return@withContext states } - mIsRestored = true - if (states.isEmpty()) { Log.d(this::class.simpleName, "Nothing here. Not restoring.") + + mIsRestored = true + return } Log.d(this::class.simpleName, "Old state found, ${states[0]}") unpackFromPlaybackState(states[0]) + + mIsRestored = true } private fun packToPlaybackState(): PlaybackState { @@ -580,6 +578,7 @@ class PlaybackStateManager private constructor() { callbacks.forEach { it.onSeekConfirm(mPosition) it.onModeUpdate(mMode) + it.onRestoreFinish() } } @@ -600,7 +599,7 @@ class PlaybackStateManager private constructor() { fun onShuffleUpdate(isShuffling: Boolean) {} fun onLoopUpdate(mode: LoopMode) {} fun onSeekConfirm(position: Long) {} - fun onNeedContextToRestoreState() {} + fun onRestoreFinish() {} } companion object {