-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Fixed #5927 - Now remembers page rotation on reload #5949
Conversation
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/4b6e97818553124/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/4b6e97818553124/output.txt Total script time: 0.78 mins Published |
@@ -1786,7 +1789,8 @@ window.addEventListener('updateviewarea', function () { | |||
'page': location.pageNumber, | |||
'zoom': location.scale, | |||
'scrollLeft': location.left, | |||
'scrollTop': location.top | |||
'scrollTop': location.top, | |||
'pageRotation': PDFViewerApplication.pdfViewer.pagesRotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should add rotation: this._pagesRotation
at pdf_viewer.js#L572 instead, and change this line to 'rotation': location.rotation
.
When testing this locally, and in the preview, the following is printed in the console:
|
@@ -902,6 +902,9 @@ var PDFViewerApplication = { | |||
var left = store.get('scrollLeft', '0'); | |||
var top = store.get('scrollTop', '0'); | |||
|
|||
self.pageRotation = store.get('pageRotation', '0'); | |||
self.pdfViewer.pagesRotation = self.pageRotation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this won't work, since it breaks the thumbnails (and it's probably related to the console error). I believe that you need to use PDFViewerApplication.rotatePages
instead, see viewer.js#L1326.
Edit: To avoid unnecessary operations, you should probably do e.g.
var rotation = store.get('rotation', '0');
if (rotation !== self.pageRotation) {
self.rotatePages(rotation);
}
Edit2: Also, just calling PDFViewerApplication.rotatePages
here would cause unnecessary rendering/scrolling, so we might want to modify that function slightly to be able to avoid that.
A more general question: I'm well aware that rotation isn't specified in the "Open parameters", but do we want to expose it as a hash parameter? |
I made all of the changes mentioned. I changed the rotatePages: function pdfViewRotatePages(delta) {
this.pageRotation = (this.pageRotation + 360 + delta) % 360;
this.pdfViewer.pagesRotation = this.pageRotation;
this.pdfThumbnailViewer.pagesRotation = this.pageRotation;
this.forceRendering();
} As for the error. It occurs because I can't see where that is being set to '0'. |
@Snuffleupagus I think exposing the rotation as a parameter couldn't hurt. It would also make the code consistent here viewer.js#905. We could simply add rotation into |
@@ -197,7 +197,6 @@ var PDFViewer = (function pdfViewer() { | |||
var page = this.pages[i]; | |||
page.update(page.scale, rotation); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to the patch, please revert it.
I'm sorry, but changing it in this way will first of all break other functionality, and second of all not actually achieve the desired goal! What you should do is e.g. something like this: rotatePages: function pdfViewRotatePages(delta, noRendering) {
var pageNumber = this.page;
this.pageRotation = (this.pageRotation + 360 + delta) % 360;
this.pdfViewer.pagesRotation = this.pageRotation;
this.pdfThumbnailViewer.pagesRotation = this.pageRotation;
if (!noRendering) {
this.forceRendering();
this.pdfViewer.scrollPageIntoView(pageNumber);
}
}, Then you can call
You are probably trying to change the rotation before the document/viewer has been properly initialized. |
@Snuffleupagus updated to code to your specifications moved to code to after init so no more error. |
Unfortunately I don't think that is going to work correctly either, since I still believe that you need to set the rotation before |
@@ -901,6 +902,12 @@ var PDFViewerApplication = { | |||
store.get('zoom', self.pdfViewer.currentScale); | |||
var left = store.get('scrollLeft', '0'); | |||
var top = store.get('scrollTop', '0'); | |||
rotation = store.get('rotation', '0'); | |||
|
|||
self.setScale(DEFAULT_SCALE, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am now setting the scale and moved the code back to its original spot. There is no error. Can this be considered a viable solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be considered a viable solution?
In my opinion, no. This patch tries to add a minor feature, hence I don't think that unconditionally triggering an unnecessary rendering operation during every document load is a particularly good thing to do.
Given that the error isn't really a breaking error, but just a console message, I'd remove this line and try something else instead: Try changing https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.js#L201 to
if (this._currentScale !== UNKNOWN_SCALE)
this._setScale(this._currentScaleValue, true);
}
If this seem to work: please make sure that you test the patch thoroughly, to ensure first of all that it does not regress other functionality, and second of all that the rotation works as intended!
updated and tested. seems to work fine with no errors |
var rotation = store.get('rotation', '0') | 0; | ||
|
||
if (rotation !== self.pageRotation) { | ||
self.rotatePages(rotation,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add a space after the ,
, i.e., self.rotatePages(rotation, true);
Optimized code for rotation bug
@Snuffleupagus Can you have a look and maybe merge this? |
@@ -198,7 +198,9 @@ var PDFViewer = (function pdfViewer() { | |||
page.update(page.scale, rotation); | |||
} | |||
|
|||
this._setScale(this._currentScaleValue, true); | |||
if (this._currentScale !== UNKNOWN_SCALE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed?
Needs a rebase and comments addressed. |
Closing in favor of #6980. |
Fix for #5927