From 9a765dd0b99bbd95511d23c1765a5a32eb1d3bfd Mon Sep 17 00:00:00 2001 From: Bogdan Abaev Date: Mon, 9 Dec 2024 10:42:13 -0800 Subject: [PATCH] unify collection canDropCheck with context menu - move collectionTree canDropCheck and canDropCheckAsync to Zotero.Collection::canMoveToTarget and Zotero.Collection::canMoveToTargetAsync per https://github.com/zotero/zotero/pull/4420#issuecomment-2406579899 That way, we share the logic to determine if collection row can be dropped onto another row, as well as to disable/enable context menus. - allow to copy a collection to another library if there already is a linked collection but it is in trash. Fixes: #4862 --- chrome/content/zotero/collectionTree.jsx | 44 +------------ .../content/zotero/xpcom/data/collection.js | 66 +++++++++++++++++++ chrome/content/zotero/zoteroPane.js | 57 +++++----------- test/tests/zoteroPaneTest.js | 41 +++++++++++- 4 files changed, 124 insertions(+), 84 deletions(-) diff --git a/chrome/content/zotero/collectionTree.jsx b/chrome/content/zotero/collectionTree.jsx index c1d47d7009..524cafa249 100644 --- a/chrome/content/zotero/collectionTree.jsx +++ b/chrome/content/zotero/collectionTree.jsx @@ -1777,27 +1777,9 @@ var CollectionTree = class CollectionTree extends LibraryTree { return true; } else if (dataType == 'zotero/collection') { - if (!treeRow.isLibrary(true) && !treeRow.isCollection()) { - return false; - } - let draggedCollectionID = data[0]; let draggedCollection = Zotero.Collections.get(draggedCollectionID); - - // Dragging within same library - if (treeRow.ref.libraryID == draggedCollection.libraryID) { - // Collections cannot be dropped on themselves - if (draggedCollectionID == treeRow.ref.id) { - return false; - } - - // Nor in their children - if (draggedCollection.hasDescendent('collection', treeRow.ref.id)) { - return false; - } - } - - return true; + return draggedCollection.canMoveToTarget(treeRow.ref, { debug: true }); } } return false; @@ -1871,28 +1853,8 @@ var CollectionTree = class CollectionTree extends LibraryTree { else if (dataType == 'zotero/collection') { let draggedCollectionID = data[0]; let draggedCollection = Zotero.Collections.get(draggedCollectionID); - - // Dragging a collection to a different library - if (treeRow.ref.libraryID != draggedCollection.libraryID) { - // Disallow if linked collection already exists - if (await draggedCollection.getLinkedCollection(treeRow.ref.libraryID, true)) { - Zotero.debug("Linked collection already exists in library"); - return false; - } - - let descendents = draggedCollection.getDescendents(false, 'collection'); - for (let descendent of descendents) { - descendent = Zotero.Collections.get(descendent.id); - // Disallow if linked collection already exists for any subcollections - // - // If this is allowed in the future for the root collection, - // need to allow drag only to root - if (await descendent.getLinkedCollection(treeRow.ref.libraryID, true)) { - Zotero.debug("Linked subcollection already exists in library"); - return false; - } - } - } + let canMove = await draggedCollection.canMoveToTargetAsync(treeRow.ref, { debug: true }); + return canMove; } } return true; diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index 2c7b4f9f2b..b27314b45e 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/chrome/content/zotero/xpcom/data/collection.js @@ -987,3 +987,69 @@ Zotero.Collection.prototype._unregisterChildItem = function (itemID) { this._childItems.delete(itemID); } } + +Zotero.Collection.prototype.canMoveToTarget = function (target, options = {}) { + let logMessage = (message) => { + if (options.debug) { + Zotero.debug(message); + } + }; + if (!(target instanceof Zotero.Library || target instanceof Zotero.Collection)) { + logMessage("Can only add collection to another collection or group"); + return false; + } + let targetLibrary = target instanceof Zotero.Library + ? target + : Zotero.Libraries.get(target.libraryID); + if (!targetLibrary.editable) { + logMessage("Not editable target library."); + return false; + } + if (target instanceof Zotero.Collection) { + if (target.id == this.id) { + logMessage("Cannot add collection to itself"); + return false; + } + if (this.hasDescendent('collection', target.id)) { + logMessage("Cannot add collection to its descendent"); + return false; + } + if (this.parentID == target.id) { + logMessage("Cannot add collection to its direct parent"); + return false; + } + } + + return true; +}; + +Zotero.Collection.prototype.canMoveToTargetAsync = async function (target, options = {}) { + let logMessage = (message) => { + if (options.debug) { + Zotero.debug(message); + } + }; + if (!this.canMoveToTarget(target, options)) return false; + + if (target.libraryID !== this.libraryID) { + let linkedCollection = await this.getLinkedCollection(target.libraryID, true); + if (linkedCollection && !linkedCollection.deleted) { + logMessage(`Linked collection already exists in library ${target.libraryID}`); + return false; + } + let descendents = this.getDescendents(false, 'collection'); + for (let descendent of descendents) { + descendent = Zotero.Collections.get(descendent.id); + // Disallow if linked collection already exists for any subcollections + // + // If this is allowed in the future for the root collection, + // need to allow drag only to root + let linkedSubcollection = await descendent.getLinkedCollection(target.libraryID, true); + if (linkedSubcollection && !linkedSubcollection.deleted) { + logMessage("Linked subcollection already exists in library"); + return false; + } + } + } + return true; +}; diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 2a3ae4294f..e55d9c0327 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -4276,20 +4276,14 @@ var ZoteroPane = new function () { event.stopPropagation(); } }, - - (target) => { - // can't move collection into itself, its parent or its children - return selected == target - || selected.parentKey == target.key - || selected.hasDescendent('collection', target.id); - } + target => !selected.canMoveToTarget(target) ); popup.append(menuItem); } }; - this.buildCopyCollectionMenu = function (event) { + this.buildCopyCollectionMenu = Zotero.Utilities.Internal.serial(async function (event) { if (event.target !== event.currentTarget) return; let popup = document.getElementById("zotero-copy-collection-popup"); popup.replaceChildren(); @@ -4298,33 +4292,6 @@ var ZoteroPane = new function () { // Fetch all libraries let topLevelEntries = Zotero.Libraries.getAll().filter(lib => !(lib instanceof Zotero.Feed)); - // Check which libraries have collections linked to the selected collection - // and disable their menuitems. Same logic as in CollectionTree.canDropCheckAsync. - let linkedCollectionsExist = {}; - (async () => { - for (let library of topLevelEntries) { - if (library.libraryID == selected.libraryID) continue; - // Check which library has a collection linked to the selected collection - let linkedCollection = await selected.getLinkedCollection(library.libraryID, true); - linkedCollectionsExist[library.libraryID] = linkedCollection; - // Also check which library has collections linked to a subcollection of the selected collection - for (let descendent of selected.getDescendents(false, 'collection')) { - let subcollection = Zotero.Collections.get(descendent.id); - let linkedSubcollection = await subcollection.getLinkedCollection(library.libraryID, true); - if (linkedSubcollection) { - linkedCollectionsExist[library.libraryID] = linkedSubcollection; - } - } - } - // Libraries that have linked collections have their menus disabled - for (let libraryMenuItem of [...popup.childNodes]) { - let menuItemLibID = libraryMenuItem.getAttribute("value").substring(1); - if (linkedCollectionsExist[menuItemLibID]) { - libraryMenuItem.disabled = true; - } - } - })(); - // If there is only one library, display its collections as top-level menuitems if (topLevelEntries.length == 1) { // Manually add My Library menuitem at the top, so one can still copy into it @@ -4359,16 +4326,22 @@ var ZoteroPane = new function () { event.stopPropagation(); } }, - - (target) => { - // can't copy collection into itself or into non-editable groups - return selected == target - || (target instanceof Zotero.Group && !target.editable); - } + target => !selected.canMoveToTarget(target) ); popup.append(menuItem); } - }; + + // Disable libraries to which collection cannot be copied + if (topLevelEntries[0] instanceof Zotero.Library) { + for (let entry of topLevelEntries) { + let canCopy = await selected.canMoveToTargetAsync(entry); + if (!canCopy) { + let menuitem = popup.querySelector(`[value="L${entry.libraryID}"]`); + menuitem.disabled = true; + } + } + } + }); this.buildAddItemToCollectionMenu = function (event, items = this.getSelectedItems()) { if (event.target !== event.currentTarget) return; diff --git a/test/tests/zoteroPaneTest.js b/test/tests/zoteroPaneTest.js index 8a0a6070f1..d33cc03520 100644 --- a/test/tests/zoteroPaneTest.js +++ b/test/tests/zoteroPaneTest.js @@ -1825,7 +1825,7 @@ describe("ZoteroPane", function () { await waitForNotifierEvent("add", "collection"); - // Collection has been copies + // Collection has been copied let groupCollections = groupCollection.getDescendents(false, 'collection'); let newCollectionID = groupCollections.find(col => col.name == collectionChild.name).id; let newCollection = Zotero.Collections.get(newCollectionID); @@ -1873,6 +1873,45 @@ describe("ZoteroPane", function () { // Copied collection is a top-level collection assert.notOk(newCollection.parentID); }); + + it("should allow copying between libraries if there is a linked collection but it is in trash", async function () { + let groupDestination = await createGroup(); + let groupCollection = await createDataObject('collection', { libraryID: groupDestination.libraryID }); + + let collection = await createDataObject('collection'); + let collectionChild = await createDataObject('collection', { parentID: collection.id }); + + await zp.collectionsView.selectByID("C" + collectionChild.id); + + await zp.copyCollection(groupCollection); + + await waitForNotifierEvent("add", "collection"); + + // Collection has been copied + let newCollection = zp.getSelectedCollection(); + assert.equal(newCollection.libraryID, groupCollection.libraryID); + assert.equal(newCollection.name, collectionChild.name); + + // Send new collection to trash + newCollection.deleted = true; + await newCollection.saveTx(); + + // Right click on the selected collection + await zp.collectionsView.selectByID("C" + collectionChild.id); + await zp.buildCopyCollectionMenu({}); + await Zotero.Promise.delay(); + + let groupMenu = doc.querySelector(`#zotero-copy-collection-popup menu[value="L${groupDestination.libraryID}"]`); + // Menu of the library with linked collection should be enabled + assert.equal(groupMenu.disabled, false); + + // Copy collection again + await zp.copyCollection(groupCollection); + await waitForNotifierEvent("add", "collection"); + let anotherNewCollection = zp.getSelectedCollection(); + // Newly created collection is in the group + assert.equal(anotherNewCollection.libraryID, groupCollection.libraryID); + }); }); describe("#moveCollection", function () { it("should move collection into another collection of the same library", async function () {