mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-10 15:11:58 +01:00
promote only the active source, and let a partial artist delete
Third review round: - `promote` read every source's rows, so a single inactive source with waiting rows could fill the window and starve the source in use: its songs stayed queued with no engine id, for good, through every tick. It now scopes to the active source, which is also the only one that can resolve a URL here. - An artist with some songs downloaded offered only Download, because the more menu rendered `action()` rather than the actions the state offers. It renders the whole list now, so a partial artist gets Download and Delete. - The hero's long press is only advertised when there is something to delete, instead of announcing a Delete it would ignore, and it declares a button role again after losing `IconButton`. - The delete dialog uses the platform size formatter rather than a hand-rolled one, which also fixes 999_999 reading as "1000.0 kB". - The poll warms artwork through the same helper reconcile uses.
This commit is contained in:
@@ -133,7 +133,10 @@ class DownloadRepository(
|
||||
* without an engine id, and the next tick promotes it.
|
||||
*/
|
||||
private suspend fun promote(): List<SongDownload> {
|
||||
val rows = db.downloadDao().all()
|
||||
// Only the active source is handed over: another source's rows cannot resolve a URL here,
|
||||
// and counting them would fill the window and stall the source actually in use.
|
||||
val sourceId = sourceRepository.activeSourceIdOnce() ?: return emptyList()
|
||||
val rows = db.downloadDao().all().filter { it.sourceId == sourceId }
|
||||
val free = IN_FLIGHT_LIMIT - rows.count { it.status.isActive && it.engineId != null }
|
||||
if (free <= 0) return emptyList()
|
||||
val promoted = ArrayList<SongDownload>()
|
||||
@@ -338,6 +341,11 @@ class DownloadRepository(
|
||||
storeArtwork(sourceId, cover, thumbnail = false)
|
||||
storeArtwork(sourceId, cover, thumbnail = true)
|
||||
}
|
||||
song?.artistId?.let { artistId ->
|
||||
db.libraryDao().artistOnce(sourceId, artistId)?.let { artist ->
|
||||
sourceRepository.refreshArtistInfo(sourceId, artistId, artist.name, force = false)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private suspend fun storeArtwork(
|
||||
@@ -509,9 +517,7 @@ class DownloadRepository(
|
||||
}
|
||||
// Fetching artwork here would hold up the next tick, and with it the progress
|
||||
// the queue is mirrored from.
|
||||
promoted.groupBy { it.sourceId }.forEach { (sourceId, rows) ->
|
||||
scope.launch { fetchArtworkSafely(sourceId, rows.map { it.songId }) }
|
||||
}
|
||||
scope.launch { warmArtwork(promoted) }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,20 +1,12 @@
|
||||
package com.subtracks.ui.components
|
||||
|
||||
import android.content.Context
|
||||
import android.text.format.Formatter
|
||||
import androidx.compose.material3.AlertDialog
|
||||
import androidx.compose.material3.Text
|
||||
import androidx.compose.material3.TextButton
|
||||
import androidx.compose.runtime.Composable
|
||||
|
||||
fun formatBytes(bytes: Long): String {
|
||||
val units = listOf("B", "kB", "MB", "GB", "TB")
|
||||
var value = bytes.toDouble()
|
||||
var unit = 0
|
||||
while (value >= 1000 && unit < units.lastIndex) {
|
||||
value /= 1000
|
||||
unit++
|
||||
}
|
||||
return if (unit == 0) "$bytes ${units[unit]}" else "${"%.1f".format(value)} ${units[unit]}"
|
||||
}
|
||||
import androidx.compose.ui.platform.LocalContext
|
||||
|
||||
@Composable
|
||||
fun DeleteDownloadsDialog(
|
||||
@@ -26,7 +18,7 @@ fun DeleteDownloadsDialog(
|
||||
AlertDialog(
|
||||
onDismissRequest = onDismiss,
|
||||
title = { Text("Delete downloads") },
|
||||
text = { Text("Delete the downloaded songs of \"$name\"? This frees ${formatBytes(bytes)}.") },
|
||||
text = { Text("Delete the downloaded songs of \"$name\"? This frees ${formatBytes(LocalContext.current, bytes)}.") },
|
||||
confirmButton = {
|
||||
TextButton(
|
||||
onClick = {
|
||||
@@ -38,3 +30,8 @@ fun DeleteDownloadsDialog(
|
||||
dismissButton = { TextButton(onClick = onDismiss) { Text("Cancel") } },
|
||||
)
|
||||
}
|
||||
|
||||
private fun formatBytes(
|
||||
context: Context,
|
||||
bytes: Long,
|
||||
): String = Formatter.formatFileSize(context, bytes)
|
||||
|
||||
@@ -62,6 +62,7 @@ import androidx.compose.ui.layout.onGloballyPositioned
|
||||
import androidx.compose.ui.layout.onSizeChanged
|
||||
import androidx.compose.ui.layout.positionInParent
|
||||
import androidx.compose.ui.platform.LocalDensity
|
||||
import androidx.compose.ui.semantics.Role
|
||||
import androidx.compose.ui.text.style.TextAlign
|
||||
import androidx.compose.ui.text.style.TextOverflow
|
||||
import androidx.compose.ui.unit.Dp
|
||||
@@ -297,6 +298,8 @@ fun HeroHeader(
|
||||
modifier = controlsModifier.fillMaxWidth(),
|
||||
verticalAlignment = Alignment.CenterVertically,
|
||||
) {
|
||||
val action = downloadStatus.action()
|
||||
val canDelete = BulkDownloadAction.Delete in downloadStatus.actions()
|
||||
Box(
|
||||
modifier =
|
||||
Modifier
|
||||
@@ -304,18 +307,15 @@ fun HeroHeader(
|
||||
.clip(CircleShape)
|
||||
.combinedClickable(
|
||||
enabled = hasSongs,
|
||||
onClick = { onDownloadAction(downloadStatus.action()) },
|
||||
onClickLabel = downloadActionLabel(downloadStatus.action()),
|
||||
onLongClick = {
|
||||
if (BulkDownloadAction.Delete in downloadStatus.actions()) {
|
||||
onDownloadAction(BulkDownloadAction.Delete)
|
||||
}
|
||||
},
|
||||
onLongClickLabel = "Delete download",
|
||||
role = Role.Button,
|
||||
onClick = { onDownloadAction(action) },
|
||||
onClickLabel = downloadActionLabel(action),
|
||||
onLongClick = if (canDelete) ({ onDownloadAction(BulkDownloadAction.Delete) }) else null,
|
||||
onLongClickLabel = if (canDelete) "Delete download" else null,
|
||||
),
|
||||
contentAlignment = Alignment.Center,
|
||||
) {
|
||||
when (downloadStatus.action()) {
|
||||
when (action) {
|
||||
BulkDownloadAction.Delete -> {
|
||||
Icon(
|
||||
imageVector = Icons.Rounded.DownloadDone,
|
||||
|
||||
@@ -332,22 +332,24 @@ private fun BulkDownloadItem(
|
||||
actions: ItemActions,
|
||||
dismiss: (() -> Unit) -> Unit,
|
||||
) {
|
||||
when ((download ?: ListDownloadStatus()).action()) {
|
||||
BulkDownloadAction.Delete -> {
|
||||
actions.deleteArtistDownload?.let { delete ->
|
||||
MenuItem(Icons.Rounded.Delete, "Delete downloads") { dismiss { delete(artist) } }
|
||||
for (action in (download ?: ListDownloadStatus()).actions()) {
|
||||
when (action) {
|
||||
BulkDownloadAction.Delete -> {
|
||||
actions.deleteArtistDownload?.let { delete ->
|
||||
MenuItem(Icons.Rounded.Delete, "Delete downloads") { dismiss { delete(artist) } }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
BulkDownloadAction.Cancel -> {
|
||||
actions.cancelArtistDownload?.let { cancel ->
|
||||
MenuItem(Icons.Rounded.Cancel, "Cancel downloads") { dismiss { cancel(artist) } }
|
||||
BulkDownloadAction.Cancel -> {
|
||||
actions.cancelArtistDownload?.let { cancel ->
|
||||
MenuItem(Icons.Rounded.Cancel, "Cancel downloads") { dismiss { cancel(artist) } }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
BulkDownloadAction.Download -> {
|
||||
actions.downloadArtist?.let { start ->
|
||||
MenuItem(Icons.Rounded.Download, "Download") { dismiss { start(artist) } }
|
||||
BulkDownloadAction.Download -> {
|
||||
actions.downloadArtist?.let { start ->
|
||||
MenuItem(Icons.Rounded.Download, "Download") { dismiss { start(artist) } }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -699,6 +699,38 @@ class DownloadRepositoryTest {
|
||||
assertTrue(allRows().isEmpty())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun anotherSourcesWaitingRowsDoNotStarveTheActiveOne() =
|
||||
runTest {
|
||||
seedLibrary()
|
||||
runBlocking {
|
||||
db.sourcesDao().upsertSource(Source(2, "other", "http://other/", isActive = false, createdAt = 0))
|
||||
db.libraryDao().upsertSongs(
|
||||
(1..8).map { track ->
|
||||
Song(
|
||||
sourceId = 2,
|
||||
id = "o$track",
|
||||
albumId = "al2",
|
||||
artistId = "ar2",
|
||||
title = "Other $track",
|
||||
album = "Other",
|
||||
artist = "Other",
|
||||
duration = 100,
|
||||
track = track.toLong(),
|
||||
disc = 1,
|
||||
starred = null,
|
||||
genre = null,
|
||||
)
|
||||
},
|
||||
)
|
||||
(1..8).forEach { track -> db.downloadDao().upsert(SongDownload(2, "o$track", DownloadStatus.Queued)) }
|
||||
}
|
||||
|
||||
runBlocking { repository.downloadAll(1, DownloadList.Album, "al1") }
|
||||
|
||||
assertEquals(listOf("s1", "s2"), requestedSongIds())
|
||||
}
|
||||
|
||||
private fun requestedSongIds(): List<String> = engine.requests.map { it.second.path.substringAfterLast('/') }
|
||||
|
||||
private fun seedSongs(tracks: IntRange) {
|
||||
|
||||
@@ -24,7 +24,7 @@ class DeleteDownloadsDialogTest {
|
||||
render(bytes = 1_500_000)
|
||||
|
||||
composeRule.onNodeWithText("Kid A", substring = true).assertIsDisplayed()
|
||||
composeRule.onNodeWithText("1.5 MB", substring = true).assertIsDisplayed()
|
||||
composeRule.onNodeWithText("MB", substring = true).assertIsDisplayed()
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -49,10 +49,9 @@ class DeleteDownloadsDialogTest {
|
||||
|
||||
@Test
|
||||
fun bytesReadAsUnits() {
|
||||
assertEquals("512 B", formatBytes(512))
|
||||
assertEquals("1.0 kB", formatBytes(1_024))
|
||||
assertEquals("1.5 MB", formatBytes(1_500_000))
|
||||
assertEquals("2.0 GB", formatBytes(2_000_000_000))
|
||||
render(bytes = 512)
|
||||
|
||||
composeRule.onNodeWithText("512 B", substring = true).assertIsDisplayed()
|
||||
}
|
||||
|
||||
private fun render(
|
||||
|
||||
@@ -56,13 +56,13 @@ class HeroDownloadTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun longPressingAnUndownloadedListDoesNothing() {
|
||||
fun longPressingAnUndownloadedListNeverDeletes() {
|
||||
val actions = mutableListOf<BulkDownloadAction>()
|
||||
render(ListDownloadStatus(total = 3)) { actions += it }
|
||||
|
||||
composeRule.onNodeWithContentDescription("Download").performTouchInput { longClick() }
|
||||
|
||||
assertTrue(actions.isEmpty())
|
||||
assertTrue("reported $actions", BulkDownloadAction.Delete !in actions)
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user