Fix/Make only one instance of a song have the playing icon

[laoluogun]

Hi, when inside the queue tab, if you have multiple of the same song queued up, each instance of the song will display as currently playing - caused by isPlaying = track.videoId == songEntity?.videoId,

Before the fix :

Multiple.Playing.Icons.Before.Fix.mp4

I fixed this by having tracking currentSongIndex and changing isPlaying to isPlaying = index == currentSongIndex.

After the fix:

Multiple.Playing.Icons.After.Fix.mp4

Note: to demonstrate the bug, the recordings were taken on a build that features a playNext duplication fix (maxrave-dev/core/pull/15) to consistently get duplicate tracks. This pull request is independent of the other, but was useful for testing.

[maxrave-dev]

Index && videoId need same

[Copilot]

Pull request overview

This PR fixes the queue UI so only the actually-playing instance of a duplicated track shows the playing indicator, instead of every item with the same videoId. (PR này sửa UI hàng đợi để chỉ một bản thể của bài hát đang phát hiển thị biểu tượng đang phát, thay vì mọi mục có cùng videoId đều hiển thị.)

Changes:

• Introduces a currentSongIndex concept in QueueBottomSheet and uses it to drive isPlaying. (Giới thiệu khái niệm currentSongIndex trong QueueBottomSheet và dùng nó để điều khiển isPlaying.)
• Updates the queue item rendering to set isPlaying = index == currentSongIndex rather than comparing videoId. (Cập nhật phần render mục trong hàng đợi để đặt isPlaying = index == currentSongIndex thay vì so sánh videoId.)

Suppressed comments (1)

composeApp/src/commonMain/kotlin/com/maxrave/simpmusic/ui/component/ModalBottomSheet.kt:1175

• If currentSongIndex can be -1 (no active item yet), index == currentSongIndex will never be true, but with the previous ?: 0 fallback it could incorrectly highlight the first item.

Guarding against negative indices makes the intent explicit and prevents regressions if currentOrderIndex() returns -1.

                                    isPlaying = index == currentSongIndex,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[laoluogun]

The new changes depend on a core PR maxrave-dev/core#19 to fully work on mediaItem transitions but works otherwise.

[laoluogun]

There are two changes to the core submodule which this function depends on, however I believe it is completely worth it as they are very minor and simply fix the functionality displayed in already present functions as highlighted in the two PRs maxrave-dev/core#15 and maxrave-dev/core#19

Full functionality displayed here. Works with my keyboard's media player controller that plays next and back and works with the built in player controller.

Multiple.Playing.Icons.Final.Fixes.mp4

[laoluogun]

Here is the change demonstrated on Android.

Before

Android.Fix.Before.mp4

After

Android.Fix.Forward.and.Backward.mp4

Note: Updating <_currentSongIndex.valu = player.currentMediaItemIndex> in core is necessary for this to function maxrave-dev/core#19. This is a safe change that does not break any current functionalities.

[maxrave-dev]

Thanks for digging into this @laoluogun — the bug you found is real, and using the position instead of the id is the right instinct. Two duplicates of the same song genuinely do both light up today.

It turns out to be a much bigger bug than the queue sheet, though. The index functions themselves are wrong at the root:

1. currentSongIndex never updates on track transitions. _currentSongIndex is only written in moveMediaItem(), removeMediaItem() and moveItemUp/Down(). Neither handler's onMediaItemTransition touches it (Android or Desktop), and setCurrentSongIndex() has no callers at all. So once playback advances past whatever was current when the queue was last edited, index == currentSongIndex is false on every row and no song shows the playing icon.

2. It's in the wrong index space under shuffle. currentSongIndex is player.currentMediaItemIndex, an index into the original timeline, while the list rendered here has already been reordered by reorderShuffledQueue().

3. currentOrderIndex() isn't a safe substitute either. shuffleOrder only exposes the shuffled→original direction (getUnshuffledIndex), so currentOrderIndex() has to fall back to indexOfLast { videoId == ... } when shuffle is on — which runs straight into the exact duplicate problem this PR is trying to fix.

So I'm going to dig into the player layer myself and fix this at the source instead of patching it in the UI. Closing this PR for now.

Thanks a lot, though — the report and the before/after videos made it quick to pin down, and it surfaced something considerably worse than the symptom. Contributions are very welcome.