From 46f879dcdbdfbe889b7f955a1114cd647ab01b7d Mon Sep 17 00:00:00 2001 From: Abe Jellinek Date: Wed, 8 Oct 2025 15:16:49 -0400 Subject: [PATCH] Scaffold: UI tweaks and testing overhaul improvements (#5441) - Remember diffs from test runs, show in sidebar when test is selected - Add button to immediately update test, without a prompt - Indicate when test has a custom defer delay set (although no tests currently do) - Use a persistent test store as source of truth, instead of data attributes on the listbox rows - A bit of code cleanup Closes #5420, closes #5419 --- chrome/content/scaffold/scaffold.js | 305 ++++++++++-------- chrome/content/scaffold/scaffold.xhtml | 7 +- .../content/scaffold/scaffoldItemPreviews.js | 102 ++++-- .../default/zotero/16/universal/sync-12.svg | 5 + scss/scaffold.scss | 106 +++++- 5 files changed, 333 insertions(+), 192 deletions(-) create mode 100644 chrome/skin/default/zotero/16/universal/sync-12.svg diff --git a/chrome/content/scaffold/scaffold.js b/chrome/content/scaffold/scaffold.js index 0a44048524..41b97c28b1 100644 --- a/chrome/content/scaffold/scaffold.js +++ b/chrome/content/scaffold/scaffold.js @@ -23,7 +23,6 @@ ***** END LICENSE BLOCK ***** */ -var { E10SUtils } = ChromeUtils.importESModule("resource://gre/modules/E10SUtils.sys.mjs"); var { Subprocess } = ChromeUtils.importESModule("resource://gre/modules/Subprocess.sys.mjs"); var { RemoteTranslate } = ChromeUtils.importESModule("chrome://zotero/content/RemoteTranslate.mjs"); @@ -65,6 +64,14 @@ var Scaffold = new function () { 'textbox-hidden-prefs': 'hiddenPrefs' }; + /** @type {{ + * test: Test; + * testString: string; + * updatedTestString?: string; + * status?: string; + * previews: ScaffoldItemPreview[]; + * }[]} */ + var _testData = []; var _linesOfMetadata = 15; this.handleLoad = async function () { @@ -497,9 +504,9 @@ var Scaffold = new function () { let applyUpdatesCommand = editor.addCommand( 0, - async (_ctx, testIndices) => { + (_ctx, testIndices) => { testIndices = testIndices || [..._loadTestsFromPane().keys()]; - await this.updateTests(testIndices); + this.updateTests(testIndices); }, ''); @@ -511,9 +518,8 @@ var Scaffold = new function () { provideCodeLenses: (model, _token) => { let testsWithUpdates = new Set(); - let testItems = Array.from(document.getElementById('testing-listbox').itemChildren); - for (let [testIndex, itemChild] of testItems.entries()) { - if (itemChild.dataset.updatedTestString) { + for (let [testIndex, { updatedTestString }] of _testData.entries()) { + if (updatedTestString) { testsWithUpdates.add(testIndex); } } @@ -947,11 +953,17 @@ var Scaffold = new function () { else { codeTabBroadcaster.setAttribute('disabled', true); } + + if (tab == 'tests') { + this.handleTestingListboxSelect(); + } }; this.handleTestingContextMenuShowing = function () { - let selectedItems = Array.from(document.getElementById('testing-listbox').selectedItems); + let listbox = document.getElementById('testing-listbox'); + let selectedItems = Array.from(listbox.selectedItems); if (!selectedItems.length) return; + let selectedIndices = selectedItems.map(item => listbox.getIndexOfItem(item)); let editImport = document.getElementById('testing-editImport'); let openURL = document.getElementById('testing-openURL'); @@ -961,11 +973,23 @@ var Scaffold = new function () { openURL.disabled = true; } else { - let selectedItem = selectedItems[0]; - editImport.disabled = selectedItem.dataset.testType === 'web'; - openURL.disabled = selectedItem.dataset.testType !== 'web'; + let { test } = _testData[selectedIndices[0]]; + editImport.disabled = test.type === 'web'; + openURL.disabled = test.type !== 'web'; } - applyUpdates.disabled = selectedItems.some(item => !item.dataset.updatedTestString); + applyUpdates.disabled = selectedIndices.some(idx => !_testData[idx].updatedTestString); + }; + + this.handleTestingListboxSelect = function () { + let listbox = document.querySelector('#testing-listbox'); + let itemPreviews = document.querySelector('#item-previews'); + let previews = []; + for (let item of listbox.selectedItems) { + let index = listbox.getIndexOfItem(item); + let testData = _testData[index]; + previews.push(...testData.previews); + } + itemPreviews.setPreviews(previews); }; this.handleTestingListboxDblClick = function () { @@ -975,7 +999,6 @@ var Scaffold = new function () { // Add special keydown handling for the editors this.addEditorKeydownHandlers = function (editor) { let doc = editor.getDomNode().ownerDocument; - let tabbox = document.getElementById("left-tabbox"); // On shift-tab from the start of the first line, tab out of the editor. // Use capturing listener, since Shift-Tab keydown events do not propagate to the document. doc.addEventListener("keydown", (event) => { @@ -1459,7 +1482,8 @@ var Scaffold = new function () { */ function _loadTestsFromPane() { try { - return JSON.parse(_editors.tests.getValue().trim() || '[]'); + return JSON.parse(_editors.tests.getValue().trim() || '[]') + .map(test => new Test(test)); } catch (e) { return null; @@ -1494,6 +1518,10 @@ var Scaffold = new function () { * * Some fields, like those inside creator objects, notes, etc. are not sorted */ function _stringifyTests(value, level) { + if (value instanceof Test) { + value = value.toJSON(); + } + function processRow(key, value) { let val = _stringifyTests(value, level + 1); if (val === undefined) return undefined; @@ -1609,9 +1637,9 @@ var Scaffold = new function () { } _showTab('tests'); - let listBox = document.getElementById('testing-listbox'); - listBox.selectedIndex = listBox.getRowCount() - 1; - listBox.focus(); + let listbox = document.getElementById('testing-listbox'); + listbox.selectedIndex = listbox.getRowCount() - 1; + listbox.focus(); }; this.constructTestFromCurrent = async function (type) { @@ -1669,15 +1697,6 @@ var Scaffold = new function () { * populate tests pane and url options in browser pane */ this.populateTests = function () { - function wrapWithHBox(elem, { flex, pack, width } = {}) { - let hbox = document.createXULElement('hbox'); - hbox.append(elem); - if (flex !== undefined) hbox.setAttribute('flex', flex); - if (pack !== undefined) hbox.setAttribute('pack', pack); - if (width !== undefined) hbox.style.width = width + 'px'; - return hbox; - } - let tests = _loadTestsFromPane(); let validateTestsBroadcaster = document.getElementById('validate-tests'); if (tests === null) { @@ -1688,77 +1707,108 @@ var Scaffold = new function () { validateTestsBroadcaster.removeAttribute('disabled'); } - let listBox = document.getElementById("testing-listbox"); - let count = listBox.getRowCount(); - let oldStatusNodes = {}; - for (let i = 0; i < count; i++) { - let item = listBox.getItemAtIndex(i); - oldStatusNodes[item.dataset.testString] = item.querySelector('.status'); - } + let listbox = document.getElementById("testing-listbox"); - while (listBox.itemChildren.length > tests.length) { - listBox.itemChildren[listBox.itemChildren.length - 1].remove(); - } for (let [testIndex, test] of tests.entries()) { let testString = _stringifyTests(test, 1); + let testData = _testData[testIndex]; + if (!testData || testData.testString !== testString) { + _testData[testIndex] = testData = { + test, + testString, + previews: [], + }; + } + testData.test = test; + testData.testString = testString; + // Remove updatedTestString if updates have been applied + if (testData.updatedTestString === testString) { + delete testData.updatedTestString; + } + let needsUpdate = !!testData.updatedTestString; - // try to reuse old rows - let item = testIndex < count - ? listBox.getItemAtIndex(testIndex) + let item = testIndex < listbox.getRowCount() + ? listbox.getItemAtIndex(testIndex) : document.createXULElement('richlistitem'); - item.replaceChildren(); + let inputCell = document.createXULElement('hbox'); + inputCell.classList.add('cell', 'col-input'); + let input = document.createXULElement('label'); input.append(getTestLabel(test)); - item.appendChild(wrapWithHBox(input, { flex: 1 })); + inputCell.append(input); + item.append(inputCell); - let status = oldStatusNodes[testString]; - if (!status) { - status = document.createXULElement('label'); - status.classList.add('status'); - status.addEventListener('click', (event) => { - if (!status.classList.contains('needs-update')) { - return; - } - event.preventDefault(); - if (!Services.prompt.confirm( - null, - 'Scaffold', - `Apply updates to this test?` - )) { - return; - } - this.updateTests([testIndex]); - }); + let statusCell = document.createXULElement('hbox'); + statusCell.classList.add('cell', 'col-status'); + statusCell.classList.toggle('needs-update', needsUpdate); + let statusLabel = document.createXULElement('label'); + statusLabel.classList.add('status'); + + let statusText = _testData.find(test => test.testString === testString)?.status ?? ''; + if (needsUpdate) { + let totalStats = { added: 0, removed: 0 }; + for (let preview of testData.previews) { + let statsHere = preview.diffStats; + totalStats.added += statsHere.added; + totalStats.removed += statsHere.removed; + } + + let textElem = document.createElement('span'); + textElem.classList.add('text'); + textElem.textContent = statusText; + let addedElem = document.createElement('span'); + addedElem.classList.add('added'); + addedElem.textContent = `+${totalStats.added}`; + let removedElem = document.createElement('span'); + removedElem.classList.add('removed'); + removedElem.textContent = `-${totalStats.removed}`; + + statusLabel.replaceChildren(textElem, addedElem, removedElem); + } + else { + statusLabel.textContent = statusText; } - item.appendChild(wrapWithHBox(status, { width: 150 })); + statusCell.append(statusLabel); + let updateButton = document.createXULElement('toolbarbutton'); + updateButton.classList.add('update'); + updateButton.setAttribute('tooltiptext', 'Update Test'); + updateButton.addEventListener('command', () => this.updateTests([testIndex])); + statusCell.append(updateButton); + item.append(statusCell); + + let deferCell = document.createXULElement('hbox'); + deferCell.classList.add('cell', 'col-defer'); let defer = document.createXULElement('checkbox'); - defer.checked = test.defer; + if (typeof test.defer === 'number' && test.defer) { + defer.setAttribute('indeterminate', 'true'); + } + else { + defer.checked = test.defer; + } defer.setAttribute('native', 'true'); defer.addEventListener('command', () => { - if (defer.checked) { - test.defer = true; - } - else { - delete test.defer; - } + test.defer = defer.checked; _writeTestsToPane(tests); }); - item.appendChild(wrapWithHBox(defer, { pack: 'center', width: 75 })); + deferCell.append(defer); + item.append(deferCell); - // Remove updatedTestString if the test changed or updates have been applied - if (item.dataset.testString !== testString || item.dataset.updatedTestString === testString) { - delete item.dataset.updatedTestString; - } - item.dataset.testString = testString; - item.dataset.testType = test.type; - - if (testIndex >= count) { - listBox.appendChild(item); + if (!item.parentElement) { + listbox.append(item); } } + + // Remove unused _testData entries and listbox rows + _testData = _testData.slice(0, tests.length); + while (listbox.getRowCount() > tests.length) { + listbox.getItemAtIndex(listbox.getRowCount() - 1).remove(); + } + + // Update UI that depends on the selection + this.handleTestingListboxSelect(); }; /* @@ -1781,8 +1831,7 @@ var Scaffold = new function () { */ this.editImportFromTest = function () { var listbox = document.getElementById("testing-listbox"); - var item = listbox.selectedItems[0]; - var test = JSON.parse(item.dataset.testString); + var { test } = _testData[listbox.selectedIndex]; if (test.input === undefined) { _logOutput("Can't edit input of a non-import/search test."); } @@ -1804,14 +1853,12 @@ var Scaffold = new function () { */ this.copyToClipboard = function () { var listbox = document.getElementById("testing-listbox"); - var item = listbox.selectedItems[0]; - var url = item.getElementsByTagName("label")[0].textContent; - var test = JSON.parse(item.dataset.testString); - var urlOrData = (test.input !== undefined) ? test.input : url; - if (typeof urlOrData !== 'string') { - urlOrData = JSON.stringify(urlOrData, null, '\t'); + var { test } = _testData[listbox.selectedIndex]; + var input = test.input; // URL or input object + if (typeof input !== 'string') { + input = JSON.stringify(input, null, '\t'); } - Zotero.Utilities.Internal.copyTextToClipboard(urlOrData); + Zotero.Utilities.Internal.copyTextToClipboard(input); }; /** @@ -1821,8 +1868,8 @@ var Scaffold = new function () { **/ this.openURL = function (openExternally) { var listbox = document.getElementById("testing-listbox"); - var item = listbox.selectedItems[0]; - var url = item.getElementsByTagName("label")[0].textContent; + var { test } = _testData[listbox.selectedIndex]; + var url = test.url; if (openExternally) { Zotero.launchURL(url); } @@ -1835,28 +1882,23 @@ var Scaffold = new function () { }; this.runTests = async function (testIndices) { - let listbox = document.getElementById('testing-listbox'); - let items = [...listbox.itemChildren]; - let itemsToUpdate = testIndices.map(index => items[index]); - let itemPreviews = document.querySelector('#item-previews'); + let testDatas = testIndices.map(index => _testData[index]); - let tests = []; - for (let listItem of itemsToUpdate) { - listItem.querySelector('.status').textContent = 'Running'; - - let test = new Test(JSON.parse(listItem.dataset.testString)); - tests.push(test); + for (let testData of testDatas) { + testData.status = 'Running'; + delete testData.updatedTestString; } + this.populateTests(); - let numTests = tests.length; - let currentTest = 1; // For logging + let numTests = testDatas.length; + let testIndex = 0; _logOutput(`Running ${numTests} ${Zotero.Utilities.pluralize(numTests, 'test')}`); - for await (let { test, status, reason, updatedTest } of this.runTestsInternal(tests)) { + for await (let { test, status, reason, updatedTest } of this.runTestsInternal(testDatas.map(d => d.test))) { let statusText; let needsUpdate = false; - let logPrefix = `Test ${currentTest}/${numTests}: `; + let logPrefix = `Test ${testIndex + 1}/${numTests}: `; if (status === 'success') { statusText = 'Succeeded'; _logOutput(logPrefix + statusText); @@ -1867,45 +1909,29 @@ var Scaffold = new function () { _logOutput(logPrefix + statusText); } - // Show the preview pane as long as we got new item data, + // Create previews as long as we got new item data, // even if there weren't substantive changes - let totalStats = { added: 0, removed: 0 }; + let previews = []; if (Array.isArray(test.items) && Array.isArray(updatedTest?.items)) { for (let i = 0; i < test.items.length || i < updatedTest.items.length; i++) { let preItem = i < test.items.length ? test.items[i] : {}; let postItem = i < updatedTest.items.length ? updatedTest.items[i] : {}; - - let preview = itemPreviews.addItemPair(preItem, postItem); - let statsHere = preview.diffStats; - totalStats.added += statsHere.added; - totalStats.removed += statsHere.removed; + previews.push(itemPreviews.createPreviewForItemPair(preItem, postItem)); } } - let listItem = itemsToUpdate[tests.indexOf(test)]; - let statusLabel = listItem.querySelector('.status'); - statusLabel.classList.toggle('needs-update', needsUpdate); - - let textElem = document.createElement('span'); - textElem.classList.add('text'); - textElem.textContent = statusText; - let addedElem = document.createElement('span'); - addedElem.classList.add('added'); - addedElem.textContent = `+${totalStats.added}`; - let removedElem = document.createElement('span'); - removedElem.classList.add('removed'); - removedElem.textContent = `-${totalStats.removed}`; - - statusLabel.replaceChildren(textElem, addedElem, removedElem); - + let testData = testDatas[testIndex]; + testData.status = statusText; + testData.previews = previews; if (needsUpdate) { - listItem.dataset.updatedTestString = _stringifyTests(updatedTest.toJSON(), 1); + testData.updatedTestString = _stringifyTests(updatedTest.toJSON(), 1); } else { - delete listItem.dataset.updatedTestString; + delete testData.updatedTestString; } + this.populateTests(); - currentTest++; + testIndex++; } _invalidateCodeLenses?.(); @@ -1935,24 +1961,21 @@ var Scaffold = new function () { } }; - this.updateTests = async function (testIndices) { - let listbox = document.getElementById('testing-listbox'); - let items = [...listbox.itemChildren]; - let itemsToUpdate = testIndices.map(index => items[index]); - + this.updateTests = function (testIndices) { let tests = _loadTestsFromPane(); - for (let item of itemsToUpdate) { - let updatedTestString = item.dataset.updatedTestString; + for (let testIndex of testIndices) { + let testData = _testData[testIndex]; + let updatedTestString = testData.updatedTestString; if (!updatedTestString) { continue; } let updatedTest = JSON.parse(updatedTestString); - tests[items.indexOf(item)] = updatedTest; - item.dataset.testString = _stringifyTests(updatedTest, 1); - let status = item.querySelector('.status'); - status.textContent = 'Updated'; - status.classList.remove('needs-update'); + tests[testIndex] = updatedTest; + testData.test = new Test(updatedTest); + testData.testString = _stringifyTests(updatedTest, 1); + delete testData.updatedTestString; + testData.status = 'Updated'; } _writeTestsToPane(tests); _logOutput('Tests updated.'); @@ -1961,7 +1984,7 @@ var Scaffold = new function () { this.updateSelectedTests = async function () { let listbox = document.getElementById('testing-listbox'); let selectedItems = [...listbox.selectedItems]; - await this.updateTests( + this.updateTests( [...selectedItems].map(item => listbox.getIndexOfItem(item)) ); }; @@ -2003,7 +2026,7 @@ var Scaffold = new function () { function _clearOutput() { document.getElementById('output').value = ''; let itemPreviews = document.querySelector('#item-previews'); - itemPreviews.clearItemPairs(); + itemPreviews.clearPreviews(); } /* diff --git a/chrome/content/scaffold/scaffold.xhtml b/chrome/content/scaffold/scaffold.xhtml index c87ce2023a..24214c87a5 100644 --- a/chrome/content/scaffold/scaffold.xhtml +++ b/chrome/content/scaffold/scaffold.xhtml @@ -482,9 +482,9 @@ - - - + + + diff --git a/chrome/content/scaffold/scaffoldItemPreviews.js b/chrome/content/scaffold/scaffoldItemPreviews.js index c0da1d9639..fda4f6685e 100644 --- a/chrome/content/scaffold/scaffoldItemPreviews.js +++ b/chrome/content/scaffold/scaffoldItemPreviews.js @@ -35,34 +35,62 @@ `); + + _previews = []; _deck; _switcher; - _itemPairs = []; - init() { this._deck = this.querySelector('deck'); this._switcher = this.querySelector('.switcher'); this.querySelector('.previous').addEventListener('command', () => { this._deck.selectedIndex--; - this._renderSwitcher(); + this._renderPreviews(); }); this.querySelector('.next').addEventListener('command', () => { this._deck.selectedIndex++; - this._renderSwitcher(); + this._renderPreviews(); }); } - clearItemPairs() { - this._itemPairs = []; + /** + * @param {any} preItem JSON item + * @param {any} [postItem] JSON item (defaults to cleaned version of preItem) + * @returns {ScaffoldItemPreview} + */ + createPreviewForItemPair(preItem, postItem) { + let preview = document.createXULElement('scaffold-item-preview'); + preview.itemPair = [preItem, postItem]; + return preview; + } + + clearPreviews() { + this._previews = []; this._deck.selectedIndex = 0; this._deck.replaceChildren(); - this._renderSwitcher(); - - this.hidden = true; + this._updateVisibility(); + } + + /** + * @param {ScaffoldItemPreview[]} previews + */ + setPreviews(previews) { + this._previews = previews; + this._deck.selectedIndex = 0; + this._renderPreviews(); + this._updateVisibility(); + } + + /** + * @param {ScaffoldItemPreview} preview + */ + addPreview(preview) { + this._previews.push(preview); + this._renderPreviews(); + this._updateVisibility(); } /** @@ -71,33 +99,45 @@ * @returns {ScaffoldItemPreview} */ addItemPair(preItem, postItem) { - let preview = document.createXULElement('scaffold-item-preview'); - preview.itemPair = [preItem, postItem]; - this._deck.append(preview); - this._renderSwitcher(); - - if (this.hidden) { - let splitterPane = this.closest('splitter + *'); - if (splitterPane) { - // If the pane hasn't been resized, it won't have a fixed width, - // so it'll grow when wrapping text is added. Fix its width now. - splitterPane.style.width = splitterPane.getBoundingClientRect().width + 'px'; - } - this.hidden = false; - } - + let preview = this.createPreviewForItemPair(preItem, postItem); + this.addPreview(preview); return preview; } - _renderSwitcher() { - let switcher = this.querySelector('.switcher'); - if (this._deck.children.length > 1) { - switcher.hidden = false; - switcher.querySelector('.current').textContent = this._deck.selectedIndex + 1; - switcher.querySelector('.max').textContent = this._deck.children.length; + _updateVisibility() { + if (this._deck.childElementCount) { + if (this.hidden) { + let splitterPane = this.closest('splitter + *'); + if (splitterPane) { + // If the pane hasn't been resized, it won't have a fixed width, + // so it'll grow when wrapping text is added. Fix its width now. + splitterPane.style.width = splitterPane.getBoundingClientRect().width + 'px'; + } + this.hidden = false; + } } else { - switcher.hidden = true; + this.hidden = true; + } + } + + _renderPreviews() { + this._deck.replaceChildren( + ...this._previews.map((preview, i) => { + if (i === this._deck.selectedIndex) { + return preview; + } + return document.createXULElement('hbox'); + }) + ); + + if (this._deck.children.length > 1) { + this._switcher.hidden = false; + this._switcher.querySelector('.current').textContent = this._deck.selectedIndex + 1; + this._switcher.querySelector('.max').textContent = this._deck.children.length; + } + else { + this._switcher.hidden = true; } } } diff --git a/chrome/skin/default/zotero/16/universal/sync-12.svg b/chrome/skin/default/zotero/16/universal/sync-12.svg new file mode 100644 index 0000000000..83ed6ccfd1 --- /dev/null +++ b/chrome/skin/default/zotero/16/universal/sync-12.svg @@ -0,0 +1,5 @@ + + + + + diff --git a/scss/scaffold.scss b/scss/scaffold.scss index 1aecdfa383..e71786463b 100644 --- a/scss/scaffold.scss +++ b/scss/scaffold.scss @@ -184,6 +184,30 @@ browser, max-width: 100%; gap: 8px; + listheader, richlistbox { + .col-input { + flex: 1; + } + + .col-status { + width: 175px; + } + + .col-defer { + width: 60px; + + & { + margin-inline: 0; + padding-inline: 0; + } + + .treecol-text { + margin-inline-end: 0 !important; + padding-inline-end: 0; + } + } + } + listheader { // Keep aligned with item columns when scrollbars are visible scrollbar-gutter: stable; @@ -202,25 +226,34 @@ browser, background: var(--material-stripe); } - .status { + .cell { display: flex; align-items: center; gap: 0.5em; - - &:not(.needs-update) { - .added, .removed { - opacity: 0.8; - } + + label { + margin: 0 5px; + padding: 0 4px; } - - &.needs-update { - cursor: pointer; + } + + .cell.col-status { + .status { + display: flex; + gap: 0.5em; + width: unset; + flex: 1; .text { - color: LinkText; - text-decoration: underline; + flex-shrink: 1; + overflow: hidden; + text-overflow: ellipsis; } + .added, .removed { + flex-shrink: 0; + } + .added { color: rgb(40 161 40 / 0.8); } @@ -229,20 +262,58 @@ browser, color: rgb(210 50 50 / 0.8); } } + + .update { + width: 16px; + height: 16px; + padding: 0; + flex-shrink: 0; + + @include svgicon-menu('sync-12', 'universal', '16'); + + .toolbarbutton-text { + display: none; + } + } + + &:not(.needs-update) { + .status { + .added, .removed { + color: revert; + opacity: 0.8; + } + } + + .update { + display: none; + } + } + } + + .cell.col-defer { + justify-content: center; } } &:focus richlistitem[selected] { - .status { - &.needs-update { - .text { - color: var(--color-accent-text); - } - + .cell.col-status.needs-update { + .status { .added, .removed { filter: brightness(1.5); } } + + .update { + color: inherit; + + &:hover { + background: #ffffff1a; + } + + &:active { + background: #ffffff33; + } + } } } } @@ -361,6 +432,7 @@ scaffold-item-preview { display: flex; flex-direction: column; padding-block: 6px; + gap: 6px; &:empty { display: none;