mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-10 23:21:59 +01:00
downloads: address review on the cancel/progress rows
Name the cancel and delete buttons after the row so screen readers can tell them apart, show an indeterminate bar when a running track has no known total, and drop the no-op media refresh on cancel. Trim the dead size alias and a redundant guard, and cover a queued row with no engine id plus a failed track in the progress denominator.
This commit is contained in:
@@ -107,7 +107,7 @@ interface DownloadDao {
|
|||||||
@Query(
|
@Query(
|
||||||
"SELECT d.sourceId AS sourceId, d.songId AS songId, s.title AS title, s.albumId AS albumId, al.name AS albumName, " +
|
"SELECT d.sourceId AS sourceId, d.songId AS songId, s.title AS title, s.albumId AS albumId, al.name AS albumName, " +
|
||||||
"al.artistId AS artistId, ar.name AS artistName, d.status AS status, " +
|
"al.artistId AS artistId, ar.name AS artistName, d.status AS status, " +
|
||||||
"d.bytes AS bytes, d.total AS total, d.total AS size " +
|
"d.bytes AS bytes, d.total AS total " +
|
||||||
"FROM song_downloads d " +
|
"FROM song_downloads d " +
|
||||||
"JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " +
|
"JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " +
|
||||||
"LEFT JOIN albums al ON al.sourceId = s.sourceId AND al.id = s.albumId " +
|
"LEFT JOIN albums al ON al.sourceId = s.sourceId AND al.id = s.albumId " +
|
||||||
|
|||||||
@@ -262,11 +262,11 @@ private fun NodeRow(
|
|||||||
trailingContent = {
|
trailingContent = {
|
||||||
if (progress != null) {
|
if (progress != null) {
|
||||||
IconButton(onClick = onCancel) {
|
IconButton(onClick = onCancel) {
|
||||||
Icon(Icons.Rounded.Close, contentDescription = "Cancel download")
|
Icon(Icons.Rounded.Close, contentDescription = "Cancel download of $name")
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
IconButton(onClick = onDelete) {
|
IconButton(onClick = onDelete) {
|
||||||
Icon(Icons.Rounded.Delete, contentDescription = "Delete downloads")
|
Icon(Icons.Rounded.Delete, contentDescription = "Delete downloads of $name")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
@@ -280,7 +280,7 @@ private fun activeDownloads(songs: List<DownloadedSong>): List<DownloadedSong> =
|
|||||||
|
|
||||||
internal fun nodeProgress(songs: List<DownloadedSong>): Float? {
|
internal fun nodeProgress(songs: List<DownloadedSong>): Float? {
|
||||||
val active = activeDownloads(songs)
|
val active = activeDownloads(songs)
|
||||||
if (active.isEmpty() || songs.isEmpty()) return null
|
if (active.isEmpty()) return null
|
||||||
val completed = songs.count { it.status == DownloadStatus.Completed }
|
val completed = songs.count { it.status == DownloadStatus.Completed }
|
||||||
val partial = active.sumOf { (it.progress ?: 0f).toDouble() }
|
val partial = active.sumOf { (it.progress ?: 0f).toDouble() }
|
||||||
return ((completed + partial) / songs.size).toFloat().coerceIn(0f, 1f)
|
return ((completed + partial) / songs.size).toFloat().coerceIn(0f, 1f)
|
||||||
@@ -298,27 +298,30 @@ private fun SongRow(
|
|||||||
encoding = if (song.status == DownloadStatus.Completed) loadEncoding(song) else null
|
encoding = if (song.status == DownloadStatus.Completed) loadEncoding(song) else null
|
||||||
}
|
}
|
||||||
val active = song.status == DownloadStatus.Queued || song.status == DownloadStatus.Running
|
val active = song.status == DownloadStatus.Queued || song.status == DownloadStatus.Running
|
||||||
|
val barModifier = Modifier.fillMaxWidth().padding(top = 4.dp)
|
||||||
ListItem(
|
ListItem(
|
||||||
headlineContent = { Text(song.title, maxLines = 1, overflow = TextOverflow.Ellipsis) },
|
headlineContent = { Text(song.title, maxLines = 1, overflow = TextOverflow.Ellipsis) },
|
||||||
supportingContent = {
|
supportingContent = {
|
||||||
Column {
|
Column {
|
||||||
Text(songSubtitle(song, encoding), maxLines = 1, overflow = TextOverflow.Ellipsis)
|
Text(songSubtitle(song, encoding), maxLines = 1, overflow = TextOverflow.Ellipsis)
|
||||||
if (active) {
|
if (active) {
|
||||||
LinearProgressIndicator(
|
val progress = song.progress
|
||||||
progress = { song.progress ?: 0f },
|
if (progress == null) {
|
||||||
modifier = Modifier.fillMaxWidth().padding(top = 4.dp),
|
LinearProgressIndicator(modifier = barModifier)
|
||||||
)
|
} else {
|
||||||
|
LinearProgressIndicator(progress = { progress }, modifier = barModifier)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
trailingContent = {
|
trailingContent = {
|
||||||
if (active) {
|
if (active) {
|
||||||
IconButton(onClick = onCancel) {
|
IconButton(onClick = onCancel) {
|
||||||
Icon(Icons.Rounded.Close, contentDescription = "Cancel download")
|
Icon(Icons.Rounded.Close, contentDescription = "Cancel download of ${song.title}")
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
IconButton(onClick = onDelete) {
|
IconButton(onClick = onDelete) {
|
||||||
Icon(Icons.Rounded.Delete, contentDescription = "Delete download")
|
Icon(Icons.Rounded.Delete, contentDescription = "Delete download of ${song.title}")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -82,7 +82,6 @@ class DownloadsViewModel(
|
|||||||
fun cancelSongs(songIds: List<String>) {
|
fun cancelSongs(songIds: List<String>) {
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
downloadRepository.cancel(sourceId.first(), songIds)
|
downloadRepository.cancel(sourceId.first(), songIds)
|
||||||
playbackController.refreshMediaItems()
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -58,6 +58,20 @@ class DownloadTreeTest {
|
|||||||
assertNull(nodeProgress(listOf(song("s3", "Three", "ar1", "Artist", "al1", "Album", size = 1))))
|
assertNull(nodeProgress(listOf(song("s3", "Three", "ar1", "Artist", "al1", "Album", size = 1))))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun nodeProgressCountsAFailedTrackInTheDenominator() {
|
||||||
|
val songs =
|
||||||
|
listOf(
|
||||||
|
song("s1", "One", "ar1", "Artist", "al1", "Album", size = 100),
|
||||||
|
song("s2", "Two", "ar1", "Artist", "al1", "Album", size = 100)
|
||||||
|
.copy(status = DownloadStatus.Failed),
|
||||||
|
song("s3", "Three", "ar1", "Artist", "al1", "Album", size = 100)
|
||||||
|
.copy(status = DownloadStatus.Running, bytes = 50, total = 100),
|
||||||
|
)
|
||||||
|
|
||||||
|
assertEquals(0.5f, nodeProgress(songs)!!, 0.0001f)
|
||||||
|
}
|
||||||
|
|
||||||
private fun song(
|
private fun song(
|
||||||
id: String,
|
id: String,
|
||||||
title: String,
|
title: String,
|
||||||
|
|||||||
@@ -53,10 +53,10 @@ class DownloadsScreenTest {
|
|||||||
onCancel = { cancelled = it },
|
onCancel = { cancelled = it },
|
||||||
)
|
)
|
||||||
|
|
||||||
composeRule.onAllNodesWithContentDescription("Cancel download").assertCountEquals(1)
|
composeRule.onAllNodesWithContentDescription("Cancel download", substring = true).assertCountEquals(1)
|
||||||
composeRule.onAllNodesWithContentDescription("Delete downloads").assertCountEquals(1)
|
composeRule.onAllNodesWithContentDescription("Delete downloads", substring = true).assertCountEquals(1)
|
||||||
|
|
||||||
composeRule.onNodeWithContentDescription("Cancel download").performClick()
|
composeRule.onNodeWithContentDescription("Cancel download of Active").performClick()
|
||||||
assertEquals(listOf("s1"), cancelled)
|
assertEquals(listOf("s1"), cancelled)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -66,8 +66,8 @@ class DownloadsScreenTest {
|
|||||||
|
|
||||||
expandArtistAndAlbum()
|
expandArtistAndAlbum()
|
||||||
|
|
||||||
composeRule.onAllNodesWithContentDescription("Cancel download").assertCountEquals(3)
|
composeRule.onAllNodesWithContentDescription("Cancel download", substring = true).assertCountEquals(3)
|
||||||
composeRule.onAllNodesWithContentDescription("Delete download").assertCountEquals(1)
|
composeRule.onAllNodesWithContentDescription("Delete download of Done").assertCountEquals(1)
|
||||||
}
|
}
|
||||||
|
|
||||||
private fun artist(
|
private fun artist(
|
||||||
|
|||||||
@@ -150,6 +150,17 @@ class DownloadsViewModelTest {
|
|||||||
assertNotNull("a completed download should survive a cancel", row("s2"))
|
assertNotNull("a completed download should survive a cancel", row("s2"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun cancelSongsRemovesAQueuedDownloadWithoutAnEngineId() {
|
||||||
|
seedDownloads()
|
||||||
|
runBlocking { db.downloadDao().upsert(SongDownload(sourceId, "s1", DownloadStatus.Queued, engineId = null)) }
|
||||||
|
|
||||||
|
viewModel.cancelSongs(listOf("s1"))
|
||||||
|
await { row("s1") == null }
|
||||||
|
|
||||||
|
assertNull(row("s1"))
|
||||||
|
}
|
||||||
|
|
||||||
private fun seedDownloads() {
|
private fun seedDownloads() {
|
||||||
sourceId =
|
sourceId =
|
||||||
runBlocking {
|
runBlocking {
|
||||||
|
|||||||
Reference in New Issue
Block a user