Refactoring to move page display code into separate class #5295

yurydelendik · 2014-09-13T00:31:16Z

Idea is to create reusable PDFViewer that will not have dependency on the PDFView (toolbars, find controller, etc).

Move pages container logic into PDFViewer (?) out of PDFView
Move thumbs container logic into PDFThumbnailsViewer (?) out of PDFView
Remove all dependency on PDFView in PDFViewer and classes it's using
Remove any rendering logic from PDFView
Isolate PDFPageSource logic and use in PDFViewer, PDFThumbnailsViewer, PDFFindController
Refactor PresentationMode (minimal, to remove dependency from PDFViewer)

NOTE: The refactoring shall not include any optimization or bug fixes

Snuffleupagus · 2014-09-13T10:01:32Z

Adding a class named PDFViewer might be slightly confusing when PDFView already exists. Should we rename one (or both) of them to mitigate this?

I've done some quick testing, and the viewer still appears to (mostly) work as before :-)

Given that one of the TODOs is "Refactor PresentationMode", I'm not sure how relevant this comment is, but I thought I should mention it anyway. I noticed that the zoom level isn't reset properly when exiting presentation mode. To reproduce:

Set the zoom level to one of the predefined levels (e.g. auto or page-width).
Enter presentation mode.
Exit presentation mode.
Resize the browser window.

Actual result:
The zoom level of the document is now page-fit.

Expected result:
The zoom level of the document should be the same as before (e.g. auto or page-width).

Snuffleupagus · 2014-09-13T20:22:04Z

web/pdf_viewer.js

+
+'use strict';
+
+var PDFViewer = (function pdfViewer() {


Nit: function PDFViewerClosure() instead?

yurydelendik · 2014-09-15T12:50:01Z

Adding a class named PDFViewer might be slightly confusing when PDFView already exists. Should we rename one (or both) of them to mitigate this?

I have to decisions made yet, however we started to name everything as PDFXxxx. I'm proposing to keep PDFViewer (since other names will not fit here). PDFView is a bad name here. So I decided to deprecate it, in the future it can be e.g. an alias for PDFViewerApplication.instance.

yurydelendik · 2014-09-15T19:28:13Z

I noticed that the zoom level isn't reset properly when exiting presentation mode.

Fixed that.

PresentationMode stuff is not was I expected, I'm going to keep that mostly as is, except removing dependency from PDFViewer on this object.

yurydelendik · 2014-09-15T20:47:02Z

I think I'm done. Here is minimal viewer https://gist.github.com/yurydelendik/5e7eda82bfd67a5a15df

Snuffleupagus · 2014-09-15T21:04:25Z

I noticed that the zoom level isn't reset properly when exiting presentation mode.

Fixed that.

Confirmed fixed, thank you!

Snuffleupagus · 2014-09-15T21:19:37Z

web/pdf_viewer.js

+'use strict';
+
+var PresentationModeState = {
+  UKNOWN: 0,


Typo: UNKNOWN.

Snuffleupagus · 2014-09-20T22:57:22Z

web/viewer.js

      downloadedPromise.then(function () {
        var event = document.createEvent('CustomEvent');
        event.initCustomEvent('documentload', true, true, {});
        window.dispatchEvent(event);
      });

-      PDFView.loadingBar.setWidth(container);
+      self.loadingBar.setWidth(document.getElementById('viewerContainer'));


This needs to use the DOM element viewer instead, otherwise it won't work.

For some reason, in this scope, the variable container doesn't refer to what you would expect; see https://github.com/mozilla/pdf.js/pull/5295/files#diff-dd42e9b2d2a652b8e312efa567698aa6L994

Snuffleupagus · 2014-09-22T09:58:52Z

web/viewer.js

+           OverlayManager, PDFFindController, PDFFindBar, getVisibleElements,
+           watchScroll, PDFViewer, PDFRenderingQueue, PresentationModeState,
+           DEFAULT_SCALE, UNKNOWN_SCALE,
+           IGNORE_CURRENT_POSITION_ON_ZOOM: true */


Not typo -- signals lint that property can be overwritten

existentialism · 2014-09-28T14:13:21Z

web/pdf_viewer.js

+     * @returns {number}
+     */
+    get pagesRotation() {
+      return this._pageRotation;


Should this (and in the setter below) be this._pagesRotation to match L#286?

Yes, thank you

Snuffleupagus · 2014-09-28T14:33:25Z

web/page_view.js

        return;
      }
      dest = null;
-      PDFView.setScale(PDFView.currentScaleValue, true, true);


Won't removing this line reintroduce the issue that PR #3829 fixed?

yep, /test/pdfs/rotation.pdf a test case

Conflicts: web/pdf_viewer.js

Refactoring to move page display code into separate class

Fixes typo/regression of #5295 for presentation mode

* 'master' of https://github.com/mozilla/pdf.js: Move text_layer_builder and pdf_viewer styles out Adds css import preprocessing Moves scrollPageIntoView to the PDFViewer. Followup fix for entering/exiting Presentation mode Fixes typo/regression of mozilla#5295 for presentation mode Renames and refactors PDFView to PDFViewerApplication. Adds types definitions (jsdoc) for the PDFViewer code. Marks some private methods in PDFViewer and PDFThumbnailViewer Moves constants to avoid dependency on PDFView Removes any usage of PDFView in the PageView Removes PresentationMode dependency from PDFViewer Moves rendering queue logic from PDFView Moves pdfDocument.getPage/getTextContent requests out of PDFView Moves viewer code into PDFViewer and some code from PageView. Moves thumbs logic into PDFThumbnailViewer. Moves watchScroll and getVisibleElements from PDFView Fix Symbol fonts without font file but with Encoding dictionary (issue 5238)

yurydelendik added 4-work-in-progress viewer labels Sep 13, 2014

yurydelendik force-pushed the pdfviewer branch 2 times, most recently from 9917d97 to 35b278e Compare September 13, 2014 02:27

Snuffleupagus reviewed Sep 13, 2014
View reviewed changes

web/pdf_viewer.js

'use strict';

var PDFViewer = (function pdfViewer() {

Copy link

Collaborator

Snuffleupagus Sep 13, 2014

Choose a reason for hiding this comment

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

Nit: function PDFViewerClosure() instead?

yurydelendik force-pushed the pdfviewer branch 3 times, most recently from 2d7273a to ca279b2 Compare September 15, 2014 19:25

brendandahl self-assigned this Sep 15, 2014

yurydelendik force-pushed the pdfviewer branch from 15b7352 to 2874a7f Compare September 15, 2014 21:03

Snuffleupagus reviewed Sep 15, 2014
View reviewed changes

web/pdf_viewer.js

'use strict';

var PresentationModeState = {

UKNOWN: 0,

Copy link

Collaborator

Snuffleupagus Sep 15, 2014

Choose a reason for hiding this comment

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

Typo: UNKNOWN.

yurydelendik removed the 4-work-in-progress label Sep 15, 2014

yurydelendik added this to the 2014 Q3 milestone Sep 15, 2014

yurydelendik force-pushed the pdfviewer branch from 2874a7f to eac6d69 Compare September 17, 2014 22:35

yurydelendik added 2 commits September 20, 2014 08:19

Moves watchScroll and getVisibleElements from PDFView

bfefadb

Moves thumbs logic into PDFThumbnailViewer.

3bce147

yurydelendik force-pushed the pdfviewer branch 2 times, most recently from 013c35d to 03ea82d Compare September 20, 2014 20:08

Snuffleupagus reviewed Sep 20, 2014
View reviewed changes

Moves viewer code into PDFViewer and some code from PageView.

7af8748

yurydelendik added 5 commits September 21, 2014 12:47

Moves pdfDocument.getPage/getTextContent requests out of PDFView

7642c39

Moves rendering queue logic from PDFView

a06a974

Removes PresentationMode dependency from PDFViewer

fbd7eed

Removes any usage of PDFView in the PageView

f1851c6

Moves constants to avoid dependency on PDFView

a1eca20

yurydelendik force-pushed the pdfviewer branch from 03ea82d to 86af3d7 Compare September 21, 2014 17:50

Snuffleupagus reviewed Sep 22, 2014
View reviewed changes

yurydelendik force-pushed the pdfviewer branch from 86af3d7 to 25b1633 Compare September 22, 2014 11:41

yurydelendik mentioned this pull request Sep 23, 2014

Release zips should contain more files #4885

Closed

existentialism reviewed Sep 28, 2014
View reviewed changes

yurydelendik force-pushed the pdfviewer branch from 25b1633 to 5510652 Compare September 28, 2014 14:25

Snuffleupagus reviewed Sep 28, 2014
View reviewed changes

yurydelendik added 3 commits September 28, 2014 10:39

Marks some private methods in PDFViewer and PDFThumbnailViewer

3773972

Conflicts: web/pdf_viewer.js

Adds types definitions (jsdoc) for the PDFViewer code.

a89bba3

Renames and refactors PDFView to PDFViewerApplication.

44779f1

yurydelendik force-pushed the pdfviewer branch from 5510652 to 44779f1 Compare September 28, 2014 15:40

brendandahl added a commit that referenced this pull request Sep 29, 2014

Merge pull request #5295 from yurydelendik/pdfviewer

1145eb8

Refactoring to move page display code into separate class

brendandahl merged commit 1145eb8 into mozilla:master Sep 29, 2014

yurydelendik added a commit to yurydelendik/pdf.js that referenced this pull request Sep 30, 2014

Fixes typo/regression of mozilla#5295 for presentation mode

504d2c1

yurydelendik added a commit that referenced this pull request Sep 30, 2014

Merge pull request #5359 from yurydelendik/pdfviewer2

f852a44

Fixes typo/regression of #5295 for presentation mode

Snuffleupagus mentioned this pull request Oct 24, 2014

Remove custom |pagerender| event from page_view.js, except for |GENERIC| builds #5441

Closed

Snuffleupagus mentioned this pull request Jun 24, 2015

Move the page switching code into set currentPageNumber in PDFViewer instead of placing it in the pagechange event handler #6140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring to move page display code into separate class #5295

Refactoring to move page display code into separate class #5295

yurydelendik commented Sep 13, 2014

Snuffleupagus commented Sep 13, 2014

Snuffleupagus Sep 13, 2014

yurydelendik commented Sep 15, 2014

yurydelendik commented Sep 15, 2014

yurydelendik commented Sep 15, 2014

Snuffleupagus commented Sep 15, 2014

Snuffleupagus Sep 15, 2014

Snuffleupagus Sep 20, 2014

Snuffleupagus Sep 22, 2014

yurydelendik Sep 22, 2014

existentialism Sep 28, 2014

yurydelendik Sep 28, 2014

Snuffleupagus Sep 28, 2014

yurydelendik Sep 28, 2014

yurydelendik Sep 28, 2014

Refactoring to move page display code into separate class #5295

Refactoring to move page display code into separate class #5295

Conversation

yurydelendik commented Sep 13, 2014

Snuffleupagus commented Sep 13, 2014

Choose a reason for hiding this comment

yurydelendik commented Sep 15, 2014

yurydelendik commented Sep 15, 2014

yurydelendik commented Sep 15, 2014

Snuffleupagus commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment