From 3fc8e4d02ecf68b9ce0a5e10035d31ac9ca05d5f Mon Sep 17 00:00:00 2001 From: abaevbog Date: Fri, 2 May 2025 22:30:50 -0700 Subject: [PATCH] Fix leftover annotation rows and disappearing attachment rows after drag-drop (#5248) - collapse all rows before removing them. It makes sure that annotation rows will be cleaned up properly when an attachment row is moved into another parent. - refactor conditionals to handle changes of parent regardless of whether the item is a container (attachment with annotations) or not (a note). - added tests for itemTree's handling of changing the parent of attachments and notes, as well as ensuring there are no leftover annotation rows in Unfiled Items. Fixes: zotero#5246 --- chrome/content/zotero/itemTree.jsx | 31 +++---- test/tests/itemTreeTest.js | 126 ++++++++++++++++++++++++++++- 2 files changed, 141 insertions(+), 16 deletions(-) diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index 9e179ba44b..f872359c2f 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -686,24 +686,15 @@ var ItemTree = class ItemTree extends LibraryTree { if (row !== undefined) { let parentItemID = this.getRow(row).ref.parentItemID; let parentIndex = this.getParentIndex(row); - - // Top-level item - if (item.isTopLevelItem()) { - // If Unfiled Items and itm was added to a collection, remove from view - if (collectionTreeRow.isUnfiled() && item.getCollections().length) { - this._removeRow(row); - } - // Otherwise just resort - else { - sort = true; - } - } - // If item moved from top-level to under another item, remove the old row. - else if (parentIndex == -1 && parentItemID) { + + // If item moved from top level to under another item, remove the old row + if (parentIndex == -1 && parentItemID) { + this._closeContainer(row); this._removeRow(row); } // If moved from under another item to top level, remove old row and add new one else if (parentIndex != -1 && !parentItemID) { + this._closeContainer(row); this._removeRow(row); let beforeRow = this.rowCount; @@ -711,10 +702,20 @@ var ItemTree = class ItemTree extends LibraryTree { sort = true; } - // If item was moved from one parent to another, remove from old parent + // If moved from one parent to another, remove from old parent else if (parentItemID && parentIndex != -1 && this._rowMap[parentItemID] != parentIndex) { + this._closeContainer(row); this._removeRow(row); } + // If Unfiled Items and item was added to a collection, remove from view + else if (this.isContainer(row) && collectionTreeRow.isUnfiled() && item.getCollections().length) { + this._closeContainer(row); + this._removeRow(row); + } + // Resort everything if a container is updated, just in case + else if (this.isContainer(row)) { + sort = true; + } // If not moved from under one item to another, just resort the row, // which also invalidates it and refreshes it else { diff --git a/test/tests/itemTreeTest.js b/test/tests/itemTreeTest.js index abe3b41d93..44fd166c52 100644 --- a/test/tests/itemTreeTest.js +++ b/test/tests/itemTreeTest.js @@ -785,14 +785,138 @@ describe("Zotero.ItemTree", function() { var userLibraryID = Zotero.Libraries.userLibraryID; var collection = await createDataObject('collection'); var item = await createDataObject('item', { title: "Unfiled Item" }); + var attachment = await importFileAttachment('test.png', { parentItemID: item.id }); await zp.setVirtual(userLibraryID, 'unfiled', true, true); assert.equal(zp.getCollectionTreeRow().id, 'U' + userLibraryID); await waitForItemsLoad(win); - assert.isNumber(zp.itemsView.getRowIndexByID(item.id)); + let rowIndex = zp.itemsView.getRowIndexByID(item.id); + assert.isNumber(rowIndex); + await zp.itemsView.toggleOpenState(rowIndex); + let attachmentRowIndex = zp.itemsView.getRowIndexByID(attachment.id); + assert.isNumber(attachmentRowIndex); await Zotero.DB.executeTransaction(async function () { await collection.addItem(item.id); }); assert.isFalse(zp.itemsView.getRowIndexByID(item.id)); + // Ensure there is no leftover attachment row + assert.isFalse(zp.itemsView.getRowIndexByID(attachment.id)); + }); + + describe("Change parent item", function () { + let item1, item2, attachment1, highlight1; + + beforeEach(async function () { + // Two top-level items + item1 = await createDataObject('item', { title: "Parent Item 1" }); + item2 = await createDataObject('item', { title: "Parent Item 2" }); + + // A child attachment with an annotation for the first item + attachment1 = await importFileAttachment('test.pdf', { title: 'Attachment 1', parentItemID: item1.id }); + highlight1 = await createAnnotation('highlight', attachment1); + + // Make sure tree is expanded to show all items + zp.itemsView.expandAllRows(); + }); + + it("should remove old attachment and annotation rows on attachment parent change", async function () { + // Change attachment parent + attachment1.parentID = item2.id; + await attachment1.saveTx(); + + let secondItemRowIndex = itemsView.getRowIndexByID(item2.id); + let attachmentRowIndex = itemsView.getRowIndexByID(attachment1.id); + let annotationRowIndex = itemsView.getRowIndexByID(highlight1.id); + + // Verify that the attachment has been moved into the item + assert.isTrue(itemsView.isContainerOpen(itemsView.getRowIndexByID(item2.id))); + assert.equal(attachmentRowIndex, secondItemRowIndex + 1); + assert.equal(itemsView.getRow(attachmentRowIndex).level, 1); + // Verify there is no leftover annotation row + assert.isFalse(annotationRowIndex); + }); + + it("should remove old attachment and annotation rows after a child attachment is moved to top level", async function () { + // Make attachment top level + attachment1.parentID = null; + await attachment1.saveTx(); + + let attachmentRowIndex = itemsView.getRowIndexByID(attachment1.id); + let annotationRowIndex = itemsView.getRowIndexByID(highlight1.id); + + // Verify that the attachment has been moved to top level + assert.equal(itemsView.getRow(attachmentRowIndex).level, 0); + // Verify there is no leftover annotation row + assert.isFalse(annotationRowIndex); + }); + + it("should remove old attachment and annotation rows after a top-level attachment is made a child", async function () { + // Make a top-level attachment + let topLevelAttachment = await importFileAttachment('test.pdf', { title: 'Top Level Attachment', parentItemID: null }); + let highlightOfTopLevel = await createAnnotation('highlight', topLevelAttachment); + + // Move top-level attachment into item + topLevelAttachment.parentID = item2.id; + await topLevelAttachment.saveTx(); + + let secondItemRowIndex = itemsView.getRowIndexByID(item2.id); + let attachmentRowIndex = itemsView.getRowIndexByID(topLevelAttachment.id); + let annotationRowIndex = itemsView.getRowIndexByID(highlightOfTopLevel.id); + + // Verify that the attachment has been moved into the item + assert.isTrue(itemsView.isContainerOpen(itemsView.getRowIndexByID(item2.id))); + assert.equal(attachmentRowIndex, secondItemRowIndex + 1); + assert.equal(itemsView.getRow(attachmentRowIndex).level, 1); + // Verify there is no leftover annotation row + assert.isFalse(annotationRowIndex); + }); + + it("should handle child note being moved to top level", async function () { + let note1 = await createDataObject('item', { itemType: 'note', parentID: item1.id }); + let itemRowIndex = itemsView.getRowIndexByID(item1.id); + let noteRowIndex = itemsView.getRowIndexByID(note1.id); + assert.equal(noteRowIndex, itemRowIndex + 1); + + // Make the note top level + note1.parentID = null; + await note1.saveTx(); + + noteRowIndex = itemsView.getRowIndexByID(note1.id); + // Verify that the note has been moved to top level + assert.equal(itemsView.getRow(noteRowIndex).level, 0); + }); + + it("should handle top-level note being made a child note", async function () { + // Make a top-level note + let note = await createDataObject('item', { itemType: 'note', parentID: null }); + + // Move top-level note into item + note.parentID = item2.id; + await note.saveTx(); + + let secondItemRowIndex = itemsView.getRowIndexByID(item2.id); + let noteRowIndex = itemsView.getRowIndexByID(note.id); + + // Verify that the note row has been moved into the item + assert.isTrue(itemsView.isContainerOpen(itemsView.getRowIndexByID(item2.id))); + assert.equal(noteRowIndex, secondItemRowIndex + 1); + }); + + it("should handle child note being moved between items", async function () { + let note1 = await createDataObject('item', { itemType: 'note', parentID: item1.id }); + let itemRowIndex = itemsView.getRowIndexByID(item1.id); + let noteRowIndex = itemsView.getRowIndexByID(note1.id); + assert.equal(noteRowIndex, itemRowIndex + 1); + + // Move to another parent + note1.parentID = item2.id; + await note1.saveTx(); + + let secondItemRowIndex = itemsView.getRowIndexByID(item2.id); + noteRowIndex = itemsView.getRowIndexByID(note1.id); + // Verify that the note row has been moved into the item + assert.isTrue(itemsView.isContainerOpen(itemsView.getRowIndexByID(item2.id))); + assert.equal(noteRowIndex, secondItemRowIndex + 1); + }); }); describe("Trash", function () {