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.

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

Add Chapters for Galleries #3289

Merged
merged 32 commits into from
Mar 16, 2023
Merged

Add Chapters for Galleries #3289

merged 32 commits into from
Mar 16, 2023

Conversation

yoshnopa
Copy link
Contributor

This request adds chapters to galleries.

These can be understood as the equivalent to markers for scenes, but instead of a timestamp, there is a page number associated. The identification is done via a freely chosen title instead of a primary tag, as this is more in the spirit of chapterization. Also, no Tags can be associated with a chapter, although not entirely useless, I found the possibility to tag a certain range of pages in a gallery to be too specific.

The Implementation is done with a new Tab in Galleries that allows the creation of Chapters

image
image

Clicking on a chapter will open the gallery on the associated page.
Also, the lightbox is enhanced to provide access to the gallery. In the top left corner there is a new menu item if this lightbox is coming from a gallery (details page or wall view of the main page).
image
Folded up it presents the Chapters, which on click will transport the lightbox to the associated page as well.
image

There is also a new filter for galleries called has chapters, with obvious functionality.

loosely related to #1659 #999 (as this feature is specifically useful for hentai/comics).

Disclaimer: This is not only my first serious commit here but also the first time I worked with react, go or graphql, so please forgive if my coding style is not quite in the vein of those languages. I am of course open to suggestions on that, as well as to the general look of this implementation, which is pretty minimal (something I personally prefer, but is also due to my lack of experience with the Codebase) at this point.

@cj12312021 cj12312021 added the feature Pull requests that add a new feature label Dec 23, 2022
@WithoutPants
Copy link
Collaborator

Using page as the indexing point for chapters is not really ideal. Page sizes are freely selectable, so the page field does not have a strong meaning here. It would be better to use an index/order number to reference a chapter - ie Chapter 3 points to image #43. That however is predicated on having images numbers in the gallery-image association table. I think that feature is a prerequisite for this one.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Jan 3, 2023

I am not sure if I understand correctly. For some explanation on how this works:

The Lightbox has a range of pages that is displayed/preloaded, defaulting (hardcoded) to 40 when using the preview in the gallery tab without going into the galleries detail page. This function is also used when clicking on a chapter in the chapters tab on the details page, so the behavior is identical here. I implemented a function to jump to the correct range and to the correct index when choosing a chapter (so for example 86 is resolved to page 2 index 6).
HOWEVER, if I understood your problem correctly, there is indeed flaw in that: When going to the galleries details page and choosing a custom number of images displayed, this number will be used in the Lightbox, making my function with the hardcoded 40 in it faulty by design.
I fixed this behavior in the last commit by using the filter data of the wall to provide the range to the lightbox, making the method flexible.

The page number given in the chapters tab on creation/editing as well as the one displayed is always referring to the absolute number in the gallery. So you should not give it the number 3 when you are referring to the third page on the second imagewall page, but simply 43. Where you referring to that? In that case, I found this to be quite selfexplaining to the user, when I refer to a book or a gallery, I would count the pages/images from front to back and not in the arbitrary way they are loaded on the current view. Maybe I am wrong on that and better labeling is needed here.

@ghost
Copy link

ghost commented Jan 3, 2023

I think the word "page" is confusing in the chapter edit view. Maybe call it "image number" or "index".
This also assumes a certain sorting for the gallery. What happens if I change the sorting from title to random for instance ?

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Jan 3, 2023

Thank you for explaining.
I can rename that, no problem. I think Index sounds about right.

About the sorting, yes, this confuses the chapterization. However, I am not sure whether this is a useful scenario to consider. For one, this is only the case when you are in the galleries detail page and start sorting them differently and use the chapter menu in the lightbox itself. The sorting is always default and immutable for the gallery view, which is used when clicking a chapter in the details page or entering the lightbox from the general gallery view. It also needs multiple clicks of interaction every time, as you cannot save a filter in the gallery details view, so the user should be aware why this looks the way it looks.
Apart from that, I can't think of a scenario where someone would use this feature just to look through the gallery randomly. The name chapter was used for a reason, and for getting to a singular image quickly the image tab and/or the tags seem to be more in line with what you want. Even if there was a table linking the images to the chapter not by index but by image ID, changing the order would mean that there is no useful after and before, which again kind of takes the use away for a chapter and makes it more of a link to a singular image (which is more in line with tagging or naming). If there are several ways to see the same gallery (basically different orders, not quite sure how that could happen but technically possible), creating a second gallery with the same images seems to be more in line with that in my opinion. Right now, you have the possibility to access the "sorted" gallery lightbox at any time by choosing a chapter in the chapters tab, while still being able to have the differently arranged gallery lightbox available below that.
What I could offer is deactivating the chapters entirely if the lightbox was called with a sorting order that is NOT by path, so no confusion through false options arises.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Jan 3, 2023

