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

Various bugfixes due to humans being humans #54

Merged
merged 4 commits into from
Jan 18, 2025

Conversation

onigoetz
Copy link
Contributor

Hi,

I'm contributing these fixes to this library as it is used in the https://github.com/biblioverse/biblioteca project, more specifically to scan ebooks.

I've encountered three errors with this library and decided to make a single Pull Request for all three, please tell me if you'd rather have separate pull requests.

1. Missing escaping of CLI arguments

When calling 7zip, it happens that the file called contains a space, a - or a quote. These characters are directly interpreted in the terminal and cause the library to behave improperly.

I added escaping of arguments to avoid this issue.

Note that the method used comes from Symfony; https://github.com/symfony/process/blob/7.2/Process.php#L1637 I don't know what the proper process of attribution is for this kind of copy.

2. Errors swallowed hiding the original error

When extracting a page from a PDF, if it fails only a Error, page_0 is not an image message is displayed.

The actual error was cache resources exhausted /tmp/magick-MEUnRhIPjodKoNbmiZiE_XiYy0Tn9dQV1' @ error/cache.c/OpenPixelCache/4095` and this message helped me figure out a fix.

This second change makes the original error message visible.

3. Sometimes a .cbz is a RAR file, sometimes a .cbr is a ZIP file

I'm open to discuss this one as it could be controversial.
I have many comic books and it happens that the person who created them didn't know that .cbZ = ZIP and .cbR = RAR.
The result is that sometimes a .cbz is a RAR file or a .cbr is a ZIP file

The change I propose is to check failed parsings of a .cbr/.cbz as the opposite format.

If you prefer, I could propose an alternative which would be to test-parse the file and provide an helpful message such as "This file has a .cbz extension and should be a ZIP archive but is actually a RAR archive, please rename this file to .cbr"

note for me: This partially fixes biblioverse/biblioteca#317

@onigoetz
Copy link
Contributor Author

Hi @ewilan-riviere,

I fixed the failing tests, the two remaining errors seem unrelated to my changes as they also appear in this PR from november; #53
I'm not sure how to approach fixing this

For the rar extension issue, maybe it works if it's installed with pecl -v install rar as proposed in the documentation https://www.php.net/manual/en/rar.installation.php ?

@ewilan-riviere
Copy link
Contributor

Thanks for your contribution!

Yes, current fail on macOS isn't a problem here, I will merge your PR today. RAR extension is not maintained anymore, so installation is a cumberstone, I will try to fix it before new release ;)

@onigoetz
Copy link
Contributor Author

Oh BTW, I didn't know that you also had your own ebook library manager it looks really nice !

I started contributing to Biblioteca recently after managing my own tool for years (https://github.com/onigoetz/Comics-Reader).
If you would like to join forces together on a project you would be welcome, we speak french and love books :)

@ewilan-riviere ewilan-riviere merged commit 4470f17 into kiwilan:main Jan 18, 2025
6 of 8 checks passed
@ewilan-riviere
Copy link
Contributor

I looked at the https://github.com/biblioverse/biblioteca project you took part in and it seems to offer so many features that my own project seems quite limited :D
And for https://github.com/onigoetz/Comics-Reader, it's exactly the tool I've been looking for! I'll be testing it shortly, thanks!

At the moment, I'm retraining and don't have much time for development, but as soon as I have more time, I'll see if I can help in some way! Thanks for the offer :D

@ewilan-riviere ewilan-riviere mentioned this pull request Jan 18, 2025
@onigoetz
Copy link
Contributor Author

it seems to offer so many features that my own project seems quite limited :D

Yeah it's the reason why I decided to contribute on it as well. I'm happy with my own implementation of the online reader and will contribute something similar to Biblioteca and probably stop development on my own after that.

I'll be testing it shortly, thanks!

Cool, feel free to drop an issue if anything is unclear :)

No worries the proposition is still open in the future, good luck on your retraining

@ewilan-riviere
Copy link
Contributor

Online reader is THE feature I need to integrate, I'll have to work on it ;)

No worries the proposition is still open in the future, good luck on your retraining

Thanks you!

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.

Scanning seems to not like spaces in filenames
2 participants