Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass imageLayer to PDFPageView #9681

Closed

Conversation

websirnik
Copy link

This pull request allows passing a custom imageLayer when creating a new PDFPageView.

var pdfPageView = new pdfjsViewer.PDFPageView({
    container: container,
    id: PAGE_TO_VIEW,
    scale: SCALE,
    defaultViewport: pdfPage.getViewport(SCALE),
    imageLayer: {
        beginLayout: function (){},
        endLayout: function (){},
        appendImage: function(img){}
    }
});

@timvandermeij
Copy link
Contributor

Could you elaborate a bit on the use case for this? I'm not really seeing why we should have this as a parameter in the library (it sounds like something that may be useful for a custom version, but not really for the library itself).

@@ -576,6 +577,7 @@ class PDFPageView {
viewport: this.viewport,
enableWebGL: this.enableWebGL,
renderInteractiveForms: this.renderInteractiveForms,
imageLayer: this.imageLayer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing trailing comma.

@@ -87,6 +87,7 @@ class PDFPageView {
this.renderingQueue = options.renderingQueue;
this.textLayerFactory = options.textLayerFactory;
this.annotationLayerFactory = options.annotationLayerFactory;
this.imageLayer = options.imageLayer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for PDFPageViewOptions above needs to be updated too.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jun 5, 2018

Could you elaborate a bit on the use case for this? I'm not really seeing why we should have this as a parameter in the library (it sounds like something that may be useful for a custom version, but not really for the library itself).

Isn't another problem also that the imageLayer functionality is both completely unused and untested as-is. Before surfacing this functionality more prominently, shouldn't e.g. unit-tests be added first such that the relevant code-paths are actually exercised!?

(Also, if a new option is added in PDFPageView, it really should be added to BaseViewer as well.)

@websirnik
Copy link
Author

imageLayer implementation was partly already in the codebase but was never finished. The use case for me was to collect images on a page via appendImage(img) and create a custom image zoom-in experience after.

Since the is no API call to get the images on a page, imageLayer feels like a good solution. I can add documentation & tests. Should I proceed?

@timvandermeij
Copy link
Contributor

Thank you for sharing the use case. I'm personally fine with this as long as there are (unit) tests for it so that we can make sure that it doesn't break in a future update.

@timvandermeij
Copy link
Contributor

I'm going to close this since it cannot be merged in the current state and there was no more activity. If you want to add tests and documentation and fix the linting issues, please open a new pull request for the patch so it can be reviewed again. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants