Skip to content

Commit

Permalink
Move the page switching code into set currentPageNumber in `PDFView…
Browse files Browse the repository at this point in the history
…er` instead of placing it in the `pagechange` event handler

The reason that this code can be moved is that the `if (this.loading && page === 1)` check, in the `pagechange` event handler in viewer.js, is never satisfied since `this.loading` is not defined in that scope.
This *could* be considered a regression from PR 5295, since prior to that `this.loading` was using the `PDFViewerApplication` scope (or `PDFView` as it were).
However, I don't think that we need to fix that since we've been shipping this code in no less than *three* Firefox releases (uplifted in https://bugzilla.mozilla.org/show_bug.cgi?id=1084158), without breaking the world.

An explanation of why the `pagechange` code works, despite `this.loading === undefined`, is that `set currentPageNumber` (in `PDFViewer`) returns early whenever `this.pdfDocument` isn't set. This check is, for all intents and purposes, functionally equivalent to checking `PDFViewerApplication.loading`.
Hence we can move the page switching code into `PDFViewer`, and also remove `PDFViewerApplication.loading` since it's not used any more.
(The `this.loading` property was added in PR 686, which was before the current viewer even existed.)

*Note:* The changes in this patch should also be beneficial to the viewer `components`, since requiring every implementer to provide their own `pagechange` event handler just to get `PDFViewer.currentPageNumber` to actually work seems like an unnecessary complication.
  • Loading branch information
Snuffleupagus committed Jun 24, 2015
1 parent 18e1a14 commit ffeba9c
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 15 deletions.
10 changes: 8 additions & 2 deletions web/pdf_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,12 @@ var PDFViewer = (function pdfViewer() {
this._currentPageNumber = val;
event.pageNumber = val;
this.container.dispatchEvent(event);

// Check if the caller is `PDFViewer_update`, to avoid breaking scrolling.
if (this.updateInProgress) {
return;
}
this.scrollPageIntoView(val);
},

/**
Expand Down Expand Up @@ -566,7 +572,7 @@ var PDFViewer = (function pdfViewer() {
};
},

update: function () {
update: function PDFViewer_update() {
var visible = this._getVisiblePages();
var visiblePages = visible.views;
if (visiblePages.length === 0) {
Expand All @@ -581,7 +587,7 @@ var PDFViewer = (function pdfViewer() {

this.renderingQueue.renderHighestPriority(visible);

var currentId = this.currentPageNumber;
var currentId = this._currentPageNumber;
var firstPage = visible.first;

for (var i = 0, ii = visiblePages.length, stillFullyVisible = false;
Expand Down
13 changes: 0 additions & 13 deletions web/viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,6 @@ var PDFViewerApplication = {
}

var self = this;
self.loading = true;
self.downloadComplete = false;

var passwordNeeded = function passwordNeeded(updatePassword, reason) {
Expand All @@ -543,7 +542,6 @@ var PDFViewerApplication = {
getDocumentProgress).then(
function getDocumentCallback(pdfDocument) {
self.load(pdfDocument, scale);
self.loading = false;
},
function getDocumentError(exception) {
var message = exception && exception.message;
Expand Down Expand Up @@ -571,7 +569,6 @@ var PDFViewerApplication = {
message: message
};
self.error(loadingErrorMessage, moreInfo);
self.loading = false;
}
);

Expand Down Expand Up @@ -1822,16 +1819,6 @@ window.addEventListener('pagechange', function pagechange(evt) {
Stats.add(page, pageView.stats);
}
}

// checking if the this.page was called from the updateViewarea function
if (evt.updateInProgress) {
return;
}
// Avoid scrolling the first page during loading
if (this.loading && page === 1) {
return;
}
PDFViewerApplication.pdfViewer.scrollPageIntoView(page);
}, true);

function handleMouseWheel(evt) {
Expand Down

0 comments on commit ffeba9c

Please sign in to comment.