Fix the alphabet buttons scroll bar on the Artists tab (#1775)
## Description positionOfLetter counted through /Items no matter which endpoint the grid was actually paging. A music library's Artists tab pages /Artists, so the index it returned had counted every album and song before the letter - an order of magnitude more rows than the artist grid holds - and every letter jumped to the last row. Count through the same endpoint createPager builds its pager from: /Artists for the ARTIST override, /Items for NONE. The artist request is now built once in createGetArtistsRequest and shared by both, so the two cannot drift apart. GetPersonsRequest has neither nameLessThan nor startIndex, so /Persons cannot be counted up to a letter at all; the PERSON override returns null, which the call sites already map to the "no jump" sentinel. That path is unreachable today - the only PERSON tab sorts by ItemSortBy.DEFAULT, so showLetterButtons is never true there and AlphabetButtons is never composed. ### Related issues https://github.com/damontecres/Wholphin/issues/1774 ### Testing Select a music library -> Go to Artist tab -> Select a letter on the right side alphabet scroll bar ## AI or LLM usage I used Cursor and Claude Code for planning, implementation and code review. --------- Co-authored-by: Damontecres <damontecres@gmail.com>
This commit is contained in:
Stefan Bullones
GitHub
parent
6ad96865e7
commit
d7d63f52c3
3 changed files with 259 additions and 33 deletions
|
|
@ -71,6 +71,7 @@ import com.github.damontecres.wholphin.util.GetArtistsHandler
|
|||
import com.github.damontecres.wholphin.util.GetItemsRequestHandler
|
||||
import com.github.damontecres.wholphin.util.GetPersonsHandler
|
||||
import com.github.damontecres.wholphin.util.LoadingState
|
||||
import com.github.damontecres.wholphin.util.RequestHandler
|
||||
import com.github.damontecres.wholphin.util.WholphinDispatchers
|
||||
import com.github.damontecres.wholphin.util.successValue
|
||||
import dagger.assisted.Assisted
|
||||
|
|
@ -378,26 +379,30 @@ class CollectionFolderViewModel
|
|||
}
|
||||
|
||||
GetItemsFilterOverride.ARTIST -> {
|
||||
val item = state.value.item.successValue
|
||||
val request =
|
||||
filter.applyTo(
|
||||
GetArtistsRequest(
|
||||
parentId = item?.id,
|
||||
enableImageTypes = listOf(ImageType.PRIMARY, ImageType.THUMB),
|
||||
),
|
||||
)
|
||||
val newPager =
|
||||
ApiRequestPager(
|
||||
api,
|
||||
request,
|
||||
GetArtistsHandler,
|
||||
viewModelScope,
|
||||
useSeriesForPrimary = useSeriesForPrimary,
|
||||
)
|
||||
newPager
|
||||
ApiRequestPager(
|
||||
api,
|
||||
createGetArtistsRequest(filter),
|
||||
GetArtistsHandler,
|
||||
viewModelScope,
|
||||
useSeriesForPrimary = useSeriesForPrimary,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Shared with [positionOfLetter] so that the alphabet jump counts the same artists that are
|
||||
* actually paged through
|
||||
*/
|
||||
private fun createGetArtistsRequest(filter: GetItemsFilter): GetArtistsRequest {
|
||||
val item = state.value.item.successValue
|
||||
return filter.applyTo(
|
||||
GetArtistsRequest(
|
||||
parentId = item?.id,
|
||||
enableImageTypes = listOf(ImageType.PRIMARY, ImageType.THUMB),
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
private fun createGetItemsRequest(
|
||||
sortAndDirection: SortAndDirection,
|
||||
recursive: Boolean,
|
||||
|
|
@ -460,26 +465,57 @@ class CollectionFolderViewModel
|
|||
filterOption,
|
||||
)
|
||||
|
||||
/**
|
||||
* The count must come from the same endpoint that [createPager] pages through, otherwise
|
||||
* the index refers to a different result set and lands past the end of the list.
|
||||
*/
|
||||
suspend fun positionOfLetter(letter: Char): Int? =
|
||||
withContext(WholphinDispatchers.IO) {
|
||||
val sort = state.value.sortAndDirection
|
||||
val filter = state.value.filter
|
||||
val request =
|
||||
createGetItemsRequest(
|
||||
sortAndDirection = sort,
|
||||
recursive = recursive,
|
||||
filter = filter,
|
||||
).copy(
|
||||
enableImageTypes = null,
|
||||
fields = null,
|
||||
nameLessThan = letter.toString(),
|
||||
limit = 0,
|
||||
enableTotalRecordCount = true,
|
||||
)
|
||||
val result by GetItemsRequestHandler.execute(api, request)
|
||||
result.totalRecordCount
|
||||
when (filter.override) {
|
||||
GetItemsFilterOverride.ARTIST -> {
|
||||
GetArtistsHandler.countMatching(
|
||||
createGetArtistsRequest(filter).copy(
|
||||
enableImageTypes = null,
|
||||
nameLessThan = letter.toString(),
|
||||
limit = 0,
|
||||
enableTotalRecordCount = true,
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
// GetPersonsRequest has no nameLessThan or startIndex, so /Persons cannot be
|
||||
// counted up to a letter at all
|
||||
GetItemsFilterOverride.PERSON -> {
|
||||
null
|
||||
}
|
||||
|
||||
GetItemsFilterOverride.NONE -> {
|
||||
GetItemsRequestHandler.countMatching(
|
||||
createGetItemsRequest(
|
||||
sortAndDirection = state.value.sortAndDirection,
|
||||
recursive = recursive,
|
||||
filter = filter,
|
||||
).copy(
|
||||
enableImageTypes = null,
|
||||
fields = null,
|
||||
nameLessThan = letter.toString(),
|
||||
limit = 0,
|
||||
enableTotalRecordCount = true,
|
||||
),
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* [request] must ask for the count, ie `limit = 0` and `enableTotalRecordCount = true`
|
||||
*/
|
||||
private suspend fun <T> RequestHandler<T>.countMatching(request: T): Int {
|
||||
val result by execute(api, request)
|
||||
return result.totalRecordCount
|
||||
}
|
||||
|
||||
override fun setWatched(
|
||||
position: Int,
|
||||
itemId: UUID,
|
||||
|
|
|
|||
|
|
@ -17,4 +17,7 @@ fun SemanticsNodeInteraction.performClickEnter() =
|
|||
|
||||
fun <T> successResponse(content: T) = Response(content, 200, emptyMap())
|
||||
|
||||
fun successQueryResult(items: List<BaseItemDto>) = successResponse(BaseItemDtoQueryResult(items, items.size, 0))
|
||||
fun successQueryResult(
|
||||
items: List<BaseItemDto> = emptyList(),
|
||||
totalRecordCount: Int = items.size,
|
||||
) = successResponse(BaseItemDtoQueryResult(items, totalRecordCount, 0))
|
||||
|
|
|
|||
|
|
@ -0,0 +1,187 @@
|
|||
package com.github.damontecres.wholphin.ui.components
|
||||
|
||||
import androidx.lifecycle.SavedStateHandle
|
||||
import com.github.damontecres.wholphin.data.ServerRepository
|
||||
import com.github.damontecres.wholphin.data.model.CollectionFolderFilter
|
||||
import com.github.damontecres.wholphin.data.model.GetItemsFilter
|
||||
import com.github.damontecres.wholphin.data.model.GetItemsFilterOverride
|
||||
import com.github.damontecres.wholphin.services.DeletedItem
|
||||
import com.github.damontecres.wholphin.services.MediaManagementService
|
||||
import com.github.damontecres.wholphin.ui.successQueryResult
|
||||
import com.github.damontecres.wholphin.ui.successResponse
|
||||
import com.github.damontecres.wholphin.util.GetArtistsHandler
|
||||
import com.github.damontecres.wholphin.util.GetItemsRequestHandler
|
||||
import com.github.damontecres.wholphin.util.GetPersonsHandler
|
||||
import com.github.damontecres.wholphin.util.WholphinDispatchers
|
||||
import com.github.damontecres.wholphin.util.configure
|
||||
import com.github.damontecres.wholphin.util.reset
|
||||
import io.mockk.coEvery
|
||||
import io.mockk.coVerify
|
||||
import io.mockk.every
|
||||
import io.mockk.mockk
|
||||
import io.mockk.mockkObject
|
||||
import io.mockk.slot
|
||||
import io.mockk.unmockkObject
|
||||
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||
import kotlinx.coroutines.flow.MutableSharedFlow
|
||||
import kotlinx.coroutines.test.StandardTestDispatcher
|
||||
import kotlinx.coroutines.test.advanceUntilIdle
|
||||
import kotlinx.coroutines.test.runTest
|
||||
import org.jellyfin.sdk.api.client.ApiClient
|
||||
import org.jellyfin.sdk.api.client.extensions.userLibraryApi
|
||||
import org.jellyfin.sdk.api.operations.UserLibraryApi
|
||||
import org.jellyfin.sdk.model.api.BaseItemDto
|
||||
import org.jellyfin.sdk.model.api.BaseItemKind
|
||||
import org.jellyfin.sdk.model.api.CollectionType
|
||||
import org.jellyfin.sdk.model.api.request.GetArtistsRequest
|
||||
import org.jellyfin.sdk.model.api.request.GetItemsRequest
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import java.util.UUID
|
||||
|
||||
/**
|
||||
* Each endpoint is stubbed with a distinct total, so a returned index identifies which endpoint
|
||||
* produced it.
|
||||
*/
|
||||
@OptIn(ExperimentalCoroutinesApi::class)
|
||||
class CollectionFolderViewModelTest {
|
||||
private val testDispatcher = StandardTestDispatcher()
|
||||
|
||||
private val mockApi = mockk<ApiClient>(relaxed = true)
|
||||
private val mockServerRepository = mockk<ServerRepository>(relaxed = true)
|
||||
private val mockMediaManagementService = mockk<MediaManagementService>(relaxed = true)
|
||||
private val mockUserLibraryApi = mockk<UserLibraryApi>()
|
||||
|
||||
private val artistsRequest = slot<GetArtistsRequest>()
|
||||
private val itemsRequest = slot<GetItemsRequest>()
|
||||
|
||||
private val libraryId = UUID.randomUUID()
|
||||
|
||||
private val library =
|
||||
BaseItemDto(
|
||||
id = libraryId,
|
||||
type = BaseItemKind.COLLECTION_FOLDER,
|
||||
collectionType = CollectionType.MUSIC,
|
||||
)
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
WholphinDispatchers.configure(testDispatcher)
|
||||
|
||||
every { mockApi.userLibraryApi } returns mockUserLibraryApi
|
||||
coEvery { mockUserLibraryApi.getItem(libraryId) } returns successResponse(library)
|
||||
|
||||
// No current user, so no saved LibraryDisplayInfo is consulted
|
||||
every { mockServerRepository.currentUser } returns null
|
||||
every { mockMediaManagementService.deletedItemFlow } returns MutableSharedFlow<DeletedItem>()
|
||||
|
||||
// mockkObject spies by default, so every handler must be stubbed or the real one runs
|
||||
// against the relaxed ApiClient
|
||||
mockkObject(GetItemsRequestHandler)
|
||||
mockkObject(GetArtistsHandler)
|
||||
mockkObject(GetPersonsHandler)
|
||||
coEvery { GetItemsRequestHandler.execute(mockApi, capture(itemsRequest)) } returns
|
||||
successQueryResult(totalRecordCount = LIBRARY_ITEM_COUNT)
|
||||
coEvery { GetArtistsHandler.execute(mockApi, capture(artistsRequest)) } returns
|
||||
successQueryResult(totalRecordCount = ARTIST_COUNT)
|
||||
coEvery { GetPersonsHandler.execute(mockApi, any()) } returns
|
||||
successQueryResult(totalRecordCount = PERSON_COUNT)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
WholphinDispatchers.reset()
|
||||
unmockkObject(GetItemsRequestHandler)
|
||||
unmockkObject(GetArtistsHandler)
|
||||
unmockkObject(GetPersonsHandler)
|
||||
}
|
||||
|
||||
private fun createViewModel(filter: GetItemsFilter) =
|
||||
CollectionFolderViewModel(
|
||||
savedStateHandle = SavedStateHandle(),
|
||||
api = mockApi,
|
||||
context = mockk(relaxed = true),
|
||||
serverRepository = mockServerRepository,
|
||||
libraryDisplayInfoDao = mockk(relaxed = true),
|
||||
favoriteWatchManager = mockk(relaxed = true),
|
||||
backdropService = mockk(relaxed = true),
|
||||
navigationManager = mockk(relaxed = true),
|
||||
themeSongPlayer = mockk(relaxed = true),
|
||||
userPreferencesService = mockk(relaxed = true),
|
||||
mediaManagementService = mockMediaManagementService,
|
||||
musicService = mockk(relaxed = true),
|
||||
streamChoiceService = mockk(relaxed = true),
|
||||
mediaReportService = mockk(relaxed = true),
|
||||
filterOptionCache = mockk(relaxed = true),
|
||||
itemId = libraryId.toString(),
|
||||
initialSortAndDirection = null,
|
||||
recursive = true,
|
||||
collectionFilter = CollectionFolderFilter(filter = filter),
|
||||
useSeriesForPrimary = false,
|
||||
defaultViewOptions = ViewOptionsSquare,
|
||||
)
|
||||
|
||||
/** Counting via /Items would return every song and album, overshooting the artist grid */
|
||||
@Test
|
||||
fun `positionOfLetter counts artists for the ARTIST override`() =
|
||||
runTest(testDispatcher) {
|
||||
val viewModel = createViewModel(GetItemsFilter(override = GetItemsFilterOverride.ARTIST))
|
||||
advanceUntilIdle()
|
||||
|
||||
val position = viewModel.positionOfLetter('K')
|
||||
|
||||
assertEquals(ARTIST_COUNT, position)
|
||||
assertEquals("K", artistsRequest.captured.nameLessThan)
|
||||
assertEquals(libraryId, artistsRequest.captured.parentId)
|
||||
assertEquals(0, artistsRequest.captured.limit)
|
||||
|
||||
// Counting library items rather than artists is the bug this guards against
|
||||
coVerify(exactly = 0) { GetItemsRequestHandler.execute(any(), any<GetItemsRequest>()) }
|
||||
}
|
||||
|
||||
/**
|
||||
* /Persons has no nameLessThan or startIndex, so counting up to a letter is not expressible.
|
||||
* Falling back to /Items would count media rather than people, ie the same defect as the artist
|
||||
* grid, so guard against it even though no screen currently sorts people by name.
|
||||
*/
|
||||
@Test
|
||||
fun `positionOfLetter does not fall back to counting items for the PERSON override`() =
|
||||
runTest(testDispatcher) {
|
||||
val viewModel = createViewModel(GetItemsFilter(override = GetItemsFilterOverride.PERSON))
|
||||
advanceUntilIdle()
|
||||
|
||||
val position = viewModel.positionOfLetter('K')
|
||||
|
||||
assertEquals(null, position)
|
||||
coVerify(exactly = 0) { GetItemsRequestHandler.execute(any(), any<GetItemsRequest>()) }
|
||||
coVerify(exactly = 0) { GetArtistsHandler.execute(any(), any<GetArtistsRequest>()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `positionOfLetter counts items for the NONE override`() =
|
||||
runTest(testDispatcher) {
|
||||
val viewModel =
|
||||
createViewModel(
|
||||
GetItemsFilter(
|
||||
override = GetItemsFilterOverride.NONE,
|
||||
includeItemTypes = listOf(BaseItemKind.MUSIC_ALBUM),
|
||||
),
|
||||
)
|
||||
advanceUntilIdle()
|
||||
|
||||
val position = viewModel.positionOfLetter('K')
|
||||
|
||||
assertEquals(LIBRARY_ITEM_COUNT, position)
|
||||
assertEquals("K", itemsRequest.captured.nameLessThan)
|
||||
assertEquals(libraryId, itemsRequest.captured.parentId)
|
||||
assertEquals(0, itemsRequest.captured.limit)
|
||||
}
|
||||
|
||||
companion object {
|
||||
private const val LIBRARY_ITEM_COUNT = 7315
|
||||
private const val ARTIST_COUNT = 730
|
||||
private const val PERSON_COUNT = 12
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue