Improve image provider sorting

Remove irrelevant check for ILocalImageProvider
Providers that are not IHasOrder default to middle, not beginning

MediaBrowser.Providers/Manager/ProviderManager.cs

@@ -310,31 +310,25 @@ namespace MediaBrowser.Providers.Manager
 
         private IEnumerable<IImageProvider> GetImageProviders(BaseItem item, LibraryOptions libraryOptions, MetadataOptions options, ImageRefreshOptions refreshOptions, bool includeDisabled)
         {
-            // Avoid implicitly captured closure
-            var currentOptions = options;
-
             var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name);
-            var typeFetcherOrder = typeOptions?.ImageFetcherOrder;
+            var fetcherOrder = typeOptions?.ImageFetcherOrder ?? options.ImageFetcherOrder;
 
             return ImageProviders.Where(i => CanRefresh(i, item, libraryOptions, refreshOptions, includeDisabled))
-                .OrderBy(i =>
-                {
-                    // See if there's a user-defined order
-                    if (i is not ILocalImageProvider)
-                    {
-                        var fetcherOrder = typeFetcherOrder ?? currentOptions.ImageFetcherOrder;
-                        var index = Array.IndexOf(fetcherOrder, i.Name);
+                .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name))
+                .ThenBy(GetOrder);
+        }
 
-                        if (index != -1)
-                        {
-                            return index;
-                        }
-                    }
+        private static int GetConfiguredOrder(string[] order, string providerName)
+        {
+            var index = Array.IndexOf(order, providerName);
 
-                    // Not configured. Just return some high number to put it at the end.
-                    return 100;
-                })
-            .ThenBy(GetOrder);
+            if (index != -1)
+            {
+                return index;
+            }
+
+            // default to end
+            return int.MaxValue;
         }
 
         /// <inheritdoc />
@@ -450,21 +444,6 @@ namespace MediaBrowser.Providers.Manager
             }
         }
 
-        /// <summary>
-        /// Gets the order.
-        /// </summary>
-        /// <param name="provider">The provider.</param>
-        /// <returns>System.Int32.</returns>
-        private int GetOrder(IImageProvider provider)
-        {
-            if (provider is not IHasOrder hasOrder)
-            {
-                return 0;
-            }
-
-            return hasOrder.Order;
-        }
-
         private int GetConfiguredOrder(BaseItem item, IMetadataProvider provider, LibraryOptions libraryOptions, MetadataOptions globalMetadataOptions)
         {
             // See if there's a user-defined order
@@ -500,6 +479,17 @@ namespace MediaBrowser.Providers.Manager
             return 100;
         }
 
