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

Skip externalAgentSupport call when spineItem not available #409

Conversation

wisfan
Copy link
Contributor

@wisfan wisfan commented Sep 14, 2017

This pull request is a Work In Progress

Related issue(s) and/or pull request(s)

Issue #407

Alternately, ensure that spineItem is always populated?

ReflowableView:863
ScrollView:281
ScrollView:554
FixedView:200

@danielweck
Copy link
Member

Thank you! @jccr and I had a look, but need to figure out a "proper" fix so that further occurrences of this bug can be prevented in other places in the code.

@@ -280,6 +280,8 @@ var ReaderView = function (options) {
_.defer(function () {
Globals.logEvent("PAGINATION_CHANGED", "EMIT", "reader_view.js");
self.emit(Globals.Events.PAGINATION_CHANGED, pageChangeData);

if (!pageChangeData.spineItem) return;
_.defer(function () {
Copy link
Member

Choose a reason for hiding this comment

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

pageChangeData.spineItem is also used in the Media Overlays player, via the line above (_mediaOverlayPlayer.onPageChanged(pageChangeData);). Thankfully, there are safeguards inside the MO code, so no crashes. Unfortunately, we use var spineItems = reader.getLoadedSpineItems(); to match pageChangeData.spineItem in multiple page spreads (fixed layout), so it would be nice if the spineItem property was set correctly in all view types (inc. reflow and scroll).

Copy link
Member

Choose a reason for hiding this comment

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

Note that I fixed the reflow view: 6d8beaa

@danielweck
Copy link
Member

Merged this as reasonable safety guard for nil spineItem.
However, keeping #407 open to work on scroll and fixed views.

@wisfan
Copy link
Contributor Author

wisfan commented Nov 2, 2017

Great, thanks, Daniel!

@wisfan wisfan deleted the feature/fix-pagination-external-agent-support branch November 2, 2017 17:26
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