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

Feat : add cover display option #2643

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Feat : add cover display option #2643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ab0ea0
Add CoverDisplayOption to 'SeriesMetadata' class
charles7668 Jan 22, 2024
484ae40
GET API "api/image/series-cover" support CoverDisplayOption
charles7668 Jan 22, 2024
9fa7c5b
Add API "api/metadata/cover-display-options" to get available option …
charles7668 Jan 22, 2024
af55bd4
POST API "api/series/metadata" update CoverDisplayOption
charles7668 Jan 22, 2024
b4d08b4
Add a feature to change how the series cover is displayed
charles7668 Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions API/Controllers/ImageController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
Expand Down Expand Up @@ -101,10 +101,38 @@ public async Task<ActionResult> GetSeriesCoverImage(int seriesId, string apiKey)
{
var userId = await _unitOfWork.UserRepository.GetUserIdByApiKeyAsync(apiKey);
if (userId == 0) return BadRequest();
var path = Path.Join(_directoryService.CoverImageDirectory, await _unitOfWork.SeriesRepository.GetSeriesCoverImageAsync(seriesId));
if (string.IsNullOrEmpty(path) || !_directoryService.FileSystem.File.Exists(path)) return BadRequest(await _localizationService.Translate(userId, "no-cover-image"));
var format = _directoryService.FileSystem.Path.GetExtension(path);
var seriesMetadata = await _unitOfWork.SeriesRepository.GetSeriesMetadata(seriesId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use a dedicated repo method to get the CoverImageOption for a given series. No need to load the whole SeriesMetadata in for one field.

if (seriesMetadata == null) return BadRequest();
string path;
switch (seriesMetadata.CoverDisplayOption)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be hidden in an extension or method.

{
case CoverDisplayOption.Random:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of supporting this. I'd need to see a use case to adding it in. I would remove for now.

var rand = new Random(Guid.NewGuid().GetHashCode());
var volumeIds = (await _unitOfWork.VolumeRepository.GetVolumes(seriesId)).Select(v => v.Id).ToList();
if (volumeIds.Count < 1)
return BadRequest(await _localizationService.Translate(userId, "no-cover-image"));
var randIndex = rand.Next(0, volumeIds.Count);
path = Path.Join(_directoryService.CoverImageDirectory,
await _unitOfWork.VolumeRepository.GetVolumeCoverImageAsync(volumeIds[randIndex]));
break;
case CoverDisplayOption.LatestVolume:
var volumes = (await _unitOfWork.VolumeRepository.GetVolumes(seriesId)).ToList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need dedicated repository methods for this to reduce the amount of memory you are using during cover generation. No need to load the whole Volume object.

if (volumes.Count < 1)
return BadRequest(await _localizationService.Translate(userId, "no-cover-image"));
var latestVolumeNumber = volumes.Max(x => x.Number);
var latestId = volumes.First(v => v.Number == latestVolumeNumber).Id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need latestVolumeNumber? You can just do:
var latestId = volumes.OrderByDescending(v => v.Number).First().Id;

path = Path.Join(_directoryService.CoverImageDirectory,
await _unitOfWork.VolumeRepository.GetVolumeCoverImageAsync(latestId));
break;
default:
path = Path.Join(_directoryService.CoverImageDirectory,
await _unitOfWork.SeriesRepository.GetSeriesCoverImageAsync(seriesId));
break;
}

if (string.IsNullOrEmpty(path) || !_directoryService.FileSystem.File.Exists(path))
return BadRequest(await _localizationService.Translate(userId, "no-cover-image"));
var format = _directoryService.FileSystem.Path.GetExtension(path);
Response.AddCacheHeader(path);

return PhysicalFile(path, MimeTypeMap.GetMimeType(format), _directoryService.FileSystem.Path.GetFileName(path));
Expand Down
13 changes: 12 additions & 1 deletion API/Controllers/MetadataController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
Expand Down Expand Up @@ -198,4 +198,15 @@ public async Task<ActionResult<SeriesDetailPlusDto>> GetKavitaPlusSeriesDetailDa
return Ok(await metadataService.GetSeriesDetail(User.GetUserId(), seriesId));

}

/// <summary>
/// Get cover display options
/// </summary>
/// <returns></returns>
[HttpGet("cover-display-options")]
public ActionResult<IList<string>> GetCoverDisplayOptions()
{
var optionStrings = Enum.GetNames(typeof(CoverDisplayOption));
return optionStrings.ToList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can combine this into one return. No need to make it a List, IEnumerable works fine.

}
}
6 changes: 5 additions & 1 deletion API/DTOs/SeriesMetadataDto.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Generic;
using System.Collections.Generic;
using API.DTOs.CollectionTags;
using API.DTOs.Metadata;
using API.Entities.Enums;
Expand Down Expand Up @@ -62,6 +62,10 @@ public class SeriesMetadataDto
/// A comma-separated list of Urls
/// </summary>
public string WebLinks { get; set; }
/// <summary>
/// Series cover display option
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't helpful. Explain a bit more for the users using the API

/// </summary>
public CoverDisplayOption CoverDisplayOption { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this belongs on the Series.

It is likely this is used for Magazines, which are likely to be in their own library, rather than on a series. Plus if a user has a lot of magazines, it is quite painful to update this for all those series. Maybe @lyingloverr, @MortenCB, or @raub21 can comment on their opinion.


public bool LanguageLocked { get; set; }
public bool SummaryLocked { get; set; }
Expand Down
5 changes: 5 additions & 0 deletions API/Data/DataContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ protected override void OnModelCreating(ModelBuilder builder)
builder.Entity<AppUserSideNavStream>()
.HasIndex(e => e.Visible)
.IsUnique(false);

builder.Entity<SeriesMetadata>()
.Property(metadata => metadata.CoverDisplayOption)
.HasDefaultValue(CoverDisplayOption.Default)
.HasConversion<string>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the enum. Enums are stored as enums, not as Text.

}


Expand Down
Loading