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.

Sign up for GitHub

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

Display a notification on the sidebarToggle button for PDF documents with outline/attachments #7959

Merged
merged 2 commits into from
Feb 1, 2017
Merged

Display a notification on the sidebarToggle button for PDF documents with outline/attachments #7959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
616e5fa
Display a notification on the `sidebarToggle` button for PDF document…
Snuffleupagus Jan 15, 2017
c102232
Append the contents of `FileAttachment` annotations to the attachment…
Snuffleupagus Jan 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions l10n/en-US/viewer.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ print_progress_close=Cancel
# (the _label strings are alt text for the buttons, the .title strings are
# tooltips)
toggle_sidebar.title=Toggle Sidebar
toggle_sidebar_notification.title=Toggle Sidebar (document contains outline/attachments)
toggle_sidebar_label=Toggle Sidebar
document_outline.title=Show Document Outline (double-click to expand/collapse all items)
document_outline_label=Document Outline
Expand Down
1 change: 1 addition & 0 deletions l10n/sv-SE/viewer.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ print_progress_close=Avbryt
# (the _label strings are alt text for the buttons, the .title strings are
# tooltips)
toggle_sidebar.title=Visa/dölj sidofält
toggle_sidebar_notification.title=Visa/dölj sidofält (dokumentet innehåller disposition/bilagor)
toggle_sidebar_label=Visa/dölj sidofält
document_outline.title=Visa dokumentdisposition (dubbelklicka för att expandera/komprimera alla objekt)
document_outline_label=Dokumentöversikt
Expand Down
18 changes: 12 additions & 6 deletions src/display/annotation_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

var AnnotationBorderStyleType = sharedUtil.AnnotationBorderStyleType;
var AnnotationType = sharedUtil.AnnotationType;
var stringToPDFString = sharedUtil.stringToPDFString;
var Util = sharedUtil.Util;
var addLinkAttributes = displayDOMUtils.addLinkAttributes;
var LinkTarget = displayDOMUtils.LinkTarget;
Expand Down Expand Up @@ -1007,8 +1008,15 @@ var FileAttachmentAnnotationElement = (
function FileAttachmentAnnotationElement(parameters) {
AnnotationElement.call(this, parameters, true);

this.filename = getFilenameFromUrl(parameters.data.file.filename);
this.content = parameters.data.file.content;
var file = this.data.file;
this.filename = getFilenameFromUrl(file.filename);
this.content = file.content;

this.linkService.onFileAttachmentAnnotation({
id: stringToPDFString(file.filename),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why we don't use this.filename and this.content in these three lines instead. Especially the file name has been sanitized then.

Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a bit confused why we don't use this.filename and this.content in these three lines instead.

I wanted to make sure that what we (eventually) pass on to pdf_attachment_viewer.js has the exact same format as what PDFDocumentProxy_getAttachments returns, by explicitly using the file attachment data from the API side here, to avoid any possible issues/surprises later on.

Especially the file name has been sanitized then.

Please note that we already need to use getFilenameFromUrl in pdf_attachment_viewer.js, so using it here as well does not help us much and again wouldn't be compatible with what we get from PDFDocumentProxy_getAttachments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the intention now, thanks!

filename: file.filename,
content: file.content,
});
}

Util.inherit(FileAttachmentAnnotationElement, AnnotationElement, {
Expand Down Expand Up @@ -1086,8 +1094,7 @@ var AnnotationLayer = (function AnnotationLayerClosure() {
if (!data) {
continue;
}

var properties = {
var element = annotationElementFactory.create({
data: data,
layer: parameters.div,
page: parameters.page,
Expand All @@ -1097,8 +1104,7 @@ var AnnotationLayer = (function AnnotationLayerClosure() {
imageResourcesPath: parameters.imageResourcesPath ||
getDefaultSetting('imageResourcesPath'),
renderInteractiveForms: parameters.renderInteractiveForms || false,
};
var element = annotationElementFactory.create(properties);
});
if (element.isRenderable) {
parameters.div.appendChild(element.render());
}
Expand Down
1 change: 1 addition & 0 deletions src/main_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
exports.renderTextLayer = displayTextLayer.renderTextLayer;
exports.AnnotationLayer = displayAnnotationLayer.AnnotationLayer;
exports.CustomStyle = displayDOMUtils.CustomStyle;
exports.createPromiseCapability = sharedUtil.createPromiseCapability;
exports.PasswordResponses = sharedUtil.PasswordResponses;
exports.InvalidPDFException = sharedUtil.InvalidPDFException;
exports.MissingPDFException = sharedUtil.MissingPDFException;
Expand Down
2 changes: 2 additions & 0 deletions src/pdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
exports.AnnotationLayer =
pdfjsLibs.pdfjsDisplayAnnotationLayer.AnnotationLayer;
exports.CustomStyle = pdfjsLibs.pdfjsDisplayDOMUtils.CustomStyle;
exports.createPromiseCapability =
pdfjsLibs.pdfjsSharedUtil.createPromiseCapability;
exports.PasswordResponses = pdfjsLibs.pdfjsSharedUtil.PasswordResponses;
exports.InvalidPDFException = pdfjsLibs.pdfjsSharedUtil.InvalidPDFException;
exports.MissingPDFException = pdfjsLibs.pdfjsSharedUtil.MissingPDFException;
Expand Down
14 changes: 13 additions & 1 deletion test/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ var LinkServiceMock = (function LinkServiceMockClosure() {
function LinkServiceMock() {}

LinkServiceMock.prototype = {
get page() {
return 0;
},

set page(value) {},

navigateTo: function (dest) {},

getDestinationHash: function (dest) {
Expand All @@ -36,7 +42,13 @@ var LinkServiceMock = (function LinkServiceMockClosure() {
return '#';
},

executeNamedAction: function (action) {}
setHash: function (hash) {},

executeNamedAction: function (action) {},

onFileAttachmentAnnotation: function (params) {},

cachePageRef: function (pageNum, pageRef) {},
};

return LinkServiceMock;
Expand Down
52 changes: 47 additions & 5 deletions web/pdf_attachment_viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,26 @@ var PDFAttachmentViewer = (function PDFAttachmentViewerClosure() {
this.container = options.container;
this.eventBus = options.eventBus;
this.downloadManager = options.downloadManager;

this._renderedCapability = pdfjsLib.createPromiseCapability();
this.eventBus.on('fileattachmentannotation',
this._appendAttachment.bind(this));
}

PDFAttachmentViewer.prototype = {
reset: function PDFAttachmentViewer_reset() {
reset: function PDFAttachmentViewer_reset(keepRenderedCapability) {
this.attachments = null;

var container = this.container;
while (container.firstChild) {
container.removeChild(container.firstChild);
}

if (!keepRenderedCapability) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate a bit on why we need the keepRenderedCapability option? It's not entirely clear to me what its use is.

Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure that we're on the same page, a bit of background first:
I wanted to ensure that what PDFDocumentProxy_getAttachments returns always takes precedence over any file attachments, to not change the current behavior. This is why the _renderedCapability is used, to ensure that we wait for the "regular" attachments to load before attempting to add any existing file attachments.

Now, when resetting PDFAttachmentViewer we generally want to replace the _renderedCapability to avoid the situation where opening a new document (in the viewer) could lead to file attachments from the previous document being applied to the new one (given the async nature of Promises).
However, since the new _appendAttachments method calls render which will reset the attachment view (see pdf_attachment_viewer.js#L96) to ensure that everything is rendered correctly and in alphabetic order, replacing the _rendererCapability here could cause pending _appendAttachments calls to be incorrectly skipped.
Hence why we need to keep the _renderedCapability in this case.

Since this isn't an option that anyone should pass in when using PDFAttachmentViewer.reset(); from the outside (as in e.g.

pdf.js/web/app.js

Line 595 in a917443

close: function pdfViewClose() {
), I didn't want to add JSDocs for it. However, I'll try to add some sort of inline comment here, to briefly explain why it's there.

Sorry, this became a bit long. Hopefully it's enough to answer your question!

Edit: It just occurred to me that keepRenderedCapability should probably be an option on render instead, and not unconditionally set to true when calling reset from within render.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I now understand why we need to have this. Indeed, an inline comment (and no JSDoc comment) would be helpful to avoid any confusion. I'm also fine with moving the option to render instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, an inline comment (and no JSDoc comment) would be helpful to avoid any confusion. I'm also fine with moving the option to render instead.

Addressed in the latest version of the PR; please do let me know if the comment isn't clear enough!

// NOTE: The *only* situation in which the `_renderedCapability` should
// not be replaced is when appending file attachment annotations.
this._renderedCapability = pdfjsLib.createPromiseCapability();
}
},

/**
Expand All @@ -70,8 +80,10 @@ var PDFAttachmentViewer = (function PDFAttachmentViewerClosure() {
function PDFAttachmentViewer_dispatchEvent(attachmentsCount) {
this.eventBus.dispatch('attachmentsloaded', {
source: this,
attachmentsCount: attachmentsCount
attachmentsCount: attachmentsCount,
});

this._renderedCapability.resolve();
},

/**
Expand All @@ -89,11 +101,13 @@ var PDFAttachmentViewer = (function PDFAttachmentViewerClosure() {
* @param {PDFAttachmentViewerRenderParameters} params
*/
render: function PDFAttachmentViewer_render(params) {
var attachments = (params && params.attachments) || null;
params = params || {};
var attachments = params.attachments || null;
var attachmentsCount = 0;

if (this.attachments) {
this.reset();
var keepRenderedCapability = params.keepRenderedCapability === true;
this.reset(keepRenderedCapability);
}
this.attachments = attachments;

Expand All @@ -120,7 +134,35 @@ var PDFAttachmentViewer = (function PDFAttachmentViewerClosure() {
}

this._dispatchEvent(attachmentsCount);
}
},

/**
* Used to append FileAttachment annotations to the sidebar.
* @private
*/
_appendAttachment: function PDFAttachmentViewer_appendAttachment(item) {
this._renderedCapability.promise.then(function (id, filename, content) {
var attachments = this.attachments;

if (!attachments) {
attachments = Object.create(null);
} else {
for (var name in attachments) {
if (id === name) {
return; // Ignore the new attachment if it already exists.
}
}
}
attachments[id] = {
filename: filename,
content: content,
};
this.render({
attachments: attachments,
keepRenderedCapability: true,
});
}.bind(this, item.id, item.filename, item.content));
},
};

return PDFAttachmentViewer;
Expand Down
16 changes: 16 additions & 0 deletions web/pdf_link_service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,18 @@ var PDFLinkService = (function PDFLinkServiceClosure() {
});
},

/**
* @param {Object} params
*/
onFileAttachmentAnnotation: function (params) {
this.eventBus.dispatch('fileattachmentannotation', {
source: this,
id: params.id,
filename: params.filename,
content: params.content,
});
},

/**
* @param {number} pageNum - page number.
* @param {Object} pageRef - reference to the page.
Expand Down Expand Up @@ -462,6 +474,10 @@ var SimpleLinkService = (function SimpleLinkServiceClosure() {
* @param {string} action
*/
executeNamedAction: function (action) {},
/**
* @param {Object} params
*/
onFileAttachmentAnnotation: function (params) {},
/**
* @param {number} pageNum - page number.
* @param {Object} pageRef - reference to the page.
Expand Down
Loading