all: phase out taskguard

Phase out the dumb hack TaskGuard class in favor of yield.

For some reason, I was under the impression that yield was horribly
slow. It's not, I was just using it wrong. So now TaskGuard is no
longer needed.

app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt

@@ -24,9 +24,11 @@ import androidx.annotation.StringRes
 import androidx.lifecycle.AndroidViewModel
 import androidx.lifecycle.viewModelScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.yield
 import org.oxycblt.auxio.R
 import org.oxycblt.auxio.music.Album
 import org.oxycblt.auxio.music.Artist
@@ -40,7 +42,6 @@ import org.oxycblt.auxio.settings.Settings
 import org.oxycblt.auxio.ui.Sort
 import org.oxycblt.auxio.ui.recycler.Header
 import org.oxycblt.auxio.ui.recycler.Item
-import org.oxycblt.auxio.util.TaskGuard
 import org.oxycblt.auxio.util.application
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.logW
@@ -70,6 +71,8 @@ class DetailViewModel(application: Application) :
     val currentSong: StateFlow<DetailSong?>
         get() = _currentSong
 
+    private var currentSongJob: Job? = null
+
     private val _currentAlbum = MutableStateFlow<Album?>(null)
     val currentAlbum: StateFlow<Album?>
         get() = _currentAlbum
@@ -114,8 +117,6 @@ class DetailViewModel(application: Application) :
             currentGenre.value?.let(::refreshGenreData)
         }
 
-    private val songGuard = TaskGuard()
-
     fun setSongUid(uid: Music.UID) {
         if (_currentSong.value?.run { song.uid } == uid) return
         val library = unlikelyToBeNull(musicStore.library)
@@ -124,7 +125,6 @@ class DetailViewModel(application: Application) :
     }
 
     fun clearSong() {
-        songGuard.newHandle()
         _currentSong.value = null
     }
 
