getmango / Mango

Mango is a self-hosted manga server and web reader
https://getmango.app
MIT License
1.69k stars 120 forks source link

Support unzipped entry #305

Closed Leeingnyo closed 2 years ago

Leeingnyo commented 2 years ago

Resolve #215

What I did

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

About class name

ZippedEntry -> ArchiveEntry DirectoryEntry -> ?

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

Leeingnyo commented 2 years ago

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

Fixed.

tr7zw commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 :+1:

hkalexling commented 2 years ago

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 commented 2 years ago

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

Leeingnyo commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks <3