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

Remove a redundant PDFViewer.currentScale call from PDFViewerApplication.load #8539

Merged
merged 1 commit into from
Jun 18, 2017
Merged

Remove a redundant PDFViewer.currentScale call from PDFViewerApplication.load #8539

merged 1 commit into from
Jun 18, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jun 18, 2017

Since this call occurs before the PDFViewer.setDocument call, it won't actually cause any scale change.
Furthermore, moving it should not be necessary, since the scale is already used as the fallback case in PDFViewerApplication.setInitialView (provided it's non-zero, which isn't even the case in the default viewer).

Hence this patch should cause no functional changes at all, since it simply removes a piece of unnecessary code.

…cation.load`

Since this call occurs *before* the `PDFViewer.setDocument` call, it won't actually cause any scale change.
Furthermore, moving it should not be necessary, since the `scale` is already used as the fallback case in `PDFViewerApplication.setInitialView` (provided it's non-zero, which isn't even the case in the default viewer).

Hence this patch should cause no functional changes at all, since it simply removes a piece of unnecessary code.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/dff2c7c9ea53a9b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/dff2c7c9ea53a9b/output.txt

Total script time: 2.18 mins

Published

@timvandermeij timvandermeij merged commit a631147 into mozilla:master Jun 18, 2017
@timvandermeij
Copy link
Contributor

Nice find!

@Snuffleupagus Snuffleupagus deleted the rm-load-currentScale-call branch June 19, 2017 07:57
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Jun 20, 2017

It unfortunately appears that removing this one line uncovered existing issues elsewhere the code :-(

For example, if you load http://www.texample.net/media/pgf/builds/pgfmanualCVS2012-11-04.pdf#disableStream=true&disableAutoFetch=true.[1]
And then before the first page has begun rendering, either open the sidebar or change the scale to one of the predefined ones (page-fit/page-width/page-actual).

The pre-existing issue is that PDFViewer allows you to set various parameters; e.g. currentPageNumber, currentScale, currentScaleValue, and pagesRotation; before the setDocument method has been called. Edit: Also, note how PDFViewer.setDocument already calls _resetView to reset various state, but only if a previous document was loaded.
In particular, calling PDFViewer.currentScaleValue before setting the pdfDocument is especially problematic. For example, resizing the viewer (which is what opening the sidebar amounts to), may result in PDFViewer.currentScaleValue calls before the pdfDocument has loaded.
Prior to this PR, the pdfViewer.currentScale = scale; line thus ensured that the PDFViewer internal scale state was reset to a consistent state, but note that it actually depended on scale === 0 to work.

The following cases currently exists, and are problematic for different reasons:

  1. Calling PDFViewer.currentPageNumber before setDocument first of all cannot run the necessary validation of the value provided. Secondly, depending of whether or not the defaultRenderingQueue is used, there's a real risk that currentPageNumber won't be consistent with the actual rendered page after setDocument is called.
    I'm not certain that there's any simply solution to the above, and it's also a real risk that trying to add this._currentPageNumber validation code to setDocument could become quite messy!

  2. Calling PDFViewer.pagesRotation before setDocument will currently be ignored when creating PDFPageView instances, which means that the rotation in PDFViewer would be inconsistent with the one in PDFPageView.
    This is the only point that would be really easy to address, I've even got a WIP patch (see master...Snuffleupagus:viewer-components-rotation). Edit: Probably not as simple as it first seems, for pages with non-zero default rotation.

  3. Calling PDFViewer.currentScale before setDocument is the only thing that currently works.

  4. Calling PDFViewer.currentScaleValue before setDocument does not, and cannot work, since it means that _currentScale and _currentScaleValue will not be consistent.
    Normally, when calling PDFViewer.currentScaleValue the _setScale method will compute the correct scale corresponding to a predefined scale string. However, that cannot be done if pdfDocument is undefined, since it relies on the pages already being available, and we need to know the currentScale when creating the PDFPageView instances, in web/pdf_viewer.js#L354.

So, to try and summarize, there's a couple of methods that simply does not work if called before setDocument.
What I'm actually wondering is why we even need to support setting those properties at all, since if we just did if (!this.pdfDocument) { return; } in currentPageNumber/currentScale/currentScaleValue/pagesRotation, none of these issues would exist!?

Also, in the simpleviewer example, we're showing how e.g. the scale can be changed after setDocument is called (since the pagesinit event is dispatched at the end of setDocument).

A compromise would perhaps be to only allow currentScale if this.pdfDocument isn't defined yet, but the inconsistency doesn't seem very user friendly to me!


@timvandermeij, @yurydelendik How do you think that we should proceed here, to solve the underlying issue this PR uncovered?
Can we simply return early in the various setters discussed above if setDocument hasn't been called, or do need to try and support at least some of them?


[1] The debugging parameters are merely intended to slow down loading somewhat, such that you have more time to do the next step.

@timvandermeij
Copy link
Contributor

I personally think we should return early and not allow setting any of these properties before setDocument is called. Doing so doesn't make sense in my opinion and it complicates the code a lot, leading to these kind of subtle issues that are hard to find beforehand. I'm not sure why one would want to set these properties before actually setting a document to apply them on, so I'm more than fine with disallowing that in favor of cleaner and more predictable code.

apoorv-mishra pushed a commit to apoorv-mishra/pdf.js that referenced this pull request Jul 6, 2017
…currentScale`/`currentScaleValue`/`pagesRotation`, before `{PDFViewer, PDFThumbnailViewer}.setDocument` has been called

Currently a number of these properties do not work correctly if set *before* calling `setDocument`; please refer to the discussion starting in mozilla#8539 (comment).

Rather than trying to have *some* of these methods working, but not others, it seems much more consistent to simply always require that `setDocument` has been called.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 29, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent/confusing state.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 29, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent/confusing state.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 29, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 29, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 30, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Jun 30, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…le-call

Remove a redundant `PDFViewer.currentScale` call from `PDFViewerApplication.load`
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…currentScale`/`currentScaleValue`/`pagesRotation`, before `{PDFViewer, PDFThumbnailViewer}.setDocument` has been called

Currently a number of these properties do not work correctly if set *before* calling `setDocument`; please refer to the discussion starting in mozilla#8539 (comment).

Rather than trying to have *some* of these methods working, but not others, it seems much more consistent to simply always require that `setDocument` has been called.
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants