Merge pull request #17234 from Eneo-org/fix/syncplay-playqueue-index

Fix play queue index handling in SyncPlay

MediaBrowser.Controller/SyncPlay/Queue/PlayQueueManager.cs

@@ -272,7 +272,7 @@ namespace MediaBrowser.Controller.SyncPlay.Queue
         public void SetPlayingItemByIndex(int playlistIndex)
         {
             var playlist = GetPlaylistInternal();
-            if (playlistIndex < 0 || playlistIndex > playlist.Count)
+            if (playlistIndex < 0 || playlistIndex >= playlist.Count)
             {
                 PlayingItemIndex = NoPlayingItemIndex;
             }
@@ -293,6 +293,15 @@ namespace MediaBrowser.Controller.SyncPlay.Queue
         {
             var playingItem = GetPlayingItem();
 
+            // Removed items that precede the playing item shift its index as well.
+            var removedBeforePlayingItem = 0;
+            if (playingItem is not null)
+            {
+                removedBeforePlayingItem = GetPlaylistInternal()
+                    .Take(PlayingItemIndex)
+                    .Count(item => playlistItemIds.Contains(item.PlaylistItemId));
+            }
+
             _sortedPlaylist.RemoveAll(item => playlistItemIds.Contains(item.PlaylistItemId));
             _shuffledPlaylist.RemoveAll(item => playlistItemIds.Contains(item.PlaylistItemId));
 
@@ -303,12 +312,12 @@ namespace MediaBrowser.Controller.SyncPlay.Queue
                 if (playlistItemIds.Contains(playingItem.PlaylistItemId))
                 {
                     // Playing item has been removed, picking previous item.
-                    PlayingItemIndex--;
+                    PlayingItemIndex -= removedBeforePlayingItem + 1;
                     if (PlayingItemIndex < 0)
                     {
                         // Was first element, picking next if available.
                         // Default to no playing item otherwise.
-                        PlayingItemIndex = _sortedPlaylist.Count > 0 ? 0 : NoPlayingItemIndex;
+                        PlayingItemIndex = GetPlaylistInternal().Count > 0 ? 0 : NoPlayingItemIndex;
                     }
 
                     return true;
@@ -444,6 +453,11 @@ namespace MediaBrowser.Controller.SyncPlay.Queue
         /// <returns><c>true</c> if the playing item changed; <c>false</c> otherwise.</returns>
         public bool Next()
         {
+            if (GetPlaylistInternal().Count == 0)
+            {
+                return false;
+            }
+
             if (RepeatMode.Equals(GroupRepeatMode.RepeatOne))
             {
                 LastChange = DateTime.UtcNow;
@@ -474,6 +488,11 @@ namespace MediaBrowser.Controller.SyncPlay.Queue
         /// <returns><c>true</c> if the playing item changed; <c>false</c> otherwise.</returns>
         public bool Previous()
         {
+            if (GetPlaylistInternal().Count == 0)
+            {
+                return false;
+            }
+
             if (RepeatMode.Equals(GroupRepeatMode.RepeatOne))
             {
                 LastChange = DateTime.UtcNow;

tests/Jellyfin.Server.Implementations.Tests/SyncPlay/PlayQueueManagerTests.cs

@@ -0,0 +1,156 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using MediaBrowser.Controller.SyncPlay.Queue;
+using MediaBrowser.Model.SyncPlay;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.SyncPlay;
+
+public class PlayQueueManagerTests
+{
+    private static PlayQueueManager CreateQueue(int itemCount)
+    {
+        var items = Enumerable.Range(0, itemCount).Select(_ => Guid.NewGuid()).ToList();
+        var queue = new PlayQueueManager();
+        queue.SetPlaylist(items);
+        return queue;
+    }
+
+    [Fact]
+    public void RemoveFromPlaylist_PlayingItemAndPrecedingItemRemoved_PicksPreviousItem()
+    {
+        var queue = CreateQueue(5);
+        queue.SetPlayingItemByIndex(3);
+
+        var playlist = queue.GetPlaylist();
+        var expectedItemId = playlist[2].ItemId;
+        var toRemove = new List<Guid> { playlist[0].PlaylistItemId, playlist[3].PlaylistItemId };
+        var playingItemRemoved = queue.RemoveFromPlaylist(toRemove);
+
+        Assert.True(playingItemRemoved);
+        Assert.Equal(3, queue.GetPlaylist().Count);
+        Assert.Equal(1, queue.PlayingItemIndex);
+        Assert.Equal(expectedItemId, queue.GetPlayingItemId());
+    }
+
+    [Fact]
+    public void RemoveFromPlaylist_PlayingItemAndAllPrecedingItemsRemoved_PicksFirstRemainingItem()
+    {
+        var queue = CreateQueue(3);
+        queue.SetPlayingItemByIndex(2);
+
+        var playlist = queue.GetPlaylist();
+        var expectedItemId = playlist[1].ItemId;
+        var toRemove = new List<Guid> { playlist[0].PlaylistItemId, playlist[2].PlaylistItemId };
+        var playingItemRemoved = queue.RemoveFromPlaylist(toRemove);
+
+        Assert.True(playingItemRemoved);
+        Assert.Single(queue.GetPlaylist());
+        Assert.Equal(0, queue.PlayingItemIndex);
+        Assert.Equal(expectedItemId, queue.GetPlayingItemId());
+    }
+
+    [Fact]
+    public void RemoveFromPlaylist_AllItemsRemoved_ResetsPlayingItem()
+    {
+        var queue = CreateQueue(2);
+        queue.SetPlayingItemByIndex(1);
+
+        var toRemove = queue.GetPlaylist().Select(item => item.PlaylistItemId).ToList();
+        var playingItemRemoved = queue.RemoveFromPlaylist(toRemove);
+
+        Assert.True(playingItemRemoved);
+        Assert.Empty(queue.GetPlaylist());
+        Assert.False(queue.IsItemPlaying());
+        Assert.Equal(Guid.Empty, queue.GetPlayingItemPlaylistId());
+    }
+
+    [Fact]
+    public void RemoveFromPlaylist_ShuffleMode_PicksPreviousItem()
+    {
+        var queue = CreateQueue(5);
+        queue.SetShuffleMode(GroupShuffleMode.Shuffle);
+        queue.SetPlayingItemByIndex(3);
+
+        var playlist = queue.GetPlaylist();
+        var expectedItemId = playlist[2].ItemId;
+        var toRemove = new List<Guid> { playlist[0].PlaylistItemId, playlist[3].PlaylistItemId };
+        var playingItemRemoved = queue.RemoveFromPlaylist(toRemove);
+
+        Assert.True(playingItemRemoved);
+        Assert.Equal(3, queue.GetPlaylist().Count);
+        Assert.Equal(1, queue.PlayingItemIndex);
+        Assert.Equal(expectedItemId, queue.GetPlayingItemId());
+    }
+
+    [Fact]
+    public void RemoveFromPlaylist_PlayingItemNotRemoved_RestoresPlayingItem()
+    {
+        var queue = CreateQueue(3);
+        queue.SetPlayingItemByIndex(2);
+
+        var playlist = queue.GetPlaylist();
+        var expectedItemId = playlist[2].ItemId;
+        var toRemove = new List<Guid> { playlist[0].PlaylistItemId };
+        var playingItemRemoved = queue.RemoveFromPlaylist(toRemove);
+
+        Assert.False(playingItemRemoved);
+        Assert.Equal(1, queue.PlayingItemIndex);
+        Assert.Equal(expectedItemId, queue.GetPlayingItemId());
+    }
+
+    [Theory]
+    [InlineData(GroupRepeatMode.RepeatNone)]
+    [InlineData(GroupRepeatMode.RepeatOne)]
+    [InlineData(GroupRepeatMode.RepeatAll)]
+    public void Next_EmptyPlaylist_ReturnsFalse(GroupRepeatMode repeatMode)
+    {
+        var queue = new PlayQueueManager();
+        queue.SetRepeatMode(repeatMode);
+
+        Assert.False(queue.Next());
+        Assert.False(queue.IsItemPlaying());
+        Assert.Equal(Guid.Empty, queue.GetPlayingItemPlaylistId());
+    }
+
+    [Theory]
+    [InlineData(GroupRepeatMode.RepeatNone)]
+    [InlineData(GroupRepeatMode.RepeatOne)]
+    [InlineData(GroupRepeatMode.RepeatAll)]
+    public void Previous_EmptyPlaylist_ReturnsFalse(GroupRepeatMode repeatMode)
+    {
+        var queue = new PlayQueueManager();
+        queue.SetRepeatMode(repeatMode);
+
+        Assert.False(queue.Previous());
+        Assert.False(queue.IsItemPlaying());
+        Assert.Equal(Guid.Empty, queue.GetPlayingItemPlaylistId());
+    }
+
+    [Theory]
+    [InlineData(-1)]
+    [InlineData(2)]
+    [InlineData(3)]
+    public void SetPlayingItemByIndex_OutOfBounds_ResetsPlayingItem(int playlistIndex)
+    {
+        var queue = CreateQueue(2);
+
+        queue.SetPlayingItemByIndex(playlistIndex);
+
+        Assert.False(queue.IsItemPlaying());
+        Assert.Equal(Guid.Empty, queue.GetPlayingItemPlaylistId());
+    }
+
+    [Fact]
+    public void SetPlayingItemByIndex_InBounds_SetsPlayingItem()
+    {
+        var queue = CreateQueue(2);
+        var expectedItemId = queue.GetPlaylist()[1].ItemId;
+
+        queue.SetPlayingItemByIndex(1);
+
+        Assert.True(queue.IsItemPlaying());
+        Assert.Equal(expectedItemId, queue.GetPlayingItemId());
+    }
+}