fix(api): Use fixed-layout epub format in Kobo sync when necessary (Booklore PR Port)#16
Conversation
|
I've good good news and bad news. Bad news is I can't find the reproducer I wrote. Good news is that I tested the PR, and everything still works: |
|
Does this also fix it for when CBX comics get converted to EPUBs during the Kobo sync? Or just EPUB to EPUB? Since this was an issue I was annoyed by when I added the CBX to EPUB conversion for kobo sync, as I didn't know this fixed layout was an option when I was working on that. |
|
Atm no. I dont know if all cbx files should be treated as fixed layout since I never used any, and they dont have a similar metadata property that I could check for. If this should apply to all cbx files its a pretty easy change though. |
|
Here's the change needed to make it apply to cbx files as well: N00byKing@d2a2106 If you try it out and it works I'll put it in the PR as well. |
|
I'll test it out and see how it reacts, but effectively Kobo sync doesn't support CBX files so I added functionality that creates new EPUB files using the images extracted from a CBX file. A CBX file is just a ZIP/RAR/7zip file with a bunch of images inside, so they're effectively always comics or manga. So when a CBX file is picked up in the kobo sync, it triggers the below to convert it to an EPUB. That basically just builds an EPUB out of the template files present below, with each image (pulled from the CBX file) being put on it's own page inside the EPUB. If it's easier, I'm happy to wait until this is merged in (As it is without the CBX stuff) and things are stable, and then loop back to look at it. |
Could you not just change this one line to |
|
For the fixed layout stuff there's a new entry in the database, so imo it makes more sense to populate it just the same as for the epubs for consistency. |
89113d4 to
37ca101
Compare
📝 WalkthroughWalkthroughThis PR adds fixed-layout EPUB support to the booklore API. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant EpubProcessor
participant EpubMetadataExtractor
participant EpubReaderService
participant BookFileEntity
participant KoboEntitlementService
Client->>EpubProcessor: Process EPUB file
EpubProcessor->>EpubMetadataExtractor: Extract metadata
EpubMetadataExtractor->>EpubReaderService: getOPFDocument(epubFile)
EpubReaderService->>EpubReaderService: parseContainerXml (static)
EpubReaderService-->>EpubMetadataExtractor: OPF Document with metadata
EpubMetadataExtractor-->>EpubProcessor: Metadata including isFixedLayout
EpubProcessor->>BookFileEntity: setFixedLayout(boolean)
EpubProcessor-->>Client: File processed & stored
Client->>KoboEntitlementService: Map to Kobo format
KoboEntitlementService->>BookFileEntity: isFixedLayout()
alt Fixed Layout EPUB
KoboEntitlementService->>KoboEntitlementService: Set format = EPUB3FL
else Standard EPUB
KoboEntitlementService->>KoboEntitlementService: Apply conversion logic (KEPUB/EPUB3)
end
KoboEntitlementService-->>Client: Kobo metadata with format
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (2)
164-167: Double ZIP open for cover resolution.
getOPFDocument(line 154) andgetOPFPath(line 167) each open the EPUB file separately. Since this method is a fallback heuristic, consider caching the OPF path from the first call or using a combined method to reduce I/O overhead.♻️ Suggested fix
private String findManifestCoverByHeuristic(File epubFile) { try { + String opfPath = EpubReaderService.getOPFPath(epubFile); Document doc = EpubReaderService.getOPFDocument(epubFile); NodeList manifestItems = doc.getElementsByTagName("item"); for (int i = 0; i < manifestItems.getLength(); i++) { Element item = (Element) manifestItems.item(i); String id = item.getAttribute("id"); String href = item.getAttribute("href"); String mediaType = item.getAttribute("media-type"); if ((id != null && id.toLowerCase().contains("cover")) || (href != null && href.toLowerCase().contains("cover"))) { if (mediaType != null && mediaType.startsWith("image/")) { String decodedHref = URLDecoder.decode(href, StandardCharsets.UTF_8); - return resolvePath(EpubReaderService.getOPFPath(epubFile), decodedHref); + return resolvePath(opfPath, decodedHref); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java` around lines 164 - 167, The cover-resolution fallback currently calls EpubReaderService.getOPFPath(...) again causing the EPUB to be opened twice; modify EpubMetadataExtractor so the OPF path obtained earlier by getOPFDocument(...) is reused (cache the opfPath in a local variable or return it from getOPFDocument) and pass that cached opfPath into resolvePath(...) instead of calling getOPFPath(...) a second time; update references around getOPFDocument, getOPFPath, and resolvePath to use the cached opfPath to avoid double ZIP openings.
596-614: Same double ZIP open pattern here.The same improvement applies: retrieve the OPF path before iterating, or use a combined approach to avoid opening the file twice.
♻️ Suggested fix
private String findCoverImageHrefInOpf(File epubFile) { try { + String opfPath = EpubReaderService.getOPFPath(epubFile); Document doc = EpubReaderService.getOPFDocument(epubFile); NodeList manifestItems = doc.getElementsByTagName("item"); for (int i = 0; i < manifestItems.getLength(); i++) { Element item = (Element) manifestItems.item(i); String properties = item.getAttribute("properties"); if (properties != null && properties.contains("cover-image")) { String href = item.getAttribute("href"); String decodedHref = URLDecoder.decode(href, StandardCharsets.UTF_8); - return resolvePath(EpubReaderService.getOPFPath(epubFile), decodedHref); + return resolvePath(opfPath, decodedHref); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java` around lines 596 - 614, In findCoverImageHrefInOpf, avoid opening the EPUB twice by first calling EpubReaderService.getOPFPath(epubFile) to obtain the OPF path and then use a single call (or a new overload) to parse the OPF document from that path instead of calling EpubReaderService.getOPFDocument(epubFile) which opens the ZIP again; update the logic in findCoverImageHrefInOpf to resolve the href using the retrieved opfPath and decoded href, or add a helper in EpubReaderService that returns both Document and opfPath in one ZIP read so you only open the EPUB once when iterating manifest items.booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java (1)
350-361: Consider consolidating to avoid opening the EPUB file multiple times.
getOPFPathandgetOPFDocumenteach open a newZipFile. When both are called sequentially (as inEpubMetadataExtractor.findManifestCoverByHeuristiclines 154 and 167), the EPUB is opened twice. Consider providing a combined method or caching the result.Additionally, these static methods use the default charset, while
parseEpubWithEncodinghandles multiple encodings. This could cause issues with EPUBs containing non-UTF8 filenames.♻️ Suggested approach to avoid double-opening
- public static String getOPFPath(File epubFile) throws Exception { - try (ZipFile zip = new ZipFile(epubFile)) { - return parseContainerXml(zip); - } - } - - public static Document getOPFDocument(File epubFile) throws Exception { - try (ZipFile zip = new ZipFile(epubFile)) { - String opfPath = parseContainerXml(zip); - return parseXmlEntry(zip, opfPath); - } - } + public static String getOPFPath(File epubFile) throws Exception { + try (ZipFile zip = ZipFile.builder().setFile(epubFile).get()) { + return parseContainerXml(zip); + } + } + + public static Document getOPFDocument(File epubFile) throws Exception { + try (ZipFile zip = ZipFile.builder().setFile(epubFile).get()) { + String opfPath = parseContainerXml(zip); + return parseXmlEntry(zip, opfPath); + } + } + + /** Returns both OPF path and document in a single ZIP open operation. */ + public static OPFInfo getOPFInfo(File epubFile) throws Exception { + try (ZipFile zip = ZipFile.builder().setFile(epubFile).get()) { + String opfPath = parseContainerXml(zip); + Document doc = parseXmlEntry(zip, opfPath); + return new OPFInfo(opfPath, doc); + } + } + + public record OPFInfo(String path, Document document) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java` around lines 350 - 361, Both getOPFPath and getOPFDocument reopen the EPUB (ZipFile) and ignore encoding handling used by parseEpubWithEncoding; to fix, add overloads that accept an open ZipFile (e.g., getOPFPath(ZipFile) and getOPFDocument(ZipFile)) or a single combined method (e.g., resolveOPF(ZipFile) returning both path and Document) and change callers such as EpubMetadataExtractor.findManifestCoverByHeuristic to open the ZipFile once and call the new overload; also reuse the encoding-aware logic from parseEpubWithEncoding when resolving entry names (or accept a Charset/encoding parameter) to ensure non-UTF8 filenames are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java`:
- Around line 164-167: The cover-resolution fallback currently calls
EpubReaderService.getOPFPath(...) again causing the EPUB to be opened twice;
modify EpubMetadataExtractor so the OPF path obtained earlier by
getOPFDocument(...) is reused (cache the opfPath in a local variable or return
it from getOPFDocument) and pass that cached opfPath into resolvePath(...)
instead of calling getOPFPath(...) a second time; update references around
getOPFDocument, getOPFPath, and resolvePath to use the cached opfPath to avoid
double ZIP openings.
- Around line 596-614: In findCoverImageHrefInOpf, avoid opening the EPUB twice
by first calling EpubReaderService.getOPFPath(epubFile) to obtain the OPF path
and then use a single call (or a new overload) to parse the OPF document from
that path instead of calling EpubReaderService.getOPFDocument(epubFile) which
opens the ZIP again; update the logic in findCoverImageHrefInOpf to resolve the
href using the retrieved opfPath and decoded href, or add a helper in
EpubReaderService that returns both Document and opfPath in one ZIP read so you
only open the EPUB once when iterating manifest items.
In
`@booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java`:
- Around line 350-361: Both getOPFPath and getOPFDocument reopen the EPUB
(ZipFile) and ignore encoding handling used by parseEpubWithEncoding; to fix,
add overloads that accept an open ZipFile (e.g., getOPFPath(ZipFile) and
getOPFDocument(ZipFile)) or a single combined method (e.g., resolveOPF(ZipFile)
returning both path and Document) and change callers such as
EpubMetadataExtractor.findManifestCoverByHeuristic to open the ZipFile once and
call the new overload; also reuse the encoding-aware logic from
parseEpubWithEncoding when resolving entry names (or accept a Charset/encoding
parameter) to ensure non-UTF8 filenames are handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83a3e9b6-6c0c-44b1-ba66-315e821e7e35
📒 Files selected for processing (8)
booklore-api/src/main/java/org/booklore/model/dto/BookMetadata.javabooklore-api/src/main/java/org/booklore/model/entity/BookFileEntity.javabooklore-api/src/main/java/org/booklore/model/enums/KoboBookFormat.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboEntitlementService.javabooklore-api/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/resources/db/migration/V133__Add_Fixed_Layout.sql
balazs-szucs
left a comment
There was a problem hiding this comment.
Thank you! (merging this because it's blocking me 🤣 , but everything look really clean, and works.)
…ooklore PR Port) (#16) * fix(api): Correct format for fixed-layout epub when using Kobo Sync * chore: Cleanup duplicate code * feat: Cache fixed-layout property in db * fix: Missing nullcheck in after retrieving BookFileEntity * fix: Remove memory leak in EpubReaderService.java * chore: Avoid opening the epub twice for fixed-layout extraction * chore: Fix DB migration from rebase --------- Co-authored-by: brios <127139797+balazs-szucs@users.noreply.github.com>
…ooklore PR Port) (grimmory-tools#16) * fix(api): Correct format for fixed-layout epub when using Kobo Sync * chore: Cleanup duplicate code * feat: Cache fixed-layout property in db * fix: Missing nullcheck in after retrieving BookFileEntity * fix: Remove memory leak in EpubReaderService.java * chore: Avoid opening the epub twice for fixed-layout extraction * chore: Fix DB migration from rebase --------- Co-authored-by: brios <127139797+balazs-szucs@users.noreply.github.com>
…ooklore PR Port) (grimmory-tools#16) * fix(api): Correct format for fixed-layout epub when using Kobo Sync * chore: Cleanup duplicate code * feat: Cache fixed-layout property in db * fix: Missing nullcheck in after retrieving BookFileEntity * fix: Remove memory leak in EpubReaderService.java * chore: Avoid opening the epub twice for fixed-layout extraction * chore: Fix DB migration from rebase --------- Co-authored-by: brios <127139797+balazs-szucs@users.noreply.github.com>
…ooklore PR Port) (#16) * fix(api): Correct format for fixed-layout epub when using Kobo Sync * chore: Cleanup duplicate code * feat: Cache fixed-layout property in db * fix: Missing nullcheck in after retrieving BookFileEntity * fix: Remove memory leak in EpubReaderService.java * chore: Avoid opening the epub twice for fixed-layout extraction * chore: Fix DB migration from rebase --------- Co-authored-by: brios <127139797+balazs-szucs@users.noreply.github.com>
…ooklore PR Port) (#16) * fix(api): Correct format for fixed-layout epub when using Kobo Sync * chore: Cleanup duplicate code * feat: Cache fixed-layout property in db * fix: Missing nullcheck in after retrieving BookFileEntity * fix: Remove memory leak in EpubReaderService.java * chore: Avoid opening the epub twice for fixed-layout extraction * chore: Fix DB migration from rebase --------- Co-authored-by: brios <127139797+balazs-szucs@users.noreply.github.com>
📝 Description
Unfortunately, I can't get the old PR description back, which sucks.
Also no linked issue because its gone :/
This PR fixes the format specifier in the Kobo Sync endpoint for fixed-layout epubs.
Previously, fixed layout epubs (comics/mangas/etc.) would use the normal book viewer, which does not do double page spreads on landscape, and always shows some header/footer margin.
With this PR, the ereader uses the actual comic reader which doesnt have these issues.
It has already gone through two rounds of review, first one moving the check from sync-time to book-import-time,
then some minor cleanup throughout.
I've made sure that the commit history is preserved when getting this PR ready, so you'll be able to browse the changes individually.
Linked Issue: Fixes #
🏷️ Type of Change
🔧 Changes
The
book_filedatabase now has an additional property specifying whether or not a book is a fixed-layout epub.This property is then read during Kobo Sync attempts to correctly populate the ebook format in the response.
Also deduplicates a lot of code that opens and reads the epub OPF file.
🧪 Testing (MANDATORY)
Manual testing steps you performed:
Regression testing:
Edge cases covered:
Test output:
Backend test output (
./gradlew test)Frontend test output (
ng test)📸 Screen Recording / Screenshots (MANDATORY)
Lost to time.
I can re-do the screenshots once I'm home, but its just some pictures of my ereader showing the epubs in fullscreen.
✅ Pre-Submission Checklist
develop(merge conflicts resolved)🤖 AI-Assisted Contributions
No AI was used for this PR
💬 Additional Context (optional)
Though it was working fine on the booklore code, I still need to do an end-to-end test on the forked repo.
It passes all tests atm, but probably best not to merge until I can try it on my actual device tonight (Like, 8ish hours from now)
Feel free to try it out yourself though.
Thanks for forking booklore, I really did not feel like migrating my book collection again
Summary by CodeRabbit
New Features
Refactor