Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Tests, Fix metadata refresh deletes backgrounds #6752

Merged
merged 7 commits into from
Nov 3, 2021
Merged

Add Tests, Fix metadata refresh deletes backgrounds #6752

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
080b02c
Add comments, minor cleanup, add tests
1337joe Oct 31, 2021
0fbd8d8
Validate multi-images, lazy-delete bg on refresh
1337joe Oct 31, 2021
b478b11
Refactor to validate all images up front
1337joe Nov 1, 2021
6881cf6
Merge remote-tracking branch 'origin/master' into fix-metadata-refres…
1337joe Nov 1, 2021
7da6bd9
Fix edge case in multi-image replacing
1337joe Nov 1, 2021
7fcf012
Change RemoveImages to array, improve download test
1337joe Nov 2, 2021
149c77d
Remove commented theory data, merge tests
1337joe Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 2 additions & 17 deletions MediaBrowser.Controller/Entities/BaseItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2495,11 +2495,11 @@ public IEnumerable<ItemImageInfo> GetImages(ImageType imageType)
}

/// <summary>
/// Adds the images.
/// Adds the images, updating metadata if they already are part of this item.
/// </summary>
/// <param name="imageType">Type of the image.</param>
/// <param name="images">The images.</param>
/// <returns><c>true</c> if XXXX, <c>false</c> otherwise.</returns>
/// <returns><c>true</c> if images were added or updated, <c>false</c> otherwise.</returns>
/// <exception cref="ArgumentException">Cannot call AddImages with chapter images.</exception>
public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
{
Expand All @@ -2512,7 +2512,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
.ToList();

var newImageList = new List<FileSystemMetadata>();
var imageAdded = false;
var imageUpdated = false;

foreach (var newImage in images)
Expand All @@ -2528,7 +2527,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
if (existing == null)
{
newImageList.Add(newImage);
imageAdded = true;
}
else
{
Expand All @@ -2549,19 +2547,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
}
}

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();
Expand Down
131 changes: 74 additions & 57 deletions MediaBrowser.Providers/Manager/ItemImageProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#nullable disable

#pragma warning disable CA1002, CS1591

using System;
using System.Collections.Generic;
using System.IO;
Expand All @@ -25,6 +23,9 @@

