Annotations

[jpallen]

I've been looking at implementing hyperlinks within PdfListView. External links are easy, since we just get the href and turn it into a normal HTML hyperlink. Internal links are a bit trickier because they need to change the view of the top level ListView. At the moment I've created an AnnotationsLayerBuilder, very similar in structure to TextLayerBuilder (see commit jpallen@3d121f2). This means it's nested in the stack like PdfListView -> ListView -> PageContainerView -> PageView -> Page -> AnnotationsLayerBuidler, where each class doesn't have access or knowledge about the class above. This is a problem however, since AnnotationsLayerBuilder needs to be able to call PdfListView to tell it to go to a certain destination.

One solution, that fits well with modular structure, would be to have custom AnnotationsLayerBuilder for each instance of PdfListView. I.e.

pdfListView = new PdfListView();
AnnotationsLayerBuilder = AnnotationLayerBuilderBuilder(pdfListView); # :(
pdfListView.listView.options.annotationLayerBuilder = AnnotationLayerBuilder;

Then the instances of AnnotationsLayerBuilder at the end of the chain will have knowledge about the top level PdfListView without needing to pass the PdfListView object all the way down the chain.

It's not pretty, but I can't think of a solution that is. Thoughts?

[jpallen]

Another possibility might be to have events which bubble back up the chain, but introducing events for this might be overkill if only a single feature needs it.

[jviereck]

The pageView knows about the listView, which contains the options passed to the PDFListView during construction.

Isn't the way the TextLayerBuilder is invoked in the current iteration exactly what you're looking for when constructing the AnnotationLayer?

https://github.com/jviereck/pdfListView/blob/38f71e4095d5158731bcd72e4f752b8d685fb08f/src/PdfListView.js#L490..L501

In case we end up with more layers, we should have an abstraction for them, such that you do:

var options = {
  layerBuilder: [TextLayerBuilder, AnnotationLayerBuilder]
}

and then call Page.prototype.render:

var layers = options.layerBuilder.map(function(builder) {
  return builder(pageView, pdfPage, renderContext);
})

But maybe this is over engeneering at this point and I'm happy if we have separate code paths at the beginning and clean up later on.

[jpallen]

The problem I'm having is that AnnotationLayerBuilder doesn't know about the top level PdfListView, which it needs to so that it can call a method like PdfListView::navigateTo when clicking on an internal link. I don't think you've touched on this in your reply.

Certainly we could do something like this within AnnotationLayerBuilder:

this.pageView.listView.pdfListView.nagivateTo(dest);

but that's breaking the nice encapsulation of each class.

I think it's unrelated to the problem, but I like the idea of having an abstraction, even for only two layers since the current code is not very DRY.

[jviereck]

this.pageView.listView.pdfListView.nagivateTo(dest);

Shouldn't the nagivateTo(dest); function be on the listView anyway? The PDFListView feels like the thing that "builds the components together and drives the logic", e.g. make the listView rerender etc. I'm fine if there is a PDFListView.prototype.nagivateTo(...) function which is just a wrapper for listView.nagivateTo(...).

I think it's unrelated to the problem, but I like the idea of having an abstraction, even for only two layers since the current code is not very DRY.

Agreed.

[jpallen]

I would agree about the ListView being the correct place for it, except at the moment the ListView doesn't know about the RenderController and so cannot call updateRenderList when the view changes. Perhaps ListView should know about the RenderController (I think that makes sense from an abstractions point of view). This would make PdfListView basically a redundant wrapper around ListView, and I don't think it be worth having the extra layer anymore. Thoughts?

[jpallen]

Actually, ignore my last comment about PdfListView being redundant. It still does quite a bit of instantiating that it makes sense to keep separate.

[jviereck]

I want to keep the separation. In PDF.JS we don't have this separation and that causes some problems.

How about this:

• we add a simple event system to this project
• the listView emits a "scroll" event whenever the dom is scrolled
• change https://github.com/jviereck/pdfListView/blob/master/src/PdfListView.js#L561 to use `this.listView.on('scroll', ...)
• calling listView.navigate(...) fires a scroll event