mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-10 15:11:58 +01:00
ui: address review feedback on search focus and artist art
Keep the search field from stealing focus again when a result is dismissed, observe album and credit tables in artist paging, and simplify the cover fallback to one ranked union. Cover the association-table and invalidation paths in tests.
This commit is contained in:
@@ -10,6 +10,7 @@ import androidx.room3.Transaction
|
||||
import androidx.room3.Upsert
|
||||
import androidx.room3.paging.PagingSourceDaoReturnTypeConverter
|
||||
import com.subtracks.data.model.Album
|
||||
import com.subtracks.data.model.AlbumArtistRef
|
||||
import com.subtracks.data.model.AlbumSongItem
|
||||
import com.subtracks.data.model.Artist
|
||||
import com.subtracks.data.model.Disc
|
||||
@@ -18,6 +19,7 @@ import com.subtracks.data.model.Playlist
|
||||
import com.subtracks.data.model.PlaylistSong
|
||||
import com.subtracks.data.model.PlaylistSongItem
|
||||
import com.subtracks.data.model.Song
|
||||
import com.subtracks.data.model.SongArtistRef
|
||||
import com.subtracks.data.model.Source
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
|
||||
@@ -163,34 +165,28 @@ internal const val ARTISTS_FILTER =
|
||||
"OR (length(:search) < 3 AND (instr(lower(name), lower(:search)) > 0 " +
|
||||
"OR instr(lower(sortName), lower(:search)) > 0))) "
|
||||
|
||||
internal const val ARTIST_ALBUM_CREDIT_ART =
|
||||
"SELECT albums.coverArt FROM (" +
|
||||
"SELECT albums.id, albums.coverArt, albums.year, albums.sortName FROM albums INDEXED BY index_albums_sourceId_artistId " +
|
||||
internal const val ARTIST_COVER_ART =
|
||||
"COALESCE(artists.coverArt, (" +
|
||||
"SELECT credit_albums.coverArt FROM (" +
|
||||
"SELECT 0 AS creditRank, albums.id, albums.coverArt, albums.year, albums.sortName FROM albums " +
|
||||
"INDEXED BY index_albums_sourceId_artistId " +
|
||||
"WHERE albums.sourceId = artists.sourceId AND albums.artistId = artists.id " +
|
||||
"UNION " +
|
||||
"SELECT albums.id, albums.coverArt, albums.year, albums.sortName FROM album_artists " +
|
||||
"SELECT 0, albums.id, albums.coverArt, albums.year, albums.sortName FROM album_artists " +
|
||||
"JOIN albums ON albums.sourceId = album_artists.sourceId AND albums.id = album_artists.albumId " +
|
||||
"WHERE album_artists.sourceId = artists.sourceId AND album_artists.artistId = artists.id" +
|
||||
") albums ORDER BY albums.year DESC, albums.sortName COLLATE NOCASE, albums.id LIMIT 1"
|
||||
|
||||
internal const val ARTIST_TRACK_CREDIT_ART =
|
||||
"SELECT albums.coverArt FROM (" +
|
||||
"SELECT albums.id, albums.coverArt, albums.year, albums.sortName FROM songs " +
|
||||
"WHERE album_artists.sourceId = artists.sourceId AND album_artists.artistId = artists.id " +
|
||||
"UNION " +
|
||||
"SELECT 1, albums.id, albums.coverArt, albums.year, albums.sortName FROM songs " +
|
||||
"JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"WHERE songs.sourceId = artists.sourceId AND songs.artistId = artists.id " +
|
||||
"UNION " +
|
||||
"SELECT albums.id, albums.coverArt, albums.year, albums.sortName FROM song_artists " +
|
||||
"SELECT 1, albums.id, albums.coverArt, albums.year, albums.sortName FROM song_artists " +
|
||||
"JOIN songs ON songs.sourceId = song_artists.sourceId AND songs.id = song_artists.songId " +
|
||||
"JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"WHERE song_artists.sourceId = artists.sourceId AND song_artists.artistId = artists.id" +
|
||||
") albums ORDER BY albums.year DESC, albums.sortName COLLATE NOCASE, albums.id LIMIT 1"
|
||||
|
||||
internal const val ARTIST_COVER_ART =
|
||||
"COALESCE(artists.coverArt, CASE " +
|
||||
"WHEN EXISTS (SELECT 1 FROM albums WHERE albums.sourceId = artists.sourceId AND albums.artistId = artists.id) " +
|
||||
"OR EXISTS (SELECT 1 FROM album_artists " +
|
||||
"WHERE album_artists.sourceId = artists.sourceId AND album_artists.artistId = artists.id) " +
|
||||
"THEN ($ARTIST_ALBUM_CREDIT_ART) ELSE ($ARTIST_TRACK_CREDIT_ART) END)"
|
||||
") credit_albums " +
|
||||
"ORDER BY credit_albums.creditRank, credit_albums.year DESC, credit_albums.sortName COLLATE NOCASE, " +
|
||||
"credit_albums.id LIMIT 1))"
|
||||
|
||||
internal const val ARTIST_COLUMNS =
|
||||
"artists.sourceId, artists.id, artists.name, artists.albumCount, artists.starred, " +
|
||||
@@ -640,7 +636,16 @@ interface LibraryDao {
|
||||
@RawQuery(observedEntities = [Album::class])
|
||||
fun albumsPaging(query: RoomRawQuery): PagingSource<Int, Album>
|
||||
|
||||
@RawQuery(observedEntities = [Artist::class, Source::class])
|
||||
@RawQuery(
|
||||
observedEntities = [
|
||||
Artist::class,
|
||||
Source::class,
|
||||
Album::class,
|
||||
AlbumArtistRef::class,
|
||||
Song::class,
|
||||
SongArtistRef::class,
|
||||
],
|
||||
)
|
||||
fun artistsPaging(query: RoomRawQuery): PagingSource<Int, Artist>
|
||||
|
||||
@RawQuery(observedEntities = [Playlist::class])
|
||||
|
||||
@@ -20,7 +20,6 @@ import androidx.compose.material3.ModalBottomSheet
|
||||
import androidx.compose.material3.Text
|
||||
import androidx.compose.material3.rememberModalBottomSheetState
|
||||
import androidx.compose.runtime.Composable
|
||||
import androidx.compose.runtime.rememberCoroutineScope
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.graphics.Color
|
||||
import androidx.compose.ui.platform.testTag
|
||||
@@ -28,7 +27,6 @@ import androidx.compose.ui.res.stringResource
|
||||
import androidx.compose.ui.unit.dp
|
||||
import com.subtracks.R
|
||||
import com.subtracks.data.model.ArtistRef
|
||||
import kotlinx.coroutines.launch
|
||||
|
||||
const val ARTIST_CHOOSER_TAG = "artistChooser"
|
||||
|
||||
@@ -40,7 +38,6 @@ fun ArtistChooserSheet(
|
||||
onDismiss: () -> Unit,
|
||||
) {
|
||||
val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
|
||||
val scope = rememberCoroutineScope()
|
||||
|
||||
ModalBottomSheet(
|
||||
onDismissRequest = onDismiss,
|
||||
@@ -66,11 +63,7 @@ fun ArtistChooserSheet(
|
||||
headlineContent = { Text(artist.name.ifBlank { stringResource(R.string.resources_filter_artist) }) },
|
||||
leadingContent = { Icon(Icons.Rounded.Person, contentDescription = null) },
|
||||
colors = ListItemDefaults.colors(containerColor = Color.Transparent),
|
||||
modifier =
|
||||
Modifier.clickable {
|
||||
onArtistClick(artist)
|
||||
scope.launch { sheetState.hide() }
|
||||
},
|
||||
modifier = Modifier.clickable { onArtistClick(artist) },
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -23,6 +23,7 @@ import androidx.compose.runtime.LaunchedEffect
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.runtime.mutableStateOf
|
||||
import androidx.compose.runtime.remember
|
||||
import androidx.compose.runtime.saveable.rememberSaveable
|
||||
import androidx.compose.runtime.setValue
|
||||
import androidx.compose.ui.Alignment
|
||||
import androidx.compose.ui.Modifier
|
||||
@@ -55,7 +56,13 @@ fun SearchField(
|
||||
textFieldValue = TextFieldValue(value, selection = TextRange(value.length))
|
||||
}
|
||||
}
|
||||
LaunchedEffect(Unit) { focusRequester.requestFocus() }
|
||||
var focusRequested by rememberSaveable { mutableStateOf(false) }
|
||||
LaunchedEffect(focusRequested) {
|
||||
if (!focusRequested) {
|
||||
focusRequester.requestFocus()
|
||||
focusRequested = true
|
||||
}
|
||||
}
|
||||
Surface(
|
||||
shape = RoundedCornerShape(8.dp),
|
||||
color = MaterialTheme.colorScheme.surfaceContainerHigh,
|
||||
|
||||
@@ -92,6 +92,7 @@ import androidx.compose.ui.layout.layout
|
||||
import androidx.compose.ui.layout.onGloballyPositioned
|
||||
import androidx.compose.ui.layout.onSizeChanged
|
||||
import androidx.compose.ui.platform.LocalDensity
|
||||
import androidx.compose.ui.platform.LocalFocusManager
|
||||
import androidx.compose.ui.res.stringResource
|
||||
import androidx.compose.ui.semantics.clearAndSetSemantics
|
||||
import androidx.compose.ui.semantics.contentDescription
|
||||
@@ -446,6 +447,7 @@ fun LibraryScreen(
|
||||
}
|
||||
|
||||
val density = LocalDensity.current
|
||||
val focusManager = LocalFocusManager.current
|
||||
val statusBarTop = with(density) { WindowInsets.statusBars.getTop(this).toDp() }
|
||||
val filtersActive = listQuery.starred != StarredFilter.Any || listQuery.downloaded || search.isNotEmpty()
|
||||
var tabBarHeightPx by remember { mutableFloatStateOf(with(density) { (TAB_BAR_CONTENT_HEIGHT + bottomInset).toPx() }) }
|
||||
@@ -495,7 +497,10 @@ fun LibraryScreen(
|
||||
albums.collectAsLazyPagingItems(),
|
||||
coverArt,
|
||||
listBottomInset,
|
||||
onAlbumClick,
|
||||
{
|
||||
focusManager.clearFocus()
|
||||
onAlbumClick(it)
|
||||
},
|
||||
onLongClick = onItemLongClick,
|
||||
filtered = filtersActive,
|
||||
onClearFilters = onClearFilters,
|
||||
@@ -512,7 +517,10 @@ fun LibraryScreen(
|
||||
artists.collectAsLazyPagingItems(),
|
||||
coverArt,
|
||||
listBottomInset,
|
||||
onArtistClick,
|
||||
{
|
||||
focusManager.clearFocus()
|
||||
onArtistClick(it)
|
||||
},
|
||||
onLongClick = onItemLongClick,
|
||||
filtered = filtersActive,
|
||||
onClearFilters = onClearFilters,
|
||||
@@ -529,7 +537,10 @@ fun LibraryScreen(
|
||||
playlists.collectAsLazyPagingItems(),
|
||||
coverArt,
|
||||
listBottomInset,
|
||||
onPlaylistClick,
|
||||
{
|
||||
focusManager.clearFocus()
|
||||
onPlaylistClick(it)
|
||||
},
|
||||
onLongClick = onItemLongClick,
|
||||
filtered = filtersActive,
|
||||
onClearFilters = onClearFilters,
|
||||
|
||||
@@ -32,6 +32,7 @@ import androidx.compose.runtime.saveable.rememberSaveable
|
||||
import androidx.compose.runtime.setValue
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.focus.FocusRequester
|
||||
import androidx.compose.ui.platform.LocalFocusManager
|
||||
import androidx.compose.ui.res.stringResource
|
||||
import androidx.compose.ui.unit.dp
|
||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||
@@ -85,6 +86,7 @@ fun SearchRoute(
|
||||
val query by viewModel.query.collectAsStateWithLifecycle()
|
||||
val results by viewModel.results.collectAsStateWithLifecycle()
|
||||
val playback by playbackController.state.collectAsStateWithLifecycle()
|
||||
val focusManager = LocalFocusManager.current
|
||||
var filterMask by rememberSaveable { mutableIntStateOf(allFilterMask()) }
|
||||
val actions =
|
||||
ItemActions(
|
||||
@@ -106,10 +108,22 @@ fun SearchRoute(
|
||||
coverArt = viewModel::coverArt,
|
||||
playingSongId = playback.item?.id,
|
||||
onBack = onBack,
|
||||
onAlbumClick = onAlbumClick,
|
||||
onArtistClick = onArtistClick,
|
||||
onPlaylistClick = onPlaylistClick,
|
||||
onSongClick = viewModel::play,
|
||||
onAlbumClick = {
|
||||
focusManager.clearFocus()
|
||||
onAlbumClick(it)
|
||||
},
|
||||
onArtistClick = {
|
||||
focusManager.clearFocus()
|
||||
onArtistClick(it)
|
||||
},
|
||||
onPlaylistClick = {
|
||||
focusManager.clearFocus()
|
||||
onPlaylistClick(it)
|
||||
},
|
||||
onSongClick = {
|
||||
focusManager.clearFocus()
|
||||
viewModel.play(it)
|
||||
},
|
||||
onLongClick = { contextMenuHost?.show(it, actions) },
|
||||
)
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import android.content.Context
|
||||
import androidx.paging.PagingSource
|
||||
import androidx.room3.Room
|
||||
import androidx.room3.useReaderConnection
|
||||
import androidx.room3.useWriterConnection
|
||||
import androidx.sqlite.driver.bundled.BundledSQLiteDriver
|
||||
import androidx.test.core.app.ApplicationProvider
|
||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||
@@ -556,6 +557,8 @@ class LibraryDaoTest {
|
||||
artist(sourceId, "ar-own", "Own", albumCount = 1).copy(coverArt = "own-art"),
|
||||
artist(sourceId, "ar-album", "Album", albumCount = 2),
|
||||
artist(sourceId, "ar-track", "Track", albumCount = 1),
|
||||
artist(sourceId, "ar-album-ref", "Album Ref", albumCount = 1),
|
||||
artist(sourceId, "ar-track-ref", "Track Ref", albumCount = 1),
|
||||
artist(sourceId, "ar-null-art", "Null Art", albumCount = 2),
|
||||
artist(sourceId, "ar-none", "None", albumCount = 0),
|
||||
),
|
||||
@@ -569,6 +572,10 @@ class LibraryDaoTest {
|
||||
.copy(artistId = "ar-album", coverArt = "old-art"),
|
||||
album(sourceId, "track", "Track", year = 2021, starred = null)
|
||||
.copy(artistId = "ar-other", coverArt = "track-art"),
|
||||
album(sourceId, "album-ref", "Album Ref", year = 2021, starred = null)
|
||||
.copy(artistId = "ar-other", coverArt = "album-ref-art"),
|
||||
album(sourceId, "track-ref", "Track Ref", year = 2019, starred = null)
|
||||
.copy(artistId = "ar-other", coverArt = "track-ref-art"),
|
||||
album(sourceId, "null-new", "Null New", year = 2022, starred = null)
|
||||
.copy(artistId = "ar-null-art", coverArt = null),
|
||||
album(sourceId, "null-old", "Null Old", year = 1990, starred = null)
|
||||
@@ -580,6 +587,10 @@ class LibraryDaoTest {
|
||||
song(sourceId, "s-own", "Own", starred = null).copy(albumId = "own", artistId = "ar-own"),
|
||||
song(sourceId, "s-album", "Album", starred = null).copy(albumId = "album-new", artistId = "ar-album"),
|
||||
song(sourceId, "s-track", "Track", starred = null).copy(albumId = "track", artistId = "ar-track"),
|
||||
song(sourceId, "s-album-ref", "Album Ref", starred = null)
|
||||
.copy(albumId = "album-ref", artistId = "ar-other"),
|
||||
song(sourceId, "s-track-ref", "Track Ref", starred = null)
|
||||
.copy(albumId = "track-ref", artistId = "ar-other"),
|
||||
song(sourceId, "s-null-new", "Null New", starred = null)
|
||||
.copy(albumId = "null-new", artistId = "ar-null-art"),
|
||||
song(sourceId, "s-null-old", "Null Old", starred = null)
|
||||
@@ -587,14 +598,41 @@ class LibraryDaoTest {
|
||||
),
|
||||
)
|
||||
|
||||
db.useWriterConnection { connection ->
|
||||
connection.usePrepared(
|
||||
"INSERT INTO album_artists (sourceId, albumId, position, artistId, name) " +
|
||||
"VALUES (?, 'album-ref', 0, 'ar-album-ref', 'Album Ref')",
|
||||
) { statement ->
|
||||
statement.bindLong(1, sourceId)
|
||||
statement.step()
|
||||
}
|
||||
connection.usePrepared(
|
||||
"INSERT INTO song_artists (sourceId, songId, position, artistId, name) " +
|
||||
"VALUES (?, 's-track-ref', 0, 'ar-track-ref', 'Track Ref')",
|
||||
) { statement ->
|
||||
statement.bindLong(1, sourceId)
|
||||
statement.step()
|
||||
}
|
||||
}
|
||||
|
||||
val artists = dao.artistsByName(sourceId, 0, "").page().associateBy { it.id }
|
||||
|
||||
assertEquals("own-art", artists.getValue("ar-own").coverArt)
|
||||
assertEquals("new-art", artists.getValue("ar-album").coverArt)
|
||||
assertEquals("track-art", artists.getValue("ar-track").coverArt)
|
||||
assertEquals("album-ref-art", artists.getValue("ar-album-ref").coverArt)
|
||||
assertEquals("track-ref-art", artists.getValue("ar-track-ref").coverArt)
|
||||
assertNull(artists.getValue("ar-null-art").coverArt)
|
||||
assertNull(artists.getValue("ar-none").coverArt)
|
||||
assertEquals("new-art", dao.artistOnce(sourceId, "ar-album")!!.coverArt)
|
||||
assertEquals(
|
||||
"album-ref-art",
|
||||
dao
|
||||
.artistsPaging(artistsDefaultQuery(sourceId, 0, "", ArtistSort.Name, false))
|
||||
.page()
|
||||
.first { it.id == "ar-album-ref" }
|
||||
.coverArt,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -614,6 +652,33 @@ class LibraryDaoTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun artistArtChangesInvalidateTheArtistPagingSource() =
|
||||
runTest {
|
||||
val sourceId = source()
|
||||
val dao = db.libraryDao()
|
||||
dao.upsertArtists(listOf(artist(sourceId, "ar-1", "Artist", albumCount = 1)))
|
||||
dao.upsertAlbums(listOf(album(sourceId, "al-1", "Album", year = null, starred = null)))
|
||||
|
||||
val albumInvalidated = AtomicBoolean(false)
|
||||
val albumPaging = dao.artistsPaging(artistsDefaultQuery(sourceId, 0, "", ArtistSort.Name, false))
|
||||
albumPaging.registerInvalidatedCallback { albumInvalidated.set(true) }
|
||||
albumPaging.page()
|
||||
|
||||
dao.upsertAlbums(
|
||||
listOf(album(sourceId, "al-1", "Album", year = null, starred = null).copy(coverArt = "cover")),
|
||||
)
|
||||
awaitUntil("artist paging observes album writes") { albumInvalidated.get() }
|
||||
|
||||
val songInvalidated = AtomicBoolean(false)
|
||||
val songPaging = dao.artistsPaging(artistsDefaultQuery(sourceId, 0, "", ArtistSort.Name, false))
|
||||
songPaging.registerInvalidatedCallback { songInvalidated.set(true) }
|
||||
songPaging.page()
|
||||
|
||||
dao.upsertSongs(listOf(song(sourceId, "s-1", "Song", starred = null)))
|
||||
awaitUntil("artist paging observes song writes") { songInvalidated.get() }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun albumCreditIdsFindCreditsThroughArtistIndexes() =
|
||||
runTest {
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
package com.subtracks.ui.search
|
||||
|
||||
import android.view.inputmethod.InputMethodManager
|
||||
import androidx.activity.ComponentActivity
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.runtime.mutableStateOf
|
||||
@@ -109,8 +110,14 @@ class SearchScreenTest {
|
||||
composeRule.onNode(hasSetTextAction()).performTextInput("!")
|
||||
assertEquals("roads!", query)
|
||||
|
||||
composeRule.runOnIdle { composeRule.activity.currentFocus?.clearFocus() }
|
||||
composeRule.runOnIdle { show = false }
|
||||
composeRule.waitForIdle()
|
||||
// performTextInput opens the IME; close it after the field is gone so later tests in the JVM
|
||||
// do not inherit a visible keyboard.
|
||||
composeRule.activity
|
||||
.getSystemService(InputMethodManager::class.java)
|
||||
.hideSoftInputFromWindow(composeRule.activity.window.decorView.windowToken, 0)
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user