search

xgi / houdoku

Manga reader and library manager for the desktop
https://houdoku.org
MIT License
789 stars 41 forks source link

Account for decimals in unread chapter count #335

Closed crxssed7 closed 9 months ago

crxssed7 commented 9 months ago

Fixes #298

I was hesitating on whether I should open a PR for this as I think the behaviour is intentional (however I don't really agree with the current implementation).

I tracked it down to the markChapter method which at first seemed like it was over engineering something really simple, but I think the original dev who wrote this intended for chapters with decimal points to be consolidated as one chapter (e.g 41.1, 41.2, 41.3, 41.4 is just chapter 41). I think this is a pretty odd way of looking at it and is very confusing at first glance - it took me a second to figure out why this was implemented the way that it is. As it relies heavily on chapter numbers being present, it means that any chapter with no chapter number is completely ignored (as seen in the Moon Knight screenshot in the original issue).

Feel free to close this PR if you think the current implementation is correct.

xgi commented 9 months ago

So the current logic you see is mainly for 3 reasons:

This is why I chose to work with chapterNumbers instead of chapter objects. Of course, it introduces 2 other issues:

I think it's worth addressing those points, but it's not trivial to consider all of these (and wouldn't be as simple as either of our implementations). Let me know if you disagree!

crxssed7 commented 9 months ago

So the current logic you see is mainly for 3 reasons:

* The `chapterList` passed to `getNumberUnreadChapters` can contain chapters from multiple languages (depending on the user's settings). This is the main reason for the "highestRead" logic -- sometimes only one language will have recent chapters, and oftentimes a chapter will be available in both languages. I think showing the user a "2" if there's one new chapter available in 2 languages is almost certainly wrong.

* The `chapterList` can also contain multiple chapter objects with the same number and language (e.g. from different scanlation groups). Showing "2" in this case would probably also be unexpected.

* Dealing with unmarked chapters. If there are chapters 1-100 available, and the user has only read the 90th, I would expect to show "10". The client makes it easy to mark previous chapters as read, but I'm not sure many people do that, especially when adding a series they started elsewhere. I think it makes sense to only consider chapters above the highest read one.

This is why I chose to work with chapterNumbers instead of chapter objects. Of course, it introduces 2 other issues:

* Dealing with gaps. If chapter 1 is read and the only other chapter is 10, currently we would show the user "9". That's probably wrong.

* Rounding. If chapter 40 is read and there are also 40.1, 40.2, 40.3, we currently show "1". That's probably wrong too.

I think it's worth addressing those points, but it's not trivial to consider all of these (and wouldn't be as simple as either of our implementations). Let me know if you disagree!

That makes a lot more sense now with context 👍🏾

I agree with all your considerations - sounds like a tricky situation whichever way you look at it. When I get a chance (most likely this weekend) I'll have a go at updating the implementation.

crxssed7 commented 9 months ago

This should be ready for a re-review. I've tried my best to follow all the considerations but with there being so many it isn't perfect. The code has also become a lot more complicated.

Trade paperback comics on ReadComicOnline will show the unread count inverted (basically the read count) which is completely wrong. This is because there are no chapter numbers present so we can't exactly sort it to get the absolute chapter number. I'm planning on opening a PR on Tiyo that'll hopefully address this.