From 3b81c226209f243958217e892d77533036f8064f Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Thu, 10 Sep 2026 16:11:29 -0400 Subject: [PATCH] Draw a link only where there is somewhere to go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three parameters in a user's model rendered as links and did nothing when clicked: a Bus Selector's `OutputSignals=a,b`, a Math Function's `Operator=square`, and a Gain's `Gain=finalGain`. Two separate defects, and both are now fixed. The rows themselves were wrong for the first two, and that is core's half: an option-list value and a bus signal name are not references to data, and core v1.15.0 stops reporting them (the Bus Selector case was worse than a spare row — it put the block on the Usage cell of a variable it does not read). The pin moves to v1.15.0 here. This repo's half is the link. `Gain=finalGain` IS a real reference — MATLAB resolves it to the base workspace, which is a live session and not a file — so there is nothing to open, and `linkTarget` is empty. The Value column has always gated its anchor on that; both `paramLinks` arms did not. So a parameter the graph could not resolve still painted accent-blue, underlined on hover, and dispatched a click carrying an empty target that routed nowhere: three ways of saying "link" with nothing behind any of them. The two arms were also spelled twice, which is how they came to disagree with the Value column and with the same rule one branch away. They are now one `_renderParamLinks` shared by the DataType and UsedBy columns, gating on `linkTarget` exactly as the Value column does. The value stays visible either way — an unresolved parameter is an answer, not a defect to hide — and the cell's TEXT is untouched, so what a user sorts, filters and copies does not depend on whether the target resolved. Six tests in `treeTableCells.test.ts`, three of which fail with the fix reverted: no anchor in either column, the property and value still shown, the cell text identical to a resolved one, only the resolved entries of a mixed cell linked, no navigation dispatched from any anchor the cell offers, and the value still highlighted by a search. --- package-lock.json | 10 +-- package.json | 4 +- src/webview/components/dex-tree-table.ts | 53 +++++++++---- test/treeTableCells.test.ts | 94 ++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 21 deletions(-) diff --git a/package-lock.json b/package-lock.json index 060f89e..aad813e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,16 +1,16 @@ { "name": "simulink-data-explorer", - "version": "1.16.0", + "version": "1.17.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "simulink-data-explorer", - "version": "1.16.0", + "version": "1.17.0", "license": "BSD-3-Clause", "dependencies": { "@lit/context": "^1.1.6", - "data-explorer-core": "github:mathworks/data-explorer-core#v1.14.0", + "data-explorer-core": "github:mathworks/data-explorer-core#v1.15.0", "fast-xml-parser": "^5.8.0", "fflate": "^0.8.3", "highlight.js": "^11.11.1", @@ -3184,8 +3184,8 @@ } }, "node_modules/data-explorer-core": { - "version": "1.14.0", - "resolved": "git+ssh://git@github.com/mathworks/data-explorer-core.git#3c3f0645e36cfc74ab05901a83caeeb812434c85", + "version": "1.15.0", + "resolved": "git+ssh://git@github.com/mathworks/data-explorer-core.git#45375876def00580e8d00a3afaa3a476ec689d4c", "license": "BSD-3-Clause", "dependencies": { "fast-xml-parser": "^5.8.0", diff --git a/package.json b/package.json index 3fe6c12..7a4bff5 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "simulink-data-explorer", "displayName": "Simulink Data Explorer", "description": "Explore Simulink models, data dictionaries, MAT-files, and projects as interactive tables and relationship trees.", - "version": "1.16.0", + "version": "1.17.0", "publisher": "mathworks", "icon": "media/icon.png", "private": true, @@ -195,7 +195,7 @@ }, "dependencies": { "@lit/context": "^1.1.6", - "data-explorer-core": "github:mathworks/data-explorer-core#v1.14.0", + "data-explorer-core": "github:mathworks/data-explorer-core#v1.15.0", "fast-xml-parser": "^5.8.0", "fflate": "^0.8.3", "highlight.js": "^11.11.1", diff --git a/src/webview/components/dex-tree-table.ts b/src/webview/components/dex-tree-table.ts index 741b728..33dc334 100644 --- a/src/webview/components/dex-tree-table.ts +++ b/src/webview/components/dex-tree-table.ts @@ -221,6 +221,17 @@ export interface BlockLinkGroup { blocks: { blockName: string; blockPath: string; linkTarget: string }[]; } +// One entry of a `paramLinks` cell, as the host's usageCells.ParamLink builds it. +// `linkTarget` is EMPTY for a parameter whose value the graph could not resolve to +// anything in this workspace, which is a normal answer and not a defect — see +// `_renderParamLinks`, the one place that decides what an empty target renders as. +interface ParamLinkCell { + property: string; + paramName: string; + source: string; + linkTarget: string; +} + // A Usage cell's block links, one group per model: `AFR, AFRMonitor, // MixTarget(EngineCtrl); AFRConst, AFRCheck(FuelInjector)`. A dictionary variable is // used by as many blocks as a model has, so naming the model once per block spent most @@ -2811,6 +2822,32 @@ export class DexTreeTable extends LitElement { return html``; } + // A `paramLinks` list — `Gain=Kp (dict.sldd)` — rendered once for the two columns + // that carry one (DataType and UsedBy). It used to be spelled twice, which is how + // it came to disagree with itself: the Value branch above gates its anchor on + // `linkTarget`, both copies of this did not, and so a param the graph could NOT + // resolve still painted accent-blue, underlined on hover, and dispatched a click + // carrying an empty target that routes nowhere. Three ways of saying "link" and + // nothing behind any of them. + // + // An unresolved param is not a rendering accident to hide — MATLAB itself reports + // `Gain = finalGain` as a real variable reference when `finalGain` lives in the BASE + // workspace, which is a live MATLAB session and not a file anything here can open. + // So the value stays visible and only its linkhood goes. The TEXT is untouched + // either way (`_getCellText` builds it from the same list), so what a user sorts, + // filters and copies does not depend on whether the target resolved. + private _renderParamLinks(paramLinks: ParamLinkCell[], columnId: string): unknown { + return html`${paramLinks.map((p, i) => { + const name = this._highlight(p.paramName, columnId); + return html`${i > 0 ? ', ' : ''}${p.property + '='}${p.linkTarget + ? html` this._onLinkClick(p.linkTarget, e)} + >${name}` + : name}${p.source ? html`${'(' + p.source + ')'}` : ''}`; + })}`; + } + private _renderCellValue(row: TreeTableRow, columnId: string): unknown { const isEditing = this._editingCell?.rowId === row.ID && this._editingCell?.columnId === columnId; @@ -2908,13 +2945,7 @@ export class DexTreeTable extends LitElement { if (columnId === 'DataType') { const val = isCellObject(row.DataType) ? (row.DataType as any) : { text: cellText(row.DataType) }; if ('paramLinks' in val) { - return html`${val.paramLinks.map( - (p: { property: string; paramName: string; source: string; linkTarget: string }, i: number) => - html`${i > 0 ? ', ' : ''}${p.property + '='} this._onLinkClick(p.linkTarget, e)} - >${this._highlight(p.paramName, columnId)}${p.source ? html`${'(' + p.source + ')'}` : ''}`, - )}`; + return this._renderParamLinks(val.paramLinks, columnId); } if ('links' in val) { return html`${val.links.map( @@ -2964,13 +2995,7 @@ export class DexTreeTable extends LitElement { if (!row.UsedBy) return html``; const val = isCellObject(row.UsedBy) ? (row.UsedBy as any) : { text: cellText(row.UsedBy) }; if ('paramLinks' in val) { - return html`${(val as any).paramLinks.map( - (p: { property: string; paramName: string; source: string; linkTarget: string }, i: number) => - html`${i > 0 ? ', ' : ''}${p.property + '='} this._onLinkClick(p.linkTarget, e)} - >${this._highlight(p.paramName, columnId)}${p.source ? html`${'(' + p.source + ')'}` : ''}`, - )}`; + return this._renderParamLinks((val as any).paramLinks, columnId); } if ('blockLinks' in val) { // One `(model)` per group, after its last block. The qualifier is dropped diff --git a/test/treeTableCells.test.ts b/test/treeTableCells.test.ts index 413293c..0fd2883 100644 --- a/test/treeTableCells.test.ts +++ b/test/treeTableCells.test.ts @@ -332,6 +332,100 @@ describe('links navigate rather than following an href', () => { table.remove(); }); + // A param the graph could not resolve is a NORMAL answer, not a defect to hide. MATLAB + // itself calls `Gain = finalGain` a real variable reference when `finalGain` lives in + // the BASE workspace — a live MATLAB session, which is not a file anything here can + // open. Every other unresolved case (a Bus Selector's `OutputSignals = a,b`, a Math + // block's `Operator = square`) has nothing to open either. + // + // So the value must READ but not CLAIM to be clickable. It used to claim it: both + // paramLinks columns wrote the anchor unconditionally while the Value column next to + // them gated on `linkTarget`, so an unresolvable param painted accent-blue, underlined + // on hover, and fired a navigation carrying an empty target. One rule, three spellings. + describe('a param the graph could not resolve is text, not a dead link', () => { + const UNRESOLVED = { property: 'Gain', paramName: 'finalGain', source: '', linkTarget: '' }; + + it('renders no anchor, in either column that carries paramLinks', async () => { + const table = await mount([ + makeRow('u', 'u', { UsedBy: { paramLinks: [UNRESOLVED] } as any }), + makeRow('d', 'd', { DataType: { paramLinks: [UNRESOLVED] } as any }), + ]); + expect(cell(table, 'u', 'UsedBy').querySelector('a.value-link')).toBeNull(); + expect(cell(table, 'd', 'DataType').querySelector('a.value-link')).toBeNull(); + table.remove(); + }); + + it('still shows the property and the value, so nothing is lost by not linking it', async () => { + const table = await mount([makeRow('u', 'u', { UsedBy: { paramLinks: [UNRESOLVED] } as any })]); + const td = cell(table, 'u', 'UsedBy'); + expect(td.querySelector('.param-property')!.textContent).toBe('Gain='); + expect(text(table, 'u', 'UsedBy')).toBe('Gain=finalGain'); + table.remove(); + }); + + it('leaves the cell TEXT identical to a resolved one, so sorting and copying do not shift', async () => { + // The text comes from `_getCellText`, which reads the same list and knows nothing + // about targets. Gating the anchor must not become a second, quieter answer to + // "what does this cell say". + const table = await mount([ + makeRow('n', 'n', { UsedBy: { paramLinks: [UNRESOLVED] } as any }), + makeRow('y', 'y', { UsedBy: { paramLinks: [{ ...UNRESOLVED, linkTarget: 'workspace:finalGain@f' }] } as any }), + ]); + const [n, y] = table.rows; + expect((table as any)._getCellText(n, 'UsedBy')).toBe((table as any)._getCellText(y, 'UsedBy')); + table.remove(); + }); + + it('links only the resolved entries of a cell that holds both', async () => { + // The ordinary case for a block: `Operator=square` resolves to nothing while the + // gain beside it resolves to a dictionary. A cell that linked both would send one + // of the two clicks nowhere; a cell that linked neither would lose a real jump. + const table = await mount([ + makeRow('u', 'u', { + UsedBy: { + paramLinks: [ + { property: 'Operator', paramName: 'square', source: '', linkTarget: '' }, + { property: 'Gain', paramName: 'Kp', source: 'params.sldd', linkTarget: 'Kp@file:///w/params.sldd' }, + ], + } as any, + }), + ]); + const td = cell(table, 'u', 'UsedBy'); + const links = Array.from(td.querySelectorAll('a.value-link')); + expect(links.map((a) => a.textContent!.trim())).toEqual(['Kp']); + expect(text(table, 'u', 'UsedBy')).toBe('Operator=square, Gain=Kp(params.sldd)'); + table.remove(); + }); + + it('cannot dispatch a navigation with an empty target', async () => { + // The defect as the user met it: the value looked like a link, the click was + // swallowed by preventDefault, and `dex-link-clicked` carried '' — which routes + // nowhere, so nothing happened at all. + const table = await mount([makeRow('u', 'u', { UsedBy: { paramLinks: [UNRESOLVED] } as any })]); + const clicked: string[] = []; + table.addEventListener('dex-link-clicked', (e) => clicked.push((e as CustomEvent).detail.target)); + // Whatever the cell offers as clickable, clicking it must not ask the host to + // navigate to nothing. Asserted by exercising every link in the cell rather than + // by counting them, so this stays a statement about behaviour and not about markup. + for (const a of Array.from(cell(table, 'u', 'UsedBy').querySelectorAll('a.value-link'))) { + (a as HTMLElement).click(); + } + expect(clicked).toEqual([]); + table.remove(); + }); + + it('is still highlighted by a search that matches it', async () => { + // Highlighting is about finding the text, which is there whether or not it links. + const table = await mount([makeRow('u', 'u', { UsedBy: { paramLinks: [UNRESOLVED] } as any })]); + const input = table.shadowRoot!.querySelector('.filter-input') as HTMLInputElement; + input.value = 'final'; + input.dispatchEvent(new Event('input', { bubbles: true })); + await table.updateComplete; + expect(cell(table, 'u', 'UsedBy').querySelector('mark')!.textContent).toBe('final'); + table.remove(); + }); + }); + it('blockLinks name the block and its model', async () => { // Two models can hold blocks with the same name, so the model qualifier is // what makes the reference identifiable.