Skip to content
This repository has been archived by the owner on Jul 29, 2022. It is now read-only.

Refactoring Publication models + XmlParser rewrite with namespaces handling #88

Merged
merged 47 commits into from
Feb 25, 2020

Conversation

mickael-menu
Copy link
Member

@mickael-menu mickael-menu commented Jan 30, 2020

Refactoring of the Publication models to align with Swift and add missing features/models.

Fixes readium/kotlin-toolkit#192, Fixes readium/kotlin-toolkit#193, Fixes #38, Fixes #35, Fixes #36, Fixes readium/architecture#113

  • Move all the related models to the publication package.
    • There's one additional sub-package per module/extension (eg. EPUB, Presentation Hints).
  • All Publication models are now Kotlin data classes to take advantage of equality and copy (used in unit tests).
    • Properties are now immutable to improve the safety of the model.
    • A few properties in Publication are still mutable because R2's implementation relies on it (e.g. adding the self link).
  • Add extensibility through JSON maps in Publication, Metadata and Properties.
  • Add the Presentation Hints module.
  • Add a WarningLogger interface to forward issues while building a Publication to the test-app.
    • The rationale is that sometimes we want to ignore non-fatal errors while parsing a RWPM or an EPUB package to still return the parsed Publication. Having a way to observe internal warnings can be of interest for the test-app/debugging.
    • This is related to this Swift issue: https://github.com/readium/r2-shared-swift/issues/60
    • Note 1: For now, this is internal, because it might change with the specification of the Streamer API.
    • Note 2: I think this strategy should be expanded for other internal logs that we might want to emit from R2 projects. This way the third-party app will have the control on what to do with these logs.
  • Add a unit test suite for the models JSON parsing/serializing
  • Use Parcelable instead of Serializable to share Publication through Intent.
  • Rename types and enums to follow the Google Android Style coding convention regarding case.

A few things are left to be refactored later:

  • Publication.TYPE is mixing up a few things together, but the test-app is still relying a lot on it. I opened an issue for that: Model for the Publication's format/type architecture#112
  • Publication.internalData should be removed, but is used right now in the streamer to send the rootFile. I think that this information is already available in the Container though.
  • I didn't touch the MediaOverlays models since it is not yet fully specified and a WIP (same thing on Swift).

This issue still needs to be resolved before merging this refactoring: readium/webpub-manifest#42


This PR now also contains this one from @qnga : #85

In a robust and conformant implementation, attributes and elements should not be identified just by their name or usual prefixes. Default namespace can differ from one XML file to another and can change along the way. Prefixes are completely arbitrary and authors are allowed to use anything that comes to their mind.

Migration Guide for the Client Apps

See these changes that were done in the test app to guide you.

See the Migration Guide to upgrade our app.

qnga and others added 18 commits January 18, 2020 11:38
…m values and rewrite mapping to ContentLayoutStyle for better conformance to epub spec
In a robust and conformant implementation, attributes and elements should not be identified just by their name or usual prefixes.
Default namespace can differ from one XML file to another and can change along the way.
Prefixes are completely arbitrary and authors are allowed to use anything that comes to their mind.

Add some features along the way:
-Make rendition properties match Readium Manifest specification
-Add basic support for subtitle and epub fallback in Publications
@mickael-menu
Copy link
Member Author

@qnga There's one thing that you need to keep in mind for the EPUB parser: when extracting hrefs from XML, they need to be URL decoded (this is used in encryption.xml, or when creating Link, for example).

Some checks used to be done directly in Publication but this was very fragile because you needed to use these APIs for comparison. I removed this and instead Link.href must be "normalized" in the EPUB parser itself. This is done like that in the Swift parser: readium/r2-streamer-swift@f1471e4

If you do it in the encryption.xml parser as well, this will fix this issue: readium/kotlin-toolkit#212

@mickael-menu
Copy link
Member Author

This PR now also contains this one from @qnga : #85

In a robust and conformant implementation, attributes and elements should not be identified just by their name or usual prefixes. Default namespace can differ from one XML file to another and can change along the way. Prefixes are completely arbitrary and authors are allowed to use anything that comes to their mind.

@mickael-menu mickael-menu changed the title Refactoring Publication models Refactoring Publication models + XmlParser rewrite with namespaces handling Jan 31, 2020
@qnga
Copy link
Contributor

qnga commented Jan 31, 2020

@qnga There's one thing that you need to keep in mind for the EPUB parser: when extracting hrefs from XML, they need to be URL decoded (this is used in encryption.xml, or when creating Link, for example).

To keep track, we have decided to decode URLs in PublicationParser.normalize, always used to resolve relative paths.

@mickael-menu mickael-menu marked this pull request as ready for review February 5, 2020 07:46
@mickael-menu mickael-menu force-pushed the fixes/publication-models branch from b4142a9 to 362fa97 Compare February 10, 2020 12:37
@mickael-menu mickael-menu force-pushed the fixes/publication-models branch from 946e6bf to 06b39d8 Compare February 12, 2020 15:52
@qnga qnga mentioned this pull request Feb 14, 2020
links, metadata and readingOrder are mandatory when producing a RWPM
qnga
qnga previously approved these changes Feb 14, 2020
Copy link
Contributor

@qnga qnga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many points have been discussed and resolved privately.

@mickael-menu
Copy link
Member Author

I intend to merge this next Monday.

@mickael-menu
Copy link
Member Author

I put WarningLogger as internal until the Streamer API is specified, since it might change to fit the needs of the streamer.

@mickael-menu mickael-menu merged commit 936ea48 into develop Feb 25, 2020
@mickael-menu mickael-menu deleted the fixes/publication-models branch February 25, 2020 09:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants