From 6fbc98952f31cb6ad284233a4dbd7c7075b9df83 Mon Sep 17 00:00:00 2001 From: Denis Groshev Date: Thu, 12 Nov 2026 16:53:43 +0100 Subject: [PATCH] fix(cockpit): a provider's methods keep their `this`, a hold answers true or false, and a cut text never splits a surrogate pair Provider methods and worded members are called on the provider itself, so a class-based provider works. A hold that answers false refuses entry and is released; any answer but true or false refuses it too, released, with one warning. An answer whose `then` getter throws is handled as a throw, and a text cut at its cap never leaves half an emoji. --- docs/APPLICATION.md | 15 ++-- src/cockpitSubjectProviders.js | 97 +++++++++++++-------- src/cockpitSubjectProviders.test.mjs | 123 +++++++++++++++++++++++++++ src/ui/cockpitSubjectView.test.mjs | 113 ++++++++++++++++++++++-- src/ui/cockpitTrackingController.js | 20 +++-- 5 files changed, 315 insertions(+), 53 deletions(-) diff --git a/docs/APPLICATION.md b/docs/APPLICATION.md index 5ab894d48..3478aafc4 100644 --- a/docs/APPLICATION.md +++ b/docs/APPLICATION.md @@ -239,7 +239,8 @@ providers, checked once when the controls are built warning. Entry rides the most recently selected subject of all kinds and needs the provider's `layerId` on; the provider then holds its subject, hands the camera a frame and the generic HUD a view model on every update, and releases -it on exit. Provider methods are synchronous; one that throws or returns a +it on exit. Provider methods are synchronous and are called on the provider, +so a class instance's methods keep their `this`; one that throws or returns a promise is treated as having failed. ```js @@ -317,7 +318,11 @@ user's selection. A frame with `heightM: null` stands on the shared floor, or where it last stood while that floor is cold. If no height has been known since entry, the view waits, showing ACQUIRING SURFACE, for at most five seconds -(`COCKPIT_GROUND_WAIT_TIMEOUT_MS`), then ends and releases the subject. A -`hold` that throws or returns a promise refuses entry and is followed by -`release(id, { restore: false })`, again once a returned promise settles, so -`release` must be harmless when called more than once. +(`COCKPIT_GROUND_WAIT_TIMEOUT_MS`), then ends and releases the subject. + +`hold(id)` returns `true` once it holds the subject, or `false` to refuse +entry; any other answer, `undefined` included, refuses entry too, with one +warning. A refused hold, and one that throws or returns a promise, is followed +by `release(id, { restore: false })`, again once a returned promise settles, +so `release` must be harmless for a subject it does not hold and when called +more than once. diff --git a/src/cockpitSubjectProviders.js b/src/cockpitSubjectProviders.js index aba0a2b75..bc58d8d6f 100644 --- a/src/cockpitSubjectProviders.js +++ b/src/cockpitSubjectProviders.js @@ -66,12 +66,17 @@ const clamp = (value, low, high) => Math.min(high, Math.max(low, value)); /** * `value` as display text: trimmed, at most `max` characters (the last one - * an ellipsis when cut); '' for anything that is not a string or a number. + * an ellipsis when cut, the cut never inside a surrogate pair, so an emoji is + * kept whole or left out); '' for anything that is not a string or a number. */ export function cockpitSubjectText(value, max) { if (typeof value !== 'string' && !finite(value)) return ''; const text = String(value).replace(/\s+/g, ' ').trim(); - return text.length > max ? `${text.slice(0, max - 1)}…` : text; + if (text.length <= max) return text; + let end = max - 1; + const last = text.charCodeAt(end - 1); + if (last >= 0xd800 && last <= 0xdbff) end -= 1; + return `${text.slice(0, end)}…`; } const tone = (value) => (TONES.has(value) ? value : 'normal'); @@ -101,13 +106,13 @@ function labelsOf(raw) { /** * A provider's worded member as it reads now: the value, or what the - * function it is returns (so it can follow the page's language); undefined - * when that throws. + * function it is returns when called on `provider` (so it can follow the + * page's language); undefined when that throws. */ -function wordedNow(value) { +function wordedNow(value, provider) { if (typeof value !== 'function') return value; try { - return value(); + return value.call(provider); } catch { return undefined; } @@ -132,10 +137,11 @@ function refusal(provider, kinds) { typeof provider.isEntryAllowed !== 'function' ) return 'isEntryAllowed is not a function'; - if (!faceOf(wordedNow(provider.entryFace))) return 'malformed entry face'; - if (!labelsOf(wordedNow(provider.labels))) + if (!faceOf(wordedNow(provider.entryFace, provider))) + return 'malformed entry face'; + if (!labelsOf(wordedNow(provider.labels, provider))) return 'labels need view, exit and exitText'; - const look = wordedNow(provider.look); + const look = wordedNow(provider.look, provider); if ( provider.look != null && (!isObject(look) || !cockpitSubjectText(look.aheadLabel, 64)) @@ -159,21 +165,24 @@ function refusal(provider, kinds) { * on a bridge, resting at `restPitchDeg` (default 0, -90..90), its Look * ahead button named `aheadLabel`; * `entryFace`, `labels` and `look` may each be a function returning the - * object: it is called again whenever they are shown and when the page's - * language changes, so its words can follow it (`./i18n/locale`). A read - * that throws or is malformed keeps the last good one; `look` is on or off - * and `restPitchDeg` fixed as first read; + * object: it is called on the provider again whenever they are shown and + * when the page's language changes, so its words can follow it + * (`./i18n/locale`). A read that throws or is malformed keeps the last + * good one; `look` is on or off and `restPitchDeg` fixed as first read; * - `cameraIntervalMs` (optional): 0..1000, null for the Cockpit's default; 0 * updates the camera every rendered frame; * - `headingSlewDps` (optional): how fast the view turns to a new heading, * positive or Infinity, null for the Cockpit's default; * - `candidate()`, `isEntryAllowed(candidate)` (optional), `hold(id)`, * `read(nowMs, ctx)`, `hud(frame, ctx)` and `release(id, {restore})`, all - * synchronous (createCockpitSubjectCaller). `isEntryAllowed` allows entry - * only by returning `true`. A `hold` that throws or returns a promise - * refuses entry and is followed by `release(id, {restore: false})`, and by - * another once that promise settles; `release` must therefore be harmless - * when called again for a subject it already gave back. + * synchronous (createCockpitSubjectCaller) and called on the provider, so + * a class instance's methods keep their `this`. `isEntryAllowed` allows + * entry only by returning `true`. `hold` returns `true` once it holds the + * subject, or `false` to refuse entry; any other answer refuses it too, + * with one warning. A refused hold, and one that throws or returns a + * promise, is followed by `release(id, {restore: false})`, and by another + * once that promise settles; `release` must therefore be harmless for a + * subject it does not hold or already gave back. * * A provider that is not so is left out with one warning naming it. * @param {unknown} list @@ -199,9 +208,10 @@ export function cockpitSubjectProviders( kinds.add(provider.kind); // Worded members are read again on each use (see above); the last good // read stands in for a bad one. - let face = faceOf(wordedNow(provider.entryFace)); - let labels = labelsOf(wordedNow(provider.labels)); - const firstLook = wordedNow(provider.look); + const worded = (value) => wordedNow(value, provider); + let face = faceOf(worded(provider.entryFace)); + let labels = labelsOf(worded(provider.labels)); + const firstLook = worded(provider.look); const lookOf = (raw) => Object.freeze({ restPitchDeg: finite(firstLook.restPitchDeg) @@ -217,16 +227,16 @@ export function cockpitSubjectProviders( priority: finite(provider.priority) ? provider.priority : 0, order, get entryFace() { - face = faceOf(wordedNow(provider.entryFace)) ?? face; + face = faceOf(worded(provider.entryFace)) ?? face; return face; }, get labels() { - labels = labelsOf(wordedNow(provider.labels)) ?? labels; + labels = labelsOf(worded(provider.labels)) ?? labels; return labels; }, get look() { if (!look) return null; - const raw = wordedNow(provider.look); + const raw = worded(provider.look); if (isObject(raw) && cockpitSubjectText(raw.aheadLabel, 64)) look = lookOf(raw); return look; @@ -242,12 +252,16 @@ export function cockpitSubjectProviders( (finite(provider.headingSlewDps) && provider.headingSlewDps > 0) ? provider.headingSlewDps : null, - candidate: provider.candidate, - isEntryAllowed: provider.isEntryAllowed ?? (() => true), - hold: provider.hold, - read: provider.read, - hud: provider.hud, - release: provider.release, + // Bound, so a method that uses `this` (a class instance's, say) + // still reads its own provider when called on this copy. + candidate: provider.candidate.bind(provider), + isEntryAllowed: provider.isEntryAllowed + ? provider.isEntryAllowed.bind(provider) + : () => true, + hold: provider.hold.bind(provider), + read: provider.read.bind(provider), + hud: provider.hud.bind(provider), + release: provider.release.bind(provider), }), ); }); @@ -506,6 +520,10 @@ export function cockpitSubjectHudView(raw) { * would be (its rejection, if any, is swallowed), with one warning, and * `onSettled`, when given, runs once the promise has settled, so a caller * can undo work that lands late. + * + * `call.warnOnce(provider, method, message, error)` warns on the same + * once-per-provider-and-method footing, for an answer the caller finds + * malformed. * @param {{warn?: (message: string, error: unknown) => void}} [options] */ export function createCockpitSubjectCaller({ @@ -521,7 +539,7 @@ export function createCockpitSubjectCaller({ error, ); }; - return function call( + function call( provider, method, args = [], @@ -529,17 +547,20 @@ export function createCockpitSubjectCaller({ onSettled = null, ) { let result; + let thenable; try { result = provider[method](...args); + // Reading `then` runs provider code too (a getter may throw), so it is + // guarded as the call is. + thenable = + result !== null && + (typeof result === 'object' || typeof result === 'function') && + typeof result.then === 'function'; } catch (error) { warnOnce(provider, method, 'failed', error); return fallback; } - if ( - result !== null && - (typeof result === 'object' || typeof result === 'function') && - typeof result.then === 'function' - ) { + if (thenable) { // Its work may still land: `onSettled` runs once it has, either way. const settled = () => { if (typeof onSettled === 'function') onSettled(); @@ -554,5 +575,7 @@ export function createCockpitSubjectCaller({ return fallback; } return result; - }; + } + call.warnOnce = warnOnce; + return call; } diff --git a/src/cockpitSubjectProviders.test.mjs b/src/cockpitSubjectProviders.test.mjs index b49545eb2..54424d5bf 100644 --- a/src/cockpitSubjectProviders.test.mjs +++ b/src/cockpitSubjectProviders.test.mjs @@ -389,6 +389,21 @@ test('texts are trimmed, collapsed and cut with an ellipsis', () => { assert.equal(cockpitSubjectText(NaN, 10), ''); }); +test('a cut never splits a surrogate pair', () => { + // The cut would fall between the emoji's halves: it is left out whole. + assert.equal(cockpitSubjectText('abcd😀efgh', 6), 'abcd…'); + // One that fits before the cut is kept whole. + assert.equal(cockpitSubjectText('abc😀efgh', 6), 'abc😀…'); + const lone = + /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(? { const warnings = []; const call = createCockpitSubjectCaller({ @@ -427,6 +442,114 @@ test('a provider method that returns a promise is refused as a throw is, warning await new Promise((resolve) => setTimeout(resolve, 0)); }); +test('an answer whose `then` throws when read is handled as a throw', () => { + const warnings = []; + const call = createCockpitSubjectCaller({ + warn: (message) => warnings.push(message), + }); + const tricky = { + kind: 'example-kind', + hold: () => ({ + get then() { + throw new Error('no then'); + }, + }), + }; + assert.equal(call(tricky, 'hold', ['a1'], 'refused'), 'refused'); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /example-kind hold\(\) failed/); +}); + +test('the caller’s warnOnce shares its once-per-provider-and-method record', () => { + const warnings = []; + const call = createCockpitSubjectCaller({ + warn: (message) => warnings.push(message), + }); + const ride = { + kind: 'example-kind', + hold() { + throw new Error('boom'); + }, + }; + call.warnOnce(ride, 'read', 'is odd'); + call.warnOnce(ride, 'read', 'is odd again'); + call(ride, 'hold', ['a1']); + call.warnOnce(ride, 'hold', 'is odd'); + assert.deepEqual(warnings, [ + '[Cockpit] subject provider example-kind read() is odd', + '[Cockpit] subject provider example-kind hold() failed', + ]); +}); + +test('a class-based provider keeps its `this`, in its methods and its worded members', () => { + class Ride { + #word = 'RIDE'; + #held = null; + kind = 'example-kind'; + layerId = 'example-layer'; + speak(word) { + this.#word = word; + } + entryFace() { + return { icon: 'commute', label: this.#word }; + } + get labels() { + return { + view: `${this.#word} view`, + exit: 'Exit ride view', + exitText: 'EXIT RIDE', + }; + } + look() { + return { aheadLabel: `Look ahead, ${this.#word}` }; + } + candidate() { + return { id: 'a1', name: this.#word }; + } + isEntryAllowed(candidate) { + return candidate.id === 'a1' && this.#held === null; + } + hold(id) { + this.#held = id; + return true; + } + read() { + return this.#held; + } + hud(frame) { + return { title: `${this.#word} ${frame}` }; + } + release() { + this.#held = null; + return 'released'; + } + } + const instance = new Ride(); + const { kept, warnings } = keep([instance]); + assert.deepEqual(warnings, []); + const [ride] = kept; + assert.equal(ride.entryFace.label, 'RIDE'); + assert.equal(ride.labels.view, 'RIDE view'); + assert.equal(ride.look.aheadLabel, 'Look ahead, RIDE'); + instance.speak('FAHRT'); + assert.equal(ride.entryFace.label, 'FAHRT', 'read again, on the instance'); + assert.equal(ride.look.aheadLabel, 'Look ahead, FAHRT'); + // Called on the kept copy, as the Cockpit calls them. + const failures = []; + const call = createCockpitSubjectCaller({ + warn: (message) => failures.push(message), + }); + assert.deepEqual(call(ride, 'candidate'), { id: 'a1', name: 'FAHRT' }); + assert.equal(ride.isEntryAllowed({ id: 'a1' }), true); + assert.equal(call(ride, 'hold', ['a1']), true); + assert.equal(ride.isEntryAllowed({ id: 'a1' }), false); + assert.equal(call(ride, 'read', [0, {}]), 'a1'); + assert.deepEqual(call(ride, 'hud', ['a1', {}]), { title: 'FAHRT a1' }); + assert.equal(call(ride, 'release', ['a1', { restore: true }]), 'released'); + assert.equal(call(ride, 'read', [0, {}]), null); + assert.deepEqual(failures, []); +}); + test('labels, entry face and look given as functions are read again on each use', () => { let lang = 'en'; const words = { diff --git a/src/ui/cockpitSubjectView.test.mjs b/src/ui/cockpitSubjectView.test.mjs index 53c870d47..339045af2 100644 --- a/src/ui/cockpitSubjectView.test.mjs +++ b/src/ui/cockpitSubjectView.test.mjs @@ -485,15 +485,16 @@ test('leaving by the exit control asks the provider to restore its subject', asy assert.deepEqual(lastRelease(ride.calls), ['release', 'a1', true]); }); -test('entry fails, releasing nothing it did not hold, when hold or the first frame fails', async (t) => { - const refused = subjectProvider(); - refused.subject.hold = false; - let app = fixture(t, { providers: [refused] }); +test('entry fails when the gate is shut, the hold fails or no first frame comes, releasing only what it held', async (t) => { + const shut = subjectProvider(); + shut.subject.allowed = false; + let app = fixture(t, { providers: [shut] }); app.cockpit.syncEntry(); assert.equal(app.cockpit.enter(), false); assert.equal( - refused.calls.some(([name]) => name === 'release'), + shut.calls.some(([name]) => name === 'hold' || name === 'release'), false, + 'a shut gate neither holds nor releases', ); const blank = subjectProvider(); blank.subject.frame = null; @@ -525,6 +526,108 @@ test('entry fails, releasing nothing it did not hold, when hold or the first fra assert.match(app.warned[0], /returned a promise/); }); +test('a hold that answers false refuses entry; any answer but true or false warns once; each is released', async (t) => { + for (const [answer, warns] of [ + [false, false], + [undefined, true], + [1, true], + ['held', true], + ]) { + const ride = subjectProvider(); + ride.subject.hold = answer; + const { cockpit, warned } = fixture(t, { providers: [ride] }); + const holdWarnings = () => + warned.filter((message) => + /example-kind hold\(\) must return true or false/.test(message), + ).length; + cockpit.syncEntry(); + assert.equal(cockpit.enter(), false, `hold answered ${String(answer)}`); + assert.equal(cockpit.active, false); + assert.deepEqual(lastRelease(ride.calls), ['release', 'a1', false]); + assert.equal(holdWarnings(), warns ? 1 : 0); + assert.equal(warned.length, warns ? 1 : 0, 'no other warning'); + // Once per provider and method. + assert.equal(cockpit.enter(), false); + assert.equal(holdWarnings(), warns ? 1 : 0); + assert.equal( + ride.calls.filter(([name]) => name === 'release').length, + 2, + 'released each time', + ); + } +}); + +test('a class-based provider keeps its `this` from entry to release', async (t) => { + manualClock(t, 10_000); + const { frame } = subjectProvider().subject; + class Ride { + #word = 'RIDE'; + #held = null; + calls = []; + kind = 'example-kind'; + layerId = 'example-layer'; + entryFace() { + return { icon: 'commute', label: this.#word }; + } + get labels() { + return { + view: `${this.#word} view`, + exit: 'Exit ride view', + exitText: `EXIT ${this.#word}`, + }; + } + look() { + return { aheadLabel: `Look ahead, ${this.#word}` }; + } + candidate() { + return { id: 'a1', name: `Example ${this.#word}` }; + } + isEntryAllowed(candidate) { + return this.#held === null && candidate.id === 'a1'; + } + hold(id) { + this.#held = id; + this.calls.push(['hold', id]); + return true; + } + read() { + this.calls.push(['read', this.#held]); + return this.#held ? { ...frame, id: this.#held } : null; + } + hud(view) { + this.calls.push(['hud', view.id]); + return { ...HUD, title: `${this.#word} ${view.id}` }; + } + release(id, { restore }) { + this.calls.push(['release', id, restore]); + this.#held = null; + } + } + const ride = new Ride(); + const { $, cockpit, warned } = fixture(t, { + providers: [{ definition: ride }], + }); + cockpit.syncEntry(); + await nextTick(); + assert.match($('#cockpit-entry').textContent, /RIDE/); + $('#cockpit-entry').click(); + await settle(); + assert.equal(cockpit.active, true); + assert.equal($('#cockpit-callsign').textContent, 'RIDE a1'); + assert.equal($('#cockpit-hud').getAttribute('aria-label'), 'RIDE view'); + assert.match($('#map-view-switch').textContent, /EXIT RIDE/); + assert.equal( + $('#cockpit-look-ahead').getAttribute('aria-label'), + 'Look ahead, RIDE', + ); + assert.ok(ride.calls.some(([name, id]) => name === 'read' && id === 'a1')); + assert.ok(ride.calls.some(([name, id]) => name === 'hud' && id === 'a1')); + $('#map-view-switch').click(); + assert.equal(cockpit.active, false); + assert.deepEqual(lastRelease(ride.calls), ['release', 'a1', true]); + assert.deepEqual(warned, []); +}); + test('a frame with no height and a cold floor never stands the eye on the ellipsoid', async (t) => { const clock = manualClock(t, 10_000); const ride = subjectProvider(); diff --git a/src/ui/cockpitTrackingController.js b/src/ui/cockpitTrackingController.js index 9f2a6129f..a71ee137e 100644 --- a/src/ui/cockpitTrackingController.js +++ b/src/ui/cockpitTrackingController.js @@ -388,9 +388,9 @@ export function _adoptTrackedEntity(nowMs, suppliedInfo = null) { /** * Enter a provider's view on its candidate: hold it, read a first frame, then - * take the camera as the built-in views do. Entry fails, releasing what was - * held, when the gate is shut, the provider refuses the hold or no frame - * comes. + * take the camera as the built-in views do. Entry fails when the gate is shut, + * the hold answers anything but `true` or no frame comes; past the gate, a + * failed entry releases the subject, since the hold may have taken it. * @param {object} provider From cockpitSubjectProviders. * @returns {boolean} */ @@ -419,12 +419,20 @@ function enterSubject(provider) { failed, releaseLate, ); - if (held === failed) { - // A hold that threw (or broke the contract) may have half happened. + if (held !== true) { + // `false` refuses entry; a hold that threw, returned a promise or + // answered anything else (warned of once) may still have taken the + // subject. Each is released. + if (held !== false && held !== failed) + this.callSubject.warnOnce( + provider, + 'hold', + 'must return true or false; entry refused', + held, + ); release(); return false; } - if (held !== true) return false; const frame = cockpitSubjectFrame( this.callSubject( provider,