From 34455197149ce15ea24156804e8412fcf2b7b9f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adomas=20Ven=C4=8Dkauskas?= Date: Mon, 26 Mar 2018 15:31:26 +0300 Subject: [PATCH] Fix citationByIndex[i].sortedItem errors during citation insertion Caused by inproper handling of copy-pasted citations in documents --- chrome/content/zotero/xpcom/integration.js | 72 +++++++++++++++------- test/tests/integrationTest.js | 53 ++++++++++++---- 2 files changed, 90 insertions(+), 35 deletions(-) diff --git a/chrome/content/zotero/xpcom/integration.js b/chrome/content/zotero/xpcom/integration.js index f0e8081cc0..0d0fb403bd 100644 --- a/chrome/content/zotero/xpcom/integration.js +++ b/chrome/content/zotero/xpcom/integration.js @@ -565,7 +565,7 @@ Zotero.Integration.Interface.prototype.editBibliography = Zotero.Promise.corouti throw new Zotero.Exception.Alert("integration.error.mustInsertBibliography", [], "integration.error.title"); } - let bibliography = new Zotero.Integration.Bibliography(bibliographyField); + let bibliography = new Zotero.Integration.Bibliography(bibliographyField, bibliographyField.unserialize()); var citationsMode = FORCE_CITATIONS_FALSE; if(this._session.data.prefs.delayCitationUpdates) { // Refreshes citeproc state before proceeding @@ -604,7 +604,7 @@ Zotero.Integration.Interface.prototype.addEditBibliography = Zotero.Promise.coro bibliographyField.clearCode(); } - let bibliography = new Zotero.Integration.Bibliography(bibliographyField); + let bibliography = new Zotero.Integration.Bibliography(bibliographyField, bibliographyField.unserialize()); var citationsMode = FORCE_CITATIONS_FALSE; if(this._session.data.prefs.delayCitationUpdates) { // Refreshes citeproc state before proceeding @@ -893,7 +893,8 @@ Zotero.Integration.Fields.prototype._processFields = Zotero.Promise.coroutine(fu let field = Zotero.Integration.Field.loadExisting(this._fields[i]); if (field.type === INTEGRATION_TYPE_ITEM) { var noteIndex = field.getNoteIndex(), - citation = new Zotero.Integration.Citation(field, noteIndex); + data = field.unserialize(), + citation = new Zotero.Integration.Citation(field, noteIndex, data); yield this._session.addCitation(i, noteIndex, citation); } else if (field.type === INTEGRATION_TYPE_BIBLIOGRAPHY) { @@ -905,7 +906,8 @@ Zotero.Integration.Fields.prototype._processFields = Zotero.Promise.coroutine(fu } } if (this._bibliographyFields.length) { - this._session.bibliography = new Zotero.Integration.Bibliography(this._bibliographyFields[0]); + var data = this._bibliographyFields[0].unserialize() + this._session.bibliography = new Zotero.Integration.Bibliography(this._bibliographyFields[0], data); yield this._session.bibliography.loadItemData(); } else { delete this._session.bibliography; @@ -1118,6 +1120,7 @@ Zotero.Integration.Fields.prototype._updateDocument = async function(forceCitati */ Zotero.Integration.Fields.prototype.addEditCitation = Zotero.Promise.coroutine(function* (field) { var newField; + var citation; if (field) { field = Zotero.Integration.Field.loadExisting(field); @@ -1125,13 +1128,13 @@ Zotero.Integration.Fields.prototype.addEditCitation = Zotero.Promise.coroutine(f if (field.type != INTEGRATION_TYPE_ITEM) { throw new Zotero.Exception.Alert("integration.error.notInCitation"); } + citation = new Zotero.Integration.Citation(field, field.getNoteIndex(), field.unserialize()); } else { newField = true; field = new Zotero.Integration.CitationField(yield this.addField(true)); - field.clearCode(); + citation = new Zotero.Integration.Citation(field); } - var citation = new Zotero.Integration.Citation(field); yield citation.prepareForEditing(); // ------------------- @@ -1158,19 +1161,31 @@ Zotero.Integration.Fields.prototype.addEditCitation = Zotero.Promise.coroutine(f }.bind(this)); } - var previewFn = Zotero.Promise.coroutine(function* (citation) { - let idx = yield fieldIndexPromise; - yield citationsByItemIDPromise; + var previewFn = async function (citation) { + let idx = await fieldIndexPromise; + await citationsByItemIDPromise; + var fields = await this.get(); - var [citations, fieldToCitationIdxMapping, citationToFieldIdxMapping] = this._session.getCiteprocLists(); - let citationsPre = citations.slice(0, fieldToCitationIdxMapping[idx]); - let citationsPost = citations.slice(fieldToCitationIdxMapping[idx]+1); + var [citations, fieldToCitationIdxMapping, citationToFieldIdxMapping] = this._session.getCiteprocLists(true); + for (var prevIdx = idx-1; prevIdx >= 0; prevIdx--) { + if (prevIdx in fieldToCitationIdxMapping) break; + } + for (var nextIdx = idx+1; nextIdx < fields.length; nextIdx++) { + if (nextIdx in fieldToCitationIdxMapping) break; + } + let citationsPre = citations.slice(0, fieldToCitationIdxMapping[prevIdx]+1); + let citationsPost = citations.slice(fieldToCitationIdxMapping[nextIdx]); try { - return this._session.style.previewCitationCluster(citation, citationsPre, citationsPost, "rtf"); + var result = this._session.style.previewCitationCluster(citation, citationsPre, citationsPost, "rtf"); } catch(e) { throw e; + } finally { + // CSL.previewCitationCluster() sets citationID, which means that we do not mark it + // as a new citation in Session.addCitation() if the ID is still present + delete citation.citationID; } - }.bind(this)); + return result; + }.bind(this); var io = new Zotero.Integration.CitationEditInterface( citation, this._session.style.opt.sort_citations, @@ -1528,8 +1543,15 @@ Zotero.Integration.Session.prototype.addCitation = Zotero.Promise.coroutine(func } // We need a new ID if there's another citation with the same citation ID in this document - var needNewID = !citation.citationID || this.documentCitationIDs[citation.citationID]; + var duplicateIndex = this.documentCitationIDs[citation.citationID]; + var needNewID = !citation.citationID || duplicateIndex != undefined; if(needNewID || this.regenAll) { + if (duplicateIndex != undefined) { + // If this is a duplicate, we need to mark both citations as "new" + // since we do not know which one was the "original" one + // and either one may need to be updated + this.newIndices[duplicateIndex] = true; + } if(needNewID) { Zotero.debug("Integration: "+citation.citationID+" ("+index+") needs new citationID"); citation.citationID = Zotero.randomString(); @@ -1537,17 +1559,21 @@ Zotero.Integration.Session.prototype.addCitation = Zotero.Promise.coroutine(func this.newIndices[index] = true; } Zotero.debug("Integration: Adding citationID "+citation.citationID); - this.documentCitationIDs[citation.citationID] = citation.citationID; + this.documentCitationIDs[citation.citationID] = index; }); -Zotero.Integration.Session.prototype.getCiteprocLists = function() { +Zotero.Integration.Session.prototype.getCiteprocLists = function(excludeNew) { var citations = []; - var fieldToCitationIdxMapping = []; + var fieldToCitationIdxMapping = {}; var citationToFieldIdxMapping = {}; var i = 0; // This relies on the order of citationsByIndex keys being stable and sorted in ascending order // Which it seems to currently be true for every modern JS engine, so we're probably fine for(let idx in this.citationsByIndex) { + if (excludeNew && this.newIndices[idx]) { + i++; + continue; + } citations.push([this.citationsByIndex[idx].citationID, this.citationsByIndex[idx].properties.noteIndex]); fieldToCitationIdxMapping[i] = idx; citationToFieldIdxMapping[idx] = i++; @@ -2266,8 +2292,10 @@ Zotero.Integration.BibliographyField = class extends Zotero.Integration.Field { }; Zotero.Integration.Citation = class { - constructor(citationField, noteIndex) { - let data = citationField.unserialize(); + constructor(citationField, noteIndex, data) { + if (!data) { + data = {citationItems: [], properties: {}}; + } this.citationID = data.citationID; this.citationItems = data.citationItems; this.properties = data.properties; @@ -2511,9 +2539,9 @@ Zotero.Integration.Citation = class { }; Zotero.Integration.Bibliography = class { - constructor(bibliographyField) { + constructor(bibliographyField, data) { this._field = bibliographyField; - this.data = bibliographyField.unserialize(); + this.data = data; this.uncitedItemIDs = new Set(); this.omittedItemIDs = new Set(); diff --git a/test/tests/integrationTest.js b/test/tests/integrationTest.js index a63663ee2b..026aabf2ec 100644 --- a/test/tests/integrationTest.js +++ b/test/tests/integrationTest.js @@ -277,11 +277,12 @@ describe("Zotero.Integration", function () { function setAddEditItems(items) { if (items.length == undefined) items = [items]; - dialogResults.quickFormat = function(dialogName, io) { + dialogResults.quickFormat = async function(dialogName, io) { io.citation.citationItems = items.map(function(item) { item = Zotero.Cite.getItem(item.id); return {id: item.id, uris: item.cslURIs, itemData: item.cslItemData}; }); + await io.previewFn(io.citation); io._acceptDeferred.resolve(() => {}); }; } @@ -306,21 +307,15 @@ describe("Zotero.Integration", function () { return applications[docID]; }); - displayDialogStub = sinon.stub(Zotero.Integration, 'displayDialog', - // @TODO: This sinon.stub syntax is deprecated! - // However when using callsFake tests won't work. This is due to a - // possible bug that reset() erases callsFake. - // @NOTE: https://github.com/sinonjs/sinon/issues/1341 - // displayDialogStub.callsFake(function(doc, dialogName, prefs, io) { - function(dialogName, prefs, io) { + displayDialogStub = sinon.stub(Zotero.Integration, 'displayDialog'); + displayDialogStub.callsFake(async function(dialogName, prefs, io) { Zotero.debug(`Display dialog: ${dialogName}`, 2); var ioResult = dialogResults[dialogName.substring(dialogName.lastIndexOf('/')+1, dialogName.length-4)]; if (typeof ioResult == 'function') { - ioResult(dialogName, io); + await ioResult(dialogName, io); } else { Object.assign(io, ioResult); } - return Zotero.Promise.resolve(); }); addEditCitationSpy = sinon.spy(Zotero.Integration.Interface.prototype, 'addEditCitation'); @@ -371,7 +366,7 @@ describe("Zotero.Integration", function () { Zotero.Styles.get(style.styleID).remove(); } catch (e) {} initDoc(docID, {style}); - displayDialogStub.reset(); + displayDialogStub.resetHistory(); displayAlertStub.reset(); }); @@ -577,6 +572,38 @@ describe("Zotero.Integration", function () { }); }); + describe('when there are duplicated citations (copy-paste)', function() { + it('should resolve duplicate citationIDs and mark both as new citations', async function() { + var docID = this.test.fullTitle(); + if (!(docID in applications)) initDoc(docID); + var doc = applications[docID].doc; + + setAddEditItems(testItems[0]); + await execCommand('addEditCitation', docID); + assert.equal(doc.fields.length, 1); + // Add a duplicate + doc.fields.push(new DocumentPluginDummy.Field(doc)); + doc.fields[1].code = doc.fields[0].code; + doc.fields[1].text = doc.fields[0].text; + + var originalUpdateDocument = Zotero.Integration.Fields.prototype.updateDocument; + var stubUpdateDocument = sinon.stub(Zotero.Integration.Fields.prototype, 'updateDocument'); + try { + var indicesLength; + stubUpdateDocument.callsFake(function() { + indicesLength = Object.keys(Zotero.Integration.currentSession.newIndices).length; + return originalUpdateDocument.apply(this, arguments); + }); + + setAddEditItems(testItems[1]); + await execCommand('addEditCitation', docID); + assert.equal(indicesLength, 3); + } finally { + stubUpdateDocument.restore(); + } + }); + }); + describe('when delayCitationUpdates is set', function() { it('should insert a citation with wave underlining', function* (){ yield insertMultipleCitations.call(this); @@ -631,7 +658,7 @@ describe("Zotero.Integration", function () { }); it('should insert bibliography if no bibliography field present', function* () { - displayDialogStub.reset(); + displayDialogStub.resetHistory(); yield execCommand('addEditBibliography', docID); assert.isFalse(displayDialogStub.called); var biblPresent = false; @@ -647,7 +674,7 @@ describe("Zotero.Integration", function () { it('should display the edit bibliography dialog if bibliography present', function* () { yield execCommand('addEditBibliography', docID); - displayDialogStub.reset(); + displayDialogStub.resetHistory(); yield execCommand('addEditBibliography', docID); assert.isTrue(displayDialogStub.calledOnce); assert.isTrue(displayDialogStub.lastCall.args[0].includes('editBibliographyDialog'));