Did both those things in the new commit.
Right now, chapters will only be shown if the sorting is by path and ascending, which are the default settings. Changing those will simply not display the chapter menu in the lightbox.

@WithoutPants WithoutPants added this to the Version 0.20.0 milestone Feb 16, 2023
@WithoutPants
Copy link
Collaborator

My apologies on the delay in getting back to this PR.

To expand on my previous comments, this goes beyond just labelling, it is an interface design issue as well. Leaving aside the UI, in the graphql schema, GalleryChapter has the page_number field, which from the client's point of view is highly ambiguous. It doesn't know the page size or the sorting order for which this page number refers to.

Having spent some more time thinking about this, my current thinking is that GalleryChapter should be indexing on a particular image. This gives us more flexibility and specificity for a GalleryChapter - we can still make a page indexed chapter by selecting the first image in that page as the indexed image, but we don't have to.

There is still some ambiguity involved in this in that the external client doesn't know the sort order, but it is less of an issue - we can assume a default sorting order for the time being, with a view to add gallery ordering at a later date.

This changes the Chapter UI creation - we can no longer just enter in a page, but instead must select an image to mark as the chapter index.

I realise that this is a significant change to the PR, and I am open to workshopping this design further, and assisting with the rework. I'm not fully on board the term chapter, and I wonder if there is a more generic term for it - such as for example bookmark?

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Mar 7, 2023

I know this is a more complex Pull, so no worries, thank you for doing this :)

I'm afraid there is still a misunderstanding about what the page_number value represents, probably because of the term page.
This is NOT referring to page as in homepage or pagination, there is no such thing as pagesize associated with that number. Maybe it is more understandable to call it image_number or index_number, they could be used synonymously.
This number gives the index of the image in the gallery (assuming default sorting), so in a gallery with 90 images, this value can have a value from 1 to 90, regardless of the fact if there are 40 images per page or 1000. They are associated with the gallery as a whole, not with a certain page of a gallery.
The conversion in the lightbox the account for the pagination, the "jumping algorithm", is already implemented in the frontend and fully functional. This is what I tried to explain in my first response. All the information is already there.
Maybe compile the code for yourself and try it if that helps, I think that part is really intuitive.

However, it is correct that the sorting is not part of the chapter object right now, something that could maybe be fixed by associating it with the according image, but it would have to be resorted after that anyway to have the correct follow-up images (again, I don't want this to simply mark favorites, but a certain range of images, the same way a marker does for a scene or a chapter splits a comic/book), which would make the association again obsolete (if the frontend first changes the order of the images, the index can again be used with no reference to the image. As this is necessary anyway, there is no advantage in associating the actual image). That means that my understanding for fixing this would be to simply add a value sorting to the chapter, if this would be expanded.

The only advantage of associating the image I can imagine is to have a performant or intuitive access to a "chapter thumbnail" of the different chapters, or something like that. Although even that can be done with one request by getting all the gallery information, which include the chapter numbers and the images. In our current frontend, I think something like an image thumbnail for the chapters could be realized without an additional graphql request.
Beside of this, it can also be a choice to do this for a certain design philosophy. I won't argue with that if that's what you are trying to hold up here.

About the naming, just quickly my thoughts on the term bookmark: Bookmarks seem to me on internet pages synonymous with a favorite tag, it is meant to mark a singular point of interest. A chapter however is a way to split up a longer body of content into smaller, coherent pieces, so they apply to a certain range. Also, the differenciation with the scene marker is clearer without being too far fetched I believe.
But if you think that bookmark is more fitting I will change it, that's not something I will argue with.

@WithoutPants
Copy link
Collaborator

Ok, I've finally got a grasp of this. I assume that the term page was used with the assumption that a gallery consists of "pages" instead of "images". My apologies for the confusion.

With that in mind, the names of the interfaces must be changed to eliminate this ambiguity:

  • page_number should be renamed to image_index in the graphql schema
  • likewise, page_number should be renamed to image_index in the database schema, and the models should be adjusted accordingly in the front- and back-end code

In the UI, I think the display of chapters should be adjusted:

  • if there is no title, then the leading - string should be stripped and only show the image index
  • instead of Page x, I think #x would be more suitable

Index in the create/edit chapter panel should be Image Index or Image #.

The image index number should be validated during create/edit in the backend. I was able to create a chapter outside the bounds of the gallery image count, and it broke the display of the gallery lightbox. Similarly, we need to consider the behaviour when adding and removing images from a gallery with chapters. Do we adjust them? How should we handle the case where removing an image causes a chapter index to become invalid?

I noticed that the Chapter list in the lightbox does not show up unless I click on the chapter link in the gallery page. Should it not show up in the Gallery lightbox regardless of how I caused the lightbox to appear?

The chapter list is pretty ugly imo in its current incarnation, and needs a bit better styling in it. It should have a similar look and feel to the options popover.
image

About the naming, just quickly my thoughts on the term bookmark: Bookmarks seem to me on internet pages synonymous with a favorite tag, it is meant to mark a singular point of interest. A chapter however is a way to split up a longer body of content into smaller, coherent pieces, so they apply to a certain range. Also, the differenciation with the scene marker is clearer without being too far fetched I believe.
But if you think that bookmark is more fitting I will change it, that's not something I will argue with.

Fair points. I'm still not absolutely convinced that Chapter is the right term, but I can't think of a better term at this point. :)

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Mar 8, 2023