@@ -159,11 +159,11 @@ class DetailViewModel(application: Application) :
     }
 
     private fun generateDetailSong(song: Song) {
+        currentSongJob?.cancel()
         _currentSong.value = DetailSong(song, null)
-        viewModelScope.launch(Dispatchers.IO) {
+        currentSongJob = viewModelScope.launch(Dispatchers.IO) {
-            val handle = songGuard.newHandle()
             val info = generateDetailSongInfo(song)
-            songGuard.yield(handle)
+            yield()
             _currentSong.value = DetailSong(song, info)
         }
     }

app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt

@@ -25,7 +25,6 @@ import coil.request.Disposable
 import coil.request.ImageRequest
 import coil.size.Size
 import org.oxycblt.auxio.music.Song
-import org.oxycblt.auxio.util.TaskGuard
 
 /**
  * A utility to provide bitmaps in a manner less prone to race conditions.
@@ -38,7 +37,8 @@ import org.oxycblt.auxio.util.TaskGuard
  */
 class BitmapProvider(private val context: Context) {
     private var currentRequest: Request? = null
-    private var guard = TaskGuard()
+    private var currentHandle = 0L
+    private var handleLock = Any()
 
     /** If this provider is currently attempting to load something. */
     val isBusy: Boolean
@@ -50,7 +50,9 @@ class BitmapProvider(private val context: Context) {
      */
     @Synchronized
     fun load(song: Song, target: Target) {
-        val handle = guard.newHandle()
+        val handle = synchronized(handleLock) {
+            ++currentHandle
+        }
 
         currentRequest?.run { disposable.dispose() }
         currentRequest = null
@@ -62,13 +64,17 @@ class BitmapProvider(private val context: Context) {
                     .size(Size.ORIGINAL)
                     .target(
                         onSuccess = {
-                            if (guard.check(handle)) {
+                            synchronized(handleLock) {
-                                target.onCompleted(it.toBitmap())
+                                if (currentHandle == handle) {
+                                    target.onCompleted(it.toBitmap())
+                                }
                             }
                         },
                         onError = {
-                            if (guard.check(handle)) {
+                            synchronized(handleLock) {
-                                target.onCompleted(null)
+                                if (currentHandle == handle) {
+                                    target.onCompleted(null)
+                                }
                             }
                         }
                     )

app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataLayer.kt

@@ -54,7 +54,7 @@ class MetadataLayer(private val context: Context, private val mediaStoreLayer: M
     /** Finalize the sub-layers that this layer relies on. */
     fun finalize(rawSongs: List<Song.Raw>) = mediaStoreLayer.finalize(rawSongs)
 
-    fun parse(emit: (Song.Raw) -> Unit) {
+    suspend fun parse(emit: suspend (Song.Raw) -> Unit) {
         while (true) {
             val raw = Song.Raw()
             if (mediaStoreLayer.populateRaw(raw) ?: break) {

app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt

@@ -25,6 +25,7 @@ import androidx.core.content.ContextCompat
 import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
+import kotlinx.coroutines.yield
 import org.oxycblt.auxio.BuildConfig
 import org.oxycblt.auxio.music.Album
 import org.oxycblt.auxio.music.Artist
@@ -37,7 +38,6 @@ import org.oxycblt.auxio.music.extractor.Api30MediaStoreLayer
 import org.oxycblt.auxio.music.extractor.CacheLayer
 import org.oxycblt.auxio.music.extractor.MetadataLayer
 import org.oxycblt.auxio.ui.Sort
-import org.oxycblt.auxio.util.TaskGuard
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.logE
 import org.oxycblt.auxio.util.logW
@@ -62,14 +62,11 @@ import org.oxycblt.auxio.util.logW
  * directly work with music loading, making such redundant.
  *
  * @author OxygenCobalt
- *
- * TODO: Try to replace TaskGuard with yield when possible
  */
 class Indexer {
     private var lastResponse: Response? = null
     private var indexingState: Indexing? = null
 
-    private var guard = TaskGuard()
     private var controller: Controller? = null
     private var callback: Callback? = null
 
@@ -136,21 +133,19 @@ class Indexer {
      * complete, a new completion state will be pushed to each callback.
      */
     suspend fun index(context: Context) {
-        val handle = guard.newHandle()
-
         val notGranted =
             ContextCompat.checkSelfPermission(context, PERMISSION_READ_AUDIO) ==
                 PackageManager.PERMISSION_DENIED
 
         if (notGranted) {
-            emitCompletion(Response.NoPerms, handle)
+            emitCompletion(Response.NoPerms)
             return
         }
 
         val response =
             try {
                 val start = System.currentTimeMillis()
-                val library = indexImpl(context, handle)
+                val library = indexImpl(context)
                 if (library != null) {
                     logD(
                         "Music indexing completed successfully in " +
@@ -171,7 +166,7 @@ class Indexer {
                 Response.Err(e)
             }
 
-        emitCompletion(response, handle)
+        emitCompletion(response)
     }
 
     /**
@@ -192,16 +187,14 @@ class Indexer {
     @Synchronized
     fun cancelLast() {
         logD("Cancelling last job")
-        val handle = guard.newHandle()
+        emitIndexing(null)
-        emitIndexing(null, handle)
     }
 
     /**
-     * Run the proper music loading process. [handle] must be a truthful handle of the task calling
+     * Run the proper music loading process.
-     * this function.
      */
-    private fun indexImpl(context: Context, handle: Long): MusicStore.Library? {
+    private suspend fun indexImpl(context: Context): MusicStore.Library? {
-        emitIndexing(Indexing.Indeterminate, handle)
+        emitIndexing(Indexing.Indeterminate)
 
         // Create the chain of layers. Each layer builds on the previous layer and
         // enables version-specific features in order to create the best possible music
@@ -221,7 +214,7 @@ class Indexer {
 
         val metadataLayer = MetadataLayer(context, mediaStoreLayer)
 
-        val songs = buildSongs(metadataLayer, handle)
+        val songs = buildSongs(metadataLayer)
         if (songs.isEmpty()) {
             return null
         }
@@ -248,12 +241,15 @@ class Indexer {
      * [buildGenres] functions must be called with the returned list so that all songs are properly
      * linked up.
      */
-    private fun buildSongs(metadataLayer: MetadataLayer, handle: Long): List<Song> {
+    private suspend fun buildSongs(metadataLayer: MetadataLayer): List<Song> {
+        logD("Starting indexing process")
+
         val start = System.currentTimeMillis()
 
         // Initialize the extractor chain. This also nets us the projected total
         // that we can show when loading.
         val total = metadataLayer.init()
+        yield()
 
         // Note: We use a set here so we can eliminate effective duplicates of
         // songs (by UID).
@@ -263,7 +259,10 @@ class Indexer {
         metadataLayer.parse { raw ->
             songs.add(Song(raw))
             rawSongs.add(raw)
-            emitIndexing(Indexing.Songs(songs.size, total), handle)
+
+            // Check if we got cancelled after every song addition.
+            yield()
+            emitIndexing(Indexing.Songs(songs.size, total))
         }
 
         metadataLayer.finalize(rawSongs)
@@ -351,15 +350,7 @@ class Indexer {
     }
 
     @Synchronized
-    private fun emitIndexing(indexing: Indexing?, handle: Long) {
+    private fun emitIndexing(indexing: Indexing?) {
-        guard.yield(handle)
-
-        if (indexing == indexingState) {
-            // Ignore redundant states used when the backends just want to check for
-            // a cancellation
-            return
-        }
-
         indexingState = indexing
 
         // If we have canceled the loading process, we want to revert to a previous completion
@@ -371,8 +362,8 @@ class Indexer {
         callback?.onIndexerStateChanged(state)
     }
 
-    private suspend fun emitCompletion(response: Response, handle: Long) {
+    private suspend fun emitCompletion(response: Response) {
-        guard.yield(handle)
+        yield()
 
         // Swap to the Main thread so that downstream callbacks don't crash from being on
         // a background thread. Does not occur in emitIndexing due to efficiency reasons.

app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt

@@ -57,6 +57,7 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback {
 
     private val serviceJob = Job()
     private val indexScope = CoroutineScope(serviceJob + Dispatchers.IO)
+    private var currentIndexJob: Job? = null
 
     private val playbackManager = PlaybackStateManager.getInstance()
 
@@ -118,10 +119,11 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback {
 
     override fun onStartIndexing() {
         if (indexer.isIndexing) {
+            currentIndexJob?.cancel()
             indexer.cancelLast()
         }
 
-        indexScope.launch { indexer.index(this@IndexerService) }
+        currentIndexJob = indexScope.launch { indexer.index(this@IndexerService) }
     }
 
     override fun onIndexerStateChanged(state: Indexer.State?) {

app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt

@@ -307,6 +307,7 @@ class PlaybackViewModel(application: Application) :
 
     override fun onStateChanged(state: InternalPlayer.State) {
         _isPlaying.value = state.isPlaying
+        _positionDs.value = state.calculateElapsedPosition().msToDs()
 
         // Start watching the position again
         lastPositionJob?.cancel()

app/src/main/java/org/oxycblt/auxio/playback/state/InternalPlayer.kt

@@ -30,7 +30,7 @@ interface InternalPlayer {
     /** Whether the player should rewind instead of going to the previous song. */
     val shouldRewindWithPrev: Boolean
 
-    val currentState: State
+    fun makeState(durationMs: Long): State
 
     /** Called when a new song should be loaded into the player. */
     fun loadSong(song: Song?, play: Boolean)

app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt

@@ -342,7 +342,7 @@ class PlaybackStateManager private constructor() {
             return
         }
 
-        val newState = internalPlayer.currentState
+        val newState = internalPlayer.makeState(song?.durationMs ?: 0)
         if (newState != playerState) {
             playerState = newState
             notifyStateChanged()

app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt

@@ -58,6 +58,7 @@ import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.widgets.WidgetComponent
 import org.oxycblt.auxio.widgets.WidgetProvider
 import kotlin.math.max
+import kotlin.math.min
 
 /**
  * A service that manages the system-side aspects of playback, such as:
@@ -214,6 +215,60 @@ class PlaybackService :
         logD("Service destroyed")
     }
 
+    // --- CONTROLLER OVERRIDES ---
+
+    override val audioSessionId: Int
+        get() = player.audioSessionId
+
+    override val shouldRewindWithPrev: Boolean
+        get() = settings.rewindWithPrev && player.currentPosition > REWIND_THRESHOLD
+
+    override fun makeState(durationMs: Long) =
+        InternalPlayer.State.new(
+            player.playWhenReady,
+            player.isPlaying,
+            max(min(player.currentPosition, durationMs), 0)
+        )
+
+    override fun loadSong(song: Song?, play: Boolean) {
+        if (song == null) {
+            // Stop the foreground state if there's nothing to play.
+            logD("Nothing playing, stopping playback")
+            player.stop()
+            if (openAudioEffectSession) {
+                // Make sure to close the audio session when we stop playback.
+                broadcastAudioEffectAction(AudioEffect.ACTION_CLOSE_AUDIO_EFFECT_CONTROL_SESSION)
+                openAudioEffectSession = false
+            }
+
+            stopAndSave()
+            return
+        }
+
+        logD("Loading ${song.rawName}")
+        player.setMediaItem(MediaItem.fromUri(song.uri))
+        player.prepare()
+
+        if (!openAudioEffectSession) {
+            // Android does not like it if you start an audio effect session without having
+            // something within your player buffer. Make sure we only start one when we load
+            // a song.
+            broadcastAudioEffectAction(AudioEffect.ACTION_OPEN_AUDIO_EFFECT_CONTROL_SESSION)
+            openAudioEffectSession = true
+        }
+
+        player.playWhenReady = play
+    }
+
+    override fun seekTo(positionMs: Long) {
+        logD("Seeking to ${positionMs}ms")
+        player.seekTo(positionMs)
+    }
+
+    override fun changePlaying(isPlaying: Boolean) {
+        player.playWhenReady = isPlaying
+    }
+
     // --- PLAYER OVERRIDES ---
 
     override fun onEvents(player: Player, events: Player.Events) {
@@ -270,52 +325,27 @@ class PlaybackService :
         }
     }
 
-    // --- CONTROLLER OVERRIDES ---
+    // --- MUSICSTORE OVERRIDES ---
 
-    override val audioSessionId: Int
+    override fun onLibraryChanged(library: MusicStore.Library?) {
-        get() = player.audioSessionId
+        if (library != null) {
-
+            playbackManager.requestAction(this)
-    override val shouldRewindWithPrev: Boolean
-        get() = settings.rewindWithPrev && player.currentPosition > REWIND_THRESHOLD
-
-    override val currentState: InternalPlayer.State
-        get() =
-            InternalPlayer.State.new(
-                player.playWhenReady,
-                player.isPlaying,
-                max(player.currentPosition, 0)
-            )
-
-    override fun loadSong(song: Song?, play: Boolean) {
-        if (song == null) {
-            // Stop the foreground state if there's nothing to play.
-            logD("Nothing playing, stopping playback")
-            player.stop()
-            if (openAudioEffectSession) {
-                // Make sure to close the audio session when we stop playback.
-                broadcastAudioEffectAction(AudioEffect.ACTION_CLOSE_AUDIO_EFFECT_CONTROL_SESSION)
-                openAudioEffectSession = false
-            }
-
-            stopAndSave()
-            return
         }
-
-        logD("Loading ${song.rawName}")
-        player.setMediaItem(MediaItem.fromUri(song.uri))
-        player.prepare()
-
-        if (!openAudioEffectSession) {
-            // Android does not like it if you start an audio effect session without having
-            // something within your player buffer. Make sure we only start one when we load
-            // a song.
-            broadcastAudioEffectAction(AudioEffect.ACTION_OPEN_AUDIO_EFFECT_CONTROL_SESSION)
-            openAudioEffectSession = true
-        }
-
-        player.playWhenReady = play
     }
 
+    // --- SETTINGSMANAGER OVERRIDES ---
+
+    override fun onSettingChanged(key: String) {
+        if (key == getString(R.string.set_key_replay_gain) ||
+            key == getString(R.string.set_key_pre_amp_with) ||
+            key == getString(R.string.set_key_pre_amp_without)
+        ) {
+            onTracksChanged(player.currentTracks)
+        }
+    }
+
+    // --- OTHER FUNCTIONS ---
+
     private fun broadcastAudioEffectAction(event: String) {
         sendBroadcast(
             Intent(event)
@@ -337,15 +367,6 @@ class PlaybackService :
         }
     }
 
-    override fun seekTo(positionMs: Long) {
-        logD("Seeking to ${positionMs}ms")
-        player.seekTo(positionMs)
-    }
-
-    override fun changePlaying(isPlaying: Boolean) {
-        player.playWhenReady = isPlaying
-    }
-
     override fun onAction(action: InternalPlayer.Action): Boolean {
         val library = musicStore.library
         if (library != null) {
@@ -397,27 +418,6 @@ class PlaybackService :
         }
     }
 
-    // --- MUSICSTORE OVERRIDES ---
-
-    override fun onLibraryChanged(library: MusicStore.Library?) {
-        if (library != null) {
-            playbackManager.requestAction(this)
-        }
-    }
-
-    // --- SETTINGSMANAGER OVERRIDES ---
-
-    override fun onSettingChanged(key: String) {
-        if (key == getString(R.string.set_key_replay_gain) ||
-            key == getString(R.string.set_key_pre_amp_with) ||
-            key == getString(R.string.set_key_pre_amp_without)
-        ) {
-            onTracksChanged(player.currentTracks)
-        }
-    }
-
-    // --- OTHER FUNCTIONS ---
-
     /** A [BroadcastReceiver] for receiving general playback events from the system. */
     private inner class PlaybackReceiver : BroadcastReceiver() {
         private var initialHeadsetPlugEventHandled = false

app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt

@@ -23,9 +23,11 @@ import androidx.annotation.IdRes
 import androidx.lifecycle.AndroidViewModel
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.yield
 import org.oxycblt.auxio.R
 import org.oxycblt.auxio.music.Album
 import org.oxycblt.auxio.music.Artist
@@ -38,7 +40,6 @@ import org.oxycblt.auxio.ui.DisplayMode
 import org.oxycblt.auxio.ui.Sort
 import org.oxycblt.auxio.ui.recycler.Header
 import org.oxycblt.auxio.ui.recycler.Item
-import org.oxycblt.auxio.util.TaskGuard
 import org.oxycblt.auxio.util.application
 import org.oxycblt.auxio.util.logD
 import java.text.Normalizer
@@ -62,15 +63,16 @@ class SearchViewModel(application: Application) :
         get() = settings.searchFilterMode
 
     private var lastQuery: String? = null
-    private var guard = TaskGuard()
+    private var currentSearchJob: Job? = null
 
     /**
      * Use [query] to perform a search of the music library. Will push results to [searchResults].
      */
     fun search(query: String?) {
-        val handle = guard.newHandle()
         lastQuery = query
 
+        currentSearchJob?.cancel()
+
         val library = musicStore.library
         if (query.isNullOrEmpty() || library == null) {
             logD("No music/query, ignoring search")
@@ -81,7 +83,7 @@ class SearchViewModel(application: Application) :
         logD("Performing search for $query")
 
         // Searching can be quite expensive, so get on a co-routine
-        viewModelScope.launch {
+        currentSearchJob = viewModelScope.launch {
             val sort = Sort(Sort.Mode.ByName, true)
             val results = mutableListOf<Item>()
 
@@ -115,7 +117,7 @@ class SearchViewModel(application: Application) :
                 }
             }
 
-            guard.yield(handle)
+            yield()
             _searchResults.value = results
         }
     }

app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt

@@ -21,7 +21,6 @@ import android.os.Looper
 import org.oxycblt.auxio.BuildConfig
 import java.lang.reflect.Field
 import java.lang.reflect.Method
-import java.util.concurrent.CancellationException
 import kotlin.reflect.KClass
 
 /** Assert that we are on a background thread. */
@@ -57,34 +56,3 @@ fun lazyReflectedField(clazz: KClass<*>, field: String) = lazy {
 fun lazyReflectedMethod(clazz: KClass<*>, method: String) = lazy {
     clazz.java.getDeclaredMethod(method).also { it.isAccessible = true }
 }
-
-/**
- * An abstraction that allows cheap cooperative multi-threading in shared object contexts. Every new
- * task should call [newHandle], while every running task should call [check] or [yield] depending
- * on the situation to determine if it should continue. Failure to follow the expectations of this
- * class will result in bugs.
- *
- * @author OxygenCobalt
- */
-class TaskGuard {
-    private var currentHandle = 0L
-
-    /**
-     * Returns a new handle to the calling task while invalidating the handle of the previous task.
-     */
-    @Synchronized fun newHandle() = ++currentHandle
-
-    /** Check if the given [handle] is still valid. */
-    @Synchronized fun check(handle: Long) = handle == currentHandle
-
-    /**
-     * Alternative to [kotlinx.coroutines.yield], that achieves the same behavior but in a much
-     * cheaper manner.
-     */
-    @Synchronized
-    fun yield(handle: Long) {
-        if (!check(handle)) {
-            throw CancellationException()
-        }
-    }
-}