diff --git a/CHANGELOG.md b/CHANGELOG.md index 825e0e74e..5c0810d7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,11 @@ store's `connect`/`act`, and its allocation and 3D model choices are the kit's `UiSegmented`. `initDrawTool` takes `connect(actions)`, which returns their disconnect, and no longer listens on the Draw controls; its view - publishes `color` and `label` too, and the shape choice is a `UiSegmented`. `syncScopeControls` (`./ui/display`) and + publishes `color` and `label` too, and the shape choice is a `UiSegmented`. + `initGeoFileImport` takes `connect(actions)` the same way and no longer + listens on the Import button, picker or list, nor restores focus itself: + the `GeoImport` view does both, and its `publish` need not settle once the + list renders. `syncScopeControls` (`./ui/display`) and `shouldHideCollapsedRightPanels` (`./ui/layout`) are deprecated: the Display view and the rail now own what they did. `./ui/input` imports Vue (samehkhamis). diff --git a/docs/VUE-MIGRATION.md b/docs/VUE-MIGRATION.md index 3a444ed11..8523d8e85 100644 --- a/docs/VUE-MIGRATION.md +++ b/docs/VUE-MIGRATION.md @@ -133,9 +133,10 @@ a feature supplying only its accent. `gev-display` store, which VisualSettings publishes to, and the 3D aircraft group, the Draw controls and the imported file list, which AircraftDisplay, the draw tool and the file import publish beside it; - the controls call DisplayBindings and the draw tool through the store's - `connect`/`act`, the newest owner of an action answering it; the - file import restores keyboard focus once the list has rendered; the + the controls call DisplayBindings, the draw tool and the file import + through the store's `connect`/`act`, the newest owner of an action + answering it; `GeoImport` hands keyboard focus on before a focused row + goes, with `v-focus-safe-children`; the style parameter rows render through Vue behind `createStyleParameters()`, as a render function so the portable `./ui/input` entry needs no component compiler; the other groups follow one owner at a time), diff --git a/scripts/package-boundaries.json b/scripts/package-boundaries.json index 133cde26b..f1f0de351 100644 --- a/scripts/package-boundaries.json +++ b/scripts/package-boundaries.json @@ -86,6 +86,7 @@ "src/ui/stores/streetLevel.js", "src/ui/rail/RailChips.vue", "src/ui/views/panels/DisplayPanel.vue", + "src/ui/views/panels/GeoImport.vue", "src/ui/stores/display.js", "src/ui/rail/rangeValue.js", "src/ui/rail/classToggle.js", diff --git a/src/app/tools.js b/src/app/tools.js index f1e05f74d..a39ded46b 100644 --- a/src/app/tools.js +++ b/src/app/tools.js @@ -1,4 +1,3 @@ -import { nextTick } from 'vue'; import { SceneDirector } from '../scenes/director.js'; import { initAnnotations } from '../annotations/index.js'; import { initDrawTool } from '../annotations/drawTool.js'; @@ -56,18 +55,21 @@ export function createApplicationTools({ if (window.__gevAnnotations === annotations) delete window.__gevAnnotations; annotations.destroy(); }); + const displayStore = useDisplayStore(applicationPinia()); + // The Display panel's controls answer through the store; returns the + // disconnect. + const connectDisplay = (actions) => { + const token = displayStore.connect(actions); + return () => displayStore.detach(token); + }; // DISPLAY ▸ Draw: the same whiteboard, drawn by hand. It claims the pointer // while a session is open, so its teardown belongs to the application // lifetime rather than to whoever last pressed the button. - const displayStore = useDisplayStore(applicationPinia()); const drawTool = initDrawTool({ viewer, annotations, publish: (view) => displayStore.publishDraw(view), - connect: (actions) => { - const token = displayStore.connect(actions); - return () => displayStore.detach(token); - }, + connect: connectDisplay, }); defer(() => drawTool?.destroy()); // DISPLAY ▸ Import: the person's own GeoJSON/KML/KMZ/GPX files, each its @@ -78,10 +80,8 @@ export function createApplicationTools({ navigation: styleManager, ground: operations.surface?.groundFloor, subscribeMapSource: (listener) => mapStackController?.subscribe?.(listener), - publish: (view) => { - displayStore.publishGeoImport(view); - return nextTick(); - }, + publish: (view) => displayStore.publishGeoImport(view), + connect: connectDisplay, }); defer(() => geoFileImport?.destroy()); // DATA ▸ Recent Imagery: the box tool claims the pointer like Draw and the diff --git a/src/ui/displayPanel.test.mjs b/src/ui/displayPanel.test.mjs index 602cc902f..10c25a5fb 100644 --- a/src/ui/displayPanel.test.mjs +++ b/src/ui/displayPanel.test.mjs @@ -395,9 +395,10 @@ test('the imported files render what the file import publishes and keep focus', revoke() {}, }), flyToSource() {}, - publish: (view) => { - store.publishGeoImport(view); - return nextTick(); + publish: (view) => store.publishGeoImport(view), + connect: (actions) => { + const token = store.connect(actions); + return () => store.detach(token); }, }); t.after(() => tool.destroy()); diff --git a/src/ui/geoFileImport.js b/src/ui/geoFileImport.js index 9390cbaf9..5358e6cf2 100644 --- a/src/ui/geoFileImport.js +++ b/src/ui/geoFileImport.js @@ -1,6 +1,5 @@ /** Session-local file controls, serial loading and application-owned navigation. */ import { - GEO_FILE_ACCEPT, GeoImportError, MAX_IMPORTED_FILES, loadGeoFile, @@ -19,12 +18,13 @@ import { flyToDataSource } from './flyToDataSource.js'; const COLORS = ['#39d0ff', '#ffb547', '#5dff9f', '#ff6b6b', '#8be9ff']; /** - * Wire the Import control. + * Run the Import control. * @param {{viewer: object, load?: Function, document?: Document, - * publish?: Function}} deps + * publish?: Function, connect?: Function}} deps * `load(file, {viewer, color})` defaults to `loadGeoFile`; tests pass a double. * `publish({rows, hint})` hands the file list and hint to the view that - * renders them; it may return a promise that settles once they render. + * renders them. `connect(actions)` answers the view's pick and row buttons + * and returns their disconnect. * @returns {{destroy: Function, importFiles: Function, list: Function, * setVisible: Function, flyTo: Function, remove: Function, * diagnostics: Function}|null} @@ -38,11 +38,11 @@ export function initGeoFileImport({ ground = null, subscribeMapSource = null, publish = () => {}, + connect = () => () => {}, }) { - const button = doc?.getElementById('geo-import-button'); - const input = doc?.getElementById('geo-import-input'); - const listEl = doc?.getElementById('geo-import-list'); - if (!viewer || !button || !input || !listEl) return null; + // A page without the Import control (the Display controls view omitted) + // has no file import. + if (!viewer || !doc?.getElementById('geo-import-button')) return null; const imports = []; const listeners = []; @@ -120,36 +120,6 @@ export function initGeoFileImport({ if (!pending) scheduleGround(); }); - /** Which row button has focus, so a re-render can give it back. */ - function focusedAction() { - const active = doc.activeElement; - if (!active || !listEl.contains?.(active)) return null; - const row = active.closest?.('[data-import-id]'); - return row - ? { - id: row.dataset.importId, - action: active.dataset?.action, - element: active, - next: [], - } - : null; - } - - function restoreFocus(was) { - // Focus moved on while the list rendered: leave it where it went. - const active = doc.activeElement; - if (active && active !== was.element && active !== doc.body) return; - const rows = [...listEl.querySelectorAll('[data-import-id]')]; - const rowOf = (id) => rows.find((r) => r.dataset.importId === id); - // The row is gone (removed): the next row's same control, else the - // previous row's, else the Import button. - const row = [was.id, ...was.next].map(rowOf).find(Boolean); - const target = - row?.querySelector?.(`button[data-action="${was.action}"]`) || - (row ? row.querySelector?.('button') : button); - target?.focus?.(); - } - /** Publish the file list and hint; the view renders them. */ function present() { return publish({ @@ -191,23 +161,6 @@ export function initGeoFileImport({ }); } - // The focus to give back once the view renders; a second change before - // then keeps the first, whose control the page still shows. - let pendingFocus = null; - - function renderList(keep = focusedAction()) { - pendingFocus ||= keep; - const rendered = present(); - const was = pendingFocus; - // A removed row leaves the page only once the view renders. - if (was) - Promise.resolve(rendered).then(() => { - if (destroyed || pendingFocus !== was) return; - pendingFocus = null; - restoreFocus(was); - }); - } - const find = (id) => imports.find((item) => item.id === id) || null; /** Show or hide one imported file. */ @@ -217,7 +170,7 @@ export function initGeoFileImport({ item.visible = Boolean(visible); item.dataSource.show = item.visible; if (item.visible) scheduleGround(); - renderList(); + present(); render(); return true; } @@ -262,16 +215,10 @@ export function initGeoFileImport({ function remove(id) { const at = imports.findIndex((item) => item.id === id); if (at < 0) return false; - const keep = focusedAction(); - if (keep) - keep.next = [ - ...imports.slice(at + 1), - ...imports.slice(0, at).reverse(), - ].map((other) => other.id); const [item] = imports.splice(at, 1); viewer.dataSources?.remove?.(item.dataSource, true); item.revoke?.(); - renderList(keep); + present(); render(); return true; } @@ -353,7 +300,7 @@ export function initGeoFileImport({ } loaded.mount?.(); imports.push(item); - renderList(); + present(); render(); outcomes.push({ name: fileName, @@ -375,22 +322,12 @@ export function initGeoFileImport({ } // ---- controls ---------------------------------------------------------- - input.accept = GEO_FILE_ACCEPT; - listen(button, 'click', () => input.click()); - listen(input, 'change', () => { - const files = [...(input.files || [])]; - // Clear the selection so choosing the same file again fires `change`. - input.value = ''; - if (files.length) void importFiles(files); - }); - listen(listEl, 'click', (event) => { - const btn = event.target?.closest?.('button[data-action]'); - const id = btn?.closest?.('[data-import-id]')?.dataset.importId; - if (!btn || !id) return; - const action = btn.dataset.action; - if (action === 'toggle') setVisible(id, !find(id)?.visible); - else if (action === 'fly') flyTo(id); - else if (action === 'remove') remove(id); + // The view opens the picker; its pick and the row buttons call these. + const disconnect = connect({ + importGeoFiles: (files) => void importFiles(files), + toggleGeoImport: (id) => setVisible(id, !find(id)?.visible), + flyToGeoImport: flyTo, + removeGeoImport: remove, }); // ---- drop onto the globe ---------------------------------------------- @@ -433,7 +370,7 @@ export function initGeoFileImport({ if (files.length) void importFiles(files); }); - renderList(); + present(); const handle = { importFiles, @@ -459,6 +396,7 @@ export function initGeoFileImport({ if (destroyed) return; destroyed = true; unregisterUnsavedWork(); + disconnect(); lifetime.abort(); removeMapSource?.(); clearTimeout(groundTimer); diff --git a/src/ui/geoFileImport.test.mjs b/src/ui/geoFileImport.test.mjs index 570adbb9f..693fb02de 100644 --- a/src/ui/geoFileImport.test.mjs +++ b/src/ui/geoFileImport.test.mjs @@ -32,46 +32,12 @@ class FakeClassList { } } -// The one focused element across every fake document (tests run one at a time). -const focus = { active: null }; - class FakeElement { constructor(tagName = 'div', id = '') { this.tagName = tagName.toUpperCase(); this.id = id; this.classList = new FakeClassList(); - this.dataset = {}; - this.style = {}; - this.attributes = new Map(); - this.children = []; - this.parent = null; this.listeners = []; - this.textContent = ''; - this.value = ''; - this.clicks = 0; - } - set className(v) { - this.classList = new FakeClassList(); - String(v) - .split(/\s+/) - .filter(Boolean) - .forEach((n) => this.classList.add(n)); - } - setAttribute(n, v) { - this.attributes.set(n, String(v)); - } - getAttribute(n) { - return this.attributes.get(n) ?? null; - } - append(...kids) { - for (const k of kids) { - k.parent = this; - this.children.push(k); - } - } - replaceChildren(...kids) { - this.children = []; - this.append(...kids); } addEventListener(type, listener, options) { this.listeners.push({ type, listener, options }); @@ -92,71 +58,15 @@ class FakeElement { ...event, }); } - click() { - this.clicks += 1; - this.emit('click'); - } - focus() { - focus.active = this; - } - contains(el) { - for (let at = el; at; at = at.parent) if (at === this) return true; - return false; - } - *walk() { - for (const kid of this.children) { - yield kid; - yield* kid.walk(); - } - } - querySelectorAll(selector) { - if (selector !== '[data-import-id]') throw new Error(`fake: ${selector}`); - return [...this.walk()].filter((el) => el.dataset.importId); - } - querySelector(selector) { - const exact = /^button\[data-action="([a-z]+)"\]$/.exec(selector); - for (const el of this.walk()) { - if (el.tagName !== 'BUTTON') continue; - if (selector === 'button' || (exact && el.dataset.action === exact[1])) - return el; - } - return null; - } - closest(selector) { - for (let el = this; el; el = el.parent) { - if ( - selector === 'button[data-action]' && - el.tagName === 'BUTTON' && - el.dataset.action - ) - return el; - if (selector === '[data-import-id]' && el.dataset.importId) return el; - } - return null; - } } -function setup({ - load, - navigation, - flyToSource, - subscribeMapSource, - rendered, -} = {}) { - focus.active = null; +function setup({ load, navigation, flyToSource, subscribeMapSource } = {}) { const els = { 'geo-import-button': new FakeElement('button', 'geo-import-button'), - 'geo-import-input': new FakeElement('input', 'geo-import-input'), - 'geo-import-list': new FakeElement('ul', 'geo-import-list'), - 'geo-import-hint': new FakeElement('span', 'geo-import-hint'), }; const document = { body: new FakeElement('body'), getElementById: (id) => els[id] ?? null, - createElement: (tag) => new FakeElement(tag), - get activeElement() { - return focus.active; - }, }; Object.setPrototypeOf(document, new FakeElement('document')); const added = []; @@ -164,6 +74,8 @@ function setup({ const flights = []; let renders = 0; let shown = null; + // The view's pick and row buttons, as the display store holds them. + let actions = null; const viewer = { container: new FakeElement('div', 'cesiumContainer'), scene: { requestRender: () => (renders += 1) }, @@ -203,7 +115,10 @@ function setup({ flyToSource: flyToSource || ((_viewer, ds) => flights.push(ds)), publish: (view) => { shown = view; - return rendered?.(); + }, + connect: (next) => { + actions = next; + return () => (actions = null); }, }); return { @@ -218,24 +133,21 @@ function setup({ loads, renders: () => renders, shown: () => shown, + actions: () => actions, + /** A control's change, as the view sends it. */ + act: (name, ...args) => actions?.[name]?.(...args), }; } const file = (name, extra = {}) => ({ name, size: 10, ...extra }); const rows = (t) => t.shown().rows; const action = (row, name) => row.actions.find((a) => a.action === name); -// A click on a row button, as the list's delegated listener receives it. -const press = (t, row, name) => - t.els['geo-import-list'].emit('click', { - target: { - closest: (selector) => - selector === 'button[data-action]' - ? { - dataset: { action: name }, - closest: () => ({ dataset: { importId: row.id } }), - } - : null, - }, - }); +// A row button's click, as the view sends it. +const ROW_ACTIONS = { + toggle: 'toggleGeoImport', + fly: 'flyToGeoImport', + remove: 'removeGeoImport', +}; +const press = (t, row, name) => t.act(ROW_ACTIONS[name], row.id); // Runs first: tools left by later tests would answer the same registry. test('imported files make the language switch ask first', async () => { @@ -262,19 +174,11 @@ test('returns null without its markup or viewer', () => { ); }); -test('the Import button opens the picker and a pick imports each file', async () => { +test('a pick imports each file', async () => { const t = setup(); - t.els['geo-import-button'].click(); - assert.equal(t.els['geo-import-input'].clicks, 1); - assert.equal( - t.els['geo-import-input'].accept, - '.geojson,.json,.kml,.kmz,.gpx', - ); - const out = await t.tool.importFiles([file('a.geojson'), file('b.kml')]); - assert.deepEqual( - out.map((o) => o.ok), - [true, true], - ); + t.act('importGeoFiles', [file('a.geojson'), file('b.kml')]); + for (let turn = 0; turn < 20 && t.added.length < 2; turn += 1) + await new Promise((r) => setImmediate(r)); assert.equal(t.added.length, 2); assert.notEqual( t.loads[0].opts.color, @@ -395,13 +299,9 @@ test('destroy removes every file and listener and the window handle', async () = t.tool.destroy(); assert.equal(t.removed.length, 2); assert.deepEqual(t.revoked.sort(), ['a.geojson', 'b.geojson']); - for (const el of [ - t.viewer.container, - t.els['geo-import-button'], - t.els['geo-import-input'], - t.els['geo-import-list'], - ]) - assert.equal(el.listeners.length, 0); + assert.equal(t.viewer.container.listeners.length, 0); + assert.equal(t.document.listeners.length, 0); + assert.equal(t.actions(), null, 'the view is disconnected'); assert.deepEqual(t.shown(), { rows: [], hint: '' }); assert.equal(globalThis.window.__gevGeoImport, undefined); assert.equal(t.tool.diagnostics().destroyed, true); @@ -451,13 +351,13 @@ test('the application composition wires the import and owns its teardown', () => /navigation: styleManager/, ); assert.ok(tools.indexOf('defer(() => geoFileImport?.destroy());') > importAt); - // The list renders from the store once Vue has flushed. + // The list renders from the store and its controls answer through it. assert.match( tools.slice(importAt, importAt + 400), - /displayStore\.publishGeoImport\(view\);\s*return nextTick\(\);/, + /publish: \(view\) => displayStore\.publishGeoImport\(view\),\s*connect: connectDisplay,/, ); const markup = fs.readFileSync( - new URL('./views/panels/DisplayPanel.vue', import.meta.url), + new URL('./views/panels/GeoImport.vue', import.meta.url), 'utf8', ); for (const id of [ @@ -468,7 +368,7 @@ test('the application composition wires the import and owns its teardown', () => ]) assert.ok( markup.includes(`id="${id}"`), - `${id} missing from the DISPLAY template`, + `${id} missing from the Import template`, ); }); @@ -685,83 +585,6 @@ test('a refused deferred stamp (Cockpit) imports without flying', async () => { assert.equal(flown.length, 0); }); -// The rendered view as the owner reads it back: a row per published file, -// each with its buttons. `settle()` finishes the held render. -async function heldRender(names) { - let settle = () => {}; - const t = setup({ rendered: () => new Promise((r) => (settle = r)) }); - await t.tool.importFiles(names.map((name) => file(name))); - const list = t.els['geo-import-list']; - const renderRow = ({ id }) => { - const row = new FakeElement('li'); - row.dataset.importId = id; - for (const name of ['toggle', 'fly', 'remove']) { - const btn = new FakeElement('button'); - btn.dataset.action = name; - row.append(btn); - } - return row; - }; - const render = async () => { - list.replaceChildren(...rows(t).map(renderRow)); - settle(); - await new Promise((r) => setImmediate(r)); - }; - await render(); - const removeButton = (at) => list.children[at].children[2]; - return { t, list, render, removeButton }; -} - -test('keyboard focus moves on only once the list without the removed row renders', async () => { - const { t, list, render, removeButton } = await heldRender([ - 'a.geojson', - 'b.geojson', - ]); - const first = removeButton(0); - first.focus(); - press(t, rows(t)[0], 'remove'); - await new Promise((r) => setImmediate(r)); - assert.equal(t.document.activeElement, first, 'not before the view renders'); - await render(); - assert.equal(list.children.length, 1); - assert.equal( - t.document.activeElement, - removeButton(0), - 'the same control on the next row', - ); - press(t, rows(t)[0], 'remove'); - await render(); - assert.equal(t.document.activeElement, t.els['geo-import-button']); -}); - -test('focus moved elsewhere while the list renders stays where it went', async () => { - const { t, render, removeButton } = await heldRender([ - 'a.geojson', - 'b.geojson', - ]); - removeButton(0).focus(); - press(t, rows(t)[0], 'remove'); - t.els['geo-import-input'].focus(); - await render(); - assert.equal(t.document.activeElement, t.els['geo-import-input']); -}); - -test('two removes before one render give focus to the first one’s next row', async () => { - const names = ['a', 'b', 'c', 'd', 'e'].map((n) => `${n}.geojson`); - const { t, list, render, removeButton } = await heldRender(names); - removeButton(1).focus(); - press(t, rows(t)[1], 'remove'); - // The view has not rendered: b's button still holds focus. - press(t, rows(t)[2], 'remove'); - await render(); - assert.deepEqual( - t.tool.list().map((f) => f.name), - ['a.geojson', 'c.geojson', 'e.geojson'], - ); - assert.equal(t.document.activeElement, removeButton(1)); - assert.equal(list.children[1].dataset.importId, rows(t)[1].id); -}); - test('KML and KMZ rows say they keep their own styles instead of showing a color', async () => { const t = setup({ load: async (f) => ({ diff --git a/src/ui/stores/display.js b/src/ui/stores/display.js index bbe48eb97..f5e991892 100644 --- a/src/ui/stores/display.js +++ b/src/ui/stores/display.js @@ -5,9 +5,8 @@ import { markRaw, shallowRef } from 'vue'; * The Display panel as its view renders it. VisualSettings presents its * groups (`_presentDisplay()`), AircraftDisplay the 3D aircraft group, the * draw tool its controls and the file import its list, and each publishes - * here. The controls' changes reach their owners (DisplayBindings and the - * draw tool) through act(); the file import keeps its listeners on its - * controls. VisualSettings also + * here. The controls' changes reach their owners (DisplayBindings, the draw + * tool and the file import) through act(). VisualSettings also * publishes the active style, which the dock and the scene chrome show, and * the map source controls publish their chip row beside it. */ @@ -70,7 +69,8 @@ export const useDisplayStore = defineStore('gev-display', () => { /** * The owners answering the panel's controls, oldest first: DisplayBindings - * the Display controls and the dock's style buttons, the draw tool its own. + * the Display controls and the dock's style buttons, the draw tool and the + * file import their own. * @type {Array<{actions: object}>} */ const owners = []; diff --git a/src/ui/views/panels/DisplayPanel.vue b/src/ui/views/panels/DisplayPanel.vue index b5e82c85d..bdc5644e7 100644 --- a/src/ui/views/panels/DisplayPanel.vue +++ b/src/ui/views/panels/DisplayPanel.vue @@ -6,14 +6,15 @@ import { vPanelState } from '../../rail/panelState.js'; import PanelCollapseButton from '../../rail/PanelCollapseButton.vue'; import { vRangeValue } from '../../rail/rangeValue.js'; import UiSegmented from '../../kit/UiSegmented.vue'; +import GeoImport from './GeoImport.vue'; import { useDisplayStore } from '../../stores/display.js'; // The Display panel. VisualSettings presents the HUD, Sonar, Scope, -// Celestial, detection, bloom and sharpen, AircraftDisplay the 3D group, the -// draw tool its controls and the file import its list, and each publishes -// them; until they do, the panel shows its initial markup. +// Celestial, detection, bloom and sharpen, AircraftDisplay the 3D group and +// the draw tool its controls, and each publishes them; until they do, the +// panel shows its initial markup. GeoImport renders the file import. const display = useDisplayStore(); -const { view, models3d, draw, geoImport } = storeToRefs(display); +const { view, models3d, draw } = storeToRefs(display); // A control's change goes to its owner (DisplayBindings or the draw tool). const act = (name, ...args) => display.act(name, ...args); const integer = (event) => parseInt(event.target.value, 10); @@ -48,8 +49,6 @@ const allocationOptions = [ { value: 'ELASTIC', label: t('Elastic'), attrs: { id: 'detection-allocation-elastic', 'data-allocation': 'ELASTIC' } }, { value: 'WEIGHTED', label: t('Weighted'), attrs: { id: 'detection-allocation-weighted', 'data-allocation': 'WEIGHTED' } }, ]; -const geoRows = computed(() => geoImport.value?.rows ?? []); -const geoListed = computed(() => geoRows.value.length > 0);