Convert the overlay manager to a class #8119
Conversation
This work makes the code similar to the other components and reduces the number of dependencies for those components because the overlay manager is no longer a global.
|
/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/a4540a2e1621378/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/a4540a2e1621378/output.txt Total script time: 2.33 mins Published |
|
/botio lint |
From: Bot.io (Linux)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/3a3aa8643aa4a40/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/112b3b739bf8a52/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/3a3aa8643aa4a40/output.txt Total script time: 1.73 mins
|
There was a problem hiding this comment.
I can't help wondering if it wouldn't be better to try and get Babel running as part of the builds, and then just convert the OverlayManager directly to a proper ES6 class instead since that'd reduce the churn in this code considerably (e.g. it'd remove the need for most indentation changes).
Edit: Using a basic ES6 adjusted .eslintrc file, such as this diff, the total diff for the OverlayManager would then be reduced to something like this.
| @@ -171,6 +168,7 @@ | |||
|
|
|||
| var chromeFileAccessOverlayPromise; | |||
| function requestAccessToLocalFile(fileUrl) { | |||
| var overlayManager = PDFViewerApplication.overlayManager; | |||
There was a problem hiding this comment.
Nit: I'm wondering if it might not be a bit cleaner to just have this definition at the top of the file instead (like it currently is).
| @@ -133,10 +132,11 @@ | |||
| this.scratchCanvas = null; | |||
| activeService = null; | |||
| ensureOverlay().then(function () { | |||
| if (OverlayManager.active !== 'printServiceOverlay') { | |||
| var overlayManager = PDFViewerApplication.overlayManager; | |||
There was a problem hiding this comment.
Nit: I'm wondering if it might not be a bit cleaner to just have this definition at the top of the file instead (like it currently is, since that'd reduce the total diff a bit in this file).
| this.overlays[this.active].element.classList.remove('hidden'); | ||
| this.overlays[this.active].container.classList.remove('hidden'); | ||
|
|
||
| window.addEventListener('keydown', this._keyDown.bind(this)); |
There was a problem hiding this comment.
Unfortunately you'll not be able to remove the event listener if you define it like this.
You'll need to add this._keyDownBound = this._keyDown.bind(this); to e.g. the constructor and then use this._keyDownBound here (and below).
|
Closing. I agree that it's better to do this once ES6 is up and running as that will make the change quite a bit smaller. Hopefully the UMD headers will be gone/reduced at that point as well. |
|
@timvandermeij If you've got time, now might be a good time to revisit this PR :-) Given that https://bugzilla.mozilla.org/show_bug.cgi?id=1167472 (and its dependant bugs) is still open, we may not want to use ES6 classes in parsing/rendering code for the time being (i.e. not in the However, having a couple of (smaller) ES6 classes in |
This blocked by #8287 |
|
Thank you for reminding me, @Snuffleupagus, and thank you both for the amazing ES6 work! I think I can get to this really soon. |
@timvandermeij Gentle ping, since it would be nice to get fixed considering that the |
|
I looked at this some time ago, but got stuck because the overlay manager has to be initialized in a central place (like |
At what point? Looks like initPassiveLoading is called from webViewerInitialized, which is called after we initialized UI. |
The first thing that comes to mind is perhaps not conceptually ideal, considering how we initialize all the other viewer components, but it's quite simple: |
|
Right, I think that would work. Perhaps it's not the best solution, but it would allow us to rewrite that code and improve this part later. Hopefully I have time this weekend to refactor the overlay manager, which could lead to more insights on why things did not work previously. It has been a while, so I don't remember exactly at what point the initialization was not complete yet. I'll have to look into that again. |
|
As a tip, try not to use PDFViewerApplication too often, cache its properties somewhere once (e.g. at https://github.com/timvandermeij/pdf.js/blob/ac1b78628d16322a95c11f8662057a373905876b/web/chromecom.js#L353 |
This work makes the code similar to the other components and reduces the number of dependencies for those components because the overlay manager is no longer a global.
I kept the
FIXMEcomments equal to the already existing ones as the way we amend configuration objects should be improved in a follow-up. This way it is at least clear which parts should be refactored.Much easier to review with https://github.com/mozilla/pdf.js/pull/8119/files?w=1.