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

Fixes #61: Expose spine item linear property through JNI for Android (+ rendition flow,layout,orientation,spread metadata). #128

Merged
merged 2 commits into from
Nov 27, 2014

Conversation

jccr
Copy link
Member

@jccr jccr commented Nov 25, 2014

No description provided.

@danielweck
Copy link
Member

Thank you @jccr
(I am discovering this part of the JNI code)

Remark #1: it looks like your commit includes spaces vs. tabs discrepancies

Remark #2: support for these spine properties is missing (only rendition:layout is handled):
rendition:flow
rendition:orientation
rendition:spread (which is different than page-spread)

Remark #3: page-spread is fetched via the SpineItem::Spread() method https://github.com/readium/readium-sdk/blob/develop/ePub3/ePub/spine.cpp#L67
...which is not good. Instead, we should align with iOS/OSX:
https://github.com/readium/readium-sdk/blob/develop/Platform/Apple/RDServices/Main/RDSpineItem.mm#L124
Note the "false" last parameter of the call to the SpineItem::PropertyMatching SDK method (which is very important, to not attempt to fetch absent metadata walking up the ancestor chain, i.e. typically from the SpineItem to Package level).
Also note how the "page-spread" property is obtained via optional "rendition" prefix, in order to support the newly-introduced "center" value in addition to the old FXL "left" "right" values (the latter do not use the same prefix/namespace).

@jccr
Copy link
Member Author

jccr commented Nov 26, 2014

@danielweck
Thanks for the feedback.

1 - It seems the spaces vs tabs is an issue that I'm building on top of.
For example:
https://github.com/readium/readium-sdk/blob/develop/Platform/Android/jni/package.cpp#L401
Starts with tabs, middle of block is using spaces, last statement in block uses tabs.

What do you think we should do about this? Mass-format everything into tabs? (or into spaces, or tabs because that is what the original author used) Maybe we should specify some code style guidelines and/or introduce http://editorconfig.org/ into the projects (Intellij-based IDEs which is what I use just got built-in support for this recently)

2 & 3 - I'll also take a look at these.

@jccr
Copy link
Member Author

jccr commented Nov 26, 2014

@danielweck
For #2,
what about rendition-viewport? It's in JS, but not SDK, is it not fetched for a reason?

@danielweck
Copy link
Member

@jccr no need for rendition:viewport, as this is only a newly-introduced hint (recent EPUB specification addition, so that reading systems have the option to determine canvas size prior to actually opening documents), and Readium fetches viewport dimensions from SVG / HTML metadata anyway.

As for the space vs. tabs inconsistencies, I prefer 4x spaces :)
What about you/others?
(PS the codebase-wide harmonization should probably be pushed as part of a separate Pull Request, to avoid cluttering the diff of feature-specific changes)

@jccr
Copy link
Member Author

jccr commented Nov 27, 2014

@danielweck I have updated this to include: #131 (support for rendition:orientation/flow/spread)

@danielweck
Copy link
Member

Awesome @jccr I look forward to giving it a try.

  • Note that currently none of the Readium launcher implementations support rendition:orientation (i.e. forcing landscape/portrait layout, rotating screen if necessary). It concerns mostly native launchers (e.g. tablet/phone devices that can be physical rotated in space), although the cloud reader may be able to access some mobile-specific Javascript API to lock an orientation directly via the web browser.
  • We can immediately start testing the correct application of authored rendition:spread properties, and we need to check that user overrides (settings) are correctly applied. See this Google shared doc: https://docs.google.com/spreadsheets/d/1vzylT0YKP8ITftALNzuAk_fucdZ4eipu0vsnVf-8UFk and the corresponding code which is easier to read :) (deduceSyntheticSpread): https://github.com/readium/readium-shared-js/blob/develop/js/helpers.js#L305
    Because we can't manually resize the viewport dimensions (which affects the aspect ratio, and therefore the application of rendition:spread), we can only test rotation on a physical device. PS: I'll run some tests
  • We need additional UI controls to allow the user to force the rendition:flow modes (auto, scroll-doc, scroll-continuous) UI controls for rendition:flow SDKLauncher-Android#38

@jccr are you okay working on the UI stuff?

@danielweck
Copy link
Member

@jccr let's merge this into develop as soon as you are good to go, so that @rkwright and others can test builds from the develop branch.

@jccr
Copy link
Member Author

jccr commented Nov 27, 2014

@danielweck
Yea, I'm up for the UI work.

jccr added a commit that referenced this pull request Nov 27, 2014
…r-android

Fixes #61, #131: Support for spine item linear and rendition:orientation/flow/spread properties.
@jccr jccr merged commit fd92146 into develop Nov 27, 2014
@jccr jccr deleted the feature/issue-61-spineitem-linear-android branch November 27, 2014 20:41
@danielweck danielweck changed the title Fixes #61: Expose spine item linear property through JNI for Android. Fixes #61: Expose spine item linear property through JNI for Android (+ rendition flow,layout,orientation,spread metadata). Dec 4, 2014
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.

2 participants