From ed3a9e52f7b42dccd7d828d29214393fad48277e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 09:06:57 +0000 Subject: [PATCH 1/2] feat(check): warn when a navigation menu item specifies no icon (MDL074) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mendix's navigation sidebar collapses to an icon rail, and that is the state most users leave it in. A collapsed item shows its icon; one without falls back to the first few characters of its caption, which is rarely enough to tell "Orders" from "Order lines". Nothing said anything: the icon is optional in the grammar, the model builds, `mx check` passes, and the only symptom is a column of truncated words in a browser. Covers every item at every depth, in BOTH statements that carry menu items — `create navigation`'s `menu (...)` block and `create menu`. They share one AST node (NavMenuItemDef) precisely so the two cannot diverge, and the rule walks it once rather than twice. Uniform rather than top-level-only. A sub-item renders in the flyout the collapsed rail opens, and a submenu's PARENT sits directly on the rail, so it needs one most of all. If a deep menu proves noisy, narrowing the walk is a one-line change — reporting too little is the failure that is hard to notice. A WARNING, not an error: the project builds and runs, `exec` refuses only on errors, and a rule that blocked the script would turn a design opinion into a gate. It needs no project. Only the icon's TARGET has to be resolved, and MDL-ICON01 already does that separately — so this runs in the project-free pass and fires under `make check-mdl`, rather than being inert in CI the way four of the widget rules were. Controls, because "flag every item" would pass the first assertion: a fully iconed menu stays silent (asserted both as a unit test and end-to-end on the new example script, which checks clean); an iconed sub-item beside two iconless ones is not reported; a statement with no menu, and an unrelated statement type, produce nothing. Verified the rule really fires in the corpus rather than being absent — `make check-mdl` reports pass/fail only, so its zero MDL074 lines mean nothing on their own; running check directly on an existing navigation script shows the warnings. The example is a POSITIVE one. MDL074 is a warning, so `check` still exits 0 and a `.fail.mdl` would be reported by CI as "negative test unexpectedly passed" — the trap the Makefile documents for #891/#892. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ --- .../skills/mendix/manage-navigation/SKILL.md | 9 +- docs/01-project/MDL_QUICK_REFERENCE.md | 8 + .../bug-tests/navigation-menu-item-icons.mdl | 50 ++++++ mdl/executor/validate_navigation_icons.go | 81 ++++++++++ .../validate_navigation_icons_test.go | 144 ++++++++++++++++++ mdl/executor/validate_program.go | 4 + 6 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 mdl-examples/bug-tests/navigation-menu-item-icons.mdl create mode 100644 mdl/executor/validate_navigation_icons.go create mode 100644 mdl/executor/validate_navigation_icons_test.go diff --git a/.claude/skills/mendix/manage-navigation/SKILL.md b/.claude/skills/mendix/manage-navigation/SKILL.md index 133217365d..07fb4dda7c 100644 --- a/.claude/skills/mendix/manage-navigation/SKILL.md +++ b/.claude/skills/mendix/manage-navigation/SKILL.md @@ -118,7 +118,14 @@ create or replace navigation Responsive ### Menu Icons -Both `menu item` and `menu 'caption' (...)` take an optional `icon`. It is a +**Give every menu item an icon.** It is optional in the grammar and `mxcli check` +warns when it is missing (**MDL074**), because the navigation sidebar collapses +to an icon rail and that is the state most users leave it in: a collapsed item +shows its icon, and one without falls back to the first few characters of its +caption — rarely enough to tell `Orders` from `Order lines`. Nothing else catches +it. The model builds, `mx check` passes, and the menu is simply hard to use. + +Both `menu item` and `menu 'caption' (...)` take an `icon`. It is a **qualified name** into an **icon collection** — a model reference, written like every other reference in MDL, not a string: diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 7497512b2a..db72d3bd27 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -775,6 +775,14 @@ create or replace navigation Responsive ); ``` +**An item with no icon is reported (MDL074, a warning).** The navigation sidebar +collapses to an icon rail, and that is the state most users leave it in: a +collapsed item shows its icon, and one without falls back to the first few +characters of its caption — rarely enough to tell `Orders` from `Order lines`. +The menu still builds and `mx check` passes, so the only symptom is in a browser. +The rule covers every item at every depth, in both `create navigation`'s `menu` +block and `create menu`, and needs no project. + `icon` is optional and is a **qualified name** into an **icon collection** — `Atlas_Core.Atlas`, `Atlas_Core.Atlas_Filled`, `Atlas_Core.Atlas_Styling`, or one of your own — written like any other model reference. Hyphenated Atlas names diff --git a/mdl-examples/bug-tests/navigation-menu-item-icons.mdl b/mdl-examples/bug-tests/navigation-menu-item-icons.mdl new file mode 100644 index 0000000000..3d0995daa0 --- /dev/null +++ b/mdl-examples/bug-tests/navigation-menu-item-icons.mdl @@ -0,0 +1,50 @@ +-- ============================================================================ +-- MDL074: a navigation menu item with no icon is reported +-- ============================================================================ +-- +-- The navigation sidebar collapses to an icon rail, and that is the state most +-- users leave it in. A collapsed item shows its icon; one without falls back to +-- the first few characters of its caption, which is rarely enough to tell +-- "Orders" from "Order lines". Nothing caught it: the icon is optional in the +-- grammar, the model builds, and `mx check` passes. +-- +-- This script is a POSITIVE example — every item carries an icon, so it checks +-- clean. The warning it guards against is exercised by the unit tests in +-- mdl/executor/validate_navigation_icons_test.go, which can assert the absence +-- of a warning as well as its presence; a `.fail.mdl` cannot, because MDL074 is +-- a WARNING and `check` still exits 0. +-- +-- The rule needs no project, so it fires under `make check-mdl` too. + +create module NavIcons; + +create page NavIcons.Home ( Title: 'Home', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Home') +} + +create page NavIcons.Orders ( Title: 'Orders', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Orders') +} + +create page NavIcons.Users ( Title: 'Users', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Users') +} + +-- Every item iconed, including the SUBMENU PARENT — it sits directly on the +-- collapsed rail, so it needs one most of all. +create or replace navigation Responsive + home page NavIcons.Home + menu ( + menu item 'Home' page NavIcons.Home icon Atlas_Core.Atlas.home; + menu item 'Orders' page NavIcons.Orders icon Atlas_Core.Atlas."shopping-cart"; + menu 'Admin' icon Atlas_Core.Atlas.cog ( + menu item 'Users' page NavIcons.Users icon Atlas_Core.Atlas.user; + ); + ); + +-- A standalone menu document carries the same items and gets the same rule: +-- CREATE MENU and CREATE NAVIGATION share one AST node so they cannot diverge. +create or modify menu NavIcons.Main_Menu ( + menu item 'Home' page NavIcons.Home icon Atlas_Core.Atlas.home; + menu item 'Orders' page NavIcons.Orders icon Atlas_Core.Atlas."shopping-cart"; +); diff --git a/mdl/executor/validate_navigation_icons.go b/mdl/executor/validate_navigation_icons.go new file mode 100644 index 0000000000..d2a2e17f6d --- /dev/null +++ b/mdl/executor/validate_navigation_icons.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// validateMenuItemIcons (MDL074) flags a navigation menu item that specifies no +// icon. +// +// # Why this is worth a warning +// +// Mendix's navigation sidebar collapses to an icon rail, and that is the state +// most users leave it in. A collapsed item shows its icon; an item without one +// falls back to the first few characters of its caption, which is rarely enough +// to tell "Orders" from "Order lines". The menu still builds, `mx check` passes, +// and the only symptom is a column of truncated words in a browser. +// +// The icon is optional in the grammar and nothing said anything about leaving it +// out, so an iconless menu was the easiest one to write. +// +// # Every item, at every depth +// +// A sub-item is a menu item: it renders in the flyout the collapsed rail opens, +// and a submenu's PARENT sits directly on the rail, so it needs one most of all. +// The rule is uniform rather than scoped to the top level — if that proves noisy +// on a deep menu, narrowing it is a one-line change to the walk, but reporting +// too little is the failure that is hard to notice. +// +// # A warning, not an error +// +// The project builds and runs. This is a usability defect, not a broken model, +// and `exec` refuses only on errors — a rule that blocked the script would make +// an opinion about design into a gate. +// +// # It needs no project +// +// The icon's PRESENCE is in the script; only its target has to be resolved +// against the project's icon collections, and MDL-ICON01 already does that +// separately. So this runs in the project-free pass and fires under +// `mxcli check` with no `-p` — which is how CI runs it, and how the four widget +// rules that were inert in CI got missed. +func validateMenuItemIcons(stmt ast.Statement) []linter.Violation { + var items []ast.NavMenuItemDef + var where string + + switch s := stmt.(type) { + case *ast.AlterNavigationStmt: + items, where = s.MenuItems, "navigation "+s.ProfileName + case *ast.CreateMenuStmt: + items, where = s.Items, "menu "+s.Name.String() + default: + return nil + } + + var out []linter.Violation + var walk func(list []ast.NavMenuItemDef) + walk = func(list []ast.NavMenuItemDef) { + for _, item := range list { + if item.Icon == "" { + out = append(out, linter.Violation{ + RuleID: "MDL074", + Severity: linter.SeverityWarning, + Message: fmt.Sprintf( + "%s: menu item %q specifies no icon — a collapsed navigation sidebar "+ + "shows only the icon, so this item appears as a few characters of its caption", + where, item.Caption), + Suggestion: "add `icon Atlas_Core.Atlas.` (list them with " + + "`describe icon collection Atlas_Core.Atlas`)", + }) + } + walk(item.Items) + } + } + walk(items) + return out +} diff --git a/mdl/executor/validate_navigation_icons_test.go b/mdl/executor/validate_navigation_icons_test.go new file mode 100644 index 0000000000..f9630b5ace --- /dev/null +++ b/mdl/executor/validate_navigation_icons_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +func navIconWarnings(vs []linter.Violation) []linter.Violation { + var out []linter.Violation + for _, v := range vs { + if v.RuleID == "MDL074" { + out = append(out, v) + } + } + return out +} + +// A menu item with no icon is flagged. +// +// Mendix's navigation sidebar collapses to an icon rail. A collapsed item shows +// its icon; an item without one shows the first few characters of its caption, +// which is rarely enough to tell "Orders" from "Order lines" — so the menu is +// unusable in exactly the state most users leave it in. +// +// The icon is optional in the grammar and nothing said anything, so an item +// written without one built cleanly, passed `mx check`, and only looked wrong in +// a browser. +func TestValidateMenuItemIcons_FlagsMissingIcon(t *testing.T) { + stmt := &ast.AlterNavigationStmt{ + ProfileName: "Responsive", + MenuItems: []ast.NavMenuItemDef{ + {Caption: "Home", Icon: "Atlas_Core.Atlas.home"}, + {Caption: "Orders"}, + }, + } + + got := navIconWarnings(validateMenuItemIcons(stmt)) + if len(got) != 1 { + t.Fatalf("got %d warnings, want 1: %+v", len(got), got) + } + if !strings.Contains(got[0].Message, "Orders") { + t.Errorf("warning does not name the item: %q", got[0].Message) + } + if got[0].Severity != linter.SeverityWarning { + t.Errorf("severity = %v, want warning — a menu without icons builds and runs, "+ + "it is just hard to use collapsed", got[0].Severity) + } +} + +// The control: an item WITH an icon must stay silent, or the rule is "warn on +// every menu item" and the first assertion proves nothing. +func TestValidateMenuItemIcons_IconedItemIsSilent(t *testing.T) { + stmt := &ast.AlterNavigationStmt{ + ProfileName: "Responsive", + MenuItems: []ast.NavMenuItemDef{ + {Caption: "Home", Icon: "Atlas_Core.Atlas.home"}, + {Caption: "Admin", Icon: `Atlas_Core.Atlas."align-center"`, Items: []ast.NavMenuItemDef{ + {Caption: "Users", Icon: "Atlas_Core.Atlas.user"}, + }}, + }, + } + if got := navIconWarnings(validateMenuItemIcons(stmt)); len(got) != 0 { + t.Errorf("a fully iconed menu produced %d warnings: %+v", len(got), got) + } +} + +// Sub-items are menu items too. A submenu's children render in the flyout the +// collapsed rail opens, and the parent itself sits ON the rail. +func TestValidateMenuItemIcons_RecursesIntoSubItems(t *testing.T) { + stmt := &ast.AlterNavigationStmt{ + ProfileName: "Responsive", + MenuItems: []ast.NavMenuItemDef{ + {Caption: "Admin", Icon: "Atlas_Core.Atlas.cog", Items: []ast.NavMenuItemDef{ + {Caption: "Users"}, + {Caption: "Roles", Icon: "Atlas_Core.Atlas.group"}, + {Caption: "Deep", Items: []ast.NavMenuItemDef{{Caption: "Deeper"}}}, + }}, + }, + } + got := navIconWarnings(validateMenuItemIcons(stmt)) + if len(got) != 3 { + t.Fatalf("got %d warnings, want 3 (Users, Deep, Deeper): %+v", len(got), got) + } + joined := "" + for _, v := range got { + joined += v.Message + "\n" + } + for _, want := range []string{"Users", "Deep", "Deeper"} { + if !strings.Contains(joined, want) { + t.Errorf("%q not reported; recursion stops short:\n%s", want, joined) + } + } + if strings.Contains(joined, "Roles") { + t.Errorf("an iconed sub-item was reported:\n%s", joined) + } +} + +// A standalone menu document carries the same items, so it gets the same rule — +// CREATE MENU and CREATE NAVIGATION share NavMenuItemDef precisely so the two +// cannot diverge. +func TestValidateMenuItemIcons_CoversMenuDocuments(t *testing.T) { + stmt := &ast.CreateMenuStmt{ + Name: ast.QualifiedName{Module: "MyModule", Name: "Main_Menu"}, + Items: []ast.NavMenuItemDef{{Caption: "Reports"}}, + } + got := navIconWarnings(validateMenuItemIcons(stmt)) + if len(got) != 1 { + t.Fatalf("got %d warnings, want 1 for a menu document: %+v", len(got), got) + } + if !strings.Contains(got[0].Message, "MyModule.Main_Menu") { + t.Errorf("warning does not name the document: %q", got[0].Message) + } +} + +// Nothing to say about a statement with no menu at all, or another statement +// type entirely. +func TestValidateMenuItemIcons_Quiet(t *testing.T) { + if got := validateMenuItemIcons(&ast.AlterNavigationStmt{ProfileName: "Responsive"}); len(got) != 0 { + t.Errorf("a profile with no MENU block warned: %+v", got) + } + if got := validateMenuItemIcons(&ast.CreateEntityStmt{}); len(got) != 0 { + t.Errorf("an unrelated statement warned: %+v", got) + } +} + +// The rule needs no project, so it must fire under `mxcli check` with none — +// which is how CI runs it. A rule that only works with -p is inert in CI, the +// trap that hid four defects in the widget work. +func TestValidateMenuItemIcons_RunsWithoutAProject(t *testing.T) { + prog := &ast.Program{Statements: []ast.Statement{ + &ast.AlterNavigationStmt{ + ProfileName: "Responsive", + MenuItems: []ast.NavMenuItemDef{{Caption: "Orders"}}, + }, + }} + if got := navIconWarnings(ValidateProgram(prog, "")); len(got) != 1 { + t.Errorf("got %d MDL074 from ValidateProgram with no project, want 1: %+v", len(got), got) + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 94f0bca370..2602635032 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -52,6 +52,10 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { if roleStmt, ok := stmt.(*ast.CreateUserRoleStmt); ok { violations = append(violations, ValidateUserRoleSystemModuleRole(roleStmt, securityEnabled)...) } + // A navigation menu item with no icon is unreadable once the sidebar is + // collapsed to its icon rail (MDL074). Covers both statements that carry + // menu items, which share one AST node so they cannot diverge. + violations = append(violations, validateMenuItemIcons(stmt)...) // A page with parameters and a Url must name each parameter in it (CE5601). if pageStmt, ok := stmt.(*ast.CreatePageStmtV3); ok { violations = append(violations, ValidatePageURLParameters(pageStmt)...) From bf31150a655350435e441b6374bca7a1e54e154e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 10:03:28 +0000 Subject: [PATCH 2/2] feat(navigation): author and round-trip all three menu icon kinds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIBE -> exec was destroying menu icons. Measured on testdata/expr-checker, whose Home item carries a glyph icon: describe menu item 'Home' page …; -- icon a numeric glyph code (Forms$GlyphIcon) is not reproducible … exec Navigation profile 'Responsive' updated. describe menu item 'Home' page …; <- the icon was gone Exit 0, success message, silent loss — the same shape as the pluggable-widget body loss in mendixlabs/mxcli#1036, and reachable the same way: describe -> edit -> exec is the editing loop for a project whose rule is "never hand-edit the .mpr". The giveaway was already in the output and read as harmless. That comment was written to make the loss visible, and it did — in the DESCRIBE output, not in the exec that then acted on it. A "cannot reproduce this" note beside a FULL-REPLACEMENT statement is a note saying "running this deletes it". Mendix stores THREE icon elements, and they are not spellings of one value: Forms$IconCollectionIcon a name in an icon collection Forms$GlyphIcon a numeric character code, and NO name Forms$ImageIcon a name in an IMAGE collection — a different document Every layer handled only the first. The reader captured the $Type and the Image but never the glyph's Code, so mxcli knew a glyph had been there and not which one; the AST and the write spec carried a bare name with no kind at all; and both engines' writers emitted IconCollectionIcon unconditionally. Now each variant has its own MDL form, and the BARE form still means the icon-collection icon, so every existing script keeps its meaning: icon Atlas_Core.Atlas.home Forms$IconCollectionIcon icon glyph 57377 Forms$GlyphIcon icon image MyModule.Images.logo Forms$ImageIcon Keyword-led rather than widening the bare form, because a bare name written for an image icon would rebuild it as a COLLECTION icon — a silent variant swap, the failure mode the original code avoided by emitting nothing. Nothing is inferred: the kind comes from the author or from what the reader saw in storage. The two keyword alternatives are ordered FIRST in the grammar, since qualifiedName accepts a keyword as a name segment and `ICON IMAGE …` also matches the bare form. NOT preserve-when-silent, which is what I proposed before writing the syntax. Preserving is right only while a construct is INEXPRESSIBLE — an omission cannot be a choice if the author had no way to say it. Once `icon glyph N` exists, omitting it is as deliberate as omitting a collection icon, and preserving would make it impossible to remove a glyph icon from MDL and make omission mean different things for different kinds. The stale-script path is covered by MDL074 instead: verified, an old-style script against the fixture warns per item rather than silently destroying anything. Fixes MDL074 in passing, and it was a real false positive: the rule tested `Icon == ""`, and a glyph has a code and no name, so it reported items that plainly have icons — the fixture's own Home item among them. It asks the kind now. Controls: with the describe fix stubbed, the collection case still passes while glyph and image fail, so the tests detect the defect rather than the shape of the code. A bare name with no kind must still write a collection icon, or every script predating this silently loses its icons. A glyph with no code, and an unknown $Type, are still flagged rather than guessed at — without a case that still declines, "emits everything" and "reproduces everything" are indistinguishable. Visitor-level tests assert the AST, per the #1036 lesson that a corpus diff of check output is blind to a construct that parses into the wrong shape. Round trip on the fixture is now lossless: `icon glyph 57377` before, exec, `icon glyph 57377` after. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ --- .../fix-issue/findings/mdl-executor.jsonl | 2 + .../skills/mendix/manage-navigation/SKILL.md | 20 +++- cmd/mxcli/lsp_completions_gen.go | 1 + docs/01-project/MDL_QUICK_REFERENCE.md | 25 +++- .../bug-tests/navigation-icon-variants.mdl | 65 +++++++++++ mdl/ast/ast_navigation.go | 21 +++- mdl/backend/modelsdk/menu_write.go | 59 ++++++++-- mdl/backend/modelsdk/navigation_icon_test.go | 46 ++++++-- mdl/backend/modelsdk/navigation_read.go | 20 +++- mdl/backend/modelsdk/navigation_write.go | 42 +++++-- mdl/backend/mpr/convert.go | 2 +- mdl/executor/cmd_navigation.go | 53 +++++++-- .../cmd_navigation_icon_roundtrip_test.go | 96 ++++++++++++++++ mdl/executor/cmd_navigation_icon_test.go | 67 ++++++++--- mdl/executor/validate_navigation_icons.go | 7 +- .../validate_navigation_icons_test.go | 31 ++++- mdl/grammar/MDLLexer.g4 | 3 + mdl/grammar/MDLParser.g4 | 26 ++++- mdl/grammar/domains/MDLSettings.g4 | 2 +- mdl/types/navigation.go | 108 ++++++++++++++++-- mdl/types/navigation_icon_test.go | 74 ++++++++++++ mdl/visitor/visitor_navigation.go | 81 ++++++++++--- mdl/visitor/visitor_navigation_icon_test.go | 86 ++++++++++++++ sdk/mpr/parser_misc.go | 4 + sdk/mpr/writer_navigation.go | 49 ++++++-- sdk/mpr/writer_navigation_icon_test.go | 73 +++++++++++- 26 files changed, 941 insertions(+), 122 deletions(-) create mode 100644 mdl-examples/bug-tests/navigation-icon-variants.mdl create mode 100644 mdl/executor/cmd_navigation_icon_roundtrip_test.go create mode 100644 mdl/types/navigation_icon_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index d0f2a7261b..7182b7a54b 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -545,3 +545,5 @@ {"area": "mdl/executor", "date": "2026-09-05", "symptom": "`UPDATE SECURITY` was inert on every MPR v2 project: it returned on the first module it could not reconcile, and System always is one \u2014 `Error: failed to reconcile security for module System: load domain model 00000000-...-002: .../mprcontents/00/00/...002.mxunit: no such file or directory`. Reported as \"UPDATE SECURITY does not fix the CE0066 it exists to fix\" (mendixlabs/mxcli#1047). A second defect in the same command: `update security RestLab` (without IN) parsed cleanly, dropped the module name, and ran project-wide.", "cause": "execUpdateSecurity returned mdlerrors.NewBackend on any ReconcileMemberAccesses failure. System's domain model is SYNTHESIZED rather than stored (no unit file behind the id the module carries), so loadDomainModelGen can never read it. Now System is skipped by name \u2014 reconciling it is not merely impossible but wrong, its access rules are the platform's \u2014 and any other unreadable module is reported and stepped over. The grammar's `(IN qualifiedName)?` became `(IN? qualifiedName)?`, so a bare module name scopes instead of reaching ANTLR's error recovery.", "file": "`mdl/executor/cmd_security_write.go` (execUpdateSecurity), `mdl/grammar/domains/MDLSecurity.g4`; tests `mdl/executor/cmd_security_update_test.go`; example `mdl-examples/bug-tests/update-security-runs-at-all.mdl`", "insight": "Two different System failures live in this codebase and conflating them wastes a diagnosis \u2014 I did it once in this same session. GetModuleByName/GetDomainModel answer for System perfectly well (System.FileDocument loads with its 6 attributes), which is why inherited-member resolution works; loadDomainModelGen, which reads the UNIT BY ID out of mprcontents, cannot, because System has no file there. Same module, opposite answers, different call path. The other half is the more interesting bug class: `update security RestLab` was not a parse error but a SILENT SCOPE ESCALATION \u2014 the user asked to touch one module and the command touched all of them, with `mxcli check` reporting Syntax OK. Worth looking for wherever an optional keyword precedes an optional operand. Finally, the report's stated precondition (adding an attribute leaves one CE0066) does NOT reproduce: measured on 11.13.0, `alter entity ... add attribute` on both engines and a whole-entity rewrite each left the project at 0 errors, because every mxcli write path already reconciles. So the command could not be shown repairing a real CE0066 end to end \u2014 the integration example says so explicitly rather than implying coverage it does not have."} {"area": "mdl/executor", "date": "2026-09-05", "symptom": "A datagrid whose datasource is an association path at PAGE level typed its rows as the entity it navigates AWAY from: `datagrid gA (datasource: $Customer/Bench.Order_Customer)` bound its columns against Customer, giving [CE1613] \"The selected attribute 'Bench.Customer.OrderNo' no longer exists.\" at Columns (1/1). The SAME path inside a data view was correct (mendixlabs/mxcli#1045).", "cause": "resolveAssociationDestination picks the end opposite the context, and the context passed was pb.entityContext \u2014 the ENCLOSING data container's entity. At page level nothing encloses the widget, so it is empty, neither end matches, and the function's last-resort fallback (\"default to the child (TO) side\") returns the entity the grid started from. Fixed by resolving from the NAMED context variable when there is one: `$Customer/\u2026` traverses from whatever $Customer holds, enclosed or not. pb.paramEntityNames already knew it.", "file": "`mdl/executor/cmd_pages_builder_v3.go` (the \"association\" branch of buildDataSourceV3); tests `mdl/executor/cmd_pages_builder_assoc_pagelevel_test.go`; example `mdl-examples/bug-tests/assoc-datasource-page-level.mdl`", "insight": "The report's own diagnosis was the useful part and was right: it noticed the same path worked INSIDE a data view and failed at page level, which localises the bug to the context rather than to the association logic. Worth generalising \u2014 a resolver that takes an ambient context is correct exactly where the ambient context exists, and its fallback is what runs everywhere else; that fallback had a plausible comment (\"matches the common FROM=context pattern\") and was silently wrong for the whole page-level case. Two measurement notes: check-mdl cannot catch this at all, because it only runs `mxcli check` and the defect is in what the WRITER stores \u2014 the regression net had to be exec + mx check over the six examples that use an association datasource (five clean, the sixth failing identically before and after on an unrelated CE0106). And the reverse-direction control initially failed for the wrong reason: traversing a Reference from its FROM end yields ONE object, so a grid over it is [CE8812] \"A grid association path must result in a list\" \u2014 a cardinality complaint, not a resolution one, and the control had to become a data view to test what it claimed to test."} {"area": "mdl/executor", "date": "2026-09-06", "symptom": "Every mxcli-authored Image widget failed the build. On a project at 0 errors, one `image imgProbe (Image: '...', Responsive: false)` on a new page gives [CE0463] \"The definition of this widget has changed\u2026\" at Image 'imgProbe'. A field-level diff of Atlas' brand image against a describe -> rename -> exec copy of it differs in ONE line of 1480: `maxHeight` = '0' where Atlas stores '250' (mxcli-ledger FINDINGS \u00a7142). A previous fix (ee295467) that named maxHeight explicitly was in the shipped binary and changed nothing.", "cause": "hiddenUnnamedProperties default-values a hidden property MDL cannot name, and maxHeight IS hidden (\"hidden when maxHeightUnit = none\"). Its condition property `maxHeightUnit` is unmapped too, so widgetValueMap does not know it, and the fallback read the DECLARED default \u2014 which Image 1.6.0 states as \"pixels\". Condition false -> maxHeight read as visible -> no reset -> the template's captured 0 stood. But maxHeightUnit being unmapped is exactly why the document gets the template's \"none\", not \"pixels\". Fixed by inserting the template's captured configuration (builder.PrimitiveValues(), read before any mapping is applied) between the script's values and the declared defaults in that fallback chain.", "file": "`mdl/executor/widget_engine.go` (hiddenUnnamedProperties' condition fallback + the Build call site); `mdl/backend/widgetobj/builder.go` (new PrimitiveValues, wrapping the existing primitiveValuesOf); `mdl/backend/mutation.go`, `mdl/backend/mcp/widget.go`; tests `mdl/executor/widget_hidden_reset_unmapped_test.go`; example `mdl-examples/bug-tests/image-widget-hidden-maxheight.mdl`", "insight": "The first fix passed a test built on an input that does not exist: its defaults helper declared maxHeightUnit's default as \"none\", and the real package declares \"pixels\" \u2014 the one value at which the rule fires. A fixture that encodes the value under test is not a fixture. The general shape: a visibility rule must be evaluated against the configuration that will be WRITTEN, never against the one the package declares, and the two diverge precisely on unmapped properties, which is the only place the rule matters. Two measurement traps cost time here. (1) `mxcli docker check` runs `mx update-widgets` first, which reconciles the widget and reports 0 errors while the stored value is still 0 \u2014 the same project reads 0 errors through it and 1 error through scripts/mx-check.sh. Use raw mx check for CE0463. (2) The ground truth was cheap and decisive once asked for: dumping all 69 Image widgets of a real project showed all 65 carrying a maxHeight store 250 at every combination of heightUnit/maxHeightUnit, and mxcli's was the sole outlier. Proven both ways by patching the stored 0 to 250 (1 error -> 0) and back (0 -> 1)."} +{"area": "mdl/executor", "date": "2026-09-05", "symptom": "describe navigation -> exec destroyed a menu item's glyph icon. Before: an item with Forms$GlyphIcon and a '-- not reproducible' comment. After exec of DESCRIBE's own output: no icon, no comment, exit 0, 'Navigation profile updated.'", "cause": "Mendix stores THREE icon elements (Forms$IconCollectionIcon{Image}, Forms$GlyphIcon{Code}, Forms$ImageIcon{Image}) and every layer handled only the first. The reader captured $Type and Image but never the glyph's Code, the AST/spec carried a bare name with no kind, and both writers emitted IconCollectionIcon unconditionally. Since CREATE OR REPLACE NAVIGATION is a full replacement, an icon the writer would not emit was an icon the statement DELETED.", "file": "mdl/executor/cmd_navigation.go", "insight": "The giveaway was already in the output and read as harmless: DESCRIBE printed '-- icon … is not reproducible by CREATE NAVIGATION; set it in Studio Pro'. That comment was written to make the loss VISIBLE, and it did — but only in the describe output, not in the exec that then acted on it. A note saying 'I cannot reproduce this' beside a FULL-REPLACEMENT statement is a note saying 'running this deletes it', and nobody made that connection. When a describer declines to emit something, check what the corresponding writer does with the omission. Fix shape: give each variant its own keyword (`icon glyph N` / `icon image QN`, bare = collection) so replay rebuilds the same ELEMENT — widening the bare form to cover all three would have converted an image icon into a collection icon, a silent variant swap. Also: preserve-when-silent was the right fix only while the construct was INEXPRESSIBLE; once authoring exists, omission becomes a real choice and preserving would make it impossible to remove a glyph icon.", "issue": "ako/mxcli"} +{"area": "mdl/executor", "date": "2026-09-05", "symptom": "A brand-new lint rule (MDL074, 'menu item specifies no icon') would have fired on the majority of real menus, reporting items that plainly HAVE an icon.", "cause": "The rule tested `item.Icon == \"\"`. A Forms$GlyphIcon carries a numeric Code and NO qualified name, so the obvious emptiness test calls a perfectly good icon absent. The fixture's own Home item is glyph 57377.", "file": "mdl/executor/validate_navigation_icons.go", "insight": "Caught by writing the rule and the authoring support in the same session — the round-trip test emitted `icon glyph 57377` and check then warned about it, which is what exposed it. A rule that asks 'is this field empty' about a polymorphic element is asking the wrong question: ask the KIND. Generalisable: when a model element has variants with different payload shapes, any emptiness test on one variant's payload is a latent false positive on the others.", "issue": "ako/mxcli"} diff --git a/.claude/skills/mendix/manage-navigation/SKILL.md b/.claude/skills/mendix/manage-navigation/SKILL.md index 07fb4dda7c..e87d482b1f 100644 --- a/.claude/skills/mendix/manage-navigation/SKILL.md +++ b/.claude/skills/mendix/manage-navigation/SKILL.md @@ -125,9 +125,23 @@ shows its icon, and one without falls back to the first few characters of its caption — rarely enough to tell `Orders` from `Order lines`. Nothing else catches it. The model builds, `mx check` passes, and the menu is simply hard to use. -Both `menu item` and `menu 'caption' (...)` take an `icon`. It is a -**qualified name** into an **icon collection** — a model reference, written like -every other reference in MDL, not a string: +Both `menu item` and `menu 'caption' (...)` take an `icon`, in one of three +forms — Mendix stores three different icon **elements**, not three spellings of +one value: + +```sql +menu item 'Home' page M.Home icon Atlas_Core.Atlas.home; -- icon collection +menu item 'Close' page M.Close icon glyph 57377; -- numeric glyph code +menu item 'Logo' page M.Logo icon image M.Images.logo; -- image collection +``` + +The **bare** form is the icon-collection icon and is what you normally want. Use +`glyph` only to reproduce a legacy icon a project already has — `describe +navigation` emits it for you — and `image` for a picture from an image +collection, which is a different document from an icon collection. + +The icon-collection form is a **qualified name** — a model reference, written +like every other reference in MDL, not a string: ```sql create or replace navigation Responsive diff --git a/cmd/mxcli/lsp_completions_gen.go b/cmd/mxcli/lsp_completions_gen.go index b729006912..fc7bbb899c 100644 --- a/cmd/mxcli/lsp_completions_gen.go +++ b/cmd/mxcli/lsp_completions_gen.go @@ -277,6 +277,7 @@ var mdlGeneratedKeywords = []protocol.CompletionItem{ {Label: "ATTRIBUTES", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, {Label: "FILTERTYPE", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, {Label: "IMAGE", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, + {Label: "GLYPH", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, {Label: "QUEUE", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, {Label: "QUEUES", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, {Label: "SCHEDULED", Kind: protocol.CompletionItemKindKeyword, Detail: "Widget keyword"}, diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index db72d3bd27..3889d34624 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -790,10 +790,27 @@ of your own — written like any other model reference. Hyphenated Atlas names `Atlas_Core.Atlas."align-center"`. List the available names with `describe icon collection Atlas_Core.Atlas`. -Studio Pro can also set a *glyph* icon (a numeric character code) or an *image* -icon (pointing into an image collection). Those are different elements; MDL -writes only the icon-collection form, and `describe navigation` marks the other -two with a comment instead of emitting an `icon` clause that would convert them. +Mendix stores **three different icon elements**, and each has its own form, +because they are not spellings of one value — a collection icon and an image icon +each hold a qualified name (into an icon collection and an *image* collection, +different documents), while a glyph icon holds a numeric character code and no +name at all: + +| form | element | holds | +|------|---------|-------| +| `icon Atlas_Core.Atlas.home` | `Forms$IconCollectionIcon` | a name in an icon collection | +| `icon glyph 57377` | `Forms$GlyphIcon` | a numeric character code | +| `icon image MyModule.Images.logo` | `Forms$ImageIcon` | a name in an image collection | + +The bare form is the icon-collection icon, so every existing script keeps its +meaning. The keyword forms exist because writing a bare name for an image icon +would rebuild it as a collection icon — a silent variant swap. + +`describe navigation` emits all three, so describe → exec is lossless. It +previously wrote a comment for the other two, and since `create or replace +navigation` is a **full replacement**, re-running that output DELETED the icon +the comment had just declined to describe. A `$Type` this build does not know is +still flagged rather than guessed at. ## Project Settings diff --git a/mdl-examples/bug-tests/navigation-icon-variants.mdl b/mdl-examples/bug-tests/navigation-icon-variants.mdl new file mode 100644 index 0000000000..5811a8d8e9 --- /dev/null +++ b/mdl-examples/bug-tests/navigation-icon-variants.mdl @@ -0,0 +1,65 @@ +-- ============================================================================ +-- Menu icons: all three of Mendix's icon elements round-trip +-- ============================================================================ +-- +-- Mendix stores THREE different icon elements on a menu item, and they are not +-- spellings of one value: +-- +-- Forms$IconCollectionIcon a qualified name in an icon collection +-- Forms$GlyphIcon a numeric character code, and no name at all +-- Forms$ImageIcon a qualified name in an IMAGE collection +-- +-- MDL could name only the first, so `describe navigation` emitted a comment for +-- the other two — and since CREATE NAVIGATION is a FULL REPLACEMENT, re-running +-- that output DELETED the icon the comment had just declined to describe. +-- +-- Measured on testdata/expr-checker, whose Home item carries a glyph icon: +-- +-- describe menu item 'Home' page …; +-- -- icon a numeric glyph code (Forms$GlyphIcon) is not reproducible … +-- exec Navigation profile 'Responsive' updated. +-- describe menu item 'Home' page …; <- the icon was destroyed +-- +-- Exit 0, success message, silent loss — the same shape as the pluggable-widget +-- body loss in mendixlabs/mxcli#1036. +-- +-- The bare form still means the icon-collection icon, so every script written +-- before this keeps its meaning. The keyword forms exist because writing a bare +-- name for an image icon would rebuild it as a COLLECTION icon: a silent variant +-- swap, and the reason the bare form was not simply widened to cover all three. + +create module NavIconVariants; + +create page NavIconVariants.Home ( Title: 'Home', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Home') +} + +create page NavIconVariants.Close ( Title: 'Close', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Close') +} + +create page NavIconVariants.Logo ( Title: 'Logo', Layout: Atlas_Core.Atlas_Default ) { + dynamictext t (Content: 'Logo') +} + +create or replace navigation Responsive + home page NavIconVariants.Home + menu ( + -- Forms$IconCollectionIcon — the bare form, unchanged + menu item 'Home' page NavIconVariants.Home icon Atlas_Core.Atlas.home; + -- Forms$GlyphIcon — the numeric character code IS the glyph's identity + menu item 'Close' page NavIconVariants.Close icon glyph 57377; + -- Forms$ImageIcon — an image collection, a different document + menu item 'Logo' page NavIconVariants.Logo icon image NavIconVariants.Images.logo; + -- A submenu takes any of the three too; it sits on the collapsed icon rail + menu 'Admin' icon glyph 57345 ( + menu item 'Users' page NavIconVariants.Home icon Atlas_Core.Atlas.user; + ); + ); + +-- The same item syntax serves a standalone menu document, which is why one AST +-- node backs both and the two cannot diverge. +create or modify menu NavIconVariants.Main_Menu ( + menu item 'Home' page NavIconVariants.Home icon Atlas_Core.Atlas.home; + menu item 'Close' page NavIconVariants.Close icon glyph 57377; +); diff --git a/mdl/ast/ast_navigation.go b/mdl/ast/ast_navigation.go index af81e8f317..f2bc41c883 100644 --- a/mdl/ast/ast_navigation.go +++ b/mdl/ast/ast_navigation.go @@ -2,6 +2,8 @@ package ast +import "github.com/mendixlabs/mxcli/mdl/types" + // AlterNavigationStmt represents: CREATE [OR REPLACE] NAVIGATION [clauses...] // This is a full-replacement command: omitted clauses clear that section. type AlterNavigationStmt struct { @@ -25,12 +27,19 @@ type NavHomePageDef struct { // NavMenuItemDef represents a MENU ITEM or MENU sub-menu definition. type NavMenuItemDef struct { - Caption string // from STRING_LITERAL - Page *QualifiedName // PAGE target - Microflow *QualifiedName // MICROFLOW target - SignOut bool // SIGN_OUT — the third action a menu item can carry - Icon string // ICON 'Module.Collection.name', empty for none - Items []NavMenuItemDef // Sub-items (for MENU 'caption' (...)) + Caption string // from STRING_LITERAL + Page *QualifiedName // PAGE target + Microflow *QualifiedName // MICROFLOW target + SignOut bool // SIGN_OUT — the third action a menu item can carry + Icon string // the qualified name, for the collection and image kinds + // IconKind says WHICH of Mendix's three icon elements was written. They are + // not variants of one value — a glyph carries a numeric code and no name — + // so a single Icon string could express only one of the three, and the other + // two were destroyed on rewrite. + IconKind types.MenuIconKind + // IconCode is the glyph's numeric character code, set only for MenuIconGlyph. + IconCode int + Items []NavMenuItemDef // Sub-items (for MENU 'caption' (...)) } // CreateMenuStmt is `create [or modify] menu Module.Name ( )` — a diff --git a/mdl/backend/modelsdk/menu_write.go b/mdl/backend/modelsdk/menu_write.go index 0f6e4869e1..dde62538e1 100644 --- a/mdl/backend/modelsdk/menu_write.go +++ b/mdl/backend/modelsdk/menu_write.go @@ -120,19 +120,56 @@ func menuCaptionToGen(caption string) element.Element { return t } -// menuIconToGen emits only Forms$IconCollectionIcon, the one variant MDL can -// name. A glyph icon carries a numeric code and an image icon points into an -// image collection; neither is expressible in the ICON clause, so an item that -// had one keeps no icon rather than getting a wrong one. DESCRIBE flags those on -// the way out, so the loss is visible rather than silent. +// menuIconToGen emits whichever of Mendix's three icon elements the item +// carries. +// +// It used to emit only Forms$IconCollectionIcon, because that was the one +// variant the ICON clause could name — so an item holding a glyph or image icon +// was rewritten with NO icon. `create or modify menu` is a full replacement, so +// that was deletion, not omission: the item came back without the icon it went +// in with, at exit 0. +// +// Nothing is inferred here. The kind comes from the author's `icon glyph …` / +// `icon image …` or from the kind the reader saw in storage. func menuIconToGen(item *types.NavMenuItem) element.Element { - if item.Icon == "" { - return nil + kind := types.MenuIconKindOf(item.IconType) + if kind == types.MenuIconNone && item.Icon != "" { + // An item built before the kind existed carries a name and nothing else, + // and that name has only ever meant an icon-collection icon. + kind = types.MenuIconCollection + } + switch kind { + case types.MenuIconGlyph: + // A glyph with no code identifies no glyph; an empty element is worse + // than none, because it renders as a blank where an icon should be. + if item.IconCode == 0 { + return nil + } + icon := genPages.NewGlyphIcon() + icon.SetID(element.ID(mmpr.GenerateID())) + icon.SetCode(int32(item.IconCode)) + return icon + case types.MenuIconImage: + if item.Icon == "" { + return nil + } + icon := genPages.NewImageIcon() + icon.SetID(element.ID(mmpr.GenerateID())) + icon.SetImageQualifiedName(item.Icon) + return icon + case types.MenuIconCollection: + if item.Icon == "" { + return nil + } + icon := genPages.NewIconCollectionIcon() + icon.SetID(element.ID(mmpr.GenerateID())) + icon.SetImageQualifiedName(item.Icon) + return icon } - icon := genPages.NewIconCollectionIcon() - icon.SetID(element.ID(mmpr.GenerateID())) - icon.SetImageQualifiedName(item.Icon) - return icon + // MenuIconUnknown: a stored $Type this build does not know. Emitting nothing + // would delete it, so the codec must refuse rather than guess — see the + // caller, which turns this into an error. + return nil } // menuActionToGen builds the item's client action. The gen type names are the SDK diff --git a/mdl/backend/modelsdk/navigation_icon_test.go b/mdl/backend/modelsdk/navigation_icon_test.go index 4e796ae9c6..37c7ad7b12 100644 --- a/mdl/backend/modelsdk/navigation_icon_test.go +++ b/mdl/backend/modelsdk/navigation_icon_test.go @@ -25,7 +25,7 @@ func navIconEntry(d bson.D, key string) (interface{}, bool) { // divergence here means MXCLI_ENGINE silently changes what lands in the .mpr — // and only one of the two would open in Studio Pro. func TestNavMenuIconBson_MatchesTheMprEngine(t *testing.T) { - d, ok := navMenuIconBson("Atlas_Core.Atlas.align-center").(bson.D) + d, ok := navMenuIconBson(types.NavMenuItemSpec{Icon: "Atlas_Core.Atlas.align-center"}).(bson.D) if !ok { t.Fatal("expected a bson.D") } @@ -41,8 +41,8 @@ func TestNavMenuIconBson_MatchesTheMprEngine(t *testing.T) { } func TestNavMenuIconBson_EmptyNameStaysNull(t *testing.T) { - if got := navMenuIconBson(""); got != nil { - t.Errorf("navMenuIconBson(\"\") = %v, want nil", got) + if got := navMenuIconBson(types.NavMenuItemSpec{}); got != nil { + t.Errorf("navMenuIconBson(empty spec) = %v, want nil", got) } } @@ -78,9 +78,9 @@ func TestNavFormSettingsBson_NoTitleOverrideStaysNull(t *testing.T) { } func TestMenuIconOf_NilIconYieldsNothing(t *testing.T) { - typeName, image := menuIconOf(nil) - if typeName != "" || image != "" { - t.Errorf("menuIconOf(nil) = (%q, %q), want empty", typeName, image) + typeName, image, code := menuIconOf(nil) + if typeName != "" || image != "" || code != 0 { + t.Errorf("menuIconOf(nil) = (%q, %q, %d), want empty", typeName, image, code) } } @@ -116,7 +116,7 @@ func TestMenuIconOf_ReadsTheNameOffARegisteredVariant(t *testing.T) { } { t.Run(tc.typeName, func(t *testing.T) { el := decodeIcon(t, tc.typeName, bsonv2.E{Key: "Image", Value: tc.image}) - gotType, gotImage := menuIconOf(el) + gotType, gotImage, _ := menuIconOf(el) if gotType != tc.typeName { t.Errorf("type = %q, want %q", gotType, tc.typeName) } @@ -131,7 +131,7 @@ func TestMenuIconOf_ReadsTheNameOffARegisteredVariant(t *testing.T) { // DESCRIBE from emitting a lossy ICON clause for it. func TestMenuIconOf_GlyphHasNoName(t *testing.T) { el := decodeIcon(t, "Forms$GlyphIcon", bsonv2.E{Key: "Code", Value: int32(9999)}) - gotType, gotImage := menuIconOf(el) + gotType, gotImage, _ := menuIconOf(el) if gotType != "Forms$GlyphIcon" { t.Errorf("type = %q", gotType) } @@ -139,3 +139,33 @@ func TestMenuIconOf_GlyphHasNoName(t *testing.T) { t.Errorf("image = %q, want empty: a glyph has no qualified name", gotImage) } } + +// The glyph's Code is the only thing that says WHICH glyph. Reading the $Type +// alone left a caller knowing one was there and nothing more, so DESCRIBE could +// not re-emit it and a rewrite replaced it with nothing. +func TestMenuIconOf_ReadsTheGlyphCode(t *testing.T) { + el := decodeIcon(t, "Forms$GlyphIcon", bsonv2.E{Key: "Code", Value: int32(57345)}) + gotType, gotImage, gotCode := menuIconOf(el) + if gotType != "Forms$GlyphIcon" { + t.Errorf("type = %q, want Forms$GlyphIcon", gotType) + } + if gotImage != "" { + t.Errorf("image = %q, want empty — a glyph carries no qualified name", gotImage) + } + if gotCode != 57345 { + t.Errorf("code = %d, want 57345", gotCode) + } +} + +// The control: a collection icon must NOT pick up a code, or "has a code" stops +// distinguishing a glyph from anything else. +func TestMenuIconOf_CollectionIconHasNoCode(t *testing.T) { + el := decodeIcon(t, "Forms$IconCollectionIcon", bsonv2.E{Key: "Image", Value: "Atlas_Core.Atlas.home"}) + _, gotImage, gotCode := menuIconOf(el) + if gotImage != "Atlas_Core.Atlas.home" { + t.Errorf("image = %q", gotImage) + } + if gotCode != 0 { + t.Errorf("code = %d, want 0", gotCode) + } +} diff --git a/mdl/backend/modelsdk/navigation_read.go b/mdl/backend/modelsdk/navigation_read.go index 7b98df53d5..9e4ee77662 100644 --- a/mdl/backend/modelsdk/navigation_read.go +++ b/mdl/backend/modelsdk/navigation_read.go @@ -237,7 +237,7 @@ func navMenuItemFromGen(el element.Element) *types.NavMenuItem { item := &types.NavMenuItem{ Caption: textOf(mi.Caption()), } - item.IconType, item.Icon = menuIconOf(mi.Icon()) + item.IconType, item.Icon, item.IconCode = menuIconOf(mi.Icon()) resolveMenuAction(item, mi.Action()) for _, subEl := range mi.ItemsItems() { if sub := navMenuItemFromGen(subEl); sub != nil { @@ -262,21 +262,31 @@ func navMenuItemFromGen(el element.Element) *types.NavMenuItem { // nothing for exactly the registered variants — which is what made DESCRIBE drop // every Atlas icon under this engine while the legacy engine printed them. Match // on the Raw() method instead, which both shapes satisfy. -func menuIconOf(icon element.Element) (typeName, image string) { +func menuIconOf(icon element.Element) (typeName, image string, code int) { if icon == nil { - return "", "" + return "", "", 0 } typeName = icon.TypeName() raw, ok := icon.(interface{ Raw() bson.Raw }) if !ok { - return typeName, "" + return typeName, "", 0 } if v, err := raw.Raw().LookupErr("Image"); err == nil { if s, ok := v.StringValueOK(); ok { image = s } } - return typeName, image + // Forms$GlyphIcon's Code is the ONLY thing identifying which glyph it is. + // Reading the $Type alone told a caller a glyph was there and nothing more, + // so DESCRIBE could not re-emit it and a rewrite replaced it with nothing. + if v, err := raw.Raw().LookupErr("Code"); err == nil { + if i, ok := v.Int32OK(); ok { + code = int(i) + } else if i, ok := v.Int64OK(); ok { + code = int(i) + } + } + return typeName, image, code } // resolveMenuAction sets the action type / target on a NavMenuItem from a gen diff --git a/mdl/backend/modelsdk/navigation_write.go b/mdl/backend/modelsdk/navigation_write.go index 1fd184d02e..58ce1bbe21 100644 --- a/mdl/backend/modelsdk/navigation_write.go +++ b/mdl/backend/modelsdk/navigation_write.go @@ -278,7 +278,7 @@ func navMenuItemBson(mi types.NavMenuItemSpec) bson.D { {Key: "Action", Value: navMenuAction(mi)}, {Key: "AlternativeText", Value: nil}, {Key: "Caption", Value: navCaptionBson(mi.Caption)}, - {Key: "Icon", Value: navMenuIconBson(mi.Icon)}, + {Key: "Icon", Value: navMenuIconBson(mi)}, } subItems := bson.A{navMarkerItems} for _, sub := range mi.Items { @@ -288,18 +288,42 @@ func navMenuItemBson(mi types.NavMenuItemSpec) bson.D { return item } -// navMenuIconBson mirrors sdk/mpr's buildMenuIconBson: the storage name is -// Forms$IconCollectionIcon (not the metamodel's Pages$…), and only that variant -// is emitted. See the comment there for why GlyphIcon/ImageIcon are excluded. -func navMenuIconBson(icon string) interface{} { - if icon == "" { +// navMenuIconBson mirrors sdk/mpr's buildMenuIconBson. The storage names are +// Forms$… (not the metamodel's Pages$…) — "Form" was the original term for +// "Page". +// +// All THREE variants are emitted. Only the icon-collection one used to be, so a +// glyph or image icon read off a real project came back as no icon at all, and +// `create or replace navigation` — a full replacement — wrote that nothing over +// the user's icon. Measured on testdata/expr-checker: exec of DESCRIBE's own +// output destroyed the Home item's glyph icon at exit 0. +func navMenuIconBson(spec types.NavMenuItemSpec) interface{} { + kind := spec.IconKind + if kind == types.MenuIconNone && spec.Icon != "" { + // A spec built before the kind existed carries a name and nothing else. + // That name has only ever meant an icon-collection icon. + kind = types.MenuIconCollection + } + storage := types.MenuIconStorageType(kind) + if storage == "" { return nil } - return bson.D{ + doc := bson.D{ {Key: "$ID", Value: navID()}, - {Key: "$Type", Value: "Forms$IconCollectionIcon"}, - {Key: "Image", Value: icon}, + {Key: "$Type", Value: storage}, + } + if kind == types.MenuIconGlyph { + // A glyph with no code identifies no glyph; writing one would store an + // icon nobody can see. Emit no icon rather than an empty element. + if spec.IconCode == 0 { + return nil + } + return append(doc, bson.E{Key: "Code", Value: int32(spec.IconCode)}) + } + if spec.Icon == "" { + return nil } + return append(doc, bson.E{Key: "Image", Value: spec.Icon}) } func navCaptionBson(text string) bson.D { diff --git a/mdl/backend/mpr/convert.go b/mdl/backend/mpr/convert.go index 658a82c7de..63bf8acadf 100644 --- a/mdl/backend/mpr/convert.go +++ b/mdl/backend/mpr/convert.go @@ -234,7 +234,7 @@ func convertNavProfile(in *mpr.NavigationProfile) *types.NavigationProfile { func convertNavMenuItem(in *mpr.NavMenuItem) *types.NavMenuItem { mi := &types.NavMenuItem{ Caption: in.Caption, Page: in.Page, Microflow: in.Microflow, ActionType: in.ActionType, - Icon: in.Icon, IconType: in.IconType, + Icon: in.Icon, IconType: in.IconType, IconCode: in.IconCode, } if in.Items != nil { mi.Items = make([]*types.NavMenuItem, len(in.Items)) diff --git a/mdl/executor/cmd_navigation.go b/mdl/executor/cmd_navigation.go index 5c6a36c201..543c0ee58b 100644 --- a/mdl/executor/cmd_navigation.go +++ b/mdl/executor/cmd_navigation.go @@ -117,8 +117,10 @@ func execAlterNavigation(ctx *ExecContext, s *ast.AlterNavigationStmt) error { // convertMenuItemDef converts an AST NavMenuItemDef to a writer NavMenuItemSpec. func convertMenuItemDef(def ast.NavMenuItemDef) types.NavMenuItemSpec { spec := types.NavMenuItemSpec{ - Caption: def.Caption, - Icon: def.Icon, + Caption: def.Caption, + Icon: def.Icon, + IconKind: def.IconKind, + IconCode: def.IconCode, } if def.Page != nil { spec.Page = def.Page.String() @@ -419,20 +421,49 @@ func printMenuMDL(w io.Writer, items []*types.NavMenuItem, depth int, reproducer // menuItemIconMDL renders the ICON clause for a menu item, or "" when there is // nothing CREATE NAVIGATION can reproduce. +// +// All three of Mendix's icon elements have a form now. Only the collection one +// used to, so DESCRIBE emitted a comment for a glyph or image icon — and since +// CREATE NAVIGATION is a full replacement, re-running that output DELETED the +// icon it had just declined to describe. func menuItemIconMDL(item *types.NavMenuItem) string { - if item.Icon == "" || !strings.HasSuffix(item.IconType, "IconCollectionIcon") { - return "" + switch types.MenuIconKindOf(item.IconType) { + case types.MenuIconGlyph: + // The code is the whole identity of a glyph. Without it there is nothing + // to emit that would rebuild the same icon, so fall through to the note. + if item.IconCode == 0 { + return "" + } + return fmt.Sprintf(" icon glyph %d", item.IconCode) + case types.MenuIconImage: + if item.Icon == "" { + return "" + } + return " icon image " + quoteQualifiedName(item.Icon) + case types.MenuIconCollection: + if item.Icon == "" { + return "" + } + return " icon " + quoteQualifiedName(item.Icon) } - return " icon " + quoteQualifiedName(item.Icon) + return "" } -// menuItemIconNote flags an icon DESCRIBE cannot round-trip, so re-running the -// output loses it visibly rather than silently. CREATE NAVIGATION writes only -// Forms$IconCollectionIcon; a glyph icon (numeric Code) or an image icon -// (pointing into an image collection, not an icon collection) is a different -// element and would have to be guessed at. +// menuItemIconNote flags an icon DESCRIBE still cannot round-trip, so re-running +// the output loses it visibly rather than silently. +// +// All three icon elements are reproducible now, so this fires only on what is +// genuinely beyond the language: a stored $Type this build does not know, or a +// variant whose payload is missing (a glyph with no Code, a named icon with no +// name) — where emitting a clause would rebuild a DIFFERENT icon rather than the +// same one. Guessing between polymorphic variants is the failure mode that +// produces a document mxbuild accepts and Studio Pro cannot open. func menuItemIconNote(item *types.NavMenuItem, reproducer string) string { - if item.IconType == "" || strings.HasSuffix(item.IconType, "IconCollectionIcon") { + if item.IconType == "" { + return "" + } + // If there is a clause for it, there is nothing to flag. + if menuItemIconMDL(item) != "" { return "" } target := item.Icon diff --git a/mdl/executor/cmd_navigation_icon_roundtrip_test.go b/mdl/executor/cmd_navigation_icon_roundtrip_test.go new file mode 100644 index 0000000000..0565241007 --- /dev/null +++ b/mdl/executor/cmd_navigation_icon_roundtrip_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/types" +) + +// DESCRIBE -> exec must not destroy a menu icon. +// +// # The failure this pins +// +// Measured on testdata/expr-checker, whose Home item carries a glyph icon: +// +// describe menu item 'Home' page …; +// -- icon a numeric glyph code (Forms$GlyphIcon) is not reproducible … +// exec Navigation profile 'Responsive' updated. +// describe menu item 'Home' page …; <- comment gone: the icon was DELETED +// +// CREATE NAVIGATION is a full replacement, so an icon the writer could not emit +// was an icon the statement removed. Exit 0, success message, silent loss — the +// same shape as the pluggable-widget body loss in mendixlabs/mxcli#1036. +// +// # Why this test is at this layer +// +// The round trip is describe -> parse -> write, and the defect lived in the fact +// that the three stages disagreed about what an icon IS. Asserting on the MDL +// text is what catches that: the emitted clause has to carry enough to rebuild +// the SAME element, not merely something that parses. +func TestMenuIconMDL_RoundTripsEveryVariant(t *testing.T) { + cases := []struct { + name string + item types.NavMenuItem + want string + }{ + { + "collection", + types.NavMenuItem{IconType: "Forms$IconCollectionIcon", Icon: "Atlas_Core.Atlas.home"}, + " icon Atlas_Core.Atlas.home", + }, + { + // The one that was being destroyed. The code IS the glyph's identity; + // reading only the $Type left DESCRIBE with nothing to say. + "glyph", + types.NavMenuItem{IconType: "Forms$GlyphIcon", IconCode: 57377}, + " icon glyph 57377", + }, + { + // An image icon points into an IMAGE collection, a different document + // from an icon collection — so it needs its own keyword, or replay + // would rebuild it as the wrong element. + "image", + types.NavMenuItem{IconType: "Forms$ImageIcon", Icon: "System.Images.Close"}, + " icon image System.Images.Close", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := menuItemIconMDL(&tc.item) + if got != tc.want { + t.Fatalf("menuItemIconMDL = %q, want %q", got, tc.want) + } + // It must also re-parse to the same kind, or DESCRIBE emits something + // that reads back as a different icon — which is what the bare form + // would have done for an image icon. + if note := menuItemIconNote(&tc.item, "CREATE NAVIGATION"); note != "" { + t.Errorf("still flagged as unreproducible: %q", note) + } + }) + } +} + +// The control. Without a case that still cannot be rebuilt, "emits a clause for +// everything" and "correctly reproduces everything" look identical — and the +// wrong one of those is how an unknown future variant would get silently +// converted into something else. +func TestMenuIconMDL_DeclinesWhatItCannotRebuild(t *testing.T) { + for _, item := range []types.NavMenuItem{ + // The code is the glyph's whole identity; without it there is nothing to + // emit that rebuilds the same icon. + {IconType: "Forms$GlyphIcon"}, + // A $Type this build does not know must never be guessed at. + {IconType: "Forms$SomeFutureIcon", Icon: "M.X.y"}, + } { + if got := menuItemIconMDL(&item); got != "" { + t.Errorf("%s: emitted %q, want no clause", item.IconType, got) + } + if note := menuItemIconNote(&item, "CREATE NAVIGATION"); !strings.Contains(note, "not reproducible") { + t.Errorf("%s: dropped silently instead of being flagged: %q", item.IconType, note) + } + } +} diff --git a/mdl/executor/cmd_navigation_icon_test.go b/mdl/executor/cmd_navigation_icon_test.go index 8f6cc5189e..c66676db33 100644 --- a/mdl/executor/cmd_navigation_icon_test.go +++ b/mdl/executor/cmd_navigation_icon_test.go @@ -70,27 +70,68 @@ func TestPrintMenuMDL_RoundTripsASubMenuIcon(t *testing.T) { } } -// The other two variants are real and appear in Studio Pro-authored projects, -// but CREATE NAVIGATION cannot write them. Emitting `icon '…'` for an ImageIcon -// would convert it to an IconCollectionIcon on replay — a silent variant swap. -// The loss has to be visible instead. -func TestPrintMenuMDL_FlagsAnIconItCannotReproduce(t *testing.T) { - for _, tc := range []struct{ name, iconType, icon, wantIn string }{ - {"image icon", "Forms$ImageIcon", "System.Images.Close", "System.Images.Close"}, - {"glyph icon", "Forms$GlyphIcon", "", "a numeric glyph code"}, +// All three variants round-trip now. They used to be flagged with a comment +// instead, and because CREATE NAVIGATION is a full replacement, re-running that +// output DELETED the icon the comment had just declined to describe — measured +// on testdata/expr-checker, a glyph icon destroyed at exit 0. +// +// Each form is emitted with its own keyword, so replay rebuilds the same +// ELEMENT. Writing `icon System.Images.Close` for an ImageIcon would have +// converted it to an IconCollectionIcon: a silent variant swap, which is why the +// bare form was not simply widened to cover all three. +func TestPrintMenuMDL_EmitsEachIconVariant(t *testing.T) { + for _, tc := range []struct{ name, iconType, icon, want string }{ + {"collection icon", "Forms$IconCollectionIcon", "Atlas_Core.Atlas.home", " icon Atlas_Core.Atlas.home"}, + {"image icon", "Forms$ImageIcon", "System.Images.Close", " icon image System.Images.Close"}, + } { + t.Run(tc.name, func(t *testing.T) { + got := menuMDL([]*types.NavMenuItem{{ + Caption: "Close", Page: "M.Close", Icon: tc.icon, IconType: tc.iconType, + }}) + if !strings.Contains(got, tc.want) { + t.Errorf("got %q, want it to contain %q", got, tc.want) + } + if strings.Contains(got, "-- icon") { + t.Errorf("still flagged as unreproducible: %q", got) + } + }) + } + + t.Run("glyph icon", func(t *testing.T) { + got := menuMDL([]*types.NavMenuItem{{ + Caption: "Close", Page: "M.Close", IconType: "Forms$GlyphIcon", IconCode: 57345, + }}) + if !strings.Contains(got, " icon glyph 57345") { + t.Errorf("got %q, want it to contain ` icon glyph 57345`", got) + } + if strings.Contains(got, "-- icon") { + t.Errorf("still flagged as unreproducible: %q", got) + } + }) +} + +// The note survives for what is genuinely beyond the language, and that is the +// control: without a case that still flags, "emits everything" and "flags +// nothing" are indistinguishable. +// +// A glyph with no Code cannot be rebuilt — the code IS the glyph's identity — and +// a $Type this build does not know must never be guessed at, because emitting a +// clause for it would rebuild a different element. +func TestPrintMenuMDL_StillFlagsWhatItCannotRebuild(t *testing.T) { + for _, tc := range []struct{ name, iconType, icon string }{ + {"glyph with no code", "Forms$GlyphIcon", ""}, + {"unknown variant", "Forms$SomeFutureIcon", "M.X.y"}, } { t.Run(tc.name, func(t *testing.T) { got := menuMDL([]*types.NavMenuItem{{ Caption: "Close", Page: "M.Close", Icon: tc.icon, IconType: tc.iconType, }}) - // Check the statement line only — the note below it also says "icon". stmt := strings.SplitN(got, "\n", 2)[0] if strings.Contains(stmt, " icon ") { - t.Errorf("emitted an ICON clause for %s, which replay would convert: %q", - tc.iconType, got) + t.Errorf("emitted a clause for %s, which replay could not rebuild: %q", tc.iconType, got) } - if !strings.Contains(got, "-- icon") || !strings.Contains(got, tc.wantIn) { - t.Errorf("the unreproducible icon was dropped silently: %q", got) + if !strings.Contains(got, "-- icon") { + t.Errorf("dropped silently: %q", got) } }) } diff --git a/mdl/executor/validate_navigation_icons.go b/mdl/executor/validate_navigation_icons.go index d2a2e17f6d..e5d2fdceb8 100644 --- a/mdl/executor/validate_navigation_icons.go +++ b/mdl/executor/validate_navigation_icons.go @@ -7,6 +7,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" ) // validateMenuItemIcons (MDL074) flags a navigation menu item that specifies no @@ -61,7 +62,11 @@ func validateMenuItemIcons(stmt ast.Statement) []linter.Violation { var walk func(list []ast.NavMenuItemDef) walk = func(list []ast.NavMenuItemDef) { for _, item := range list { - if item.Icon == "" { + // NOT `item.Icon == ""`. A glyph icon carries a numeric code and no + // name, so the obvious test reports an item that plainly has an + // icon — and every Studio Pro-authored menu is full of them. Ask the + // kind, which is what the writer and DESCRIBE also read. + if item.IconKind == types.MenuIconNone { out = append(out, linter.Violation{ RuleID: "MDL074", Severity: linter.SeverityWarning, diff --git a/mdl/executor/validate_navigation_icons_test.go b/mdl/executor/validate_navigation_icons_test.go index f9630b5ace..ee57c3e82c 100644 --- a/mdl/executor/validate_navigation_icons_test.go +++ b/mdl/executor/validate_navigation_icons_test.go @@ -8,6 +8,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" ) func navIconWarnings(vs []linter.Violation) []linter.Violation { @@ -34,7 +35,7 @@ func TestValidateMenuItemIcons_FlagsMissingIcon(t *testing.T) { stmt := &ast.AlterNavigationStmt{ ProfileName: "Responsive", MenuItems: []ast.NavMenuItemDef{ - {Caption: "Home", Icon: "Atlas_Core.Atlas.home"}, + {Caption: "Home", Icon: "Atlas_Core.Atlas.home", IconKind: types.MenuIconCollection}, {Caption: "Orders"}, }, } @@ -58,9 +59,9 @@ func TestValidateMenuItemIcons_IconedItemIsSilent(t *testing.T) { stmt := &ast.AlterNavigationStmt{ ProfileName: "Responsive", MenuItems: []ast.NavMenuItemDef{ - {Caption: "Home", Icon: "Atlas_Core.Atlas.home"}, - {Caption: "Admin", Icon: `Atlas_Core.Atlas."align-center"`, Items: []ast.NavMenuItemDef{ - {Caption: "Users", Icon: "Atlas_Core.Atlas.user"}, + {Caption: "Home", Icon: "Atlas_Core.Atlas.home", IconKind: types.MenuIconCollection}, + {Caption: "Admin", Icon: `Atlas_Core.Atlas."align-center"`, IconKind: types.MenuIconCollection, Items: []ast.NavMenuItemDef{ + {Caption: "Users", Icon: "Atlas_Core.Atlas.user", IconKind: types.MenuIconCollection}, }}, }, } @@ -75,9 +76,9 @@ func TestValidateMenuItemIcons_RecursesIntoSubItems(t *testing.T) { stmt := &ast.AlterNavigationStmt{ ProfileName: "Responsive", MenuItems: []ast.NavMenuItemDef{ - {Caption: "Admin", Icon: "Atlas_Core.Atlas.cog", Items: []ast.NavMenuItemDef{ + {Caption: "Admin", Icon: "Atlas_Core.Atlas.cog", IconKind: types.MenuIconCollection, Items: []ast.NavMenuItemDef{ {Caption: "Users"}, - {Caption: "Roles", Icon: "Atlas_Core.Atlas.group"}, + {Caption: "Roles", Icon: "Atlas_Core.Atlas.group", IconKind: types.MenuIconCollection}, {Caption: "Deep", Items: []ast.NavMenuItemDef{{Caption: "Deeper"}}}, }}, }, @@ -142,3 +143,21 @@ func TestValidateMenuItemIcons_RunsWithoutAProject(t *testing.T) { t.Errorf("got %d MDL074 from ValidateProgram with no project, want 1: %+v", len(got), got) } } + +// The trap this rule walked straight into: a glyph icon has a numeric code and +// NO name, so a check written as `Icon == ""` reports an item that plainly has +// an icon. Every Studio Pro-authored menu is full of them — the fixture's own +// Home item carries glyph 57377 — so the naive rule warned about the majority of +// real menus while claiming they had no icon. +func TestValidateMenuItemIcons_AGlyphIconIsAnIcon(t *testing.T) { + stmt := &ast.AlterNavigationStmt{ + ProfileName: "Responsive", + MenuItems: []ast.NavMenuItemDef{ + {Caption: "Home", IconKind: types.MenuIconGlyph, IconCode: 57377}, + {Caption: "Logo", IconKind: types.MenuIconImage, Icon: "MyMod.Images.logo"}, + }, + } + if got := navIconWarnings(validateMenuItemIcons(stmt)); len(got) != 0 { + t.Errorf("warned about items that have icons: %+v", got) + } +} diff --git a/mdl/grammar/MDLLexer.g4 b/mdl/grammar/MDLLexer.g4 index f40d3b7f44..40896bbff6 100644 --- a/mdl/grammar/MDLLexer.g4 +++ b/mdl/grammar/MDLLexer.g4 @@ -383,6 +383,9 @@ READONLY: R E A D O N L Y; ATTRIBUTES: A T T R I B U T E S; FILTERTYPE: F I L T E R T Y P E; IMAGE: I M A G E; +// GLYPH names Mendix's legacy icon element (Forms$GlyphIcon), a numeric +// character code rather than a reference into a collection. +GLYPH: G L Y P H; QUEUE: Q U E U E; QUEUES: Q U E U E S; SCHEDULED: S C H E D U L E D; diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 2725087072..a78fc1534f 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -342,8 +342,30 @@ navigationClause // which is why it sits beside PAGE and MICROFLOW rather than in a syntax of its // own. navMenuItemDef - : MENU_KW ITEM STRING_LITERAL ((PAGE qualifiedName) | (MICROFLOW qualifiedName) | SIGN_OUT)? (ICON qualifiedName)? SEMICOLON? - | MENU_KW STRING_LITERAL (ICON qualifiedName)? LPAREN navMenuItemDef* RPAREN SEMICOLON? + : MENU_KW ITEM STRING_LITERAL ((PAGE qualifiedName) | (MICROFLOW qualifiedName) | SIGN_OUT)? navMenuIcon? SEMICOLON? + | MENU_KW STRING_LITERAL navMenuIcon? LPAREN navMenuItemDef* RPAREN SEMICOLON? + ; + +// Mendix stores three DIFFERENT icon elements, and they are not variants of one +// value: an icon-collection icon and an image icon each hold a qualified name — +// into an icon collection and an image collection, which are different documents +// — while a glyph icon holds a numeric character code and no name at all. +// +// ICON Atlas_Core.Atlas.home Forms$IconCollectionIcon +// ICON GLYPH 57345 Forms$GlyphIcon +// ICON IMAGE MyModule.Images.logo Forms$ImageIcon +// +// Only the first was expressible, so DESCRIBE emitted a comment for the other +// two and re-running its own output DESTROYED them. +// +// The two keyword-led alternatives come FIRST. qualifiedName accepts a keyword +// as a name segment (identifierOrKeyword), so `ICON IMAGE …` also matches the +// bare form with `image` read as the name; listing the specific alternatives +// ahead of the general one is what settles it. +navMenuIcon + : ICON GLYPH NUMBER_LITERAL + | ICON IMAGE qualifiedName + | ICON qualifiedName ; // A standalone menu document (Menus$MenuDocument) — the reusable menu a menu diff --git a/mdl/grammar/domains/MDLSettings.g4 b/mdl/grammar/domains/MDLSettings.g4 index c4479d6975..b042833adf 100644 --- a/mdl/grammar/domains/MDLSettings.g4 +++ b/mdl/grammar/domains/MDLSettings.g4 @@ -628,7 +628,7 @@ keyword | CAPTION | CAPTIONPARAMS | CLASS | COLUMN | COLUMNS | CONTENT | CONTENTPARAMS | DATASOURCE | DEFAULT | DESIGNPROPERTIES | DESKTOPWIDTH | DISPLAY | DOCUMENTATION | EDITABLE | FILTER | FILTERTYPE | HEADER | FOOTER - | ICON | DARK | LABEL | ONCLICK | ONCHANGE | PARAMS | PASSING + | ICON | GLYPH | DARK | LABEL | ONCLICK | ONCHANGE | PARAMS | PASSING | PHONEWIDTH | TABLETWIDTH | READONLY | RENDERMODE | REQUIRED | NULLABLE | SELECTION | STYLE | STYLING | TABINDEX | TITLE | TOOLTIP | URL | POSITION | VISIBLE | WIDTH | HEIGHT | WIDGETTYPE diff --git a/mdl/types/navigation.go b/mdl/types/navigation.go index 4b55c2d277..10e71187fd 100644 --- a/mdl/types/navigation.go +++ b/mdl/types/navigation.go @@ -2,7 +2,11 @@ package types -import "github.com/mendixlabs/mxcli/model" +import ( + "strings" + + "github.com/mendixlabs/mxcli/model" +) // NavigationDocument represents a parsed navigation document. type NavigationDocument struct { @@ -54,11 +58,94 @@ type NavMenuItem struct { // Empty for no icon and for a glyph icon, which carries a numeric Code // instead. IconType keeps the storage $Type so a reader can tell the three // apart — DESCRIBE only round-trips Forms$IconCollectionIcon. - Icon string `json:"icon,omitempty"` - IconType string `json:"iconType,omitempty"` + Icon string `json:"icon,omitempty"` + IconType string `json:"iconType,omitempty"` + // IconCode is Forms$GlyphIcon's numeric Code — the ONLY thing that + // identifies a glyph icon, since it carries no qualified name. Without it a + // reader knows a glyph was there but not which one, so it can neither be + // re-emitted by DESCRIBE nor carried through a rewrite. + IconCode int `json:"iconCode,omitempty"` Items []*NavMenuItem `json:"items,omitempty"` } +// HasIcon reports whether the item carries an icon of ANY of the three kinds. +// +// Not `Icon != ""`: a glyph icon has a numeric code and no name, so the obvious +// test calls an item with a perfectly good icon iconless. MDL074 asks this +// question, and asking it the obvious way would have made the rule fire on every +// Studio Pro-authored menu. +func (m *NavMenuItem) HasIcon() bool { + if m == nil { + return false + } + switch MenuIconKindOf(m.IconType) { + case MenuIconNone: + return false + case MenuIconGlyph: + return true + default: + // A collection or image icon without a name is a malformed element, not + // an icon anyone can see. + return m.Icon != "" + } +} + +// MenuIconKind names which of Mendix's three icon elements a menu item carries. +// +// They are not variations on one shape: an icon-collection icon and an image +// icon each hold a qualified name (into an icon collection and an image +// collection respectively — different documents), while a glyph icon holds a +// numeric character code and no name at all. Treating them as one "icon string" +// is what made a rewrite silently convert a glyph into nothing. +type MenuIconKind string + +const ( + MenuIconNone MenuIconKind = "" + MenuIconCollection MenuIconKind = "collection" + MenuIconGlyph MenuIconKind = "glyph" + MenuIconImage MenuIconKind = "image" + // MenuIconUnknown is a stored $Type this build does not know. It is + // deliberately NOT MenuIconNone: reporting an unrecognised element as "no + // icon" is how a future fourth variant would get silently dropped by a + // rewrite, which is the bug this vocabulary exists to prevent. + MenuIconUnknown MenuIconKind = "unknown" +) + +// MenuIconKindOf maps a stored $Type onto the vocabulary. +// +// Matched on the suffix because the same element has two spellings: the +// metamodel calls it Pages$IconCollectionIcon and storage calls it +// Forms$IconCollectionIcon ("Form" was the original term for "Page"). A reader +// handing over either name must land on the same kind. +func MenuIconKindOf(iconType string) MenuIconKind { + switch { + case iconType == "": + return MenuIconNone + case strings.HasSuffix(iconType, "IconCollectionIcon"): + return MenuIconCollection + case strings.HasSuffix(iconType, "GlyphIcon"): + return MenuIconGlyph + case strings.HasSuffix(iconType, "ImageIcon"): + return MenuIconImage + } + return MenuIconUnknown +} + +// MenuIconStorageType is the inverse: the $Type a writer must emit for a kind. +// Empty for MenuIconNone (no Icon element at all) and for MenuIconUnknown, +// which a writer must never invent a name for. +func MenuIconStorageType(kind MenuIconKind) string { + switch kind { + case MenuIconCollection: + return "Forms$IconCollectionIcon" + case MenuIconGlyph: + return "Forms$GlyphIcon" + case MenuIconImage: + return "Forms$ImageIcon" + } + return "" +} + // MenuDocument is a standalone `Menus$MenuDocument` — a reusable menu that menu // widgets point at, stored as its own document rather than inside a navigation // profile. Atlas_Core ships two of them (Phone_Menu, Tablet_Menu). @@ -111,8 +198,15 @@ type NavMenuItemSpec struct { // as the same Forms$SignOutClientAction a button uses, so it needs no // target — which is why it is a flag rather than another name field. SignOut bool - // Icon is a qualified icon-collection name (Atlas_Core.Atlas.home). Empty - // means no icon, which serializes as a null Icon. - Icon string - Items []NavMenuItemSpec + // Icon is the qualified name for a collection or image icon + // (Atlas_Core.Atlas.home). Empty with IconKind None means no icon, which + // serializes as a null Icon. + Icon string + // IconKind selects which of the three icon elements to write. The spec used + // to carry a name and nothing else, so every icon became a + // Forms$IconCollectionIcon and a glyph turned into nothing on rewrite. + IconKind MenuIconKind + // IconCode is the glyph's numeric Code, meaningful only for MenuIconGlyph. + IconCode int + Items []NavMenuItemSpec } diff --git a/mdl/types/navigation_icon_test.go b/mdl/types/navigation_icon_test.go new file mode 100644 index 0000000000..2cc347554c --- /dev/null +++ b/mdl/types/navigation_icon_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 + +package types + +import "testing" + +// A menu item's icon is one of three Mendix elements, and mxcli could name only +// one of them. MenuIconKindOf maps the storage $Type onto a vocabulary so the +// readers, the writers, DESCRIBE and MDL074 all decide the same way instead of +// each doing its own strings.HasSuffix. +func TestMenuIconKindOf(t *testing.T) { + cases := []struct { + iconType string + want MenuIconKind + }{ + {"", MenuIconNone}, + {"Forms$IconCollectionIcon", MenuIconCollection}, + {"Forms$GlyphIcon", MenuIconGlyph}, + {"Forms$ImageIcon", MenuIconImage}, + // The metamodel spells these Pages$…; the storage name is Forms$… + // ("Form" was the original term for "Page"). Both must map, or a reader + // that hands over the metamodel name silently produces "no icon". + {"Pages$IconCollectionIcon", MenuIconCollection}, + {"Pages$GlyphIcon", MenuIconGlyph}, + {"Pages$ImageIcon", MenuIconImage}, + // Anything else is unknown rather than none: "none" would make a future + // fourth variant look like an absent icon and get silently dropped. + {"Forms$SomethingElse", MenuIconUnknown}, + } + for _, c := range cases { + if got := MenuIconKindOf(c.iconType); got != c.want { + t.Errorf("MenuIconKindOf(%q) = %q, want %q", c.iconType, got, c.want) + } + } +} + +// The inverse, used by the writers. Round-tripping the vocabulary back to the +// storage name is what lets one kind field drive both engines. +func TestMenuIconStorageType(t *testing.T) { + for _, k := range []MenuIconKind{MenuIconCollection, MenuIconGlyph, MenuIconImage} { + st := MenuIconStorageType(k) + if st == "" { + t.Errorf("no storage type for kind %q", k) + continue + } + if back := MenuIconKindOf(st); back != k { + t.Errorf("%q -> %q -> %q, want round trip", k, st, back) + } + } + if MenuIconStorageType(MenuIconNone) != "" { + t.Error("MenuIconNone must have no storage type — it is the absence of an Icon element") + } +} + +// HasIcon is what MDL074 asks. A glyph icon carries a numeric code and NO name, +// so a check written as `Icon == ""` reports an item that plainly has an icon. +func TestNavMenuItemHasIcon(t *testing.T) { + cases := []struct { + name string + item NavMenuItem + want bool + }{ + {"no icon", NavMenuItem{}, false}, + {"collection", NavMenuItem{IconType: "Forms$IconCollectionIcon", Icon: "Atlas_Core.Atlas.home"}, true}, + {"image", NavMenuItem{IconType: "Forms$ImageIcon", Icon: "MyMod.Images.logo"}, true}, + // The case that matters: a name-less icon that still IS an icon. + {"glyph", NavMenuItem{IconType: "Forms$GlyphIcon", IconCode: 57345}, true}, + } + for _, c := range cases { + if got := c.item.HasIcon(); got != c.want { + t.Errorf("%s: HasIcon() = %v, want %v", c.name, got, c.want) + } + } +} diff --git a/mdl/visitor/visitor_navigation.go b/mdl/visitor/visitor_navigation.go index 460a529ed4..787761c1ca 100644 --- a/mdl/visitor/visitor_navigation.go +++ b/mdl/visitor/visitor_navigation.go @@ -3,8 +3,11 @@ package visitor import ( + "strconv" + "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/grammar/parser" + "github.com/mendixlabs/mxcli/mdl/types" ) // ExitCreateNavigationStatement handles CREATE [OR REPLACE] NAVIGATION . @@ -93,29 +96,26 @@ func buildNavMenuItemDef(ctx parser.INavMenuItemDefContext) ast.NavMenuItemDef { item := ast.NavMenuItemDef{Caption: caption} - // Both the PAGE/MICROFLOW target and the ICON are qualifiedNames, so they - // arrive in one indexed list. The target always comes first when present; - // whatever remains after it is the icon. - names := c.AllQualifiedName() - next := 0 - switch { - case c.PAGE() != nil && len(names) > next: - built := buildQualifiedName(names[next]) - item.Page = &built - next++ - case c.MICROFLOW() != nil && len(names) > next: - built := buildQualifiedName(names[next]) - item.Microflow = &built - next++ + // The PAGE/MICROFLOW target is the item's only qualifiedName now that the + // icon is its own sub-rule — which is what removed the old positional + // bookkeeping, where the target and the icon shared one indexed list and the + // icon was "whatever remains". + if qn := c.QualifiedName(); qn != nil { + switch { + case c.PAGE() != nil: + built := buildQualifiedName(qn) + item.Page = &built + case c.MICROFLOW() != nil: + built := buildQualifiedName(qn) + item.Microflow = &built + } } - // SIGN_OUT names no target, so it consumes none of the qualifiedName list — - // which is why it is read separately rather than as a third switch arm. + // SIGN_OUT names no target, which is why it is read separately rather than + // as a third switch arm. if c.SIGN_OUT() != nil { item.SignOut = true } - if c.ICON() != nil && len(names) > next { - item.Icon = buildQualifiedName(names[next]).String() - } + applyNavMenuIcon(&item, c.NavMenuIcon()) // Recurse into sub-items (for MENU 'caption' (...)) for _, subCtx := range c.AllNavMenuItemDef() { @@ -125,3 +125,46 @@ func buildNavMenuItemDef(ctx parser.INavMenuItemDefContext) ast.NavMenuItemDef { return item } + +// applyNavMenuIcon reads the ICON clause onto the item. +// +// Mendix stores three different icon ELEMENTS, not three spellings of one +// value: a collection icon and an image icon each hold a qualified name (into an +// icon collection and an image collection — different documents), while a glyph +// icon holds a numeric character code and no name at all. The kind is recorded +// so the writer emits the right $Type; collapsing them onto one string is what +// made a rewrite turn a glyph into nothing. +// +// The bare form is the collection icon, which keeps every existing script +// meaning exactly what it did. +func applyNavMenuIcon(item *ast.NavMenuItemDef, ctx parser.INavMenuIconContext) { + if ctx == nil { + return + } + c, ok := ctx.(*parser.NavMenuIconContext) + if !ok { + return + } + switch { + case c.GLYPH() != nil: + item.IconKind = types.MenuIconGlyph + if n := c.NUMBER_LITERAL(); n != nil { + // A glyph code is a character code: whole, and small. A fractional or + // unparseable literal leaves the code at zero rather than guessing, + // and the writer refuses to emit a glyph without one. + if v, err := strconv.Atoi(n.GetText()); err == nil { + item.IconCode = v + } + } + case c.IMAGE() != nil: + item.IconKind = types.MenuIconImage + if qn := c.QualifiedName(); qn != nil { + item.Icon = buildQualifiedName(qn).String() + } + default: + item.IconKind = types.MenuIconCollection + if qn := c.QualifiedName(); qn != nil { + item.Icon = buildQualifiedName(qn).String() + } + } +} diff --git a/mdl/visitor/visitor_navigation_icon_test.go b/mdl/visitor/visitor_navigation_icon_test.go index f63430023b..2af4c1a05f 100644 --- a/mdl/visitor/visitor_navigation_icon_test.go +++ b/mdl/visitor/visitor_navigation_icon_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/types" ) // navMenuItems parses a CREATE NAVIGATION with the given menu body and returns @@ -101,3 +102,88 @@ func TestNavMenuItem_IconWithoutATarget(t *testing.T) { t.Error("no target was written, so none must be built") } } + +// Mendix stores three different icon elements and MDL could name only one, so +// DESCRIBE emitted a comment for the other two and re-running its own output +// destroyed them. These are the two new forms. +// +// A parse test is not enough here: the slice 2-3 lesson is that a corpus diff of +// `check` output is blind to a construct that parses into the WRONG SHAPE. These +// assert the AST, which is what the writer reads. +func TestNavMenuItem_ParsesAGlyphIcon(t *testing.T) { + items := navMenuItems(t, `MENU ITEM 'Dashboard' PAGE M.Dash ICON GLYPH 57345;`) + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].IconKind != types.MenuIconGlyph { + t.Errorf("IconKind = %q, want %q", items[0].IconKind, types.MenuIconGlyph) + } + if items[0].IconCode != 57345 { + t.Errorf("IconCode = %d, want 57345", items[0].IconCode) + } + if items[0].Icon != "" { + t.Errorf("Icon = %q, want empty — a glyph carries a code, not a name", items[0].Icon) + } + if items[0].Page == nil || items[0].Page.Name != "Dash" { + t.Error("the ICON clause displaced the PAGE target") + } +} + +func TestNavMenuItem_ParsesAnImageIcon(t *testing.T) { + items := navMenuItems(t, `MENU ITEM 'Dashboard' PAGE M.Dash ICON IMAGE MyMod.Images.logo;`) + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].IconKind != types.MenuIconImage { + t.Errorf("IconKind = %q, want %q", items[0].IconKind, types.MenuIconImage) + } + if items[0].Icon != "MyMod.Images.logo" { + t.Errorf("Icon = %q", items[0].Icon) + } + if items[0].Page == nil || items[0].Page.Name != "Dash" { + t.Error("the ICON clause displaced the PAGE target") + } +} + +// The control, and the one that would break first: qualifiedName accepts a +// keyword as a name segment, so `ICON IMAGE …` also matches the BARE form with +// `image` read as the name. The bare form must keep meaning icon-collection, and +// the keyword-led form must not be swallowed by it. +func TestNavMenuItem_BareIconIsStillACollectionIcon(t *testing.T) { + items := navMenuItems(t, `MENU ITEM 'Home' PAGE M.Home ICON Atlas_Core.Atlas.home;`) + if items[0].IconKind != types.MenuIconCollection { + t.Errorf("IconKind = %q, want %q — the bare form is the collection icon", + items[0].IconKind, types.MenuIconCollection) + } + if items[0].Icon != "Atlas_Core.Atlas.home" { + t.Errorf("Icon = %q", items[0].Icon) + } + if items[0].IconCode != 0 { + t.Errorf("IconCode = %d, want 0", items[0].IconCode) + } +} + +// An item with no icon at all keeps the zero kind, which is what MDL074 reads. +func TestNavMenuItem_NoIconIsKindNone(t *testing.T) { + items := navMenuItems(t, `MENU ITEM 'Home' PAGE M.Home;`) + if items[0].IconKind != types.MenuIconNone { + t.Errorf("IconKind = %q, want none", items[0].IconKind) + } +} + +// A submenu takes the new forms too — it sits directly on the collapsed rail. +func TestNavMenuItem_SubMenuTakesAGlyphIcon(t *testing.T) { + items := navMenuItems(t, "MENU 'Admin' ICON GLYPH 100 (\n MENU ITEM 'Users' PAGE M.U ICON IMAGE M.I.u;\n);") + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].IconKind != types.MenuIconGlyph || items[0].IconCode != 100 { + t.Errorf("submenu icon = (%q, %d), want (glyph, 100)", items[0].IconKind, items[0].IconCode) + } + if len(items[0].Items) != 1 { + t.Fatalf("sub-items = %d, want 1 — the icon clause ate the block", len(items[0].Items)) + } + if items[0].Items[0].IconKind != types.MenuIconImage { + t.Errorf("sub-item kind = %q, want image", items[0].Items[0].IconKind) + } +} diff --git a/sdk/mpr/parser_misc.go b/sdk/mpr/parser_misc.go index 6eeb70b12a..bd9adcb702 100644 --- a/sdk/mpr/parser_misc.go +++ b/sdk/mpr/parser_misc.go @@ -578,6 +578,10 @@ func parseNavMenuItem(raw map[string]any) *NavMenuItem { if icon, ok := raw["Icon"].(map[string]any); ok { mi.IconType = extractString(icon["$Type"]) mi.Icon = extractString(icon["Image"]) + // The glyph's Code identifies WHICH glyph; without it a caller knows one + // was there and nothing more, so it cannot be re-emitted or carried + // through a rewrite. + mi.IconCode = extractInt(icon["Code"]) } // Extract action type and target from Action diff --git a/sdk/mpr/writer_navigation.go b/sdk/mpr/writer_navigation.go index 57c3b3e305..44a90559f1 100644 --- a/sdk/mpr/writer_navigation.go +++ b/sdk/mpr/writer_navigation.go @@ -304,7 +304,7 @@ func buildMenuItemBson(mi NavMenuItemSpec) bson.D { {Key: "Action", Value: buildMenuAction(mi)}, {Key: "AlternativeText", Value: nil}, {Key: "Caption", Value: buildCaptionBson(mi.Caption)}, - {Key: "Icon", Value: buildMenuIconBson(mi.Icon)}, + {Key: "Icon", Value: buildMenuIconBson(mi)}, } // Sub-items @@ -327,21 +327,46 @@ func buildMenuItemBson(mi NavMenuItemSpec) bson.D { // the widget icon path already proven in issue #602. // // Two sibling variants exist in the same document — Forms$GlyphIcon{Code: int} -// and Forms$ImageIcon{Image: QN} — and are deliberately NOT emitted here. Both -// carry a different payload shape, and ImageIcon's qualified name is -// indistinguishable from an IconCollectionIcon's without resolving which -// collection document it lands in. Guessing between polymorphic variants is the -// failure mode that produces a document mxbuild accepts and Studio Pro cannot -// open. -func buildMenuIconBson(icon string) interface{} { - if icon == "" { +// and Forms$ImageIcon{Image: QN}. They used to be excluded because a name alone +// cannot tell an image icon from a collection icon without resolving which +// document it lands in, and guessing between polymorphic variants is the failure +// mode that produces a document mxbuild accepts and Studio Pro cannot open. +// +// Nothing is guessed now: the KIND is carried explicitly, from the author's own +// `icon image …` / `icon glyph …` or from the kind the reader saw in storage. So +// all three are emitted, and the branch is a dispatch rather than an inference. +// +// Excluding them was not neutral. `create or replace navigation` is a full +// replacement, so an icon the writer would not emit was an icon the statement +// DELETED — measured on testdata/expr-checker, exec of DESCRIBE's own output +// destroyed a glyph icon at exit 0. +func buildMenuIconBson(spec NavMenuItemSpec) interface{} { + kind := spec.IconKind + if kind == types.MenuIconNone && spec.Icon != "" { + // A spec built before the kind existed carries a name and nothing else, + // and that name has only ever meant an icon-collection icon. + kind = types.MenuIconCollection + } + storage := types.MenuIconStorageType(kind) + if storage == "" { return nil } - return bson.D{ + doc := bson.D{ {Key: "$ID", Value: idToBsonBinary(generateUUID())}, - {Key: "$Type", Value: "Forms$IconCollectionIcon"}, - {Key: "Image", Value: icon}, + {Key: "$Type", Value: storage}, + } + if kind == types.MenuIconGlyph { + // A glyph with no code identifies no glyph. Emit no icon rather than an + // element nobody can see. + if spec.IconCode == 0 { + return nil + } + return append(doc, bson.E{Key: "Code", Value: int32(spec.IconCode)}) + } + if spec.Icon == "" { + return nil } + return append(doc, bson.E{Key: "Image", Value: spec.Icon}) } // buildCaptionBson builds a Texts$Text BSON document with a single en_US translation. diff --git a/sdk/mpr/writer_navigation_icon_test.go b/sdk/mpr/writer_navigation_icon_test.go index 90efc9f593..970ec70b97 100644 --- a/sdk/mpr/writer_navigation_icon_test.go +++ b/sdk/mpr/writer_navigation_icon_test.go @@ -3,6 +3,7 @@ package mpr import ( + "github.com/mendixlabs/mxcli/mdl/types" "testing" "go.mongodb.org/mongo-driver/bson" @@ -30,7 +31,7 @@ func navIconEntry(d bson.D, key string) (interface{}, bool) { // Studio Pro-authored reference: every menu icon in ako/mxcli-ledger's // navigation document is Forms$IconCollectionIcon{Image: "Atlas_Core.Atlas.…"}. func TestBuildMenuIconBson_UsesTheFormsStorageName(t *testing.T) { - got := buildMenuIconBson("Atlas_Core.Atlas.align-center") + got := buildMenuIconBson(NavMenuItemSpec{Icon: "Atlas_Core.Atlas.align-center"}) d, ok := got.(bson.D) if !ok { t.Fatalf("expected a bson.D, got %T", got) @@ -59,8 +60,8 @@ func TestBuildMenuIconBson_UsesTheFormsStorageName(t *testing.T) { // No icon must stay a null, not an empty element: an IconCollectionIcon with a // blank Image is a dangling reference, where absent is the modelled default. func TestBuildMenuIconBson_EmptyNameStaysNull(t *testing.T) { - if got := buildMenuIconBson(""); got != nil { - t.Errorf("buildMenuIconBson(\"\") = %v, want nil", got) + if got := buildMenuIconBson(NavMenuItemSpec{}); got != nil { + t.Errorf("buildMenuIconBson(empty spec) = %v, want nil", got) } } @@ -203,3 +204,69 @@ func TestParseNavMenuItem_NoIconReadsAsNone(t *testing.T) { t.Errorf("Icon/IconType = (%q, %q), want both empty", mi.Icon, mi.IconType) } } + +// All three icon elements are written now. Only the collection variant used to +// be, and because `create or replace navigation` is a full replacement, an icon +// the writer would not emit was an icon the statement DELETED — measured on +// testdata/expr-checker, exec of DESCRIBE's own output destroyed a glyph icon at +// exit 0. +func TestBuildMenuIconBson_Glyph(t *testing.T) { + got, ok := buildMenuIconBson(NavMenuItemSpec{IconKind: types.MenuIconGlyph, IconCode: 57345}).(bson.D) + if !ok { + t.Fatal("a glyph icon produced no document") + } + m := bsonDToMap(got) + if m["$Type"] != "Forms$GlyphIcon" { + t.Errorf("$Type = %v, want Forms$GlyphIcon", m["$Type"]) + } + if m["Code"] != int32(57345) { + t.Errorf("Code = %#v, want int32(57345) — Mendix stores the character code as an int32", m["Code"]) + } + if _, hasImage := m["Image"]; hasImage { + t.Error("a glyph icon must not carry an Image; it has no qualified name") + } +} + +func TestBuildMenuIconBson_Image(t *testing.T) { + got, ok := buildMenuIconBson(NavMenuItemSpec{IconKind: types.MenuIconImage, Icon: "MyMod.Images.logo"}).(bson.D) + if !ok { + t.Fatal("an image icon produced no document") + } + m := bsonDToMap(got) + if m["$Type"] != "Forms$ImageIcon" { + t.Errorf("$Type = %v, want Forms$ImageIcon", m["$Type"]) + } + if m["Image"] != "MyMod.Images.logo" { + t.Errorf("Image = %v", m["Image"]) + } +} + +// The control that keeps the dispatch honest: a name with NO kind still means an +// icon-collection icon. Every script written before the kind existed carries +// exactly that, so treating it as "unknown" would silently drop every icon in +// the corpus. +func TestBuildMenuIconBson_BareNameIsStillACollectionIcon(t *testing.T) { + got, ok := buildMenuIconBson(NavMenuItemSpec{Icon: "Atlas_Core.Atlas.home"}).(bson.D) + if !ok { + t.Fatal("a bare name produced no document") + } + if bsonDToMap(got)["$Type"] != "Forms$IconCollectionIcon" { + t.Errorf("$Type = %v, want Forms$IconCollectionIcon", bsonDToMap(got)["$Type"]) + } +} + +// A glyph with no code identifies no glyph, and an element with no Code renders +// as a blank where an icon should be. Emit nothing instead. +func TestBuildMenuIconBson_GlyphWithoutACodeIsNoIcon(t *testing.T) { + if got := buildMenuIconBson(NavMenuItemSpec{IconKind: types.MenuIconGlyph}); got != nil { + t.Errorf("got %v, want nil", got) + } +} + +func bsonDToMap(d bson.D) map[string]any { + m := make(map[string]any, len(d)) + for _, e := range d { + m[e.Key] = e.Value + } + return m +}