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

Support unzipped entry #305

Merged
merged 24 commits into from
Jun 5, 2022
Merged

Conversation

Leeingnyo
Copy link
Member

Resolve #215

What I did

  • Change the Entry class to abstract class,
  • Implement DirectoryEntry, which treats unzipped images in a directory as an entry
    • the rule is described at here
  • Scan DirectoryEntry when Title#new and Title#examine are called.
  • Fix caching policy after some tests

Result and test

Here is an test library environment.

.:
 info.json   nested-folder/  'THE iDOLM@STER Your Mess@ge 3rd stage.zip'  'THE iDOLM@STER Your Mess@ge 4th stage.zip'   unzipped-files/

./nested-folder:
 10.jpg   12.jpg   14.jpg   16.jpg   18.jpg   1.jpg    21.jpg   23.jpg   25.jpg   2.jpg   4.jpg   6.jpg   8.jpg   info.json             'THE iDOLM@STER Your Mess@ge 1st stage.zip'
 11.jpg   13.jpg   15.jpg   17.jpg   19.jpg   20.jpg   22.jpg   24.jpg   26.jpg   3.jpg   5.jpg   7.jpg   9.jpg   nested-unziped-file/  'THE iDOLM@STER Your Mess@ge 2nd stage.zip'

./nested-folder/nested-unziped-file:
10.jpg  11.jpg  12.jpg  13.jpg  14.jpg  15.jpg  16.jpg  17.jpg  18.jpg  19.jpg  1.jpg  20.jpg  21.jpg  22.jpg  23.jpg  24.jpg  25.jpg  26.jpg  2.jpg  3.jpg  4.jpg  5.jpg  6.jpg  7.jpg  8.jpg  9.jpg

./unzipped-files:
10.jpg  11.jpg  12.jpg  13.jpg  14.jpg  15.jpg  16.jpg  17.jpg  18.jpg  19.jpg  1.jpg  20.jpg  21.jpg  22.jpg  23.jpg  24.jpg  25.jpg  26.jpg  2.jpg  3.jpg  4.jpg  5.jpg  6.jpg  7.jpg  8.jpg  9.jpg

In tree view,

root (unzipped, have 4 entries, one title)
├ nested-folder (would be an entry and a title, have 3 entries)
│ ├ 1.jpg ~ 26.jpg
│ ├ nested-unziped-file (would be an entry)
│ │ └ 1.jpg ~ 26.jpg
│ ├ THE iDOLM@STER Your Mess@ge 1st stage.zip
│ └ THE iDOLM@STER Your Mess@ge 2nd stage.zip
├ unzipped-files (would be an entry)
│ └ 1.jpg ~ 26.jpg
├ THE iDOLM@STER Your Mess@ge 3rd stage.zip
└ THE iDOLM@STER Your Mess@ge 4th stage.zip

Result

image

image

image

The directories appeared as entries and titles

What I tested

  • remove an image file from directory
  • rename an image file in a directory
  • add new folder to scan

About class name

ZippedEntry -> ArchiveEntry
DirectoryEntry -> ?

I can't come up with good names...

@Leeingnyo Leeingnyo requested a review from hkalexling May 15, 2022 07:53
@Leeingnyo
Copy link
Member Author

Leeingnyo commented May 15, 2022

I found that there's a bug after rename a directory entry and scan.

Fixed.

@tr7zw
Copy link
Contributor

tr7zw commented May 16, 2022

Since I mentioned it in the linked issue, I guess this pr does not yet do anything in regards to not treat any 2nd+ level folder like a chapter?

@Leeingnyo
Copy link
Member Author

@tr7zw Hi!

you mean that the nested-folder in the sample would be a problem, and it shouldn't be treated as an entry because it has another titles and zipped entries though it has image files, right?
I think the responsibility of configuring a library is up to the library owners. If they don't want this, they would remove those files.

@tr7zw
Copy link
Contributor

tr7zw commented May 16, 2022

Huh I guess never mind? I still had in mind that if you have a folder "Manga" and in that folder a folder "One Piece" with the chapters inside, I think in the past it treated the "Manga" folder as something containing Chapters. But apparently this was already fixed, now it shows Titles/Entries.
Still a really neat pr, since using directories as chapters allows some more flexibility, and with the new abstract Entry class it could allow some nice new features(other compressed files than .zip, dynamically loading chapters from urls/network shares).

@hkalexling
Copy link
Member

Thanks @Leeingnyo! I fixed the linter warnings and took a quick look, and it looks good overall! I think it's a nice application of abstract classes.

Re class names, yeah I think ArchiveEntry is more appropriate than ZippedEntry because we also support RAR/CBR files. I think DirectoryEntry looks fine. We could also use DirEntry to make it shorter but I have no strong feelings.

Also I think it makes more sense to break the classes into individual files, e.g., entry.cr, zipped_entry.cr, and directory_entry.cr. What do you think?

I will do a full review and some testings later this week 👍

src/routes/api.cr Outdated Show resolved Hide resolved
src/library/title.cr Outdated Show resolved Hide resolved
src/routes/reader.cr Outdated Show resolved Hide resolved
src/util/signature.cr Outdated Show resolved Hide resolved
@hkalexling
Copy link
Member

Hey @Leeingnyo sorry for the delay. I made quite some changes to the PR. Can you take a quick look when you have the time to make sure I didn't accidentally fuck up anything? Thanks!

@Leeingnyo
Copy link
Member Author

@hkalexling sorry I pushed errors occurred version... wait for a moment

@Leeingnyo Leeingnyo force-pushed the feature/unzipped-entry branch from 9d9b551 to 9ce8e91 Compare June 3, 2022 15:26
@Leeingnyo
Copy link
Member Author

@hkalexling it looks great. I like the method that you did to recover entry instances :)
Since @page was added to Entry, it caused an error to recover old library.yml.zip file. But, I don't mind of this. fine!

@hkalexling
Copy link
Member

Thanks @Leeingnyo! Sorry the comments above was just for my own reference and I accidentally published them ;-P

Since @page was added to Entry, it caused an error to recover old library.yml.zip file.

Could you elaborate a bit on this? I tried the following steps but didn't see the error.

  1. Remove library.yml.gz
  2. Build and run from the master branch to regenerate the library.yml.gz file
  3. Build and run from this branch and there's no error

@Leeingnyo
Copy link
Member Author

Oh that's the difference. I built it from the dev branch. but I had the same error when I tried from the master branch.

[ERROR]   2022/06/05 15:02:04 | Missing YAML attribute: path at line 14, column 7

because my ubuntu snap upgrades a Crystal implicitly, I use a Crystal 1.4.1... this would be a matter. :p

@hkalexling
Copy link
Member

Ah sorry my bad. It does happen on master on Crystal 1.0.0 as well, but it's a minor issue and will only happen once so I think we are fine.

@hkalexling hkalexling merged commit 2d97faa into getmango:dev Jun 5, 2022
@hkalexling
Copy link
Member

Thanks <3

@Leeingnyo Leeingnyo deleted the feature/unzipped-entry branch June 7, 2022 06:50
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.

3 participants