+        private static int GetOrder(object provider)
+        {
+            if (provider is IHasOrder hasOrder)
+            {
+                return hasOrder.Order;
+            }
+
+            // after items that want to be first (~0) but before items that want to be last (~100)
+            return 50;
+        }
+
         private int GetDefaultOrder(IMetadataProvider provider)
         {
             if (provider is IHasOrder hasOrder)

tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs

@@ -16,44 +16,34 @@ namespace Jellyfin.Providers.Tests.Manager
 {
     public class ProviderManagerTests
     {
-        private static TheoryData<int, bool[]?, int[]?, int[]?, int?[]?, int[]> GetImageProvidersOrderData()
+        private static TheoryData<int, int[]?, int[]?, int?[]?, int[]> GetImageProvidersOrderData()
             => new ()
             {
-                { 3, null, null, null, null, new[] { 0, 1, 2 } }, // no order options set
+                { 3, null, null, null, new[] { 0, 1, 2 } }, // no order options set
 
                 // library options ordering
-                { 3, null, Array.Empty<int>(), null, null, new[] { 0, 1, 2 } }, // no order provided
-                { 3, null, new[] { 1 }, null, null, new[] { 1, 0, 2 } }, // one item in order
-                { 3, null, new[] { 2, 1, 0 }, null, null, new[] { 2, 1, 0 } }, // full reverse order
+                { 3, Array.Empty<int>(), null, null, new[] { 0, 1, 2 } }, // no order provided
+                { 3, new[] { 1 }, null, null, new[] { 1, 0, 2 } }, // one item in order
+                { 3, new[] { 2, 1, 0 }, null, null, new[] { 2, 1, 0 } }, // full reverse order
 
                 // server options ordering
-                { 3, null, null, Array.Empty<int>(), null, new[] { 0, 1, 2 } }, // no order provided
-                { 3, null, null, new[] { 1 }, null, new[] { 1, 0, 2 } }, // one item in order
-                { 3, null, null, new[] { 2, 1, 0 }, null, new[] { 2, 1, 0 } }, // full reverse order
+                { 3, null, Array.Empty<int>(), null, new[] { 0, 1, 2 } }, // no order provided
+                { 3, null, new[] { 1 }, null, new[] { 1, 0, 2 } }, // one item in order
+                { 3, null, new[] { 2, 1, 0 }, null, new[] { 2, 1, 0 } }, // full reverse order
 
                 // IHasOrder ordering
-                // TODO unintuitive - default if not IHasOrder is 0, not max
-                { 3, null, null, null, new int?[] { null, 0, null }, new[] { 0, 1, 2 } }, // one item with order 0, no change because default order value is 0
-                { 3, null, null, null, new int?[] { null, 1, null }, new[] { 0, 2, 1 } }, // one item in order (goes to end, not beginning)
-                { 3, null, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order
+                { 3, null, null, new int?[] { null, 1, null }, new[] { 1, 0, 2 } }, // one item with defined order
+                { 3, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order
 
                 // multiple orders set
-                // TODO should library fall through to server if both are set on different elements?
-                { 3, null, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored
-                { 3, null, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby
-                { 3, null, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins
-
-                // ordering with ILocalImageProvider
-                // TODO what is the value of testing for ILocalImageProvider on the sort, should this be removed? Behavior is unintuitive
-                { 3, new[] { false, true, false }, new[] { 1, 0, 2 }, null, null, new[] { 0, 2, 1 } }, // ILocalImageProvider - sorts to end even when set first
-                { 3, new[] { false, true, false }, new[] { 1 }, null, null, new[] { 0, 1, 2 } }, // ILocalImageProvider - set order ignored when only value set
-                { 2, new[] { true, true }, new[] { 1, 0 }, null, null, new[] { 0, 1 } }, // ILocalImageProvider - set order ignored
-                { 2, new[] { true, true }, null, null, new int?[] { 1, 0 }, new[] { 1, 0 } }, // ILocalImageProvider - IHasOrder applies
+                { 3, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored
+                { 3, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby
+                { 3, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins
             };
 
         [Theory]
         [MemberData(nameof(GetImageProvidersOrderData))]
-        public void GetImageProviders_ProviderOrder_MatchesExpected(int providerCount, bool[]? localImageProvider, int[]? libraryOrder, int[]? serverOrder, int?[]? hasOrderOrder, int[] expectedOrder)
+        public void GetImageProviders_ProviderOrder_MatchesExpected(int providerCount, int[]? libraryOrder, int[]? serverOrder, int?[]? hasOrderOrder, int[] expectedOrder)
         {
             var item = new Movie();
 
@@ -63,14 +53,7 @@ namespace Jellyfin.Providers.Tests.Manager
             for (var i = 0; i < providerCount; i++)
             {
                 var order = hasOrderOrder?[i];
-                if (localImageProvider != null && localImageProvider[i])
-                {
-                    providerList.Add(MockIImageProvider<ILocalImageProvider>(nameProvider(i), item, order));
-                }
-                else
-                {
-                    providerList.Add(MockIImageProvider<IImageProvider>(nameProvider(i), item, order));
-                }
+                providerList.Add(MockIImageProvider<IImageProvider>(nameProvider(i), item, order));
             }
 
             var libraryOptions = new LibraryOptions();