From 55c6de23ba124b6b1df77b323fb6e8fac99e660c Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sun, 27 Dec 2020 20:03:48 -0500 Subject: [PATCH] Add "Include Annotations" checkbox to export options dialog This changes the attachment saveFile() function in translators to be async. In order for errors to be properly caught, translators will need to be changed to make doExport() async and await on saveFile() calls. (The translation architecture theoretically already allows doExport() to be async.) --- chrome/content/zotero/exportOptions.js | 55 ++++++++++++++++--- chrome/content/zotero/exportOptions.xul | 2 + .../zotero/xpcom/translation/translate.js | 10 +++- .../xpcom/translation/translate_item.js | 43 +++++++++------ chrome/locale/en-US/zotero/zotero.properties | 1 + scss/_zotero-react-client.scss | 1 + scss/components/_exportOptions.scss | 10 ++++ test/tests/translateTest.js | 9 ++- 8 files changed, 102 insertions(+), 29 deletions(-) create mode 100644 scss/components/_exportOptions.scss diff --git a/chrome/content/zotero/exportOptions.js b/chrome/content/zotero/exportOptions.js index cfe66baea9..9919f9915f 100644 --- a/chrome/content/zotero/exportOptions.js +++ b/chrome/content/zotero/exportOptions.js @@ -34,8 +34,6 @@ const OPTION_PREFIX = "export-option-"; // Class to provide options for export var Zotero_File_Interface_Export = new function() { - this.init = init; - this.updateOptions = updateOptions; this.accept = accept; this.cancel = cancel; @@ -44,7 +42,7 @@ var Zotero_File_Interface_Export = new function() { /* * add options to export */ - function init() { + this.init = function () { // Set font size from pref var sbc = document.getElementById('zotero-export-options-container'); Zotero.setFontSize(sbc); @@ -82,10 +80,25 @@ var Zotero_File_Interface_Export = new function() { // right now, option interface supports only boolean values, which // it interprets as checkboxes if(typeof(translators[i].displayOptions[option]) == "boolean") { - var checkbox = document.createElement("checkbox"); + let checkbox = document.createElement("checkbox"); checkbox.setAttribute("id", OPTION_PREFIX+option); checkbox.setAttribute("label", optionLabel); optionsBox.insertBefore(checkbox, charsetBox); + + // Add "Include Annotations" after "Export Files" + if (option == 'exportFileData') { + checkbox.onclick = () => { + setTimeout(() => this.updateAnnotationsCheckbox()); + }; + + checkbox = document.createElement("checkbox"); + checkbox.setAttribute("id", OPTION_PREFIX + 'includeAnnotations'); + checkbox.setAttribute( + "label", + Zotero.getString('exportOptions.includeAnnotations') + ); + optionsBox.insertBefore(checkbox, charsetBox); + } } addedOptions[option] = true; @@ -108,13 +121,13 @@ var Zotero_File_Interface_Export = new function() { _charsets = Zotero_Charset_Menu.populate(document.getElementById(OPTION_PREFIX+"exportCharset"), true); } - updateOptions(Zotero.Prefs.get("export.translatorSettings")); + this.updateOptions(Zotero.Prefs.get("export.translatorSettings")); } /* * update translator-specific options */ - function updateOptions(optionString) { + this.updateOptions = function (optionString) { // get selected translator var index = document.getElementById("format-menu").selectedIndex; var translatorOptions = window.arguments[0].translators[index].displayOptions; @@ -133,7 +146,9 @@ var Zotero_File_Interface_Export = new function() { var node = optionsBox.childNodes[i]; // skip non-options if(node.id.length <= OPTION_PREFIX.length - || node.id.substr(0, OPTION_PREFIX.length) != OPTION_PREFIX) { + || node.id.substr(0, OPTION_PREFIX.length) != OPTION_PREFIX + // Handled separately by updateAnnotationsCheckbox() + || node.id == 'export-option-includeAnnotations') { continue; } @@ -161,6 +176,10 @@ var Zotero_File_Interface_Export = new function() { } } + this.updateAnnotationsCheckbox( + (options && options.includeAnnotations) ? options.includeAnnotations : false + ); + // handle charset popup if(_charsets && translatorOptions && translatorOptions.exportCharset) { optionsBox.hidden = undefined; @@ -181,6 +200,21 @@ var Zotero_File_Interface_Export = new function() { window.sizeToContent(); } + this.updateAnnotationsCheckbox = function (defaultValue) { + var filesCheckbox = document.getElementById(OPTION_PREFIX + 'exportFileData'); + var annotationsCheckbox = document.getElementById(OPTION_PREFIX + 'includeAnnotations'); + if (filesCheckbox.hidden) { + annotationsCheckbox.hidden = true; + annotationsCheckbox.checked = false; + return; + } + annotationsCheckbox.hidden = false; + annotationsCheckbox.disabled = !filesCheckbox.checked; + if (defaultValue !== undefined) { + annotationsCheckbox.checked = defaultValue; + } + }; + /* * make option array reflect status */ @@ -210,6 +244,13 @@ var Zotero_File_Interface_Export = new function() { } } + // If "Export Files" is shown, add "Include Annotations" checkbox value + if (optionsAvailable && optionsAvailable.exportFileData !== undefined) { + let elem1 = document.getElementById(OPTION_PREFIX + 'exportFileData'); + let elem2 = document.getElementById(OPTION_PREFIX + 'includeAnnotations'); + displayOptions.includeAnnotations = elem1.checked && elem2.checked; + } + // save options var optionString = JSON.stringify(displayOptions); Zotero.Prefs.set("export.translatorSettings", optionString); diff --git a/chrome/content/zotero/exportOptions.xul b/chrome/content/zotero/exportOptions.xul index 8584fa6164..28d21a18e9 100644 --- a/chrome/content/zotero/exportOptions.xul +++ b/chrome/content/zotero/exportOptions.xul @@ -1,5 +1,7 @@ + + %zoteroDTD; diff --git a/chrome/content/zotero/xpcom/translation/translate.js b/chrome/content/zotero/xpcom/translation/translate.js index 5d44d57154..999126795d 100644 --- a/chrome/content/zotero/xpcom/translation/translate.js +++ b/chrome/content/zotero/xpcom/translation/translate.js @@ -2541,8 +2541,14 @@ Zotero.Translate.Export.prototype._prepareTranslation = Zotero.Promise.method(fu function rest() { // export file data, if requested - if(this._displayOptions["exportFileData"]) { - this.location = this._itemGetter.exportFiles(this.location, this.translator[0].target); + if (this._displayOptions.exportFileData) { + this.location = this._itemGetter.exportFiles( + this.location, + this.translator[0].target, + { + includeAnnotations: this._displayOptions.includeAnnotations + } + ); } // initialize IO diff --git a/chrome/content/zotero/xpcom/translation/translate_item.js b/chrome/content/zotero/xpcom/translation/translate_item.js index 1a9b029bc7..92bad615a1 100644 --- a/chrome/content/zotero/xpcom/translation/translate_item.js +++ b/chrome/content/zotero/xpcom/translation/translate_item.js @@ -1048,9 +1048,10 @@ Zotero.Translate.ItemGetter.prototype = { this.numItems = this._itemsLeft.length; }), - "exportFiles":function(dir, extension) { + exportFiles: function (dir, extension, { includeAnnotations }) { // generate directory this._exportFileDirectory = dir.parent.clone(); + this._includeAnnotations = includeAnnotations; // delete this file if it exists if(dir.exists()) { @@ -1079,6 +1080,7 @@ Zotero.Translate.ItemGetter.prototype = { var attachmentArray = Zotero.Utilities.Internal.itemToExportFormat(attachment, this.legacy); var linkMode = attachment.attachmentLinkMode; if(linkMode != Zotero.Attachments.LINK_MODE_LINKED_URL) { + let includeAnnotations = attachment.isPDFAttachment() && this._includeAnnotations; attachmentArray.localPath = attachment.getFilePath(); if(this._exportFileDirectory) { @@ -1114,7 +1116,7 @@ Zotero.Translate.ItemGetter.prototype = { * file to be overwritten. If true, the file will be silently overwritten. * defaults to false if not provided. */ - attachmentArray.saveFile = function(attachPath, overwriteExisting) { + attachmentArray.saveFile = async function (attachPath, overwriteExisting) { // Ensure a valid path is specified if(attachPath === undefined || attachPath == "") { throw new Error("ERROR_EMPTY_PATH"); @@ -1162,18 +1164,13 @@ Zotero.Translate.ItemGetter.prototype = { var directory = targetFile.parent; - // The only attachments that can have multiple supporting files are imported - // attachments of mime type text/html + // For snapshots with supporting files, check if any of the supporting + // files would cause a name conflict, and build a list of transfers + // that should be performed // - // TEMP: This used to check getNumFiles() here, but that's now async. - // It could be restored (using hasMultipleFiles()) when this is made - // async, but it's probably not necessary. (The below can also be changed - // to use OS.File.DirectoryIterator.) - if(attachment.attachmentContentType == "text/html" - && linkMode != Zotero.Attachments.LINK_MODE_LINKED_FILE) { - // Attachment is a snapshot with supporting files. Check if any of the - // supporting files would cause a name conflict, and build a list of transfers - // that should be performed + // TODO: Change the below to use OS.File.DirectoryIterator? + if (linkMode != Zotero.Attachments.LINK_MODE_LINKED_FILE + && await Zotero.Attachments.hasMultipleFiles(attachment)) { var copySrcs = []; var files = attachment.getFile().parent.directoryEntries; while (files.hasMoreElements()) { @@ -1204,10 +1201,22 @@ Zotero.Translate.ItemGetter.prototype = { for(var i = 0; i < copySrcs.length; i++) { copySrcs[i].copyTo(directory, copySrcs[i].leafName); } - } else { - // Attachment is a single file - // Copy the file to the specified location - attachFile.copyTo(directory, targetFile.leafName); + } + // For single files, just copy to the specified location + else { + if (includeAnnotations) { + // TODO: Make export async + try { + await Zotero.PDFWorker.export(attachment.id, targetFile.path); + } + catch (e) { + Zotero.logError(e); + throw e; + } + } + else { + attachFile.copyTo(directory, targetFile.leafName); + } } attachmentArray.path = targetFile.path; diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties index 902a43360e..bddcd7b941 100644 --- a/chrome/locale/en-US/zotero/zotero.properties +++ b/chrome/locale/en-US/zotero/zotero.properties @@ -803,6 +803,7 @@ fulltext.indexState.queued = Queued exportOptions.exportNotes = Export Notes exportOptions.exportFileData = Export Files +exportOptions.includeAnnotations = Include Annotations exportOptions.useJournalAbbreviation = Use Journal Abbreviation charset.UTF8withoutBOM = Unicode (UTF-8 without BOM) charset.autoDetect = (auto detect) diff --git a/scss/_zotero-react-client.scss b/scss/_zotero-react-client.scss index 42ec2d608a..6bc27557d2 100644 --- a/scss/_zotero-react-client.scss +++ b/scss/_zotero-react-client.scss @@ -25,6 +25,7 @@ @import "components/button"; @import "components/createParent"; @import "components/editable"; +@import "components/exportOptions"; @import "components/icons"; @import "components/mainWindow"; @import "components/notesList"; diff --git a/scss/components/_exportOptions.scss b/scss/components/_exportOptions.scss new file mode 100644 index 0000000000..f78b1716e4 --- /dev/null +++ b/scss/components/_exportOptions.scss @@ -0,0 +1,10 @@ +#zotero-export-options { + #export-option-includeAnnotations { + margin-top: 2px; + margin-left: 17px; + } + + checkbox[disabled=true] label { + opacity: .5; + } +} diff --git a/test/tests/translateTest.js b/test/tests/translateTest.js index 521509f5fb..605a48ff09 100644 --- a/test/tests/translateTest.js +++ b/test/tests/translateTest.js @@ -2059,7 +2059,7 @@ describe("Zotero.Translate.ItemGetter", function() { // saveFile function assert.isFunction(attachment.saveFile, prefix + 'has saveFile function' + suffix); - attachment.saveFile(attachment.defaultPath); + yield attachment.saveFile(attachment.defaultPath); assert.equal(attachment.path, OS.Path.join(exportDir, OS.Path.normalize(attachment.defaultPath)), prefix + 'path is set correctly after saveFile call' + suffix); let fileExists = yield OS.File.exists(attachment.path); @@ -2067,8 +2067,11 @@ describe("Zotero.Translate.ItemGetter", function() { fileExists = yield OS.File.exists(attachment.localPath); assert.isTrue(fileExists, prefix + 'file was not removed from original location' + suffix); - assert.throws(attachment.saveFile.bind(attachment, attachment.defaultPath), /^ERROR_FILE_EXISTS /, prefix + 'saveFile does not overwrite existing file by default' + suffix); - assert.throws(attachment.saveFile.bind(attachment, 'file/../../'), /./, prefix + 'saveFile does not allow exporting outside export directory' + suffix); + let e; + e = yield getPromiseError(attachment.saveFile(attachment.defaultPath)); + assert.match(e.message, /^ERROR_FILE_EXISTS /, prefix + 'saveFile does not overwrite existing file by default' + suffix); + e = yield getPromiseError(attachment.saveFile('file/../../')); + assert.match(e.message, /./, prefix + 'saveFile does not allow exporting outside export directory' + suffix); /** TODO: check if overwriting existing file works **/ }