Alright, I will do that the following days.
Just two Notes on that:

I thought about a check for the index being out of range, but dropped it because it was already a relatively big project. My reasoning was that the scene marker has no such check as well. It will also not adjust its markers if a scene gets rescanned and appears to be smaller or bigger.
I was therefore under the assumption that this software assumed a certain amount of care when managing the library. Although no error is shown, a breaking lightbox because the number is higher than the number of pages is very easy to figure out even for someone not familiar with coding.
As for adding and removing images, I think that there applies the same logic. While adding images to a gallery is more likely than removing if it has chapters (for example an ongoing webcomic), it will be appended at the end most of the time. Any other scenario would require manual intervention anyway, and the fact that if you remove an entire chapter that means that you have to recheck your chapter indexes seems to be reasonable enough to me.
But if you want I can still look into that. I would however consider it the last step here.

The Styling would be better if more coherent, unfortunately I am not much of a frontend developer at all. Some help would be appreciated here (like what classes would be suitable here). Although I'll try, its probably far quicker if I get a little guidance what Element/Styling/Classes to use here.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Mar 8, 2023

And it should turn up under any circumstances, as long as the sorting is default. (ASC). If this is not a case, this is a bug. Maybe you could tell me exactly how that happened? I had no problems there.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Mar 9, 2023

I adjusted the look of the chapter selection in the lightbox to match the options.

The naming in front- and backend is also now fixed (except the term chapter, which, although a bit ambiguous, still is more to my liking).

And before submitting a new chapter a check is run that gives out an errortoast telling you if the index is to high.

There is however no check when adding or removing images to a gallery. I can't think of a way to do it without creating some bad situations, which is why I think this would be the point to leave it to the user to figure out/adjust manually.
If someone feels inclined to do it, one could probably make a pop-up presenting options on what to do when images are deleted, but that seems excessive for a first introduction of this feature I think.
If you delete chapters automatically you might loose information you wanted to keep. If you alter them it might be that you wanted to keep the original chapters and simply delete some blank pages, or that the pages you deleted at different points, so the changing of the chapters gets really complicated if done automatically.
On the other hand, someone modifying a gallery is (hopefully) aware that created chapters might now point to a wrong image, and its probably easiest if he has the original indexes attached to reconstruct everything.
What could be done is remind the user in the pop-up when deleting an image from a gallery that potentially chapters might point to wrong points or become unusable.

Comment on lines 20 to 24
for _, m := range chapters {
if err := DestroyChapter(ctx, m, mqb); err != nil {
return nil, err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be unnecessary if the relationships cascade correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is... unfortunately necessary. Deletion does not work with an existing chapter if this code is not present. The Error is:
destroying galleries: executing `DELETE FROM `galleries` WHERE (`galleries`.`id` IN (MYGALLERYID))` [[]]: FOREIGN KEY constraint failed
I am not sure how this happens. I can imagine what you mean with cascading, but I don't know how it is done in this body of code. If it should be implemented here, something is wrong probably. I need your assistance to find the location where this is realized.
This is equivalent to how it is done for markers when deleting scenes, see:
pkg/scene/delete.go Line 131 following.

@WithoutPants WithoutPants merged commit 7e8f941 into stashapp:develop Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull requests that add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants