From b478b115e3194aa383f86d7d6fbf07e0f2bfadea Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Mon, 1 Nov 2021 02:38:12 +0100 Subject: Refactor to validate all images up front --- MediaBrowser.Controller/Entities/BaseItem.cs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) (limited to 'MediaBrowser.Controller/Entities') diff --git a/MediaBrowser.Controller/Entities/BaseItem.cs b/MediaBrowser.Controller/Entities/BaseItem.cs index 838a9f2f8d..7dd8e310ed 100644 --- a/MediaBrowser.Controller/Entities/BaseItem.cs +++ b/MediaBrowser.Controller/Entities/BaseItem.cs @@ -2495,11 +2495,11 @@ namespace MediaBrowser.Controller.Entities } /// - /// Adds the images. + /// Adds the images, updating metadata if they already are part of this item. /// /// Type of the image. /// The images. - /// true if XXXX, false otherwise. + /// true if images were added or updated, false otherwise. /// Cannot call AddImages with chapter images. public bool AddImages(ImageType imageType, List images) { @@ -2512,7 +2512,6 @@ namespace MediaBrowser.Controller.Entities .ToList(); var newImageList = new List(); - var imageAdded = false; var imageUpdated = false; foreach (var newImage in images) @@ -2528,7 +2527,6 @@ namespace MediaBrowser.Controller.Entities if (existing == null) { newImageList.Add(newImage); - imageAdded = true; } else { @@ -2549,19 +2547,6 @@ namespace MediaBrowser.Controller.Entities } } - if (imageAdded || images.Count != existingImages.Count) - { - var newImagePaths = images.Select(i => i.FullName).ToList(); - - var deleted = existingImages - .FindAll(i => i.IsLocalFile && !newImagePaths.Contains(i.Path.AsSpan(), StringComparison.OrdinalIgnoreCase) && !File.Exists(i.Path)); - - if (deleted.Count > 0) - { - ImageInfos = ImageInfos.Except(deleted).ToArray(); - } - } - if (newImageList.Count > 0) { ImageInfos = ImageInfos.Concat(newImageList.Select(i => GetImageInfo(i, imageType))).ToArray(); -- cgit v1.2.3 From 7fcf01235c2360ec64cad685df7f155ef3dee69a Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Tue, 2 Nov 2021 16:16:06 +0100 Subject: Change RemoveImages to array, improve download test --- MediaBrowser.Controller/Entities/BaseItem.cs | 2 +- .../Manager/ItemImageProvider.cs | 20 +++++------ .../Manager/ItemImageProviderTests.cs | 39 +++++++++++++--------- 3 files changed, 35 insertions(+), 26 deletions(-) (limited to 'MediaBrowser.Controller/Entities') diff --git a/MediaBrowser.Controller/Entities/BaseItem.cs b/MediaBrowser.Controller/Entities/BaseItem.cs index 7dd8e310ed..02ee97b23f 100644 --- a/MediaBrowser.Controller/Entities/BaseItem.cs +++ b/MediaBrowser.Controller/Entities/BaseItem.cs @@ -2345,7 +2345,7 @@ namespace MediaBrowser.Controller.Entities RemoveImages(new List { image }); } - public void RemoveImages(List deletedImages) + public void RemoveImages(IEnumerable deletedImages) { ImageInfos = ImageInfos.Except(deletedImages).ToArray(); } diff --git a/MediaBrowser.Providers/Manager/ItemImageProvider.cs b/MediaBrowser.Providers/Manager/ItemImageProvider.cs index 2c7d43c86d..8d5795f8e1 100644 --- a/MediaBrowser.Providers/Manager/ItemImageProvider.cs +++ b/MediaBrowser.Providers/Manager/ItemImageProvider.cs @@ -103,16 +103,16 @@ namespace MediaBrowser.Providers.Manager ImageRefreshOptions refreshOptions, CancellationToken cancellationToken) { - List oldBackdropImages = new List(); + var oldBackdropImages = Array.Empty(); if (refreshOptions.IsReplacingImage(ImageType.Backdrop)) { - oldBackdropImages = item.GetImages(ImageType.Backdrop).ToList(); + oldBackdropImages = item.GetImages(ImageType.Backdrop).ToArray(); } - List oldScreenshotImages = new List(); + var oldScreenshotImages = Array.Empty(); if (refreshOptions.IsReplacingImage(ImageType.Screenshot)) { - oldScreenshotImages = item.GetImages(ImageType.Screenshot).ToList(); + oldScreenshotImages = item.GetImages(ImageType.Screenshot).ToArray(); } var result = new RefreshResult { UpdateType = ItemUpdateType.None }; @@ -121,8 +121,8 @@ namespace MediaBrowser.Providers.Manager var typeOptions = libraryOptions.GetTypeOptions(typeName) ?? new TypeOptions { Type = typeName }; // track library limits, adding buffer to allow lazy replacing of current images - var backdropLimit = typeOptions.GetLimit(ImageType.Backdrop) + oldBackdropImages.Count; - var screenshotLimit = typeOptions.GetLimit(ImageType.Screenshot) + oldScreenshotImages.Count; + var backdropLimit = typeOptions.GetLimit(ImageType.Backdrop) + oldBackdropImages.Length; + var screenshotLimit = typeOptions.GetLimit(ImageType.Screenshot) + oldScreenshotImages.Length; var downloadedImages = new List(); foreach (var provider in providers) @@ -140,12 +140,12 @@ namespace MediaBrowser.Providers.Manager } // only delete existing multi-images if new ones were added - if (oldBackdropImages.Count > 0 && oldBackdropImages.Count < item.GetImages(ImageType.Backdrop).Count()) + if (oldBackdropImages.Length > 0 && oldBackdropImages.Length < item.GetImages(ImageType.Backdrop).Count()) { PruneImages(item, oldBackdropImages); } - if (oldScreenshotImages.Count > 0 && oldScreenshotImages.Count < item.GetImages(ImageType.Screenshot).Count()) + if (oldScreenshotImages.Length > 0 && oldScreenshotImages.Length < item.GetImages(ImageType.Screenshot).Count()) { PruneImages(item, oldScreenshotImages); } @@ -366,9 +366,9 @@ namespace MediaBrowser.Providers.Manager return options.IsEnabled(type); } - private void PruneImages(BaseItem item, List images) + private void PruneImages(BaseItem item, ItemImageInfo[] images) { - for (var i = 0; i < images.Count; i++) + for (var i = 0; i < images.Length; i++) { var image = images[i]; diff --git a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs index b5efd8f013..54f2cb71bf 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs @@ -409,21 +409,23 @@ namespace Jellyfin.Providers.Tests.Manager } [Theory] - [MemberData(nameof(GetImageTypesWithCount))] - public async void RefreshImages_EmptyNonStubItemPopulatedProviderRemote_DownloadsImages(ImageType imageType, int imageCount) + [InlineData(ImageType.Primary, 0, false)] // singular type only fetches if type is missing from item, no caching + [InlineData(ImageType.Backdrop, 0, false)] // empty item, no cache to check + [InlineData(ImageType.Backdrop, 1, false)] // populated item, cached so no download + [InlineData(ImageType.Backdrop, 1, true)] // populated item, forced to download + public async void RefreshImages_NonStubItemPopulatedProviderRemote_DownloadsIfNecessary(ImageType imageType, int initialImageCount, bool fullRefresh) { - // Has to exist for querying DateModified time on file, results stored but not checked so not populating - BaseItem.FileSystem ??= Mock.Of(); + var targetImageCount = 1; // Set path and media source manager so images will be downloaded (EnableImageStub will return false) - var item = new MovieWithScreenshots - { - Path = "non-empty path" - }; + var item = GetItemWithImages(imageType, initialImageCount, false); + item.Path = "non-empty path"; BaseItem.MediaSourceManager = Mock.Of(); - var libraryOptions = GetLibraryOptions(item, imageType, imageCount); + // seek 2 so it won't short-circuit out of downloading when populated + var libraryOptions = GetLibraryOptions(item, imageType, 2); + var content = "Content"; var remoteProvider = new Mock(MockBehavior.Strict); remoteProvider.Setup(rp => rp.Name).Returns("MockRemoteProvider"); remoteProvider.Setup(rp => rp.GetSupportedImages(item)) @@ -433,13 +435,19 @@ namespace Jellyfin.Providers.Tests.Manager { ReasonPhrase = url, StatusCode = HttpStatusCode.OK, - Content = new StringContent("Content", Encoding.UTF8, "image/jpeg") + Content = new StringContent(content, Encoding.UTF8, "image/jpeg") }); - var refreshOptions = new ImageRefreshOptions(null); + var refreshOptions = fullRefresh + ? new ImageRefreshOptions(null) + { + ImageRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllImages = true + } + : new ImageRefreshOptions(null); var remoteInfo = new List(); - for (int i = 0; i < imageCount; i++) + for (int i = 0; i < targetImageCount; i++) { remoteInfo.Add(new RemoteImageInfo { @@ -457,13 +465,14 @@ namespace Jellyfin.Providers.Tests.Manager callbackItem.SetImagePath(callbackType, callbackItem.AllowsMultipleImages(callbackType) ? callbackItem.GetImages(callbackType).Count() : 0, new FileSystemMetadata())) .Returns(Task.CompletedTask); var fileSystem = new Mock(); + // match reported file size to image content length - condition for skipping already downloaded multi-images fileSystem.Setup(fs => fs.GetFileInfo(It.IsAny())) - .Returns(new FileSystemMetadata { Length = 1 }); + .Returns(new FileSystemMetadata { Length = content.Length }); var itemImageProvider = GetItemImageProvider(providerManager.Object, fileSystem); var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List { remoteProvider.Object }, refreshOptions, CancellationToken.None); - Assert.True(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate)); - Assert.Equal(imageCount, item.GetImages(imageType).Count()); + Assert.Equal(initialImageCount == 0 || fullRefresh, result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate)); + Assert.Equal(targetImageCount, item.GetImages(imageType).Count()); } [Theory] -- cgit v1.2.3