[api-minor] Remove the default factories, used to initialize various layers, in the viewer #15811
Conversation
Snuffleupagus
force-pushed
the
rm-web-default-factory
branch
2 times, most recently
from
e2b128d to
9c26a6e
Compare
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/e1efd4165691eb0/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/8d2ec2b6fbbff17/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/8d2ec2b6fbbff17/output.txt Total script time: 25.70 mins
Image differences available at: http://54.241.84.105:8877/8d2ec2b6fbbff17/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/e1efd4165691eb0/output.txt Total script time: 31.81 mins
Image differences available at: http://54.193.163.58:8877/e1efd4165691eb0/reftest-analyzer.html#web=eq.log |
|
I'm happy about getting rid of this what I also experienced as unnecessary indirection. I'll do my best to review this during the weekend. Am I correct that third-party users should now simply set the relevant I'm a little bit concerned about if this will break third party usage since we also had to edit the component examples here, so I'll have to think about if |
Thank you!
Correct, but please note that it only applies to the "pageviewer" example and that the "simpleviewer"/"singlepageviewer" examples are completely unaffected here. Lines 70 to 83 in 1ab711e
That's a fair comment; although it (as mentioned above) only applies to the "pageviewer" example.
OK, how about if I just add dummy factories in the |
Let's do that so that if this is being used (and fortunately that should be very limited given that only one of the examples is affected by this, and I therefore agree that this is |
…and into `PDFPageView` By moving this code the "pageviewer"-component example will become slightly more usable on its own, it may simplify a future addition of XFA Foreground document support, and finally also serves as preparation for the following patches.
Currently we'll only initialize and render the `annotationEditorLayer` once the regular `annotationLayer` has been rendered. While it obviously makes sense to render the `annotationEditorLayer` *last*, the way that the code is currently written means that if a third-party user disables the `annotationLayer` then the editing-functionality indirectly becomes disabled as well. Given that this seems like a somewhat arbitrary limitation, this patch simply decouples these two layers while still keeping the rendering order consistent.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `annotationEditorLayerFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `annotationLayerFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `structTreeLayerFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `textHighlighterFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `textLayerFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
Please note that this functionality has never really mattered for the Firefox PDF Viewer, the GENERIC viewer, or even the "simpleviewer"/"singlepageviewer" component-examples. Hence, in practice this means that only the "pageviewer" component-example[1] have ever really utilized this. Using factories to initialize various layers in the viewer, rather than simply invoking the relevant code directly, seems (at least to me) like a somewhat roundabout way of doing things. Not only does this lead to more code, both to write and maintain, but since many of the layers have common parameters (e.g. an `AnnotationStorage`-instance) there's also some duplication. Hence this patch, which removes the `xfaLayerFactory` and instead uses a lookup-function in the `PDFPageView`-class to access the external viewer-properties as necessary. Note that this should even be an improvement for the "pageviewer" component-example, since most layers will now work by default rather than require manual configuration. --- [1] In practice we generally suggest using the "simpleviewer", or "singlepageviewer", since it does *most* things out-of-the-box and given that a lot of functionality really require *a viewer* and not just a single page in order to work.
This uses a simple version number check, to prevent breakage if the example is used with older PDF.js versions.
Snuffleupagus
force-pushed
the
rm-web-default-factory
branch
from
9c26a6e to
d9cdc46
Compare
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/6cf574df125849d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/6cf574df125849d/output.txt Total script time: 1.25 mins Published |
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/93704e70ee9da6e/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/23d6d07720bbe76/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/93704e70ee9da6e/output.txt Total script time: 4.29 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/23d6d07720bbe76/output.txt Total script time: 11.97 mins
|
|
Looks really good to me; thank you for simplifying this! |
Thanks for the review! |
The purpose of this patch series is to reduce indirection, and the amount of code that we need to maintain, related to the various layers used in the viewer. Please refer to the individual commit messages for additional details.