diff --git a/chrome/content/zotero/collectionTree.jsx b/chrome/content/zotero/collectionTree.jsx index 0f413514e8..a068430af4 100644 --- a/chrome/content/zotero/collectionTree.jsx +++ b/chrome/content/zotero/collectionTree.jsx @@ -1583,7 +1583,19 @@ var CollectionTree = class CollectionTree extends LibraryTree { } } - if ((Zotero.isMac && event.metaKey) || (!Zotero.isMac && event.shiftKey)) { + let move = (Zotero.isMac && event.metaKey) || (!Zotero.isMac && event.shiftKey); + + // A selection from a multiple-collection view can span libraries. Those items can + // only be copied, never moved (a move can't coherently move some items and copy + // others), so disallow a move rather than silently substituting a copy. + let ids = Zotero.DragDrop.getDataFromDataTransfer(event.dataTransfer).data; + let items = Zotero.Items.get(ids); + if (new Set(items.map(item => item.libraryID)).size > 1) { + this.setDropEffect(event, move ? "none" : "copy"); + return false; + } + + if (move) { this.setDropEffect(event, "move"); } else { @@ -1784,11 +1796,14 @@ var CollectionTree = class CollectionTree extends LibraryTree { } // Intra-library drag - - // Don't allow drag onto root of same library + + // An item can't be added to the root of its own library, but skip it rather + // than rejecting the whole drag, so a mixed-library selection can still copy + // its out-of-library items here. (If every item is already in this library, + // `skip` stays true and the drag is refused below.) if (treeRow.isLibrary(true)) { - Zotero.debug("Can't drag into same library root"); - return false; + Zotero.debug("Item " + item.id + " already in library " + treeRow.ref.libraryID); + continue; } // Make sure there's at least one item that's not already in this destination @@ -2338,42 +2353,46 @@ var CollectionTree = class CollectionTree extends LibraryTree { }); } - let newItems = []; - let newIDs = []; + // Route each item by its own library: items already in the target library are added + // directly, while items from other libraries are copied into the target library. A + // selection can span multiple libraries when dragging from a multiple-collection view. + let sameLibraryItems = []; + let otherLibraryItems = []; let toMove = []; - // TODO: support items coming from different sources? - let sameLibrary = items[0].libraryID == targetLibraryID - for (let item of items) { if (!item.isTopLevelItem()) { continue; } - newItems.push(item); - - if (sameLibrary) { - newIDs.push(item.id); + if (item.libraryID == targetLibraryID) { + sameLibraryItems.push(item); toMove.push(item.id); } + else { + otherLibraryItems.push(item); + } } - if (sameLibrary) { - // Add items to target container in the same library. + + // Add same-library items to the target container + if (sameLibraryItems.length) { if (targetCollectionID) { - let ids = newIDs.filter(itemID => Zotero.Items.get(itemID).isTopLevelItem()); + let ids = sameLibraryItems.map(item => item.id); await Zotero.DB.executeTransaction(async function () { let collection = await Zotero.Collections.getAsync(targetCollectionID); await collection.addItems(ids); }.bind(this)); } else if (targetTreeRow.isPublications()) { - await Zotero.Items.addToPublications(newItems, copyOptions); + await Zotero.Items.addToPublications(sameLibraryItems, copyOptions); } } - else { + + // Copy items from other libraries into the target library + if (otherLibraryItems.length) { let toReconcile = []; await Zotero.Utilities.Internal.forEachChunkAsync( - newItems, + otherLibraryItems, 100, function (chunk) { return Zotero.DB.executeTransaction(async () => { @@ -2442,11 +2461,10 @@ var CollectionTree = class CollectionTree extends LibraryTree { } - // If moving, remove items from source collection - if (dropEffect == 'move' && toMove.length) { - if (!sameLibrary) { - throw new Error("Cannot move items between libraries"); - } + // If moving, remove items from source collection. A move of a mixed-library selection + // is disallowed in onDragOver(), so it shouldn't reach here; guard against a partial + // move just in case, since only the same-library items would be moved. + if (dropEffect == 'move' && toMove.length && !otherLibraryItems.length) { if (!sourceTreeRow || !sourceTreeRow.isCollection()) { throw new Error("Drag source must be a collection for move action"); } diff --git a/test/tests/collectionTreeTest.js b/test/tests/collectionTreeTest.js index d00bf6fa2c..27d1f2d554 100644 --- a/test/tests/collectionTreeTest.js +++ b/test/tests/collectionTreeTest.js @@ -896,6 +896,46 @@ describe("Zotero.CollectionTree", function () { return canDrop; }; + // Simulate a drag over a row and return the resulting dropEffect ('copy', 'move', or + // 'none'). Pass { move: true } to simulate the platform's move modifier being held. + var dragOver = function (objectType, targetRowID, ids, { move = false } = {}) { + var index = cv.getRowIndexByID(targetRowID); + + Zotero.DragDrop.currentDragSource = objectType == "item" + ? zp.itemsView.collectionTreeRows[0] + : null; + + // Drop directly onto the middle of the row (orient 0) + var rowEl = { + classList: { contains: () => true }, + getBoundingClientRect: () => ({ y: 0, height: 100 }) + }; + var dataTransfer = { + dropEffect: 'copy', + effectAllowed: 'copyMove', + types: [`zotero/${objectType}`], + getData: function (type) { + if (type == `zotero/${objectType}`) { + return ids.join(","); + } + return ""; + }, + setDragImage: () => {} + }; + cv.onDragOver({ + preventDefault: () => {}, + stopPropagation: () => {}, + currentTarget: rowEl, + target: rowEl, + clientY: 50, + metaKey: move && Zotero.isMac, + shiftKey: move && !Zotero.isMac, + dataTransfer + }, index); + Zotero.DragDrop.currentDragSource = null; + return dataTransfer.dropEffect; + }; + describe("with items", function () { it("should add an item to a collection", async function () { var collection = await createDataObject('collection'); @@ -962,6 +1002,84 @@ describe("Zotero.CollectionTree", function () { assert.equal(treeRow.ref.id, item.id); }); + it("should add a multiple-library item selection to a collection, copying out-of-library items", async function () { + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setName(1, 'Name'); + + var collection = await createDataObject('collection'); + var libraryItem = await createDataObject('item', false, { skipSelect: true }); + + var group = await createGroup(); + var groupItem = await createDataObject('item', { libraryID: group.libraryID }); + + // Drop one item from the personal library and one from the group onto a + // personal-library collection + await onDrop('item', 'C' + collection.id, [libraryItem.id, groupItem.id]); + await collection.loadDataType('childItems'); + + // The collection now contains the same-library item plus a copy of the group item + var childItemIDs = collection.getChildItems(true); + assert.lengthOf(childItemIDs, 2); + assert.include(childItemIDs, libraryItem.id); + + // The group item was copied into the personal library, and the copy links back to it + var copiedItem = Zotero.Items.get(childItemIDs.find(id => id != libraryItem.id)); + assert.equal(copiedItem.libraryID, collection.libraryID); + assert.equal((await copiedItem.getLinkedItem(group.libraryID)).id, groupItem.id); + + await group.eraseTx(); + }); + + it("should disallow moving a multiple-library item selection", async function () { + var sourceCollection = await createDataObject('collection'); + var targetCollection = await createDataObject('collection'); + var libraryItem = await createDataObject('item', { collections: [sourceCollection.id] }); + + var group = await createGroup(); + var groupItem = await createDataObject('item', { libraryID: group.libraryID }); + + // Source collection has to be selected so it's used as the drag source + await select(win, sourceCollection); + await waitForItemsLoad(win); + + var ids = [libraryItem.id, groupItem.id]; + // A plain drag copies the selection + assert.equal(dragOver('item', 'C' + targetCollection.id, ids), 'copy'); + // A move is disallowed, since the out-of-library item can't be moved + assert.equal(dragOver('item', 'C' + targetCollection.id, ids, { move: true }), 'none'); + + await group.eraseTx(); + }); + + it("should copy out-of-library items from a multiple-library selection dropped on a library root", async function () { + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setName(1, 'Name'); + + var libraryItem = await createDataObject('item', false, { skipSelect: true }); + + var group = await createGroup(); + var groupItem = await createDataObject('item', { libraryID: group.libraryID }); + + // Drop a personal-library item and a group item onto the personal library root: the + // item already in the library is a no-op, and the group item is copied in + var ids = (await onDrop('item', 'L' + userLibraryID, [libraryItem.id, groupItem.id])).ids; + assert.lengthOf(ids, 1); + + var copiedItem = Zotero.Items.get(ids[0]); + assert.equal(copiedItem.libraryID, userLibraryID); + assert.equal((await copiedItem.getLinkedItem(group.libraryID)).id, groupItem.id); + + await group.eraseTx(); + }); + + it("should refuse a single-library selection dropped on its own library root", async function () { + var item1 = await createDataObject('item', false, { skipSelect: true }); + var item2 = await createDataObject('item', false, { skipSelect: true }); + + // With no out-of-library items to copy, the drag is refused + assert.isFalse(await canDrop('item', 'L' + userLibraryID, [item1.id, item2.id])); + }); + describe("My Publications", function () { function getItemModifyPromise(item) { // Add observer to wait for item modification