diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 523dda32e6..30ad42f7c8 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -679,7 +679,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { let selectInActiveWindow = false; let restoreSelection = true; let restoreScroll = true; - let preserveViewport = false; let rowsToSelect = null; let items = null; @@ -860,7 +859,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { // Close container to remove any sub-items this._closeContainer(row, true); this._removeRow(row); - preserveViewport = true; } // If moved from under another item to top level, remove old row and add new one else if (parentIndex != -1 && !parentItemID) { @@ -871,7 +869,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { this._addRow(this.createRow(item, 0, false), beforeRow); sort = id; - preserveViewport = true; } // If moved from one parent to another, remove from old parent else if (parentItemID && parentIndex != -1 && this._rowMap[parentItemID] != parentIndex) { @@ -881,7 +878,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { if (newParentIndex !== undefined) { this._refreshContainer(newParentIndex); } - preserveViewport = true; } // If Unfiled Items and item was added to a collection, remove from view else if (this.itemTree.isContainer(row) && this.viewMode == 'unfiled' && item.getCollections().length) { @@ -1105,7 +1101,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { await this.runListeners('update', true, { restoreSelection, restoreScroll, - preserveViewport, selectInActiveWindow, selection: rowsToSelect }); diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index b4390b8d42..515ceaee17 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -302,7 +302,6 @@ class ItemTreeRowProvider { restoreSelection: !preserveDetachedFocus, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); if (preserveDetachedFocus) { // Collapsing a container with selected descendants moves their selection to the @@ -361,7 +360,6 @@ class ItemTreeRowProvider { restoreSelection: true, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); } @@ -396,7 +394,6 @@ class ItemTreeRowProvider { restoreSelection: true, expandCollapsedParents: false, restoreScroll: true, - preserveViewport: true, }); } @@ -1233,9 +1230,17 @@ var ItemTree = class ItemTree extends LibraryTree { return this.rowProvider.refresh(options); }) - _cacheState() { + /** + * Capture selection and scroll position before changing rows. Preserve the first + * visible row by default; single-item edits and reparenting preserve selection scroll. + * The update's restoreSelection and restoreScroll flags independently apply this state. + * + * @param {Object} [options] + * @param {boolean} [options.preserveSelectionScroll=false] + */ + _cacheState({ preserveSelectionScroll = false } = {}) { this._cachedSelection = this.getSelectedObjects(); - this._cachedScrollPosition = this._saveScrollPosition(); + this._cachedScrollPosition = this._saveScrollPosition({ preserveSelectionScroll }); } /** @@ -1249,8 +1254,6 @@ var ItemTree = class ItemTree extends LibraryTree { * @param {boolean} options.restoreSelection - Whether to restore the cached selection. * @param {boolean} options.ensureRowsAreVisible - Whether to ensure selected rows are visible. * @param {boolean} options.restoreScroll - Whether to restore the cached scroll position. - * @param {boolean} options.preserveViewport - Whether to restore the scroll position by - * keeping the first visible row in place rather than the selected row. * @param {boolean} options.loading - Whether to show loading state (hides tree, shows message). * @param {string} options.message - Optional message to display (for loading, errors, intro text). */ @@ -1261,7 +1264,6 @@ var ItemTree = class ItemTree extends LibraryTree { expandCollapsedParents: true, ensureRowsAreVisible: true, restoreScroll: false, - preserveViewport: false, loading: false, message: null, }) { @@ -1310,7 +1312,7 @@ var ItemTree = class ItemTree extends LibraryTree { } if (options.restoreScroll) { - this._restoreScrollPosition(null, options.preserveViewport); + this._restoreScrollPosition(); } // Allow selection events to propagate and redraw the needed rows @@ -1358,7 +1360,17 @@ var ItemTree = class ItemTree extends LibraryTree { return; } - this._cacheState(); + // Preserve selection scroll for single-item edits and reparenting, including + // moves of multiple items. Other bulk changes preserve the first visible row. + let preserveSelectionScroll = action == 'modify' && type == 'item' + && (ids.length == 1 || ids.some(id => { + let row = this._rowMap[id]; + if (row === undefined) return false; + let parentIndex = this.getParentIndex(row); + let oldParentID = parentIndex == -1 ? null : this.getRow(parentIndex).ref.id; + return oldParentID != (this.getRow(row).ref.parentItemID || null); + })); + this._cacheState({ preserveSelectionScroll }); await this.rowProvider.notify(action, type, ids, extraData); } @@ -2690,11 +2702,8 @@ var ItemTree = class ItemTree extends LibraryTree { * If scrollPosition is provided, restores from it without touching the cache. * * @param {Object|null} scrollPosition - Scroll position to restore, or null to use cached - * @param {Boolean} preserveViewport - Keep the first visible row in position rather than the - * selected row, for changes the user made in this view (e.g. expanding a container or - * dragging an attachment to another item), which shouldn't shift what's on screen */ - _restoreScrollPosition(scrollPosition = null, preserveViewport = false) { + _restoreScrollPosition(scrollPosition = null) { if (scrollPosition === null) { scrollPosition = this._cachedScrollPosition; this._cachedScrollPosition = null; @@ -2702,79 +2711,56 @@ var ItemTree = class ItemTree extends LibraryTree { if (!scrollPosition || !this._treebox) { return; } - let anchors = preserveViewport - ? [scrollPosition.viewport, scrollPosition.selection] - : [scrollPosition.selection, scrollPosition.viewport]; - // Use the first anchor that's still in the view - for (let anchor of anchors) { - if (!anchor) { - continue; - } - let row = this._rowMap[anchor.id]; - if (row === undefined) { - continue; - } - this._treebox.scrollTo(this._treebox.getRowPosition(row) - anchor.offset); + if (scrollPosition.id === undefined) { + this._treebox.scrollTo(scrollPosition.offset); return; } + let row = this._rowMap[scrollPosition.id]; + if (row === undefined) { + return; + } + this._treebox.scrollTo(this._treebox.getRowPosition(row) - scrollPosition.offset); } /** * Return an object describing the current scroll position to restore after changes * - * @return {Object|Boolean} - Object with .selection and .viewport anchors, each with .id (a - * treeViewID) and .offset (pixels between the top of the view and the top of the row, - * so that a partly scrolled row is restored where it was), or false if there's nothing - * to anchor to + * Anchors to top visible item (viewport) scroll by default, can override to anchor to + * selected item instead. This ensures that collapse/expand doesn't move the row from under + * cursor. + * + * @param {Object} [options] + * @param {boolean} [options.preserveSelectionScroll=false] - Prefer a visible selected row over the viewport + * @return {Object|false} - A row ID and relative pixel offset */ - _saveScrollPosition() { + _saveScrollPosition({ preserveSelectionScroll = false } = {}) { if (!this._treebox) return false; var treebox = this._treebox; var first = treebox.getFirstVisibleRow(); if (first === undefined || first === null) { return false; } - var last = treebox.getLastVisibleRow(); - var scrollOffset = treebox.scrollOffset; + let scrollOffset = treebox.scrollOffset; + if (scrollOffset == 0) { + return { offset: 0 }; + } - // If an object is selected, keep the first selected one in position - var selection = null; - for (let i = first; i <= last; i++) { - if (this.selection.isSelected(i)) { - let row = this.getRow(i); - if (row) { - selection = { - id: row.ref.treeViewID, - offset: treebox.getRowPosition(i) - scrollOffset - }; + let anchorIndex = first; + if (preserveSelectionScroll) { + for (let i = first; i <= treebox.getLastVisibleRow(); i++) { + if (this.selection.isSelected(i)) { + anchorIndex = i; + break; } - break; } } - // With no selection to anchor to, don't save a scroll position when the - // view is already at the top of the list. Otherwise restoring after an - // insertion would pin the previously-top row in place (pushing the view - // down) instead of leaving the list scrolled to its new top. - if (!selection && !first) { - return false; - } - - // Keep the first visible row in position, for changes that shouldn't shift what's on - // screen even when the selected row moves - var viewport = null; - let firstRow = this.getRow(first); - if (firstRow) { - viewport = { - id: firstRow.ref.treeViewID, - offset: treebox.getRowPosition(first) - scrollOffset - }; - } - - if (!selection && !viewport) { - return false; - } - return { selection, viewport }; + let row = this.getRow(anchorIndex); + if (!row) return false; + return { + id: row.ref.treeViewID, + offset: treebox.getRowPosition(anchorIndex) - scrollOffset + }; } /** diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 2daf655d1c..8d7e1c9308 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -1121,210 +1121,125 @@ describe("CollectionViewItemTree", function () { assert.sameMembers(zp.itemsView.getSelectedItems(true), [item.id]); }); - it("should keep first visible item in view when other items are added with skipSelect and nothing in view is selected", async function () { - var collection = await createDataObject('collection'); - await waitForItemsLoad(win); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); - } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }.bind(this)); - - // Scroll halfway - treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); - - var firstVisibleItemID = itemsView.getRow(treebox.getFirstVisibleRow()).ref.id; - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true + describe("scroll position", function () { + var collection, items, treebox; + + beforeEach(async function () { + collection = await createDataObject('collection'); + await select(win, collection); + itemsView = zp.itemsView; + treebox = itemsView._treebox; + items = []; + let count = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow() + 30; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < count; i++) { + let item = createUnsavedDataObject('item', { + title: String(i).padStart(4, '0'), + collections: [collection.id], + }); + await item.save({ skipSelect: true }); + items.push(item); + } + }); + await itemsView.selectItem(items[8].id); + treebox.scrollTo(treebox.getRowPosition(4) + 7); }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } + + it("should preserve the selected item's position in viewport when an edit changes its sort position", async function () { + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(items[8].id)) - treebox.scrollOffset; + items[8].setField('title', '0012a'); + await items[8].saveTx(); + assert.equal( + treebox.getRowPosition(itemsView.getRowIndexByID(items[8].id)) - treebox.scrollOffset, + offset + ); + }); + + it("should preserve scroll position for multi-item edits", async function () { + let offset = treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i of [8, 9]) { + items[i].setField('title', '0012' + i); + await items[i].save(); + } + }); + assert.equal(treebox.scrollOffset, offset); + }); + + it("should preserve scroll position when items are added above the visible rows with no selection", async function () { + itemsView.selection.clearSelection(); + let firstItem = itemsView.getRow(treebox.getFirstVisibleRow()).ref; + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(firstItem.id)) - treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '0000a', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.equal(itemsView.getRow(treebox.getFirstVisibleRow()).ref.id, firstItem.id); + assert.equal(treebox.getRowPosition(itemsView.getRowIndexByID(firstItem.id)) - treebox.scrollOffset, offset); + }); + + it("shouldn't scroll when items are added within the visible rows with skipSelect", async function () { + let offset = treebox.scrollOffset; + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '0005a', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.sameMembers(itemsView.getSelectedItems(true), [items[8].id]); + assert.equal(treebox.scrollOffset, offset); + }); + + it("shouldn't scroll when at the top and items are added with skipSelect", async function () { + await itemsView.selectItem(items[2].id); + treebox.scrollTo(0); + await Zotero.DB.executeTransaction(async function () { + for (let i = 0; i < 3; i++) { + let item = createUnsavedDataObject('item', { + title: '000', collections: [collection.id] + }); + await item.save({ skipSelect: true }); + } + }); + assert.equal(treebox.scrollOffset, 0); + }); + + for (let count of [1, 2]) { + it(`should preserve selection scroll position when moving ${count} child item(s) to another parent`, async function () { + let attachments = []; + for (let i = 0; i < count; i++) { + attachments.push(await importFileAttachment('test.png', { parentItemID: items[0].id })); + } + itemsView.expandAllRows(true); + await itemsView.selectItems(attachments.map(item => item.id)); + treebox.scrollTo(7); + let firstSelected = itemsView.getRow(itemsView.getRowIndexByID(items[0].id) + 1).ref; + let offset = treebox.getRowPosition(itemsView.getRowIndexByID(firstSelected.id)) - treebox.scrollOffset; + + await Zotero.DB.executeTransaction(async function () { + for (let attachment of attachments) { + attachment.parentItemID = items[3].id; + await attachment.save(); + } + }); + await itemsView.waitForLoad(); + + assert.sameMembers(itemsView.getSelectedItems(true), attachments.map(item => item.id)); + assert.equal( + treebox.getRowPosition(itemsView.getRowIndexByID(firstSelected.id)) - treebox.scrollOffset, + offset ); - await item.save({ - skipSelect: true - }); - } - }.bind(this)); - - // Make sure the same item is still in the first visible row - assert.equal(itemsView.getRow(treebox.getFirstVisibleRow()).ref.id, firstVisibleItemID); - }); - - it("should keep first visible selected item in position when other items are added with skipSelect", async function () { - var collection = await createDataObject('collection'); - await select(win, collection); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); + assert.isAbove(treebox.scrollOffset, 7); + }); } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }); - - // Scroll halfway - treebox.scrollToRow(Math.round(num / 2) - Math.round(numVisibleRows / 2)); - - // Select an item - itemsView.selection.select(Math.round(num / 2)); - var selectedItem = itemsView.getSelectedItems()[0]; - var offset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true - }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.save({ - skipSelect: true - }); - } - }); - - // Make sure the selected item is still at the same position - assert.equal(itemsView.getSelectedItems()[0], selectedItem); - var newOffset = itemsView.getRowIndexByID(selectedItem.treeViewID) - treebox.getFirstVisibleRow(); - assert.equal(newOffset, offset); }); - - it("shouldn't scroll items list if at top when other items are added with skipSelect", async function () { - var collection = await createDataObject('collection'); - await select(win, collection); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - // Get a numeric string left-padded with zeroes - function getTitle(i, max) { - return new String(new Array(max + 1).join(0) + i).slice(-1 * max); - } - - var num = numVisibleRows + 10; - await Zotero.DB.executeTransaction(async function () { - // Start at "*1" so we can add items before - for (let i = 1; i < num; i++) { - let title = getTitle(i, num); - let item = createUnsavedDataObject('item', { title }); - item.addToCollection(collection.id); - await item.save(); - } - }.bind(this)); - - // Scroll to top - treebox.scrollToRow(0); - - // Add one item at the beginning - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.saveTx({ - skipSelect: true - }); - // Then add a few more in a transaction - await Zotero.DB.executeTransaction(async function () { - for (let i = 0; i < 3; i++) { - var item = createUnsavedDataObject( - 'item', { title: getTitle(0, num), collections: [collection.id] } - ); - await item.save({ - skipSelect: true - }); - } - }.bind(this)); - - // Make sure the first row is still at the top - assert.equal(treebox.getFirstVisibleRow(), 0); - }); - - it("shouldn't scroll items list when a child item is moved to a parent further down", async function () { - var collection = await createDataObject('collection'); - await select(win, collection); - itemsView = zp.itemsView; - - var treebox = itemsView._treebox; - var numVisibleRows = treebox.getLastVisibleRow() - treebox.getFirstVisibleRow(); - - var num = numVisibleRows + 10; - var parentItem1 = await createDataObject('item', { - title: String(0).padStart(num, '0'), - collections: [collection.id] - }); - var parentItem2 = await createDataObject('item', { - title: String(3).padStart(num, '0'), - collections: [collection.id] - }); - await Zotero.DB.executeTransaction(async function () { - for (let i = 4; i < num; i++) { - let item = createUnsavedDataObject('item', { - title: String(i).padStart(num, '0'), - collections: [collection.id] - }); - await item.save(); - } - }); - var attachment = await importFileAttachment('test.png', { parentItemID: parentItem1.id }); - await waitForItemsLoad(win); - - itemsView.expandAllRows(true); - // Scroll partway into a row, which should be preserved as is - treebox.scrollTo(7); - await itemsView.selectItem(attachment.id); - assert.isTrue(itemsView.tree.rowIsVisible(itemsView.getRowIndexByID(parentItem2.id))); - - // Move the attachment to the parent below - attachment.parentItemID = parentItem2.id; - await attachment.saveTx(); - await itemsView.waitForLoad(); - - assert.equal( - itemsView.getRowIndexByID(attachment.id), - itemsView.getRowIndexByID(parentItem2.id) + 1 - ); - assert.equal(treebox.scrollOffset, 7); - }); - + it("should update search results when items are added", async function () { var search = await createDataObject('search'); await select(win, search);