Add a dictionary entry from a gallery of tiles - #34
Merged
Merged
Conversation
… a Signal The Add gallery about to land here draws its tiles from SectionNode.ALLOWED_TYPES, so an entry the allow-list admits becomes a button a user can press. Architectural Data admitted Simulink.Signal, which would have offered "Signal" as architectural data; a signal is design data, and arch's interface types are its bus and connection-bus entries. Core v1.26.0 drops it from that one list, which gates add and paste only -- a derived Signal already in a file still parses, and still draws with the arch glyph. The suite is unchanged by the bump: 142 files, 2502 tests, all passing.
28 tiles in six categories named the way the domain names them -- Parameters, Signals and Buses, Interfaces, Types, Variants, Configurations -- each one carrying the class it creates, the section it writes into, and the glyph it draws. Grouping by kind means a category can hold tiles that land in different sections, which is paid for with a destination badge on the 6 tiles that depart from their heading. The catalog is in src/common/ rather than the webview because the guard test is the second reader. For every tile it builds a real dictionary, asks the real section to add the entry, and compares three things against what the tile promised: that the section admits the class, that the entry is of that class, and that the row's glyph is the tile's glyph. Nothing here can be checked from one side of that boundary -- the tile renders either way, and a click that is refused or writes the wrong class looks identical to one that worked. Six defects in the ported catalog were found this way, all the same shape. The last one arrived while writing the class check: MatlabVariable, MatlabStruct and Constant are keys into core's class map, not MATLAB classes, so they report a data type where a dotted name reports itself. That arm compares the node class instead, which matters most for Constant -- it reports 'double' exactly like MatlabVariable, leaving the constructor and the glyph as the only two facts that tell the two tiles apart. 122 tests.
A transient popover anchored under a new bar button, mirroring the Columns button beside it -- same rule, same popup machinery, same dismiss paths. It is anchored to the table it edits, it dismisses itself, and it costs no persistent screen space, which a panel docked inside the editor webview would (and would duplicate itself in every split pane showing the same file). The popover creates nothing. A click dispatches the class and the section and the table relays that outward; the host stays the one place a document is edited. The same split dex-column-filter uses, for the same reason: a second path deciding what an add means is a path that drifts from the first. The pin is the interaction model, not a convenience. Unpinned, an add closes the popover so the new row can go straight into rename -- one click and then typing, which is what New File in the Explorer costs, and the single add is the overwhelmingly common case. Pinned, adds land immediately and a run of them is a run of single clicks. The pin lives on the TABLE rather than in the popover, so it survives a close and reopen: a user who pins is describing their next few minutes, not this one popover. Focus lands on the first tile; Escape dismisses and keeps the key to itself, because the table's own Escape clears the search box and dismissing a popover must not throw away what you typed to find a tile. A read-only view gets no button at all rather than a disabled one, gated on the same editable flag as the context menu. A disabled button invites a click and explains nothing. 17 tests on the interaction model -- the tiles' own truth is the catalog guard's job. Suite: 144 files, 2641 tests.
A tile click becomes one `addEntry` message, and the host answers it with core's own
`addEntry` — the class's default value, the unique name, the uuid/namespace/isderived stamps
and the `New` status all come from there and are re-derived nowhere here.
Both editable providers answer it, through one shared rule: `createEntry` and the insert it
feeds live in `structuralEdit.ts`, and `xmlStructuralEdit.ts` imports them rather than
mirroring them. Only the splice is format-specific. `sectionByName` is likewise one spelling
of "find the section by core's key", which `resolveSectionForPaste` now routes through — the
gallery names its destination outright, so unlike a paste it has no row to resolve from.
The repaint is narrow: one `insertEntryRows`, one byte-scoped patch, one undo step. There is
no wide fallback because there is nothing to fall back for — the paste's exists for a
same-document move whose sources the model can no longer find, and an add removes nothing.
Renaming is a separate `beginRename { rowId }` message rather than something the webview
infers from a `selectRows` that arrived beside an insert, because a paste and a drop look
exactly like that from the webview's side. It is held until the row lands, for the same
reason a pending selection is, and it goes through the same `_cellEditTarget` a double-click
does, so a row the host would refuse to rename gets no editor. `rename` rides along with the
class and the section: an unpinned click asks for one, a pinned run asks for none so a batch
is not interrupted, and by the time the entry exists the pin may have been toggled.
Tests: `addEntryHost.test.ts` pins that the entry is core's, that the narrow insert and a
full re-parse agree row for row, and that a JSON add and a binary add of one tile produce
the same entry — on `parity/artifacts/{text,binary}/params.sldd`, since
`fixtures/arch_binary.sldd` has no trailing `DD.Dictionary` and so takes no insert at all,
which the same file now pins as a shared add/paste failure. `addEntryWiring.test.ts` drives
the listeners that ship, including the gate on a read-only view and the held rename.
A pinned run of tile clicks stays N undo steps rather than collapsing into one. The grouped alternative cannot be built honestly here: a JSON .sldd write is a WorkspaceEdit, one edit is one native undo step, and VS Code offers no way to merge two already applied — so grouping would mean holding the document unwritten until the popover closed, and a save, crash or reload mid-run would lose entries the user watched appear. The compressed format could group by amending its last pushEdit, which would make the same gesture undo differently depending on which .sldd flavour was open. And five clicks are five acts: five undos let the user take back only the last one, which one step cannot. So the property to hold is that the N steps are independent and compose back to where the run started — each one's bytes still where its own patch said when it is undone, each one's model op taking back its own entry and no other. That is what two adds in a row can break, and addEntryUndo.test.ts pins it in both formats. Tracing what a run does to the JSON provider turned up a race the pin makes reachable. The handler reads the document when it starts and awaits its write at the end, which is safe for gestures a person makes one at a time; the pin exists so clicks come faster than that. Two handlers straddling one applyEdit both get the same offset out of findEntriesArrayInsertion, so the second entry is written in FRONT of the first and the file lists the pair backwards from the model — rows that reorder under the user on the next full repaint. Adds now queue on the write: no click is dropped or coalesced, it just waits for the one in front. The binary provider is synchronous from chunkXml to pushEdit and needs nothing, which its comment now says out loud.
The browser harness over the shipped bundle showed `Variant Config Data` rendering as `Variant Co…`, two rows under `Variant Control` and indistinguishable from it: a user aiming for one can click the other, and both sit in the same category. At a 340px popover, two columns of `minmax(150px, 1fr)` leave a badged tile's label 75px and that label needs 110. So the label is now `Variant Config` — the `Data` it drops is what the `Config` badge already implies — and the popover is 380px, which gives that label 95px for the 80 it needs. The 40px is measured, not chosen: the slack is there so a platform whose 12px font is wider than this one's does not clip it again. 380 still clamps inside a 420px split pane with the badges visible. happy-dom lays nothing out, so no test here could have seen this. What can be pinned is a fence a new tile trips over: 24 characters plain, 16 badged, derived from the harness measurement, with the provenance in the comment and instructions to re-measure rather than raise the constant. Also drops a NUL byte out of `pairKey`. One NUL makes a file binary to git and to grep, so this test's diffs were unreviewable and a search across `test/` skipped it silently — which is how it surfaced: `grep -n pairKey` on the file printed nothing.
The extension could edit every part of a dictionary except the part that decides what is in it. An entry could be renamed, retyped, pasted, dragged and deleted, but the only way to create one was to make it somewhere else and paste it in. So: an `⊞ Add` button in the filter bar of any editable dictionary, opening a popover of 28 tiles grouped by kind — Parameters, Signals and Buses, Interfaces, Types, Variants, Configurations. Each tile names one class and the one section it writes into, so there is no "current section" to get wrong, and the six tiles that land somewhere other than their heading says are badged with where they actually go. An add writes through the host, in either .sldd format, and the new row arrives selected with its name open for typing. Unpinned is one add and then naming it. `Keep open` pins the popover for a run of adds, which land immediately and unrenamed; each one is still its own undo step, so a run of five can be taken back one at a time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The extension could edit every part of a dictionary except the part that decides what is in it. An entry could be renamed, retyped, pasted, dragged and deleted, but the only way to create one was to make it somewhere else and paste it in.
What this adds
An
⊞ Addbutton in the filter bar of any editable dictionary, opening a popover of 28 tiles grouped by kind — Parameters, Signals and Buses, Interfaces, Types, Variants, Configurations. Each tile names one class and the one section it writes into, so there is no "current section" to get wrong; the six tiles that land somewhere other than their heading says carry a badge naming where they actually go.Unpinned, an add closes the popover and the new row opens its name for typing — one add and then naming it, which is what New File in the Explorer costs.
Keep openpins the popover for a run of adds, which land immediately and unrenamed. Each add is its own undo step either way, so a run of five can be taken back one at a time.A read-only view offers no button, and the relay refuses the gesture even if one is synthesised.
How it is held together
src/common/addCatalog.ts), so a guard test can walk it against the data model: for each of the 28 tiles it builds a real dictionary, asks the real section to create the entry, and compares the class and the glyph that come back. Six real defects were found that way while the catalog was being written, including four tiles whose class and section the model would have refused outright and one tile whose art differed from the row it produced.createEntryandinsertNewEntrylive instructuralEdit.tsand the XML path imports them, so a JSON add and a compressed-binary add cannot disagree about what an entry becomes or where it goes. A test pins that one dictionary written two ways answers the same tile with the same entry.beginRename { rowId }is its own host→webview message, not an inference from aselectRowsnext to an insert — a paste and a drop look identical from the webview's side, and only the host knows the add came from a tile. It is held until the row arrives, then routed through the same gate as a double-click, so an unrenameable row gets no editor.WorkspaceEditat exit, and the pin exists precisely so clicks arrive faster than that; two handlers straddling oneapplyEditwould compute the same insertion offset and write the second entry in front of the first, leaving the file listing the pair backwards from the model. Queued, with the hazard itself pinned as a test. The binary provider is synchronous end to end and needs nothing.Upstream
Requires
data-explorer-corev1.26.0, where Architectural Data stops allowing aSimulink.Signal(mathworks/data-explorer-core#42, #43). The pin is bumped in the first commit here.Checks
npm run verifygreen: typecheck, both bundles, 147 test files / 2667 tests, leak check clean. The popover was also driven in a browser over the shippeddist/webviewbundle in dark, light and high-contrast themes plus forced-colors — which is how the one visual defect in this branch was found and fixed: a tile label clipped toVariant Co…, two rows underVariant Controland indistinguishable from it.