namespace MediaBrowser.Providers.Manager
{
/// <summary>
/// Utilities for managing images attached to items.
/// </summary>
public class ItemImageProvider
{
private readonly ILogger _logger;
Expand All @@ -47,13 +48,26 @@ public class ItemImageProvider
ImageType.Thumb
};

/// <summary>
/// Initializes a new instance of the <see cref="ItemImageProvider"/> class.
/// </summary>
/// <param name="logger">The logger.</param>
/// <param name="providerManager">The provider manager for interacting with provider image references.</param>
/// <param name="fileSystem">The filesystem.</param>
public ItemImageProvider(ILogger logger, IProviderManager providerManager, IFileSystem fileSystem)
{
_logger = logger;
_providerManager = providerManager;
_fileSystem = fileSystem;
}

/// <summary>
/// Verifies existing images have valid paths and adds any new local images provided.
/// </summary>
/// <param name="item">The <see cref="BaseItem"/> to validate images for.</param>
/// <param name="providers">The providers to use, must include <see cref="ILocalImageProvider"/>(s) for local scanning.</param>
/// <param name="directoryService">The directory service for <see cref="ILocalImageProvider"/>s to use.</param>
/// <returns><c>true</c> if changes were made to the item; otherwise <c>false</c>.</returns>
public bool ValidateImages(BaseItem item, IEnumerable<IImageProvider> providers, IDirectoryService directoryService)
{
var hasChanges = false;
Expand All @@ -73,31 +87,42 @@ public bool ValidateImages(BaseItem item, IEnumerable<IImageProvider> providers,
return hasChanges;
}

/// <summary>
/// Refreshes from the providers according to the given options.
/// </summary>
/// <param name="item">The <see cref="BaseItem"/> to gather images for.</param>
/// <param name="libraryOptions">The library options.</param>
/// <param name="providers">The providers to query for images.</param>
/// <param name="refreshOptions">The refresh options.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The refresh result.</returns>
public async Task<RefreshResult> RefreshImages(
BaseItem item,
LibraryOptions libraryOptions,
List<IImageProvider> providers,
ImageRefreshOptions refreshOptions,
CancellationToken cancellationToken)
{
List<ItemImageInfo> oldBackdropImages = new List<ItemImageInfo>();
1337joe marked this conversation as resolved.
Show resolved Hide resolved
if (refreshOptions.IsReplacingImage(ImageType.Backdrop))
{
ClearImages(item, ImageType.Backdrop);
oldBackdropImages = item.GetImages(ImageType.Backdrop).ToList();
}

List<ItemImageInfo> oldScreenshotImages = new List<ItemImageInfo>();
if (refreshOptions.IsReplacingImage(ImageType.Screenshot))
{
ClearImages(item, ImageType.Screenshot);
oldScreenshotImages = item.GetImages(ImageType.Screenshot).ToList();
}

var result = new RefreshResult { UpdateType = ItemUpdateType.None };

var typeName = item.GetType().Name;
var typeOptions = libraryOptions.GetTypeOptions(typeName) ?? new TypeOptions { Type = typeName };

// In order to avoid duplicates, only download these if there are none already
var backdropLimit = typeOptions.GetLimit(ImageType.Backdrop);
var screenshotLimit = typeOptions.GetLimit(ImageType.Screenshot);
// 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 downloadedImages = new List<ImageType>();

foreach (var provider in providers)
Expand All @@ -114,11 +139,22 @@ public bool ValidateImages(BaseItem item, IEnumerable<IImageProvider> providers,
}
}

// only delete existing multi-images if new ones were added
if (oldBackdropImages.Count > 0 && oldBackdropImages.Count < item.GetImages(ImageType.Backdrop).Count())
{
PruneImages(item, oldBackdropImages);
1337joe marked this conversation as resolved.
Show resolved Hide resolved
}

if (oldScreenshotImages.Count > 0 && oldScreenshotImages.Count < item.GetImages(ImageType.Screenshot).Count())
{
PruneImages(item, oldScreenshotImages);
}

return result;
}

/// <summary>
/// Refreshes from provider.
/// Refreshes from a dynamic provider.
/// </summary>
private async Task RefreshFromProvider(
BaseItem item,
Expand Down Expand Up @@ -153,13 +189,14 @@ public bool ValidateImages(BaseItem item, IEnumerable<IImageProvider> providers,
if (response.Protocol == MediaProtocol.Http)
{
_logger.LogDebug("Setting image url into item {0}", item.Id);
var index = item.AllowsMultipleImages(imageType) ? item.GetImages(imageType).Count() : 0;
item.SetImage(
new ItemImageInfo
{
Path = response.Path,
Type = imageType
},
0);
index);
}
else
{
Expand Down Expand Up @@ -234,7 +271,7 @@ private bool ContainsImages(BaseItem item, List<ImageType> images, TypeOptions s
}

/// <summary>
/// Refreshes from provider.
/// Refreshes from a remote provider.
/// </summary>
/// <param name="item">The item.</param>
/// <param name="provider">The provider.</param>
Expand Down Expand Up @@ -305,12 +342,12 @@ private bool ContainsImages(BaseItem item, List<ImageType> images, TypeOptions s
}

minWidth = savedOptions.GetMinWidth(ImageType.Backdrop);
await DownloadBackdrops(item, ImageType.Backdrop, backdropLimit, provider, result, list, minWidth, cancellationToken).ConfigureAwait(false);
await DownloadMultiImages(item, ImageType.Backdrop, backdropLimit, provider, result, list, minWidth, cancellationToken).ConfigureAwait(false);

if (item is IHasScreenshots hasScreenshots)
if (item is IHasScreenshots)
{
minWidth = savedOptions.GetMinWidth(ImageType.Screenshot);
await DownloadBackdrops(item, ImageType.Screenshot, screenshotLimit, provider, result, list, minWidth, cancellationToken).ConfigureAwait(false);
await DownloadMultiImages(item, ImageType.Screenshot, screenshotLimit, provider, result, list, minWidth, cancellationToken).ConfigureAwait(false);
}
}
catch (OperationCanceledException)
Expand All @@ -329,40 +366,36 @@ private bool IsEnabled(TypeOptions options, ImageType type)
return options.IsEnabled(type);
}

private void ClearImages(BaseItem item, ImageType type)
private void PruneImages(BaseItem item, List<ItemImageInfo> images)
{
var deleted = false;
var deletedImages = new List<ItemImageInfo>();

foreach (var image in item.GetImages(type))
for (var i = 0; i < images.Count; i++)
{
if (!image.IsLocalFile)
{
deletedImages.Add(image);
continue;
}
var image = images[i];

try
{
_fileSystem.DeleteFile(image.Path);
deleted = true;
}
catch (FileNotFoundException)
if (image.IsLocalFile)
{
try
{
_fileSystem.DeleteFile(image.Path);
}
catch (FileNotFoundException)
{
}
}
}

item.RemoveImages(deletedImages);

if (deleted)
{
item.ValidateImages(new DirectoryService(_fileSystem));
}
item.RemoveImages(images);
}

/// <summary>
/// Merges a list of images into the provided item, validating existing images and replacing them or adding new images as necessary.
/// </summary>
/// <param name="item">The <see cref="BaseItem"/> to modify.</param>
/// <param name="images">The new images to place in <c>item</c>.</param>
/// <returns><c>true</c> if changes were made to the item; otherwise <c>false</c>.</returns>
public bool MergeImages(BaseItem item, IReadOnlyList<LocalImageInfo> images)
{
var changed = false;
var changed = item.ValidateImages(new DirectoryService(_fileSystem));

for (var i = 0; i < _singularImages.Length; i++)
{
Expand Down Expand Up @@ -398,32 +431,16 @@ public bool MergeImages(BaseItem item, IReadOnlyList<LocalImageInfo> images)
currentImage.DateModified = newDateModified;
}
}
else
{
var existing = item.GetImageInfo(type, 0);
if (existing != null)
{
if (existing.IsLocalFile && !File.Exists(existing.Path))
{
item.RemoveImage(existing);
changed = true;
}
}
}
}

