Feat : add cover display option #2643

charles7668 · 2024-01-22T12:13:56Z

Added

Added: Add a feature to change how the series cover is displayed ([Kavita] Different display settings for cover art #2629)

Note : this PR will update Database (config/kavita.db)

Currently, there are three display options available:

Default: Shows the default display (usually Volume 1).
Random: Displays a random volume cover.
LatestVolume: Shows the cover of the latest volume.

Random:

LatestVolume:

This option determines how the series cover will be displayed.

…for cover display

Currently, there are three display options available: 1. Default: Shows the default display (usually Volume 1). 2. Random: Displays a random volume cover. 3. LatestVolume: Shows the cover of the latest volume.

majora2007

I'm not 100% sold on including this into Kavita. I need feedback from users on how this should work as well.

I don't want to dissuade you from contributing, but not everything in discussions or that you raise will be merged. Feel free to join discord and ask me before you spend time working on something.

majora2007 · 2024-01-22T12:49:29Z

UI/Web/src/app/_models/metadata/series-metadata.ts

@@ -30,6 +30,7 @@ export interface SeriesMetadata {
    language: string;
    publicationStatus: PublicationStatus;
    webLinks: string;
+    coverDisplayOption: number;


This should be an enum.

majora2007 · 2024-01-22T12:49:40Z

UI/Web/src/app/_services/metadata.service.ts

@@ -96,6 +96,10 @@ export class MetadataService {
    return this.httpClient.get<Array<Person>>(this.baseUrl + 'metadata/people-by-role?role=' + role);
  }

+  getCoverDisplayOptions(){
+    return this.httpClient.get<Array<string>>(this.baseUrl + 'metadata/cover-display-options');


Return an Array of enums

majora2007 · 2024-01-22T12:49:59Z

UI/Web/src/app/cards/_modals/edit-series-modal/edit-series-modal.component.html

+              <form [formGroup]="coverOptionForm" style="margin-bottom: 15px">
+                <label for="cover-display-option-port" class="form-label">Cover Display Options</label>
+                <select id="cover-display-option-port" class="form-select" formControlName="coverDisplayOptionSelected">
+                  <option *ngFor="let option of coverDisplayOptions" [ngValue]="option">{{ option | titlecase }}</option>


Use a pipe to localize the enum value -> string

majora2007 · 2024-01-22T12:50:25Z

UI/Web/src/app/cards/_modals/edit-series-modal/edit-series-modal.component.html

@@ -358,6 +358,12 @@ <h4 class="modal-title">
          <li [ngbNavItem]="tabs[TabID.CoverImage]">
            <a ngbNavLink>{{t(tabs[TabID.CoverImage])}}</a>
            <ng-template ngbNavContent>
+              <form [formGroup]="coverOptionForm" style="margin-bottom: 15px">
+                <label for="cover-display-option-port" class="form-label">Cover Display Options</label>


You need to use localization on any new strings. You need a tooltip to explain how this works.

majora2007 · 2024-01-22T12:50:53Z

UI/Web/src/app/cards/_modals/edit-series-modal/edit-series-modal.component.ts

@@ -157,6 +157,9 @@ export class EditSeriesModalComponent implements OnInit {
  selectedCover: string = '';
  coverImageReset = false;

+  coverDisplayOptions : Array<string> = [];
+  coverOptionForm! : FormGroup;


Can this not go in the existing Form?

majora2007 · 2024-01-22T12:57:33Z

API/Controllers/ImageController.cs

-        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);


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.

majora2007 · 2024-01-22T12:58:25Z

API/Entities/Enums/CoverDisplayOption.cs

+
+namespace API.Entities.Enums;
+
+public enum CoverDisplayOption


Option doesn't leave a good impression on an enum.

CoverGenerationSelector works a bit better. I'm open to opinion on a better name.

majora2007 · 2024-01-22T13:00:05Z

API/Controllers/ImageController.cs

+                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;


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

majora2007 · 2024-01-22T13:00:49Z

API/Controllers/ImageController.cs

+        string path;
+        switch (seriesMetadata.CoverDisplayOption)
+        {
+            case CoverDisplayOption.Random:


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.

majora2007 · 2024-01-22T13:03:22Z

API/DTOs/SeriesMetadataDto.cs

+    /// <summary>
+    /// Series cover display option
+    /// </summary>
+    public CoverDisplayOption CoverDisplayOption { get; set; }


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.

charles7668 · 2024-01-22T14:44:34Z

I'm not 100% sold on including this into Kavita. I need feedback from users on how this should work as well.

I don't want to dissuade you from contributing, but not everything in discussions or that you raise will be merged. Feel free to join discord and ask me before you spend time working on something.

Sorry, maybe I missed some of the rules for feature requests.

Should I close this PR before the discussion(#2629) is concluded?

majora2007 · 2024-01-22T15:00:03Z

I'm not 100% sold on including this into Kavita. I need feedback from users on how this should work as well.
I don't want to dissuade you from contributing, but not everything in discussions or that you raise will be merged. Feel free to join discord and ask me before you spend time working on something.

Sorry, maybe I missed some of the rules for feature requests.

Should I close this PR before the discussion(#2629) is concluded?

The way the feature request works is that users open discussions, I gather feedback and upvotes. I label the discussion as accepted then move to brainstorming on the feature in the discord with Kavita+ members to flesh out the feature further and integrate it well in Kavita before going to implementation.

I appreciate all the PRs, but I don't want to setup the expectation just because someone requested it and someone else coded it that it will be pulled in. It needs to fit well with Kavita and the maintenance needs to be worthwhile.

Again, if you have discord, reach out to me in the Kavita server. It's much easier to just ping me when you want to work on something before doing it and me rejecting it.

As for this PR, more information is needed. I can see this feature as being beneficial to Magazine users, but personally I don't see a huge need to add the complexity into Kavita. I'm waiting to see how much feedback and upvotes we get on the discussion before bringing into consideration.

charles7668 added 5 commits January 22, 2024 18:00

Add CoverDisplayOption to 'SeriesMetadata' class

9ab0ea0

This option determines how the series cover will be displayed.

GET API "api/image/series-cover" support CoverDisplayOption

484ae40

Add API "api/metadata/cover-display-options" to get available option …

9fa7c5b

…for cover display

POST API "api/series/metadata" update CoverDisplayOption

af55bd4

Add a feature to change how the series cover is displayed

b4d08b4

Currently, there are three display options available: 1. Default: Shows the default display (usually Volume 1). 2. Random: Displays a random volume cover. 3. LatestVolume: Shows the cover of the latest volume.

majora2007 requested changes Jan 22, 2024

View reviewed changes

majora2007 added enhancement New feature or request db-migration This story needs a DB Migration labels Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat : add cover display option #2643

Feat : add cover display option #2643

charles7668 commented Jan 22, 2024

majora2007 left a comment

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

majora2007 Jan 22, 2024

charles7668 commented Jan 22, 2024

majora2007 commented Jan 22, 2024

Feat : add cover display option #2643

Are you sure you want to change the base?

Feat : add cover display option #2643

Conversation

charles7668 commented Jan 22, 2024

Added

majora2007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles7668 commented Jan 22, 2024

majora2007 commented Jan 22, 2024