diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 484b0ae859..37e64fb00a 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -245,12 +245,12 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { if (newQuery) { session?.dispose(); session = Zotero.BestMatch.createSession(query); + session.onPreviewsFilled = itemIDs => this._showFilledPreviews(session, itemIDs); this._bestMatchSession = session; } try { - // Scoring also derives the matched items' previews before it - // resolves, so the rows the refresh builds draw finished match - // rows + // Scoring derives the best-scored items' previews before it + // resolves; the rest arrive through onPreviewsFilled above await session.score(candidateIDs, { topK, // A newer filter (e.g. more typed search text) makes this @@ -298,6 +298,43 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { return kept; } + /** + * Show the match rows of previews derived after the search resolved (see + * Zotero.BestMatch.Session#score()), by reopening each item -- the same + * path that builds children for an expansion the user asks for. + * + * @param {Zotero.BestMatch.Session} session - Ignored once it isn't the + * session the tree is showing + * @param {Number[]} itemIDs + */ + _showFilledPreviews(session, itemIDs) { + if (this._bestMatchSession !== session) { + return; + } + let shown = []; + for (let itemID of itemIDs) { + // Looked up per item, since reopening one shifts the rows below it + let item = Zotero.Items.get(itemID); + let index = item ? this._rowMap[item.treeViewID] : undefined; + if (index === undefined || !this.isContainer(index)) { + continue; + } + if (this.isContainerOpen(index)) { + this._toggleOpenState(index); + } + this._toggleOpenState(index); + shown.push(itemID); + } + // Redrawing is the expensive part, so a batch with no rows in the + // tree (under a collapsed parent, say) costs nothing + if (!shown.length) { + return; + } + // The twisty appears with the preview, so the rows redraw too + this.itemTree.invalidateRowCache(shown); + this.runListeners('update', true, { restoreSelection: true, restoreScroll: true }); + } + /** * The session holding the passages of the active best-match search, or * null when no such search is running @@ -814,10 +851,11 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { let attachments = item.isRegularItem() ? item.getAttachments() : []; // expand item row if it is a parent of a match // OR if it has a child that is a parent of a match - // OR if it has best-match preview rows to show + // OR if it has best-match preview rows to show -- one still + // deriving has none, and opens in _showFilledPreviews() instead let shouldBeOpened = searchParentIDs.has(item.id) || attachments.some(id => searchParentIDs.has(id)) - || !!this._bestMatchSession?.getPreviews(item.id); + || this._bestMatchSession?.getPreviews(item.id)?.state == 'filled'; if (shouldBeOpened) { this._toggleOpenState(i, true); } diff --git a/chrome/content/zotero/xpcom/bestMatch.js b/chrome/content/zotero/xpcom/bestMatch.js index 43d73b7286..ba27e835d3 100644 --- a/chrome/content/zotero/xpcom/bestMatch.js +++ b/chrome/content/zotero/xpcom/bestMatch.js @@ -62,8 +62,25 @@ Zotero.BestMatch = new function () { // the item has to offer at a glance; the rest are still derived -- // they're read whole rather than quoted, which needs no line chosen. const MAX_QUOTED_PASSAGES = 3; + // Previews derived before scoring resolves, best-scored first: enough to + // cover the top of the results. The rest follow in the background, since + // reading every matched item's text takes far longer than the ranking. + const PRELOADED_MATCH_PREVIEWS = 10; + // How long background-derived previews accumulate before the consumer + // hears about them, so a pass redraws the view about twice a second + const PREVIEW_BATCH_INTERVAL = 500; + // How long a background derivation waits for an idle main thread before + // running anyway + const PREVIEW_IDLE_TIMEOUT = 1000; + // How long the pass rests after deriving a preview, as a multiple of what + // that preview cost. Deriving reads the item's text; run flat out, that + // work lands inside the frames of whatever the user is doing and + // scrolling stutters. + const PREVIEW_PAUSE_RATIO = 3; + const PREVIEW_MAX_PAUSE = 250; this.MAX_QUOTED_PASSAGES = MAX_QUOTED_PASSAGES; + this.PRELOADED_MATCH_PREVIEWS = PRELOADED_MATCH_PREVIEWS; // // Errors @@ -90,6 +107,13 @@ Zotero.BestMatch = new function () { return !!Zotero.Items.get(itemID)?.isFileAttachment?.(); } + // Resolves the next time the main thread is idle, or after + // PREVIEW_IDLE_TIMEOUT regardless + function _idle() { + return new Promise(resolve => requestIdleCallback( + resolve, { timeout: PREVIEW_IDLE_TIMEOUT })); + } + /** * Whether a query has anything for best-match search to rank by. The * lexical engine needs at least one scoring unit; failing that, the @@ -275,19 +299,28 @@ Zotero.BestMatch = new function () { * A best-match search session: one query's scoring pass plus the * previews explaining its matches. * - * score() ranks candidates and, before it resolves, derives a preview - * for every matched item that has anything to show, so consumers read - * settled previews (getPreviews()) the moment scoring ends. An item's - * entries hold both engines' evidence, merged, deduplicated and ordered - * by strength (see getMatchingExcerpts()). A re-score keeps previews - * already derived; fill() re-derives ones invalidate() dropped back to - * pending. A disposed session derives nothing. + * score() ranks candidates and derives the best-scored few previews + * (PRELOADED_MATCH_PREVIEWS) before it resolves. The rest derive in the + * background, in score order and paced to stay out of the user's way + * (see PREVIEW_PAUSE_RATIO), reported through onPreviewsFilled and + * awaitable through previewsSettled. An item's entries hold both + * engines' evidence, merged, deduplicated and ordered by strength (see + * getMatchingExcerpts()). A re-score keeps previews already derived; + * fill() re-derives ones invalidate() dropped back to pending. A + * disposed session derives nothing. */ this.Session = class { constructor(queryText) { this._queryText = queryText; this._previews = new Map(); this._disposed = false; + // Bumped per background pass, so a re-score abandons the last one + this._derivation = 0; + this._previewsSettled = Promise.resolve(); + // Set by a consumer showing previews as they arrive: called with + // the itemIDs filled since the last call (see + // PREVIEW_BATCH_INTERVAL), never for what score() derived itself + this.onPreviewsFilled = null; } get queryText() { @@ -298,9 +331,14 @@ Zotero.BestMatch = new function () { * Score candidates for this session's query (see * Zotero.BestMatch.scoreItemIDs()), rebuild the preview set from the * engines' match sets, recompute ranks and barFractions, and derive - * every pending preview before resolving, best-scored first. Items - * still matched keep their settled previews -- a re-score doesn't - * re-derive kept text -- and items no longer matched lose theirs. + * the best-scored pending previews before resolving. Items still + * matched keep their settled previews -- a re-score doesn't re-derive + * kept text -- and items no longer matched lose theirs. + * + * The previews past PRELOADED_MATCH_PREVIEWS derive after this + * resolves (see onPreviewsFilled, previewsSettled). Ranks and + * barFractions are complete either way -- neither depends on a + * preview. * * @param {Number[]} itemIDs - Candidate item IDs to score * @param {Object} [options] - Passed through to scoreItemIDs() @@ -345,10 +383,11 @@ Zotero.BestMatch = new function () { } this._previews = previews; this._rank(scores); - for (let itemID of [...scores.entries()] + let pending = [...scores.entries()] .sort((a, b) => b[1] - a[1]) .map(([id]) => id) - .filter(id => previews.get(id)?.state == 'pending')) { + .filter(id => previews.get(id)?.state == 'pending'); + for (let itemID of pending.slice(0, PRELOADED_MATCH_PREVIEWS)) { if (this._disposed) { return scores; } @@ -357,9 +396,65 @@ Zotero.BestMatch = new function () { } await this._derive(itemID); } + this._previewsSettled = this._deriveRest( + pending.slice(PRELOADED_MATCH_PREVIEWS)); return scores; } + /** + * Resolves once the previews score() didn't wait for have derived, or + * once the session is disposed + * + * @return {Promise} + */ + get previewsSettled() { + return this._previewsSettled; + } + + // Derive the previews score() left behind, best-scored first, + // reporting them in batches (see onPreviewsFilled). Left unawaited by + // score(), so a consumer draws the ranking while the explanations + // behind it fill in. A newer pass -- another score() -- or dispose() + // abandons this one. + async _deriveRest(itemIDs) { + let derivation = ++this._derivation; + let filled = []; + let reportedAt = Date.now(); + let report = () => { + if (!filled.length) { + return; + } + let reported = filled; + filled = []; + reportedAt = Date.now(); + // A consumer that throws shouldn't strand the rest + try { + this.onPreviewsFilled?.(reported); + } + catch (e) { + Zotero.logError(e); + } + }; + for (let itemID of itemIDs) { + await _idle(); + if (this._disposed || derivation !== this._derivation) { + return; + } + let started = Date.now(); + await this._derive(itemID); + await Zotero.Promise.delay(Math.min(PREVIEW_MAX_PAUSE, + (Date.now() - started) * PREVIEW_PAUSE_RATIO)); + // A preview that derived nothing has no rows to redraw + if (this._previews.get(itemID)?.state == 'filled') { + filled.push(itemID); + } + if (Date.now() - reportedAt >= PREVIEW_BATCH_INTERVAL) { + report(); + } + } + report(); + } + /** * Ranks from this session's last scoring pass: 1-based, tied * effective scores share a rank, and every row with a match anywhere diff --git a/test/tests/bestMatchTest.js b/test/tests/bestMatchTest.js index efec827efe..f350ff29b7 100644 --- a/test/tests/bestMatchTest.js +++ b/test/tests/bestMatchTest.js @@ -747,6 +747,110 @@ describe("Zotero.BestMatch", function () { }); + describe("Session background previews", function () { + // More matches than score() derives before resolving, so the rest go + // to the background pass + const PRELOADED = Zotero.BestMatch.PRELOADED_MATCH_PREVIEWS; + var atts; + + before(async function () { + this.timeout(60000); + atts = []; + for (let i = 0; i < PRELOADED + 3; i++) { + atts.push(await importFileAttachment('test.pdf')); + } + }); + + // Scores descending in creation order, so the background pass covers + // the last three + function stubScore() { + let scores = new Map(atts.map((att, i) => [att.id, 1 - i / 100])); + stubs.push(sinon.stub(Zotero.BestMatch, 'scoreItemIDs').resolves({ + scores, + matches: { lexical: new Set(scores.keys()), semantic: new Set() } + })); + return scores; + } + + // Derives one entry per item, holding each derivation until it's let + // through, so a test can stop the pass at a known point + function stubDerive() { + let waiting = []; + let stub = sinon.stub(Zotero.BestMatch.Session.prototype, 'getMatchingExcerpts') + .callsFake(async () => { + await new Promise(resolve => waiting.push(resolve)); + return [{ source: 'title', text: 'owl', ranges: [], strength: 1 }]; + }); + stubs.push(stub); + let release = () => waiting.splice(0).forEach(resolve => resolve()); + return { + stub, + release, + // Let derivations through until `count` have started. They run + // one at a time, so the last is left waiting, not released. + advanceTo: async (count) => { + while (stub.callCount < count) { + release(); + await Zotero.Promise.delay(0); + } + } + }; + } + + it("should derive the best-scored previews before resolving and the rest after", async function () { + stubScore(); + let { stub, release, advanceTo } = stubDerive(); + let reported = []; + let session = Zotero.BestMatch.createSession('owl'); + session.onPreviewsFilled = itemIDs => reported.push(...itemIDs); + let scored = session.score(atts.map(att => att.id)); + // Only the previews score() waits for are let through, so its + // resolution can't have depended on the one left in flight + await advanceTo(PRELOADED + 1); + await scored; + + // Settled by the time score() resolves + for (let att of atts.slice(0, PRELOADED)) { + assert.equal(session.getPreviews(att.id).state, 'filled'); + } + for (let att of atts.slice(PRELOADED)) { + assert.equal(session.getPreviews(att.id).state, 'pending'); + } + // Nothing reported for what score() derived itself + assert.isEmpty(reported); + + let settled = false; + session.previewsSettled.then(() => settled = true); + while (!settled) { + release(); + await Zotero.Promise.delay(0); + } + await session.previewsSettled; + assert.equal(stub.callCount, atts.length); + for (let att of atts.slice(PRELOADED)) { + assert.equal(session.getPreviews(att.id).state, 'filled'); + } + assert.sameMembers(reported, atts.slice(PRELOADED).map(att => att.id)); + }); + + it("should stop the background pass when the session is disposed", async function () { + stubScore(); + let { stub, release, advanceTo } = stubDerive(); + let session = Zotero.BestMatch.createSession('owl'); + let scored = session.score(atts.map(att => att.id)); + await advanceTo(PRELOADED + 1); + await scored; + + session.dispose(); + release(); + await session.previewsSettled; + // The in-flight derivation settles nothing, and the ones behind + // it are never asked for + assert.equal(stub.callCount, PRELOADED + 1); + assert.equal(session.getPreviews(atts[PRELOADED].id).state, 'pending'); + }); + }); + describe("#isSearchableQuery()", function () { it("should accept any query the lexical engine can parse", function () { stubs.push(sinon.stub(Zotero.Embeddings, 'isEnabled').returns(false)); diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 9f000d4d73..234576b4fa 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -813,6 +813,54 @@ describe("CollectionViewItemTree", function () { itemsView.bestMatchSession.getPreviews(attachment.id).entries, 5); }); + it("should show match rows for previews derived after the search resolves", async function () { + this.timeout(60000); + // One more attachment than score() derives before resolving, + // so the last preview arrives after the results are on screen + let preloaded = Zotero.BestMatch.PRELOADED_MATCH_PREVIEWS; + let col = await createDataObject('collection'); + let item = await createDataObject('item', { title: "backfill A", collections: [col.id] }); + let attachments = []; + for (let i = 0; i <= preloaded; i++) { + attachments.push(await importFileAttachment('test.pdf', { parentID: item.id })); + } + // Descending, so the extra attachment is the one left over + let ids = attachments.map(att => att.id); + Zotero.Lexical.scoreItemIDs.callsFake(async (query, itemIDs) => new Map( + ids.filter(id => itemIDs.includes(id)).map((id, i) => [id, 0.9 - i / 100]))); + stubs.push(sinon.stub(Zotero.Embeddings, 'scoreItemIDs') + .callsFake(scoreEnvelope(new Map()))); + // Held open, so the search has to resolve without it + let release; + let held = new Promise(resolve => release = resolve); + let derived = 0; + stubs.push(sinon.stub(Zotero.BestMatch.Session.prototype, 'getMatchingExcerpts') + .callsFake(async () => { + if (++derived > preloaded) { + await held; + } + return [{ source: 'content', text: 'backfill owls', ranges: [], strength: 1 }]; + })); + + await select(win, col); + itemsView = zp.itemsView; + await itemsView.setFilter('search', 'some query'); + + let last = attachments[attachments.length - 1]; + // The preloaded ones are on screen with the results... + assert.notStrictEqual(itemsView.getRowIndexByID('SM' + attachments[0].id + '-0'), false); + // ...while the leftover is still deriving, so it has no rows + assert.isFalse(itemsView.getRowIndexByID('SM' + last.id + '-0')); + + release(); + await itemsView.bestMatchSession.previewsSettled; + // Filling expanded the attachment and added the match row + let matchRow = itemsView.getRowIndexByID('SM' + last.id + '-0'); + assert.notStrictEqual(matchRow, false); + assert.equal(itemsView.getParentIndex(matchRow), + itemsView.getRowIndexByID(last.id)); + }); + it("should show no match rows when derivation finds nothing to show", async function () { let col = await createDataObject('collection'); let item = await createDataObject('item', { title: "emptyfill A", collections: [col.id] });