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

Implement: Optionally use Data-Saver #29

Merged
merged 6 commits into from
Apr 24, 2023
Merged

Implement: Optionally use Data-Saver #29

merged 6 commits into from
Apr 24, 2023

Conversation

leotaku
Copy link
Owner

@leotaku leotaku commented Apr 9, 2023

This PR implements an option to enable the data-saver functionality, as well as the possibility to fall back on data-saver if downloading the normal images fails. This also represents a fix for issues #6 and #24.

@Colin1224 I would appreciate if you could test these changes and provide feedback. Also, I tried using the GitHub API to create a PR based on your issue, which seems to in the process have replaced the original issue. Sorry about that.

@leotaku leotaku changed the title Optionally use Data-Saver Implement: Optionally use Data-Saver Apr 9, 2023
@Colin1224
Copy link
Contributor Author

Sorry, didn't get a notification for this. Just tested it out and it doesn't seem to be fully working. Going based on my example in #6, running the same command with any --data-saver= option results in the below error.

PS C:\Repos\kojirou> .\kojirou.exe d7037b2a-874a-4360-8a7b-07f2899152fd -C 80 -l en --data-saver=prefer
Title: Mairimashita! Iruma-kun
Author: Nishi Osamu
Groups: Misfits Scans
Chapters: 80
Volume: 10|     | 0 / 1           |panic: runtime error: index out of range [18] with length 18

goroutine 284 [running]:
github.com/leotaku/kojirou/mangadex.convertChapter(0xc0000b2270, 0xc0006b20c0)
        C:/Repos/kojirou/mangadex/convert.go:78 +0x407
github.com/leotaku/kojirou/mangadex.(*Client).FetchPaths(0x0?, {0xd4dab0?, 0xc000558280?}, 0xc0000b2270)
        C:/Repos/kojirou/mangadex/client.go:135 +0x97
github.com/leotaku/kojirou/cmd/formats/download.chaptersToPaths.func1.1()
        C:/Repos/kojirou/cmd/formats/download/root.go:133 +0x9d
golang.org/x/sync/errgroup.(*Group).Go.func1()
        C:/Users/colin/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x64
created by golang.org/x/sync/errgroup.(*Group).Go
        C:/Users/colin/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:72 +0xa5

This seems to only happen sometimes because running the above command with -C 81 instead works without issue with all --data-saver= options. Also, with the working command "no", "prefer", and "fallback" all seem to produce the expected behavior.

@leotaku
Copy link
Owner

leotaku commented Apr 13, 2023

That is... quite the interesting bug...

What seems to be happening here is that MangaDex, like our code, also has trouble decoding the 6th image in that chapter. As such, they simply leave it out of the data-saver array, which then causes this errror. This can be seen at the following URL.

In any case, this definitely seems like a bug on the MangaDex side. I'll try to report it to them and see what happens.

@leotaku
Copy link
Owner

leotaku commented Apr 14, 2023

A MangaDex developer confirmed that this is a bug on their side and fixed it for this specific case. I've also added some code that ensures we get a proper error for any chapters where this is the case.

@Colin1224
Copy link
Contributor Author

Seems to be working well now. I also tried out the example in #24 and the fallback option worked great as a workaround for a bad image. This feature seems ready to merge.

One thing though, I think that there should still be an issue left open for broken images. Right now, the default behavior of kojirou is to still error out when a broke image is found, so until the default behavior can handle those cases there should be an issue open for it. If --data-saver=fallback is changed to be set as the default, then I think it's fine to close the issues though.

@leotaku
Copy link
Owner

leotaku commented Apr 14, 2023

Yeah, I get what you mean. I'll leave at least one "broken images" issue open, or create a new one that summarizes the problem.

However, I am not comfortable making --data-saver=fallback the default, as I think lower quality images are something that an user should have to opt in for, even if the normal images are broken.

@leotaku leotaku merged commit 837007d into master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants