Refactor viewer code to fix issues with the "pageviewer" components example (issue 6056) #6110
Conversation
|
Can we squash the second and third commit to reduce the commit count somewhat? They seem very much related. |
Sure, I'll do that once the PR has been reviewed and is ready to land. |
Currently in `PDFViewer_scrollPageIntoView`, we're accessing a number of properties in an indirect and overly complicated way. In particular, using `this.linkService.page` is a *very* roundabout way to access `this.currentPageNumber`. The reason for this appears to be entirely historical, since prior to PR 5361 the code was placed in `PDFPageView` (or `PageView` as it was called at the time).
…stance in DefaultAnnotationsLayerFactory Considering that most methods of `SimpleLinkService` are complete stubs, or practically "useless" considering what they return, we can actually simplify it even more. *Note:* This depends on the previous patch, that did a small amount of refactoring of `PDFViewer_scrollPageIntoView`, since `PDFViewer.linkService.page` is no longer accessed. ---------- Currently the `pageviewer` components example doesn't work correctly (an error is printed in the console), since no `linkService` is present when the `AnnotationsLayerBuilder` is created. *Note:* Given that this uses the `SimpleLinkService`, clicking on e.g. internal links won't actually do anything. However, given that internal links (and similar features) are pretty much useless when only *one* page is loaded the `pageviewer` example, I don't think that really matters. Also, using the complete `PDFLinkService` would require a `PDFViewer` instance. That would significantly complicate the example, thus making it both less clear and less self contained.
This is similar to the already existing, separate, CSS file used for the `textLayer`, and it's necessary in order for the `pageviewer` components example to actually show annotations correctly.
|
@timvandermeij I've squashed the commits as you asked, and after talking to Yury on IRC I'm assigning the final review to you! /botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 2 Live output at: http://107.21.233.14:8877/e91bfb138b214ed/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/e91bfb138b214ed/output.txt Total script time: 0.65 mins Published |
|
I'm having trouble verifying this patch. If I apply this patch and run the pageviewer example with the test file provided in #6056 I get
Any idea why that is happening? |
|
Strange, it works for me. Perhaps a stupid question, but did you remember to do |
|
Well, that was a silly mistake. I had built the components before applying the patch, but forgot to rebuild them after applying the patch. Now it does work for me, so I will continue the review. Thanks! |
|
Review progress:
|
Refactor viewer code to fix issues with the "pageviewer" components example (issue 6056)
|
Nice work! Moving the annotation-related CSS rules to a separate file is useful for my annotation layer work too. |
Fixes #6056.
When working of addressing the issue, it quickly became apparent that a number of (smaller) things needed to be re-factored in order for it to work. Hence the use of multiple commits (despite the total size of the PR), to make it easier to follow the reasoning.
Please refer to the individual commit messages for a more detailed description of the patches.