mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-11 15:29:15 +01:00
fix: handle artist bio failure and refresh edge cases
Retry a failed download bio fetch instead of blacklisting the artist for the rest of the process, and give a failed or offline refresh its own message so it is not mistaken for a server with no biography. Wait for the stored row before choosing the bio text, so a cached bio no longer flashes the loading state on entry, and bound the page refresh so a stalled response cannot leave the loading state up indefinitely. Render an oversized first token instead of a blank collapsed line, and check whether the whole remainder fits in one measurement.
This commit is contained in:
@@ -432,18 +432,19 @@ class DownloadRepository(
|
|||||||
artistId: String,
|
artistId: String,
|
||||||
) {
|
) {
|
||||||
if (db.libraryDao().artistBioOnce(sourceId, artistId) != null) return
|
if (db.libraryDao().artistBioOnce(sourceId, artistId) != null) return
|
||||||
if (!attemptedBios.add(sourceId to artistId)) return
|
val key = sourceId to artistId
|
||||||
val biography =
|
if (!attemptedBios.add(key)) return
|
||||||
try {
|
try {
|
||||||
sourceRepository.artistBiography(sourceId, artistId)
|
val biography = sourceRepository.artistBiography(sourceId, artistId)
|
||||||
|
db.libraryDao().upsertArtistBio(
|
||||||
|
ArtistBio(sourceId, artistId, normalizeBiography(biography)),
|
||||||
|
)
|
||||||
} catch (e: CancellationException) {
|
} catch (e: CancellationException) {
|
||||||
throw e
|
throw e
|
||||||
} catch (_: Exception) {
|
} catch (_: Exception) {
|
||||||
return
|
} finally {
|
||||||
|
attemptedBios.remove(key)
|
||||||
}
|
}
|
||||||
db.libraryDao().upsertArtistBio(
|
|
||||||
ArtistBio(sourceId, artistId, biography?.trim()?.takeIf { it.isNotEmpty() }),
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private suspend fun fetchArtistBiosSafely(
|
private suspend fun fetchArtistBiosSafely(
|
||||||
@@ -451,14 +452,9 @@ class DownloadRepository(
|
|||||||
artistIds: Set<String>,
|
artistIds: Set<String>,
|
||||||
) {
|
) {
|
||||||
if (artistIds.isEmpty()) return
|
if (artistIds.isEmpty()) return
|
||||||
try {
|
|
||||||
withTimeoutOrNull(BIO_TIMEOUT_MS) {
|
withTimeoutOrNull(BIO_TIMEOUT_MS) {
|
||||||
artistIds.forEach { fetchArtistBio(sourceId, it) }
|
artistIds.forEach { fetchArtistBio(sourceId, it) }
|
||||||
}
|
}
|
||||||
} catch (e: CancellationException) {
|
|
||||||
throw e
|
|
||||||
} catch (_: Exception) {
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private suspend fun storeArtwork(
|
private suspend fun storeArtwork(
|
||||||
|
|||||||
@@ -365,21 +365,19 @@ class LibraryRepository(
|
|||||||
suspend fun refreshArtistBio(
|
suspend fun refreshArtistBio(
|
||||||
sourceId: Long,
|
sourceId: Long,
|
||||||
artistId: String,
|
artistId: String,
|
||||||
): Result<Unit> {
|
): Result<Unit> =
|
||||||
val biography =
|
|
||||||
try {
|
try {
|
||||||
sourceRepository.artistBiography(sourceId, artistId)?.trim()?.takeIf { it.isNotEmpty() }
|
val biography = normalizeBiography(sourceRepository.artistBiography(sourceId, artistId))
|
||||||
} catch (cancellation: CancellationException) {
|
|
||||||
throw cancellation
|
|
||||||
} catch (failure: Exception) {
|
|
||||||
Log.w("artist", "failed to refresh bio for $artistId", failure)
|
|
||||||
return Result.failure(failure)
|
|
||||||
}
|
|
||||||
val current = db.libraryDao().artistBioOnce(sourceId, artistId)
|
val current = db.libraryDao().artistBioOnce(sourceId, artistId)
|
||||||
if (current == null || current.biography != biography) {
|
if (current == null || current.biography != biography) {
|
||||||
db.libraryDao().upsertArtistBio(ArtistBio(sourceId, artistId, biography))
|
db.libraryDao().upsertArtistBio(ArtistBio(sourceId, artistId, biography))
|
||||||
}
|
}
|
||||||
return Result.success(Unit)
|
Result.success(Unit)
|
||||||
|
} catch (cancellation: CancellationException) {
|
||||||
|
throw cancellation
|
||||||
|
} catch (failure: Exception) {
|
||||||
|
Log.w("artist", "failed to refresh bio for $artistId", failure)
|
||||||
|
Result.failure(failure)
|
||||||
}
|
}
|
||||||
|
|
||||||
fun song(
|
fun song(
|
||||||
@@ -555,4 +553,6 @@ const val REDISCOVER_WINDOW_MS = 60L * 24 * 60 * 60 * 1000
|
|||||||
|
|
||||||
const val SEARCH_RESULT_LIMIT = 50
|
const val SEARCH_RESULT_LIMIT = 50
|
||||||
|
|
||||||
|
internal fun normalizeBiography(value: String?): String? = value?.trim()?.takeIf { it.isNotEmpty() }
|
||||||
|
|
||||||
fun rediscoverCutoff(): Long = System.currentTimeMillis() - REDISCOVER_WINDOW_MS
|
fun rediscoverCutoff(): Long = System.currentTimeMillis() - REDISCOVER_WINDOW_MS
|
||||||
|
|||||||
@@ -116,6 +116,7 @@ import com.subtracks.ui.theme.heroBarColor
|
|||||||
import com.subtracks.ui.theme.rememberArtworkColors
|
import com.subtracks.ui.theme.rememberArtworkColors
|
||||||
import com.subtracks.ui.theme.rememberOverlaidNameBusy
|
import com.subtracks.ui.theme.rememberOverlaidNameBusy
|
||||||
import kotlinx.coroutines.flow.emptyFlow
|
import kotlinx.coroutines.flow.emptyFlow
|
||||||
|
import kotlinx.coroutines.flow.map
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
import org.koin.compose.koinInject
|
import org.koin.compose.koinInject
|
||||||
import org.koin.compose.viewmodel.koinViewModel
|
import org.koin.compose.viewmodel.koinViewModel
|
||||||
@@ -155,6 +156,12 @@ private const val SCRIM_FADE_MS = 180
|
|||||||
internal const val ARTIST_NAME_SCRIM_TAG = "artistNameScrim"
|
internal const val ARTIST_NAME_SCRIM_TAG = "artistNameScrim"
|
||||||
internal const val ARTIST_BIO_MORE_TAG = "artistBioMore"
|
internal const val ARTIST_BIO_MORE_TAG = "artistBioMore"
|
||||||
|
|
||||||
|
private data class BioSnapshot(
|
||||||
|
val known: Boolean,
|
||||||
|
val present: Boolean,
|
||||||
|
val biography: String?,
|
||||||
|
)
|
||||||
|
|
||||||
@Composable
|
@Composable
|
||||||
fun ArtistDetailRoute(
|
fun ArtistDetailRoute(
|
||||||
artistId: String,
|
artistId: String,
|
||||||
@@ -170,8 +177,16 @@ fun ArtistDetailRoute(
|
|||||||
val artist by viewModel.artist.collectAsStateWithLifecycle(initialValue = null)
|
val artist by viewModel.artist.collectAsStateWithLifecycle(initialValue = null)
|
||||||
val artistName by viewModel.artistName.collectAsStateWithLifecycle(initialValue = null)
|
val artistName by viewModel.artistName.collectAsStateWithLifecycle(initialValue = null)
|
||||||
val albums by viewModel.albums.collectAsStateWithLifecycle(initialValue = null)
|
val albums by viewModel.albums.collectAsStateWithLifecycle(initialValue = null)
|
||||||
val artistBio by viewModel.artistBio.collectAsStateWithLifecycle(initialValue = null)
|
val bioSnapshots =
|
||||||
|
remember(viewModel) {
|
||||||
|
viewModel.artistBio.map { BioSnapshot(known = true, present = it != null, biography = it?.biography) }
|
||||||
|
}
|
||||||
|
val bioSnapshot by
|
||||||
|
bioSnapshots.collectAsStateWithLifecycle(
|
||||||
|
initialValue = BioSnapshot(known = false, present = false, biography = null),
|
||||||
|
)
|
||||||
val bioLoading by viewModel.bioLoading.collectAsStateWithLifecycle()
|
val bioLoading by viewModel.bioLoading.collectAsStateWithLifecycle()
|
||||||
|
val bioUnavailable by viewModel.bioUnavailable.collectAsStateWithLifecycle()
|
||||||
val bioExpanded by viewModel.bioExpanded.collectAsStateWithLifecycle()
|
val bioExpanded by viewModel.bioExpanded.collectAsStateWithLifecycle()
|
||||||
LaunchedEffect(artistId) { viewModel.refreshBio() }
|
LaunchedEffect(artistId) { viewModel.refreshBio() }
|
||||||
val art by viewModel.art.collectAsStateWithLifecycle()
|
val art by viewModel.art.collectAsStateWithLifecycle()
|
||||||
@@ -208,8 +223,10 @@ fun ArtistDetailRoute(
|
|||||||
artistName = artistName.orEmpty(),
|
artistName = artistName.orEmpty(),
|
||||||
albums = albums ?: ArtistAlbums(),
|
albums = albums ?: ArtistAlbums(),
|
||||||
albumsLoaded = albums != null,
|
albumsLoaded = albums != null,
|
||||||
bio = artistBio?.biography,
|
bio = bioSnapshot.biography,
|
||||||
bioLoading = bioLoading && artistBio == null,
|
bioLoading = bioLoading && !bioSnapshot.present,
|
||||||
|
bioUnavailable = bioUnavailable && !bioSnapshot.present,
|
||||||
|
bioPending = !bioSnapshot.known,
|
||||||
bioExpanded = bioExpanded,
|
bioExpanded = bioExpanded,
|
||||||
art = art,
|
art = art,
|
||||||
artThumbnail = artThumbnail,
|
artThumbnail = artThumbnail,
|
||||||
@@ -246,6 +263,8 @@ fun ArtistDetailScreen(
|
|||||||
albumsLoaded: Boolean = true,
|
albumsLoaded: Boolean = true,
|
||||||
bio: String? = null,
|
bio: String? = null,
|
||||||
bioLoading: Boolean = false,
|
bioLoading: Boolean = false,
|
||||||
|
bioUnavailable: Boolean = false,
|
||||||
|
bioPending: Boolean = false,
|
||||||
bioExpanded: Boolean = false,
|
bioExpanded: Boolean = false,
|
||||||
art: CoverArtRef?,
|
art: CoverArtRef?,
|
||||||
artThumbnail: CoverArtRef? = null,
|
artThumbnail: CoverArtRef? = null,
|
||||||
@@ -284,8 +303,10 @@ fun ArtistDetailScreen(
|
|||||||
val bioCollapsed = hasBio && !bioExpanded
|
val bioCollapsed = hasBio && !bioExpanded
|
||||||
val bioText =
|
val bioText =
|
||||||
when {
|
when {
|
||||||
|
bioPending -> ""
|
||||||
hasBio -> bio.orEmpty()
|
hasBio -> bio.orEmpty()
|
||||||
bioLoading -> stringResource(R.string.artist_bio_loading)
|
bioLoading -> stringResource(R.string.artist_bio_loading)
|
||||||
|
bioUnavailable -> stringResource(R.string.artist_bio_unavailable)
|
||||||
else -> stringResource(R.string.artist_bio_empty)
|
else -> stringResource(R.string.artist_bio_empty)
|
||||||
}
|
}
|
||||||
var titleHeightPx by remember { mutableFloatStateOf(0f) }
|
var titleHeightPx by remember { mutableFloatStateOf(0f) }
|
||||||
@@ -654,6 +675,9 @@ internal fun splitBioCollapsed(
|
|||||||
if (measurer.measure(AnnotatedString(candidate), style, maxLines = 1).size.width <= available) {
|
if (measurer.measure(AnnotatedString(candidate), style, maxLines = 1).size.width <= available) {
|
||||||
first = candidate
|
first = candidate
|
||||||
index++
|
index++
|
||||||
|
} else if (first.isEmpty()) {
|
||||||
|
first = words[index]
|
||||||
|
index++
|
||||||
} else {
|
} else {
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
@@ -661,19 +685,9 @@ internal fun splitBioCollapsed(
|
|||||||
if (index >= words.size) return BioLines(listOf(AnnotatedString(first)), more = false)
|
if (index >= words.size) return BioLines(listOf(AnnotatedString(first)), more = false)
|
||||||
|
|
||||||
val secondStart = index
|
val secondStart = index
|
||||||
var second = ""
|
val remaining = words.drop(secondStart).joinToString(" ")
|
||||||
var secondIndex = secondStart
|
if (measurer.measure(AnnotatedString(remaining), style, maxLines = 1).size.width <= available) {
|
||||||
while (secondIndex < words.size) {
|
return BioLines(listOf(AnnotatedString(first), AnnotatedString(remaining)), more = false)
|
||||||
val candidate = if (second.isEmpty()) words[secondIndex] else "$second ${words[secondIndex]}"
|
|
||||||
if (measurer.measure(AnnotatedString(candidate), style, maxLines = 1).size.width <= available) {
|
|
||||||
second = candidate
|
|
||||||
secondIndex++
|
|
||||||
} else {
|
|
||||||
break
|
|
||||||
}
|
|
||||||
}
|
|
||||||
if (secondIndex >= words.size) {
|
|
||||||
return BioLines(listOf(AnnotatedString(first), AnnotatedString(second)), more = false)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
val more =
|
val more =
|
||||||
|
|||||||
@@ -29,6 +29,9 @@ import kotlinx.coroutines.flow.map
|
|||||||
import kotlinx.coroutines.flow.onStart
|
import kotlinx.coroutines.flow.onStart
|
||||||
import kotlinx.coroutines.flow.stateIn
|
import kotlinx.coroutines.flow.stateIn
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
|
import kotlinx.coroutines.withTimeoutOrNull
|
||||||
|
|
||||||
|
private const val BIO_REFRESH_TIMEOUT_MS = 15_000L
|
||||||
|
|
||||||
@OptIn(ExperimentalCoroutinesApi::class)
|
@OptIn(ExperimentalCoroutinesApi::class)
|
||||||
class ArtistDetailViewModel(
|
class ArtistDetailViewModel(
|
||||||
@@ -73,6 +76,9 @@ class ArtistDetailViewModel(
|
|||||||
private val _bioLoading = MutableStateFlow(true)
|
private val _bioLoading = MutableStateFlow(true)
|
||||||
val bioLoading: StateFlow<Boolean> = _bioLoading
|
val bioLoading: StateFlow<Boolean> = _bioLoading
|
||||||
|
|
||||||
|
private val _bioUnavailable = MutableStateFlow(false)
|
||||||
|
val bioUnavailable: StateFlow<Boolean> = _bioUnavailable
|
||||||
|
|
||||||
private val _bioExpanded = MutableStateFlow(false)
|
private val _bioExpanded = MutableStateFlow(false)
|
||||||
val bioExpanded: StateFlow<Boolean> = _bioExpanded
|
val bioExpanded: StateFlow<Boolean> = _bioExpanded
|
||||||
|
|
||||||
@@ -82,18 +88,22 @@ class ArtistDetailViewModel(
|
|||||||
if (bioRefresh?.isActive == true) return
|
if (bioRefresh?.isActive == true) return
|
||||||
bioRefresh =
|
bioRefresh =
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
|
_bioUnavailable.value = false
|
||||||
if (sourceRepository.offline.value) {
|
if (sourceRepository.offline.value) {
|
||||||
_bioLoading.value = false
|
_bioLoading.value = false
|
||||||
|
_bioUnavailable.value = true
|
||||||
return@launch
|
return@launch
|
||||||
}
|
}
|
||||||
val id = sourceRepository.activeSourceIdOnce()
|
val id = sourceRepository.activeSourceIdOnce()
|
||||||
if (id == null) {
|
if (id == null) {
|
||||||
_bioLoading.value = false
|
_bioLoading.value = false
|
||||||
|
_bioUnavailable.value = true
|
||||||
return@launch
|
return@launch
|
||||||
}
|
}
|
||||||
_bioLoading.value = true
|
_bioLoading.value = true
|
||||||
try {
|
try {
|
||||||
libraryRepository.refreshArtistBio(id, artistId)
|
val result = withTimeoutOrNull(BIO_REFRESH_TIMEOUT_MS) { libraryRepository.refreshArtistBio(id, artistId) }
|
||||||
|
_bioUnavailable.value = result == null || result.isFailure
|
||||||
} finally {
|
} finally {
|
||||||
_bioLoading.value = false
|
_bioLoading.value = false
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -39,6 +39,7 @@
|
|||||||
<string name="artist_section_appearances">Appears on</string>
|
<string name="artist_section_appearances">Appears on</string>
|
||||||
<string name="artist_bio_empty">No artist bio available.</string>
|
<string name="artist_bio_empty">No artist bio available.</string>
|
||||||
<string name="artist_bio_loading">Loading artist bio…</string>
|
<string name="artist_bio_loading">Loading artist bio…</string>
|
||||||
|
<string name="artist_bio_unavailable">Artist bio unavailable.</string>
|
||||||
<string name="artist_bio_more">More</string>
|
<string name="artist_bio_more">More</string>
|
||||||
<string name="artist_meta">%1$s · %2$s</string>
|
<string name="artist_meta">%1$s · %2$s</string>
|
||||||
<string name="resources_filter_genre">Genre</string>
|
<string name="resources_filter_genre">Genre</string>
|
||||||
|
|||||||
@@ -44,6 +44,8 @@ class ArtistDetailScreenTest {
|
|||||||
albumsLoaded: Boolean = true,
|
albumsLoaded: Boolean = true,
|
||||||
bio: String? = null,
|
bio: String? = null,
|
||||||
bioLoading: Boolean = false,
|
bioLoading: Boolean = false,
|
||||||
|
bioUnavailable: Boolean = false,
|
||||||
|
bioPending: Boolean = false,
|
||||||
) {
|
) {
|
||||||
composeRule.setContent {
|
composeRule.setContent {
|
||||||
var expanded by remember { mutableStateOf(false) }
|
var expanded by remember { mutableStateOf(false) }
|
||||||
@@ -55,6 +57,8 @@ class ArtistDetailScreenTest {
|
|||||||
albumsLoaded = albumsLoaded,
|
albumsLoaded = albumsLoaded,
|
||||||
bio = bio,
|
bio = bio,
|
||||||
bioLoading = bioLoading,
|
bioLoading = bioLoading,
|
||||||
|
bioUnavailable = bioUnavailable,
|
||||||
|
bioPending = bioPending,
|
||||||
bioExpanded = expanded,
|
bioExpanded = expanded,
|
||||||
art = null,
|
art = null,
|
||||||
coverArt = { _, _ -> null },
|
coverArt = { _, _ -> null },
|
||||||
@@ -151,6 +155,32 @@ class ArtistDetailScreenTest {
|
|||||||
composeRule.onNodeWithText("No artist bio available.").assertDoesNotExist()
|
composeRule.onNodeWithText("No artist bio available.").assertDoesNotExist()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun theBioAreaWaitsForTheStoredValueBeforeChoosingAText() {
|
||||||
|
render(
|
||||||
|
artist = Artist(sourceId = 1, id = "ar-1", name = "Radiohead", albumCount = 1, starred = null),
|
||||||
|
artistName = "Radiohead",
|
||||||
|
bioLoading = true,
|
||||||
|
bioPending = true,
|
||||||
|
)
|
||||||
|
|
||||||
|
composeRule.onNodeWithText("Loading artist bio…").assertDoesNotExist()
|
||||||
|
composeRule.onNodeWithText("No artist bio available.").assertDoesNotExist()
|
||||||
|
composeRule.onNodeWithText("Artist bio unavailable.").assertDoesNotExist()
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aFailedRefreshIsNotMistakenForAnArtistWithoutABio() {
|
||||||
|
render(
|
||||||
|
artist = Artist(sourceId = 1, id = "ar-1", name = "Radiohead", albumCount = 1, starred = null),
|
||||||
|
artistName = "Radiohead",
|
||||||
|
bioUnavailable = true,
|
||||||
|
)
|
||||||
|
|
||||||
|
composeRule.onNodeWithText("Artist bio unavailable.").assertExists()
|
||||||
|
composeRule.onNodeWithText("No artist bio available.").assertDoesNotExist()
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun aMissingBioShowsTheEmptyStateAfterTheRefresh() {
|
fun aMissingBioShowsTheEmptyStateAfterTheRefresh() {
|
||||||
render(
|
render(
|
||||||
@@ -218,6 +248,33 @@ class ArtistDetailScreenTest {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun anOversizedFirstWordIsStillShown() {
|
||||||
|
var result: BioLines? = null
|
||||||
|
composeRule.setContent {
|
||||||
|
val measurer = rememberTextMeasurer()
|
||||||
|
result =
|
||||||
|
splitBioCollapsed(
|
||||||
|
measurer,
|
||||||
|
"Supercalifragilisticexpialidocious and the rest",
|
||||||
|
TextStyle(fontSize = 14.sp),
|
||||||
|
20,
|
||||||
|
0,
|
||||||
|
"More",
|
||||||
|
)
|
||||||
|
}
|
||||||
|
composeRule.waitForIdle()
|
||||||
|
|
||||||
|
val lines = requireNotNull(result)
|
||||||
|
assertTrue(lines.more)
|
||||||
|
assertTrue(
|
||||||
|
lines.lines
|
||||||
|
.first()
|
||||||
|
.text
|
||||||
|
.isNotBlank(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
private fun album(
|
private fun album(
|
||||||
id: String,
|
id: String,
|
||||||
name: String,
|
name: String,
|
||||||
|
|||||||
Reference in New Issue
Block a user