[api-minor] Move the viewer scripting initialization/handling into a new PDFScriptingManager class
#13042
Conversation
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/049a274195b771a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/c92e05e732828fd/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/049a274195b771a/output.txt Total script time: 3.09 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/c92e05e732828fd/output.txt Total script time: 5.93 mins
|
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
4 times, most recently
from
398e68a to
7560fef
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/11a0934514d9d65/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/46f41697f8a4e95/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/11a0934514d9d65/output.txt Total script time: 3.09 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/46f41697f8a4e95/output.txt Total script time: 4.63 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e7c4c0b25eb78c0/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e7c4c0b25eb78c0/output.txt Total script time: 4.10 mins Published |
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
from
7560fef to
b66be0a
Compare
|
We could most likely simplify (or possibly even remove) the |
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
from
b66be0a to
46dcacc
Compare
|
Perhaps it would be good to split this up a bit, for example first a PR with the first two commits and maybe the |
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
from
46dcacc to
e6e838a
Compare
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
3 times, most recently
from
3031fca to
b58c44a
Compare
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/057838ac87c1c3d/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/8d9a63b7e8e48d9/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/8d9a63b7e8e48d9/output.txt Total script time: 3.10 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/057838ac87c1c3d/output.txt Total script time: 5.02 mins
|
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
3 times, most recently
from
064dbb2 to
afa6ad1
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/683363d36997428/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/214c5978482e589/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/683363d36997428/output.txt Total script time: 3.15 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/214c5978482e589/output.txt Total script time: 5.21 mins
|
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
from
afa6ad1 to
ac2d2df
Compare
…`web/app.js` and into `web/ui_utils.js` These changes will be necessary for the next patch, since we don't want to accidentally pull in the entire default viewer in the standalone viewer components.
…new `PDFScriptingManager` class The *main* purpose of this patch is to allow scripting to be used together with the viewer components, note the updated "simpleviewer"/"singlepageviewer" examples, in addition to the full default viewer. Given how the scripting functionality is currently implemented in the default viewer, trying to re-use this with the standalone viewer components would be *very* hard and ideally you'd want it to work out-of-the-box. For an initial implementation, in the default viewer, of the scripting functionality it probably made sense to simply dump all of the code in the `app.js` file, however that cannot be used with the viewer components. To address this, the functionality is moved into a new `PDFScriptingManager` class which can thus be handled in the same way as all other viewer components (and e.g. be passed to the `BaseViewer`-implementations). Obviously the scripting functionality needs quite a lot of data, during its initialization, and for the default viewer we want to maintain the current way of doing the lookups since that helps avoid a number of redundant API-calls. To that end, the `PDFScriptingManager` implementation accepts (optional) factories/functions such that we can maintain the current behaviour for the default viewer. For the viewer components specifically, fallback code-paths are provided to ensure that scripting will "just work"[1]. Besides moving the viewer handling of the scripting code to its own file/class, this patch also takes the opportunity to re-factor the functionality into a number of helper methods to improve overall readability[2]. Note that it's definitely possible that the `PDFScriptingManager` class could be improved even further (e.g. for general re-use), since it's still heavily tailored to the default viewer use-case, however I believe that this patch is still a good step forward overall. --- [1] Obviously *all* the relevant document properties might not be available in the viewer components use-case (e.g. the various URLs), but most things should work just fine. [2] The old `PDFViewerApplication._initializeJavaScript` method, where everything was simply inlined, have over time (in my opinion) become quite large and somewhat difficult to *easily* reason about.
Snuffleupagus
force-pushed
the
PDFScriptingManager
branch
from
ac2d2df to
a6d1cba
Compare
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/5cb8aa933c0e45e/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/875003d48002ba7/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/5cb8aa933c0e45e/output.txt Total script time: 3.10 mins
|
|
/botio-linux integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/5cdd9972887ca8d/output.txt |
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/875003d48002ba7/output.txt Total script time: 6.27 mins
|
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/5cdd9972887ca8d/output.txt Total script time: 3.09 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/191620b2c08468c/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/191620b2c08468c/output.txt Total script time: 4.13 mins Published |
|
Looks great; thank you for doing this! |
The main purpose of this patch is to allow scripting to be used together with the viewer components, note the updated "simpleviewer"/"singlepageviewer" examples, in addition to the full default viewer.
Given how the scripting functionality is currently implemented in the default viewer, trying to re-use this with the standalone viewer components would be very hard and ideally you'd want it to work out-of-the-box.
For an initial implementation, in the default viewer, of the scripting functionality it probably made sense to simply dump all of the code in the
app.jsfile, however that cannot be used with the viewer components.To address this, the functionality is moved into a new
PDFScriptingManagerclass which can thus be handled in the same way as all other viewer components (and e.g. be passed to theBaseViewer-implementations).Obviously the scripting functionality needs quite a lot of data, during its initialization, and for the default viewer we want to maintain the current way of doing the lookups since that helps avoid a number of redundant API-calls.
To that end, the
PDFScriptingManagerimplementation accepts (optional) factories/functions such that we can maintain the current behaviour for the default viewer. For the viewer components specifically, fallback code-paths are provided to ensure that scripting will "just work"[1].Besides moving the viewer handling of the scripting code to its own file/class, this patch also takes the opportunity to re-factor the functionality into a number of helper methods to improve overall readability[2].
Note that it's definitely possible that the
PDFScriptingManagerclass could be improved even further (e.g. for general re-use), since it's still heavily tailored to the default viewer use-case, however I believe that this patch is still a good step forward overall.[1] Obviously all the relevant document properties might not be available in the viewer components use-case (e.g. the various URLs), but most things should work just fine.
[2] The old
PDFViewerApplication._initializeJavaScriptmethod, where everything was simply inlined, have over time (in my opinion) become quite large and somewhat difficult to easily reason about.