if (UpdateMultiImages(item, images, ImageType.Backdrop))
{
changed = true;
}

var hasScreenshots = item as IHasScreenshots;
if (hasScreenshots != null)
if (item is IHasScreenshots && UpdateMultiImages(item, images, ImageType.Screenshot))
{
if (UpdateMultiImages(item, images, ImageType.Screenshot))
{
changed = true;
}
changed = true;
}

return changed;
Expand Down Expand Up @@ -536,7 +553,7 @@ private bool EnableImageStub(BaseItem item)
return true;
}

if (item is IItemByName && item is not MusicArtist)
if (item is IItemByName and not MusicArtist)
{
var hasDualAccess = item as IHasDualAccess;
if (hasDualAccess == null || hasDualAccess.IsAccessedByName)
Expand Down Expand Up @@ -569,7 +586,7 @@ private void SaveImageStub(BaseItem item, ImageType imageType, IEnumerable<strin
newIndex);
}

private async Task DownloadBackdrops(BaseItem item, ImageType imageType, int limit, IRemoteImageProvider provider, RefreshResult result, IEnumerable<RemoteImageInfo> images, int minWidth, CancellationToken cancellationToken)
private async Task DownloadMultiImages(BaseItem item, ImageType imageType, int limit, IRemoteImageProvider provider, RefreshResult result, IEnumerable<RemoteImageInfo> images, int minWidth, CancellationToken cancellationToken)
{
foreach (var image in images.Where(i => i.Type == imageType))
{
Expand Down Expand Up @@ -609,7 +626,7 @@ private async Task DownloadBackdrops(BaseItem item, ImageType imageType, int lim
break;
}

// If there's already an image of the same size, skip it
// If there's already an image of the same file size, skip it
if (response.Content.Headers.ContentLength.HasValue)
{
try
Expand Down
6 changes: 6 additions & 0 deletions tests/Jellyfin.Providers.Tests/Jellyfin.Providers.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
<CodeAnalysisRuleSet>../jellyfin-tests.ruleset</CodeAnalysisRuleSet>
</PropertyGroup>

<ItemGroup>
<None Include="Test Data\**\*.*">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.0.0" />
<PackageReference Include="Moq" Version="4.16.1" />
Expand Down