diff --git a/.claude/commands/mendix/lint.md b/.claude/commands/mendix/lint.md index b17a4b3098..0894d3f077 100644 --- a/.claude/commands/mendix/lint.md +++ b/.claude/commands/mendix/lint.md @@ -40,7 +40,7 @@ mxcli lint -p app.mpr --exclude System --exclude Administration | MPR005 | quality | ImageSource - IMAGE widgets with no source configured | | MPR006 | quality | EmptyContainer - Empty layout containers | | MPR007 | security | PageNavigationSecurity - Navigation pages need allowed roles (CE0557); every user role must be able to open its home page (at Production security, else CE2729 per widget) | -| MPR012 | correctness | LegacyImageWidget - staticimage/dynamicimage are unsupported by the React client (CE0582) | +| MPR012 | correctness | LegacyImageWidget - staticimage/dynamicimage are unsupported by the React client (CE0582; info on the classic client) | | MPR013 | correctness | SnippetCallAppearance - Class/Style/DynamicClasses/design properties on a snippet call never reach the page (it renders no element) | | SEC001 | security | NoEntityAccessRules - Persistent entities need access rules | | SEC002 | security | WeakPasswordPolicy - Password minimum length should be 8+ | diff --git a/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-mpr012-classic-client.json b/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-mpr012-classic-client.json new file mode 100644 index 0000000000..574d702adc --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-mpr012-classic-client.json @@ -0,0 +1 @@ +{"area": "mdl/linter", "date": "2026-10-31", "symptom": "MPR012 warned on every legacy static/dynamic image widget of a Mendix 11 app on the classic client (Web UI UseOptimizedClient = No), each message saying `mxbuild reports CE0582`, while mx check reported 0 errors: 57 false warnings on one app.", "cause": "The rule assumed the React client is the only client on Mendix 11 and never read the project's Web UI setting; the executor's check for the same CE0582 (usesReactClient) already did.", "file": "mdl/linter/rules/legacy_image_widget.go", "insight": "When a lint rule and an executor check guard the same CE code, they must share the condition: grep the CE number across mdl/ before writing either. CE0582 is the React client's error, so it is conditional on Web UI UseOptimizedClient == Yes; MigrationMode builds clean too. A rule that claims what mxbuild reports should be measured under each setting, not reasoned from the Mendix version.", "issue": "ako/mxcli#1287"} diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-stale-list-marker-survives-rewrite.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-stale-list-marker-survives-rewrite.json new file mode 100644 index 0000000000..7f4e64a1af --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-stale-list-marker-survives-rewrite.json @@ -0,0 +1 @@ +{"area": "modelsdk/codec", "date": "2026-10-31", "symptom": "A Data grid 2 an older mxcli stored with SortBar.SortItems marker 3 kept it through every `create or replace page`, including one that changed the page for real, while Studio Pro and a page created from nothing store 2. mx check, mx diff (names only, 0 changeDetails), lint and `fix property-sets` were all clean. Separately, a populated Forms$CallNanoflowClientAction / Forms$NanoflowSource ParameterMappings list was written with marker 3 where Studio Pro writes 2", "cause": "A marker registration only reaches lists the encoder builds. An unchanged pluggable widget's Type and Object pass through byte for byte (passStoredThrough), and the codec copies an existing element's clean properties as raw bytes, so a stored list keeps the marker an older build gave it. The nanoflow lists had no owner/key registration, and the child type (Forms$NanoflowParameterMapping) has no marker of its own, so a populated list took the default 3", "file": "modelsdk/canon/listmarkers.go; modelsdk/mpr/writer_core.go (normalizeForStorage); modelsdk/codec/defaults.go; mdl/backend/modelsdk/widget_write.go", "fix": "The marker tables moved to canon. Every write (normalizeForStorage, so insert, update and transaction alike) runs canon.NormalizeListMarkers, which gives each list with a registered owner/key marker that marker anywhere in the unit and leaves unregistered lists and child-type registrations alone. Registered the two nanoflow lists as 2", "insight": "`drop page` plus the same MDL giving 2 while the rewrite kept 3 points at pass-through, not at the writer: look for what the rewrite copies rather than what it builds. Applying a registration to stored bytes turns a wrong registration into a rewrite of Studio Pro's bytes, so the census (every owner/key registration against TestApp and PedApp, plus normalization as a fixed point on every fixture unit) is a test. A forced-wrong registration fails it twice: 164 disagreements and 114 changed units. Child-type markers are not applied: they are the encoder's rule for lists it builds, not a measurement of every list starting with that child. Do not fix this with `fix property-sets`: a stale marker breaks no merge or check, so the units nobody rewrites can keep it", "refs": "ako/mxcli#1640", "test": "mdl/executor/issue1640_stale_list_marker_test.go; mdl/backend/modelsdk/issue1640_list_marker_census_test.go; modelsdk/canon/listmarkers_test.go"} diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index fa7bf58ecc..580c166a38 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -921,8 +921,10 @@ auto units and a responsive image — which `describe page` also omits, so a round trip neither loses them nor invents them. **CE0582** is reported for `staticimage` wherever it appears, by any app running -the React client — which Mendix added in **10.7** and which is the only client on -11, so this is not a Mendix 11 rule. The replacement is the pluggable `image` +the React client — which Mendix added in **10.7**. A Mendix 11 app can still run the +classic client (Web UI → `UseOptimizedClient` is `No` or `MigrationMode`), where these +widgets build clean, and there MPR012 is an info-level "convert before switching" note +instead of a warning. The replacement is the pluggable `image` widget, which takes the same `Image:`; Studio Pro offers the conversion from the CE0582 error's context menu. mxcli still writes it, because round-tripping a model that already contains one is the point — and `mxcli lint` reports it as diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index 9cc999c031..1c00113158 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -645,7 +645,7 @@ $var/Module.AssociationName/attribute -- Chained ### Annotation Pattern ```mdl -@position(200, 200) -- optional: omit it and mxcli lays the flow out; to re-arrange an existing flow run `mxcli layout flows` +@position(200, 200) -- optional: omit it and mxcli lays the flow out in the project's layout-config.yaml style (never imitate a style with @position; keep, never hand-write, a `@mxcli-layout:` doc line); to re-arrange an existing flow run `mxcli layout flows` @caption 'Persist order' @color Green @annotation 'Note about the next activity' diff --git a/.claude/skills/packs/mendix-vega-charts/widget/package-lock.json b/.claude/skills/packs/mendix-vega-charts/widget/package-lock.json index d81b0dae80..f7b80df69a 100644 --- a/.claude/skills/packs/mendix-vega-charts/widget/package-lock.json +++ b/.claude/skills/packs/mendix-vega-charts/widget/package-lock.json @@ -19832,9 +19832,9 @@ } }, "node_modules/serialize-javascript": { - "version": "7.1.0", - "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-7.1.0.tgz", - "integrity": "sha512-RNEqWOyhhUQYN9V1GfHwu9AR/g+NTciH6Z5u3/no6X3/w+04J2lVDL+svFQVXgXrEGBMG2puMVN3gq2SNGuTGw==", + "version": "7.1.4", + "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-7.1.4.tgz", + "integrity": "sha512-FqlEuoTq/zRmA4Lg6aOqyH3lCHbUtCB0bmHCeFniX2GBdDqMJM3FwdvEhlE5Pim1dlt2FfBdVPmUNNiGhhKqpw==", "dev": true, "license": "BSD-3-Clause", "engines": { diff --git a/CHANGELOG.md b/CHANGELOG.md index 90708168cb..6576a8ca10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Configurable microflow and nanoflow layout** (ako/mxcli#1832) — a committed `layout-config.yaml` (in `.claude/` or next to the `.mpr`) chooses how `create microflow`/`create nanoflow` and `mxcli layout flows` place what a script leaves unpositioned: preset `happy-line` (the default, unchanged) or `true-right-false-down`, and the options `if-else: true-right-else-column` (ELSE stacked as a column under its decision), `case-split: first-right-rest-columns` (a CASE's or type split's first outcome on the line, the rest as columns), `guard: true-right`, `wrap: off` and `max-row-width`. A flow laid out in a non-default style records it as an `@mxcli-layout:` line in its documentation, which DESCRIBE and `create or modify` keep using for that flow, so its description stays free of `@position` when the project style changes. `mxcli layout config` shows the style in force; `mxcli layout flows --style ` tries one. Without a config file nothing changes: measured on 137 flows, the geometry and DESCRIBE output are identical to before. - **Optional page parameters** (mendixlabs/mxcli#1640, ako/mxcli#1649) — `Params: ( $Sale: Shop.Sale optional )` is Studio Pro's unticked *Required* (`Forms$PageParameter.IsRequired = false`, Mendix 11.5+): the page can be opened with no argument, from a menu item for example, where a required parameter is CE1571. `create page`, `alter page … add parameters` and `describe page` all spell it. `describe navigation` no longer invents `(Sale = $currentObject)` for a menu item opening such a page, which made its output one `exec` refused (MDL-PAGEARG01) and left the navigation profile unchangeable from MDL. Below 11.5 `optional` is refused (MDL-VERSION01), and a snippet parameter cannot be optional (MDL087). ### Changed @@ -16,11 +17,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **MPR012 follows the app's Web UI client** (ako/mxcli#1287, mendixlabs/mxcli#1226) — a Mendix 11 app can still run the classic client, where the legacy static and dynamic image widgets build clean, but MPR012 warned on each of them that "mxbuild reports CE0582". On the classic client or in migration mode it is now an info-level note that the widget needs converting before the app switches; under the React client it stays a warning. - **Every `mxcli init --tool ` installs the bundled lint rules, and `lint` says when it runs without them** (ako/mxcli#1292, mendixlabs/mxcli#1242) — only `claude` and `opencode` wrote `.claude/lint-rules/`, so `init --tool cursor` (or aider, continue, copilot, vibe, windsurf) left `mxcli lint` running the built-in Go rules alone, silently. `lint` and `report` now print a warning when no `.claude/lint-rules/` is found, naming how many rules run instead. - **`show references to` an entity, attribute or association lists the message definitions that expose it** (#1781) — a message definition (the pre-11.15 collection, `MESSAGE_DEFINITION_COLLECTION`, and the 11.15+ document, `MESSAGE_DEFINITION`) was not walked for references, so impact analysis called an exposed entity or attribute unused. The entity edges are `mapping`, the attribute and association edges `member`, as for an import or export mapping. - **A pluggable widget's hidden expression property is stored as Studio Pro stores it** (ako/mxcli#548) — a property the widget's editorConfig hides is written at its declared default, and for an Expression-typed one that default went into `PrimitiveValue` as well as `Expression` (every combo box's `customEditabilityExpression`, `PrimitiveValue: "false"`). Studio Pro leaves `PrimitiveValue` empty on all 939 Expression-typed values of ako/TestApp and PedApp; mx check accepted both, so the cost was a widget that differed from Studio Pro's. - **A source catalog build describes a project widget the same way every run** (#1799) — `refresh catalog full source` sometimes wrote a pluggable widget in the generic `pluggablewidget ''` form instead of its own keyword (`vegachart …`), and since #1802 an incremental build kept that row until a `force`. Each parallel describe loaded its own widget registry, and with one definition that never counts as current (hand-made, or its `.mpk` removed), each load rewrote every `.def.json` while the other describes read them. The describes now share the build's registry, and a definition is replaced by rename, so no reader sees a half-written file. +- **A rewrite gives a stored list the marker Studio Pro writes** (ako/mxcli#1640) — a list an older mxcli stored with the wrong typed-array marker kept it through every later rewrite, because a rewrite copies much of a stored document as bytes (an unchanged pluggable widget's Type and Object, an existing element's unchanged properties). A Data grid 2's `SortBar.SortItems` stayed 3 where Studio Pro stores 2, and `drop page` plus the same MDL was the only way out. Every write now applies the measured owner/key markers to the whole unit, and lists without a measured marker are left as they are. A populated nanoflow parameter list (on a call nanoflow action or a nanoflow data source) is now written with 2, as Studio Pro does. mx check never reported either; the effect was a document that differed from Studio Pro's at the value level. + ## [0.29.0] - 2026-10-30 Headline: **Write what the project's own Mendix version stores, and commit it the way Studio Pro does.** Every write is now checked against the element types and property shapes of the project's Mendix version, so a statement 9.x or 10.x cannot store is refused by `check -p` instead of leaving a project Studio Pro cannot open, and `mxcli git commit` / `git doctor` keep a plain-git history in the shape Studio Pro expects. Alongside it: workflows, pages and Java actions gain the settings a rewrite used to reset, `alter microflow` edits inside loop bodies, the debugger works end to end, and commands on large MPR v2 projects open only the files they need. diff --git a/cmd/mxcli/cmd_layout_flows.go b/cmd/mxcli/cmd_layout_flows.go index f27c1dfc09..fca3f92bc7 100644 --- a/cmd/mxcli/cmd_layout_flows.go +++ b/cmd/mxcli/cmd_layout_flows.go @@ -35,9 +35,18 @@ The layout is the one CREATE MICROFLOW uses for a flow written without are sized to their bodies. Name the flows to lay out, or pass --module to lay out every microflow and nanoflow in a module. -Only positions change: activity and event positions and sizes, parameter +Positions change: activity and event positions and sizes, parameter positions, and the anchors and curves of sequence flows. Content, element IDs -and everything else about the flow are left exactly as stored. +and everything else about the flow are left exactly as stored, apart from +the one documentation line described next. + +The style is the project's layout-config.yaml (in .claude/ or next to the +.mpr; see 'mxcli layout config'), or the default when there is none. --style +lays out in a preset instead, to try one. Each flow laid out in a style other +than the default records it as an '@mxcli-layout:' line in its documentation, +which DESCRIBE and CREATE OR MODIFY then keep using for that flow; laying it out +in the default removes the line. That line is the one thing besides positions +this command writes. A flow whose description does not rebuild into the same graph — something MDL cannot yet express — is skipped with the reason, never laid out by guesswork. @@ -48,7 +57,8 @@ what would move first. Marketplace modules and System are never touched unless --include-marketplace is given.`, Example: ` mxcli layout flows -p app.mpr MyModule.ACT_Order_Submit mxcli layout flows -p app.mpr --module MyModule - mxcli layout flows -p app.mpr --module MyModule --dry-run`, + mxcli layout flows -p app.mpr --module MyModule --dry-run + mxcli layout flows -p app.mpr --module MyModule --style true-right-false-down --dry-run`, RunE: func(cmd *cobra.Command, args []string) error { return runLayoutFlows(cmd, args) }, @@ -59,9 +69,58 @@ func init() { "module whose flows to lay out (repeatable)") layoutFlowsCmd.Flags().BoolVar(&layoutDryRun, "dry-run", false, "report what would move without writing") + layoutFlowsCmd.Flags().StringVar(&layoutFlowStyle, "style", "", + "lay out in this preset instead of the project's layout-config.yaml (happy-line, true-right-false-down)") layoutFlowsCmd.Flags().BoolVar(&layoutIncludeMarketplace, "include-marketplace", false, "also lay out flows in Marketplace modules (a module update replaces them, so this is normally pointless)") layoutCmd.AddCommand(layoutFlowsCmd) + layoutCmd.AddCommand(layoutConfigCmd) +} + +var layoutFlowStyle string + +var layoutConfigCmd = &cobra.Command{ + Use: "config", + Short: "Show the layout style microflows and nanoflows are laid out with", + Long: `Show the style CREATE MICROFLOW, CREATE NANOFLOW and 'mxcli layout flows' +lay flows out with, and the file it comes from. + +The style is read from layout-config.yaml in the project's .claude/ directory or +next to the .mpr — a committed file, so everyone working on the project lays +flows out the same way: + + flows: + preset: true-right-false-down # happy-line (default) | true-right-false-down + if-else: true-right-else-column # lanes | true-right-else-column + case-split: lanes # lanes | first-right-rest-columns + guard: return-below # return-below | true-right + wrap: rows # rows | off + max-row-width: 2880 # px, 640..20000 + +An unknown key or value is an error rather than a silent fallback.`, + Example: ` mxcli layout config -p app.mpr`, + RunE: func(cmd *cobra.Command, args []string) error { + projectPath, _ := cmd.Flags().GetString("project") + if projectPath == "" { + return fmt.Errorf("no project given: pass -p ") + } + exec, logger := newLoggedExecutor("subcommand") + defer logger.Close() + defer exec.Close() + exec.SetQuiet(true) + connectProg, _ := visitor.Build(fmt.Sprintf("CONNECT LOCAL '%s'", visitor.QuoteString(projectPath))) + for _, stmt := range connectProg.Statements { + if err := exec.Execute(stmt); err != nil { + return err + } + } + style, source, err := exec.FlowLayoutStyle() + if err != nil { + return err + } + fmt.Fprintf(cmd.OutOrStdout(), "Flow layout: %s\nFrom: %s\n", style, source) + return nil + }, } // flowTarget is one flow to lay out. @@ -86,6 +145,9 @@ func runLayoutFlows(cmd *cobra.Command, args []string) error { defer logger.Close() defer exec.Close() exec.SetQuiet(true) + if err := exec.SetFlowLayoutStyle(layoutFlowStyle); err != nil { + return err + } connectProg, _ := visitor.Build(fmt.Sprintf("CONNECT LOCAL '%s'", visitor.QuoteString(projectPath))) for _, stmt := range connectProg.Statements { if err := exec.Execute(stmt); err != nil { @@ -195,6 +257,13 @@ func layoutFlowTargets(out io.Writer, exec *executor.Executor, targets []flowTar changed++ fmt.Fprintf(out, "%s: moved %d of %d objects, re-anchored %d flows\n", res.Name, res.Moved, res.Objects, res.Flows) } + if res.Restyled && res.Refused == "" { + verb := "updates" + if layoutDryRun { + verb = "would update" + } + fmt.Fprintf(out, "%s: %s the %s line in its documentation\n", res.Name, verb, "@mxcli-layout:") + } } switch { diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index 9ec83d8eff..2e0d0e1ea6 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -880,6 +880,8 @@ func init() { "position", "start", "anchor", "curve", "layout", "canvas", "annotation", "caption", "color", "excluded", "bezier", "disable activity", "disabled activity", + "layout style", "layout-config", "mxcli-layout", "else column", + "wrap", "row width", }, Syntax: "@position(x, y) -- the activity's centre point\n" + "@position(x, y) -- also on a PARAMETER, in the ( … ) list\n" + @@ -977,7 +979,19 @@ func init() { "never prints it: a whole flow declares its notes. (ako/mxcli#1695)\n\n" + "position:/size: are the note's own geometry, omitted whenever they match\n" + "what a rewrite re-derives (100px above the activity, stacked 60px per extra\n" + - "note, 200x50), so an ordinary note keeps the short form. (#1077)", + "note, 200x50), so an ordinary note keeps the short form. (#1077)\n\n" + + "LAYOUT STYLE — where mxcli puts what you leave unpositioned — is the\n" + + "project's layout-config.yaml (.claude/ or next to the .mpr), never something\n" + + "to imitate with @position. Leave positions out and the style applies:\n" + + " flows:\n" + + " preset: true-right-false-down # happy-line (default)\n" + + " if-else: true-right-else-column # ELSE as a column under its decision\n" + + " case-split: first-right-rest-columns\n" + + " guard: true-right # early return on the line\n" + + " wrap: off # or max-row-width: \n" + + "`mxcli layout config` shows the style in force. A flow drawn in a non-default\n" + + "style carries a `@mxcli-layout: …` line in its /** … */ documentation: keep\n" + + "it when copying a described flow, never write one by hand. (ako/mxcli#1832)", Example: "create microflow MyModule.ACT_Flow (\n @position(145, 0)\n $In: String\n)\nreturns String as $Out\nbegin\n" + " @start(145, 100)\n @position(200, 100)\n @anchor(from: bottom)\n" + " @curve(from: (40, -90), to: (-40, 90))\n declare $Tmp String = $In;\n" + diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index 10cd58bde3..adc6fce5eb 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -215,10 +215,10 @@ LIST IMPACT OF htmlelement; "-- so a stored one round-trips through DESCRIBE (mendixlabs/mxcli#1057). Without\n" + "-- it the widget is written with no image and mxbuild reports CE0436:\n" + "STATICIMAGE imgLogo (Image: 'MyModule.Images.logo', Width: 64, Height: 64)\n\n" + - "-- Not supported by the React client — added in Mendix 10.7, and the only\n" + - "-- client on 11, so this is not a Mendix 11 rule. These are written correctly,\n" + - "-- but mxbuild reports CE0582 (\"not supported in React client\") on each\n" + - "-- wherever that client is enabled, and `mxcli lint` reports them as MPR012.\n" + + "-- Not supported by the React client (added in Mendix 10.7). These are written\n" + + "-- correctly, but mxbuild reports CE0582 (\"not supported in React client\") on\n" + + "-- each wherever that client is enabled, and `mxcli lint` reports them as MPR012\n" + + "-- (an info note while the app still runs the classic client).\n" + "-- Prefer the alternative:\n" + "-- STATICIMAGE -> IMAGE\n" + "-- DYNAMICIMAGE -> IMAGE\n" + diff --git a/docs-site/src/language/microflow-structure.md b/docs-site/src/language/microflow-structure.md index 1eca953c28..911079a64a 100644 --- a/docs-site/src/language/microflow-structure.md +++ b/docs-site/src/language/microflow-structure.md @@ -164,6 +164,11 @@ statements before it keep the places the builder derives for them, and only a or nothing: a few hand-placed statements are not measured against what is laid out around them. +That is the default style. A project can choose another — the ELSE branch as a +column under its decision, a guard's early return kept on the line, no wrapping — +in a committed `layout-config.yaml`; flows written without `@position` then follow +it. See [Choosing a layout style](../tools/flow-layout.md#choosing-a-layout-style). + To re-arrange a flow that already exists — one drawn in Studio Pro, or one whose positions no longer fit after edits — run `mxcli layout flows`. It applies this same layout to the stored flow and changes nothing but coordinates; see diff --git a/docs-site/src/tools/flow-layout.md b/docs-site/src/tools/flow-layout.md index 3c9a15f31f..16cd48756a 100644 --- a/docs-site/src/tools/flow-layout.md +++ b/docs-site/src/tools/flow-layout.md @@ -24,7 +24,64 @@ The flow is described to MDL, every layout annotation (`@position`, `@anchor`, dropped, and it is built again as `CREATE` would build it. Only the geometry of that build is kept. -## Only positions change +## Choosing a layout style + +Teams draw flows differently, so the style is a project setting. Put a +`layout-config.yaml` in the project's `.claude/` directory or next to the +`.mpr` and commit it — everyone working on the project then lays flows out the +same way: + +```yaml +flows: + preset: true-right-false-down +``` + +| Preset | What it draws | +|---|---| +| `happy-line` (default) | The TRUE path on one horizontal line, the ELSE branch in a lane under it, left to right. | +| `true-right-false-down` | The TRUE path on the line; the ELSE branch stacked top to bottom as a column under its decision, rejoining the merge from below. A CASE or split by type the same way: its first outcome on the line, every other outcome a column under it. | + +A preset is a set of options, and an option overrides its preset: + +| Option | Values | +|---|---| +| `if-else` | `lanes` (default), `true-right-else-column` | +| `case-split` | `lanes` (default): a CASE's or type split's outcomes are stacked as lanes, each left to right. `first-right-rest-columns`: the first outcome, as written, stays on the line; every other one is a column under the decision, side by side in the order written. Put the happy case first. | +| `wrap` | `rows` (default): past the row width the main line continues on a new row, back at the left. `off`: the main line never wraps. | +| `max-row-width` | Where a row wraps, in px, from 640 to 20000. Default 2880 (two screens of Studio Pro canvas). | +| `guard` | `return-below` (default): a guard's early return goes in the lane below and the happy path stays on the line. `true-right`: the early return carries straight on to the right, and the happy path drops from the decision's bottom onto a new line below. | + +A *guard* is an `if … then … return; end if` with no `else`. It has its own +option because teams that want TRUE on the right for IF/ELSE mostly still want +their guards to keep the happy path straight. + +An unknown key or value is an error, not a silent fallback to the default. +`mxcli layout config -p app.mpr` shows the style in force and the file it came +from. `CREATE MICROFLOW` / `CREATE NANOFLOW` without `@position` use it too. + +To try a style without committing it, pass `--style`: + +```bash +mxcli layout flows -p app.mpr --module Sales --style true-right-false-down --dry-run +``` + +### The style a flow was drawn in travels with it + +A flow laid out in a style other than the default gets one line at the end of +its documentation, which Studio Pro shows in the documentation pane: + +``` +@mxcli-layout: if-else=true-right-else-column +``` + +`DESCRIBE` and `CREATE OR MODIFY` use that line for that flow, whatever the +project file says now. So a flow drawn in one style describes without `@position` +noise after the project switches to another, and its description rebuilds the +same geometry. Laying a flow out in the default style removes the line. If the +line is deleted or unreadable, nothing breaks: `DESCRIBE` then prints the +positions it can no longer derive, and a warning says why. + +## What changes The result is patched onto the stored flow rather than written in its place, so nothing but layout changes: @@ -33,6 +90,9 @@ nothing but layout changes: - positions of parameters and notes (a note keeps its size); - the connection sides and curves of sequence flows. +- the `@mxcli-layout:` line in the flow's documentation (above) — the rest of + the documentation is left byte for byte. + Element IDs, captions, expressions and every property MDL cannot express are left exactly as stored. That is also why this works on flows drawn in Studio Pro. @@ -69,6 +129,7 @@ writes nothing. | `Module.Flow ...` | Lay out these microflows or nanoflows. | | `--module ` | Lay out every microflow and nanoflow in this module (repeatable). | | `--dry-run` | Report what would move, write nothing. | +| `--style ` | Lay out in this preset instead of the project's `layout-config.yaml`. | | `--include-marketplace` | Also lay out flows in Marketplace modules. A module update replaces them, so this is normally pointless. | Either name flows or pass `--module`; laying out every flow in a project is too diff --git a/docs-wiki/bug-patterns/rewrite-drops-unauthored-state.md b/docs-wiki/bug-patterns/rewrite-drops-unauthored-state.md index 4d21419c07..95192a6783 100644 --- a/docs-wiki/bug-patterns/rewrite-drops-unauthored-state.md +++ b/docs-wiki/bug-patterns/rewrite-drops-unauthored-state.md @@ -330,6 +330,15 @@ re-markers it. It hides until that first rewrite, which makes a census the right instrument: the stored marker per (owner `$Type`, key) across the Studio Pro-authored fixtures, registered rather than inferred. +**A registration reaches only the lists the encoder builds.** A rewrite passes +plenty of a stored document through as bytes — an unchanged pluggable widget's +Type and Object, an existing element's clean properties — and a marker an older +build wrote there outlived the registration that fixed it, on every later +rewrite (#1640: `drop page` + the same MDL gave 2, the rewrite kept 3). The +writer now applies each owner/key registration to the whole unit on every write; +that makes a wrong registration rewrite Studio Pro's bytes, so the census is a +test, not a one-off. + **A refusal keyed on "the stored document lacks the key" conflates two different facts.** "This project's version has no such property" and "the writer that created this document left it out" need opposite treatment, and only the diff --git a/docs/03-development/MODELSDK_ENGINE_ARCHITECTURE.md b/docs/03-development/MODELSDK_ENGINE_ARCHITECTURE.md index 679b8fbe37..81556a1e05 100644 --- a/docs/03-development/MODELSDK_ENGINE_ARCHITECTURE.md +++ b/docs/03-development/MODELSDK_ENGINE_ARCHITECTURE.md @@ -85,6 +85,9 @@ and break the MCP backend + the future format (ADR-0005). See `docs/11-proposals 2. **Read its BSON**: `mxcli bson dump -p app.mpr --type --object "Mod.Name"`, or the MCP/PED probe (`cmd/mcpprobe`) against a live Studio Pro. 3. **Write `xToGen`** (+ `xFromGen` if reads/ALTER need it), registering any TypeDefaults / list markers. + An owner/key marker (`codec.RegisterPropertyListMarker`) is applied by every write to the stored lists + it does not rebuild as well (`canon.NormalizeListMarkers`), so register only a measured one: + `TestIssue1640_RegisteredListMarkersAreWhatStudioProStores` holds each against the fixtures. 4. **`assignXIDs`** walks new sub-elements. 5. **Pin the document against the reference** — re-serialize the reference element by element and assert the keys, markers and value types match. `mdl/scheduledevents` and `mdl/regularexpressions` are the diff --git a/docs/11-proposals/PROPOSAL_configurable_flow_layout.md b/docs/11-proposals/PROPOSAL_configurable_flow_layout.md new file mode 100644 index 0000000000..21fcc7bd59 --- /dev/null +++ b/docs/11-proposals/PROPOSAL_configurable_flow_layout.md @@ -0,0 +1,400 @@ +--- +title: Configurable Microflow and Nanoflow Layout +status: draft +--- + +# Proposal: Configurable Microflow and Nanoflow Layout + +**Status:** Draft +**Date:** 2026-10-31 + +## Problem Statement + +mxcli lays out every flow it creates (`create microflow` / `create nanoflow` without +`@position`) and every flow it re-arranges (`mxcli layout flows`) with one fixed +style: the TRUE path of each decision runs straight left to right, ELSE branches +drop one lane and run left to right under it, guards put their early return in the +lane below, and a flow wider than `MaxRowWidth` (2880px) wraps back to the left +edge (`mdl/executor/layout.go`, `layout_lanes.go`, `layout_rows.go`). + +Teams do not agree on how a flow should look. Conventions seen in practice: + +- **"Happy path on one line"** — everything that is not the happy path hangs off it + (today's style). +- **"TRUE goes right, FALSE goes down"** — the exit side of a decision always says + which outcome it is, and an ELSE body is a *column* under its decision, so the eye + reads alternatives top to bottom instead of as a second horizontal line. +- **Wrapping style** — some want long flows on as few rows as possible, some want a + snake (alternate rows reversed) so no connector runs back across the diagram, some + never want wrapping. + +A Studio Pro user re-arranges by hand to match the house style, and every +`mxcli layout flows` run undoes it. The layout should be a choice the project makes +once. + +### Who benefits + +- Teams with a modelling style guide, who today hand-fix every generated flow. +- Reviewers: a consistent layout across a module makes flows comparable at a glance. +- Agents generating MDL, which never write `@position` and so always get the default. + +## Not an MDL feature, and not a BSON feature + +**No new MDL syntax.** The style is a project setting; MDL already has `@position`, +`@anchor`, `@merge` and `@curve` for a flow that deviates from whatever style is in +force. + +**No new BSON.** The layout only produces values mxcli already writes: + +| Property | On | Meaning | +|---|---|---| +| `RelativeMiddlePoint` | every flow object | centre of the object | +| `Size` | every flow object | width/height (loops are sized to their body) | +| `OriginConnectionIndex` / `DestinationConnectionIndex` | `SequenceFlow` | anchor side: 0 top, 1 right, 2 bottom, 3 left (`AnchorTop`… in `layout.go`) | +| `OriginBezierVector` / `DestinationBezierVector` | `SequenceFlow` | curve control points | + +`mxcli layout flows` already patches exactly these into stored BSON and leaves +`$ID`, `GUID` and every other property alone (`cmd_microflows_layout.go`), so the +identity rules in CLAUDE.md are not touched by this proposal. + +The one addition is a single marker line in the flow's `Documentation` string +property (see [Recording the style on the flow](#recording-the-style-on-the-flow)). +That is a plain string mxcli already reads and writes (`/** … */` in MDL); no new +field. + +## Why the setting must be committed project state + +Canonical DESCRIBE (ADR-0010, R12; `cmd_microflows_derived_layout.go`) omits every +`@position`/`@anchor`/`@curve` the layout engine *would produce on its own*, by +rebuilding the flow with the engine and keeping only annotations whose node moves. +So "derived" is relative to a style: + +- If two developers use different styles, `describe` of the *same* flow prints + different MDL for each of them — noisy diffs, and `create or modify` from one + developer's script re-lays out the flow for the other. +- If the style differs from the one a flow was drawn with, DESCRIBE stays **correct** + by construction: nodes land elsewhere, so their annotations are kept and + re-executing the description reproduces every stored position. The only cost is + verbosity. + +Two mechanisms keep this stable. The project style is a committed file (below), +so everyone lays out new flows the same way. And each flow records the style it +was laid out with ([below](#recording-the-style-on-the-flow)), so DESCRIBE checks +"derived" against the style that actually drew the flow — not against whatever the +project file says today. + +Therefore the project style is read from a **committed** file next to the project, like +`lint-config.yaml` (which `mxcli init` writes to `.claude/`) — not from `~/.mxcli/` and not from `/.mxcli/`, which +`mxcli init` git-ignores because it holds secrets and handshakes +(`cmd/mxcli/constant_gitignore.go`). A per-invocation flag exists only on +`mxcli layout flows`, for trying a style out. + +## Proposed configuration + +`layout-config.yaml`, looked up the same way `lint-config.yaml` is +(`linter.FindConfigFile`: `/.claude/`, then the project directory) — both +committed locations: + +```yaml +# layout-config.yaml — how mxcli lays out microflows and nanoflows +flows: + preset: happy-line # happy-line (default) | true-right-false-down | snake + # Any key below overrides the preset. + if-else: lanes # lanes | true-right-else-column + guard: return-below # return-below | true-right + case-split: lanes # lanes | first-right-rest-columns + wrap: rows # off | rows | snake + max-row-width: 2880 + error-handler: below # below | row-end + spacing: + horizontal: 160 + vertical: 90 + row-gap: 90 +``` + +A missing file, or a missing key, means today's value. Unknown keys and values are +an error (a typo silently falling back to the default is the failure this file +exists to prevent). + +### Presets + +| Preset | `if-else` | `guard` | `case-split` | `wrap` | Intent | +|---|---|---|---|---|---| +| `happy-line` (default) | `lanes` | `return-below` | `lanes` | `rows` | Today's layout, byte for byte | +| `true-right-false-down` | `true-right-else-column` | `return-below` | `first-right-rest-columns` | `rows` | TRUE / first case continues the line, alternatives stack below | +| `snake` | `lanes` | `return-below` | `lanes` | `snake` | Long flows without connectors back across the canvas | + +`error-handler` is `below` in every preset. + +### `if-else: true-right-else-column` + +For an IF with an ELSE (or an explicit empty ELSE): + +``` + [act] ──▶ ◇ cond ──true──▶ [A] ──▶ [B] ──▶ ◆ merge ──▶ [next] + │ ▲ + false │ + ▼ │ + [C] │ + │ │ + ▼ │ + [D] ────────────────────────────┘ +``` + +- TRUE leaves the split on the right (`OriginConnectionIndex = 1`); the TRUE body is + laid out on the centre line exactly as today. +- FALSE leaves the bottom (`OriginConnectionIndex = 2`); the ELSE body is stacked + **top to bottom** in the split's column, one `ActivityHeight + VerticalSpacing` + pitch per statement (`ErrorHandlerPitch` where the previous one has a handler + under it, so its output-variable text is not overlapped — #1598). +- The ELSE's last element connects to the merge from its right side into the + merge's bottom (`1 → 2`). The merge sits after the wider of the TRUE body and the + ELSE column. +- An ELSE that ends in RETURN ends in its own end event at the bottom of the column, + and there is no merge input from it — the same rule as today. +- **Nesting:** a nested IF inside the ELSE column applies the same rule from where it + stands: its TRUE body runs to the right of the column, its own ELSE stacks below. + A loop inside the column is placed as a box in the column, sized to its body as + today. The column's width is the widest of these. +- **Empty THEN, non-empty ELSE:** the TRUE flow is still horizontal — it runs + straight from the split's right side to the merge, which is placed one ordinary + gap to the right of the split (or right of the ELSE column, if that is wider), + on the centre line. The ELSE column hangs under the split as usual. +- **Enumeration and inheritance splits** are governed by `case-split` (below), so + `true-right-false-down` treats every decision the same way. + +### `case-split` + +Enumeration splits (`case … when …`) and inheritance splits have no TRUE/FALSE, so +the equivalent rule is by case order: + +- `lanes` (default) — today: the cases fan out in lanes under the split. +- `first-right-rest-columns` — the **first case in declaration order** leaves the + split on the right and continues the centre line. Every other case leaves the + split's bottom and is laid out as its own vertical column, exactly like an ELSE + column; the columns stand side by side, the second case under the split and each + further one to the right of the previous, separated by `BranchGap`. Each column + rejoins the merge from below. The `else`/`empty` case, when present, is the last + column. + +Since the first case is the one on the line, the author picks the happy case by +putting it first — no extra syntax. A multiple-flows-per-side split is valid in +Studio Pro (several flows may share a connection index); still to be confirmed by +opening one, see the test plan. + +### `guard` + +A guard is `if X then …; return; end if` with no ELSE (`isGuard`, `layout_lanes.go`). + +- `return-below` (default) — today: the TRUE branch with its early return goes in the + lane under the split; the happy path continues on the FALSE exit, horizontally. +- `true-right` — the TRUE branch (early return) continues on the line to the right + and the happy path leaves the bottom of the split. Consistent with + `true-right-else-column`, but the happy path turns at every guard. It is a separate + key on purpose: teams that want TRUE-right for IF/ELSE mostly do not want it for + guards. + +### `wrap` + +- `rows` — today (`layout_rows.go`): past `max-row-width` the next row starts at the + left edge, under everything the current row occupies. +- `snake` — alternate rows run right to left, so the row-to-row connector is a short + vertical drop at the end of the row instead of a line back across the diagram. + Branch lanes, guard lanes and error-handler rows are mirrored with their row. +- `off` — never wrap. + +### `error-handler` + +- `below` — today: a custom error handler's body hangs `ErrorHandlerPitch` under its + activity, in the lane under the main line. +- `row-end` — the handler's body goes at the end of the current row's lane, reached + by a connector along the lane; it keeps the area straight under the main line free. + In scope; scheduled after the other options. + +## Recording the style on the flow + +Every flow mxcli lays out under a style other than `happy-line` gets one marker +line at the end of its `Documentation`: + +``` +@mxcli-layout: if-else=true-right-else-column +@mxcli-layout: if-else=true-right-else-column; wrap=snake +``` + +The value is every option that differs from the default, `key=value` in key +order, so the same settings always produce the same line and a re-layout is +idempotent (ADR-0008). It records options, not the preset name: a preset gains +options as they are implemented, and a marker naming a preset would silently +change meaning for flows already laid out with it. + +Rules: + +1. **Written by** `create`/`create or modify` (for a flow built without a marker) + and by `mxcli layout flows`. Any existing marker line is replaced, never + duplicated; the rest of the documentation is left byte for byte. +2. **No marker means `happy-line`.** Laying a flow out under `happy-line` removes the + marker. So every flow that exists today, and every flow laid out with the + default, is unchanged — the Phase 0 control depends on this. +3. **DESCRIBE** prints the documentation as today, marker included, and runs its + derived-layout check (`cmd_microflows_derived_layout.go`) under the flow's + **recorded** style. A flow drawn in `true-right-false-down` therefore describes + with no `@position` noise, whatever the project file now says. +4. **exec** of a `create`/`create or modify` whose `/** … */` carries a marker builds + the flow under that style; without one, under the project style. That is what + makes rule 3 round-trip: the described MDL rebuilds the same geometry anywhere. +5. **A missing, edited or unknown marker is never an error.** A Studio Pro user who + deletes the line, or a marker naming a preset this mxcli does not know, falls + back to rule 2 with a warning. DESCRIBE then keeps the positions it cannot + derive, which is still correct by construction — only more verbose. +6. Studio Pro shows the line in the flow's documentation pane. That visibility is + intended: it tells a Studio Pro user that the layout was generated, and with which + style. + +This changes one statement in the `mxcli layout flows` help text, which today says +only positions change: it also maintains the marker line. + +## CLI + +```bash +# Uses layout-config.yaml (or the default) +mxcli layout flows -p app.mpr --module Orders + +# Try a style without committing it (dry run shows what would move) +mxcli layout flows -p app.mpr --module Orders --style true-right-false-down --dry-run + +# Show the effective configuration and where it came from +mxcli layout config -p app.mpr +``` + +`--style` takes a preset name; options are only set in the file. A flow laid out +with `--style` records that style in its marker, so DESCRIBE stays minimal; the +command warns that the flow now differs from the project style. + +`create`/`create or modify`/`exec` use the flow's marker if its documentation has +one, otherwise the project file; there is no other per-statement style. + +## Implementation Plan + +### Phase 0 — separate *placing* from *building* (refactor, no behaviour change) + +Today placement is interleaved with construction: `flowBuilder` holds `posX`/`posY` +and each `add*Statement` assigns positions while creating objects +(`cmd_microflows_builder*.go`), with measurement in `layoutMeasurer` (`layout.go`). +A second style written into the same code doubles every branch. + +Introduce a `flowLayout` value carried by the builder that owns the decisions a +style changes — where a branch starts, which side a flow leaves and enters, how a +body advances (horizontally or as a column), when to wrap — and route the existing +code through it. The default implementation reproduces today's numbers. This phase +ships alone: **every existing layout test must pass unchanged**, and the +derived-layout DESCRIBE tests are the control that nothing moved. + +This is a refactor touching many files, so per CLAUDE.md it is its own PR. + +### Phase 1 — configuration plumbing + +Load `layout-config.yaml`, validate it, hand the resolved `flowLayout` to the +executor; `mxcli layout config`; `--style` on `layout flows`; the `@mxcli-layout` +marker (write, replace, remove, parse with fallback). The derived-layout check +(`cmd_microflows_derived_layout.go`) must rebuild with the flow's **recorded** +layout, or DESCRIBE silently prints every position. + +### Phase 2 — options, one PR each + +1. `if-else: true-right-else-column` (the request that started this) +2. `guard: true-right` +3. `wrap: snake` / `off` +4. `case-split: first-right-rest-columns` +5. `error-handler: row-end` + +### Files to modify/create + +| File | Change | +|---|---| +| `mdl/executor/layout.go` | Constants become the default `flowLayout`'s fields; measurer asks the layout how a body advances | +| `mdl/executor/layout_lanes.go` | Guard placement behind `guard` option | +| `mdl/executor/layout_rows.go` | `wrap: snake`/`off` | +| `mdl/executor/cmd_microflows_builder.go` | `flowBuilder` carries the `flowLayout` | +| `mdl/executor/cmd_microflows_builder_control.go` | `addIfStatement`: TRUE/FALSE sides, ELSE column, empty-THEN merge | +| `mdl/executor/cmd_microflows_builder_rule_case.go` and the enum/inheritance split builders | `case-split` columns | +| `mdl/executor/cmd_microflows_derived_layout*.go` | Rebuild with the flow's recorded layout | +| `mdl/executor/flowlayout_marker.go` (new) | Read/write/strip the `@mxcli-layout:` documentation line | +| `mdl/executor/cmd_microflows_layout.go` | Accept a layout override for `--style` | +| `mdl/executor/flowlayout_config.go` (new) | Parse/validate `layout-config.yaml`, presets | +| `cmd/mxcli/cmd_layout_flows.go` | `--style` flag; `layout config` subcommand; help text mentions the marker | +| `docs-site/src/` | User manual page: choosing a flow layout | +| `.claude/skills/mendix/` (flow skill) | One line: layout is project config, not `@position` | + +## Version Compatibility + +None needed. Coordinates, sizes and connection indexes are stored identically in +every Mendix version mxcli supports; nothing here is gated. + +## Test Plan + +- **Phase 0 control:** the full existing suite (`layout_*_test.go`, + `cmd_microflows_builder_layout_test.go`, `cmd_microflows_derived_layout_test.go`, + `cmd_microflows_layout_test.go`) passes with no expectation edited. A diff in any + expected coordinate fails the refactor. +- **Per option, unit tests on the built graph** (executor, `MockBackend`): positions + and connection indexes for TRUE/FALSE exits, ELSE column pitch, nested IF in a + column, loop in a column, ELSE ending in RETURN, empty THEN (merge on the centre + line right of the split), guard both ways, case-split columns (first case on the + line, `else` case last), snake row direction. Each with a control: the same flow under `happy-line` keeps today's + coordinates. +- **No overlaps:** run `MPR008` (overlapping activities, `mdl/linter/rules/ + mpr008_overlapping_activities.go`) over every flow in `mdl-examples/` laid out + under each preset; zero findings. +- **DESCRIBE round trip under each preset:** create without `@position` → describe → + marker printed, no layout annotations → re-exec → identical geometry. +- **Marker independence from the project file:** lay out under preset A, change the + project file to B, describe → still no layout annotations (checked under A, the + recorded style) → re-exec under project style B reproduces A's geometry exactly. + Control: the same with the marker line deleted → annotations kept, geometry still + reproduced. +- **Marker hygiene:** existing documentation preserved byte for byte around the + marker; re-layout replaces rather than appends; `happy-line` removes it; an + unknown preset in the marker warns and falls back. A flow with no marker and the + default style is written byte-identical to today (control for Phase 0). +- **Idempotence:** `layout flows` twice under any preset; the second run writes + nothing (ADR-0008), with a control that the first run did write. +- **Studio Pro:** open a module laid out under each preset; flows display as laid + out, `mx check` clean (layout writes no semantic change, so this is a smoke test). + Specifically confirm a `case-split` with several flows leaving the split's bottom + displays and edits normally. + +## Alternatives considered + +- **A generic graph layout (Sugiyama/layered — Graphviz `dot`, dagre, ELK).** These + see a plain directed graph and must guess which path is the happy one; the MDL + syntax tree already knows branches, merges, loops and handlers, which is why the + structure-driven engine produces flows that look hand-drawn. No pure-Go + implementation exists (ELK is Java/JS, dagre JS, Graphviz needs CGO or WASM, and + mxcli is CGO-free). Worth revisiting only as a fallback for flows MDL cannot + express, which `layout flows` currently skips. +- **Per-user setting.** Rejected: canonical DESCRIBE would differ per developer. +- **A per-flow style annotation in MDL syntax** (e.g. `@layout(...)`). The + documentation marker gives the same per-flow recording without new grammar, and + it is visible in Studio Pro, where an MDL annotation would not be. +- **Recording the style in the catalog.** The catalog is a rebuildable local index, + not committed state, so it would be lost on a fresh clone; the documentation + travels with the flow. + +## Resolved Questions + +1. **Config file:** a separate `layout-config.yaml`. +2. **Enumeration/inheritance splits:** yes — `case-split: first-right-rest-columns`, + part of the `true-right-false-down` preset. +3. **Empty THEN:** the TRUE flow stays horizontal, straight to a merge on the centre + line. +4. **Recording the style:** yes, as an `@mxcli-layout:` line in the flow's + documentation; DESCRIBE and exec use it. +5. **`error-handler: row-end`:** kept in scope. + +## Open Questions + +None at the moment. Checked while writing: the visitor keeps an +`@mxcli-layout:` line inside `/** … */` as plain documentation text — a microflow +documented `Submits.` plus the marker parses to +`Documentation = "Submits.\n@mxcli-layout: true-right-false-down"` — so the +marker is not mistaken for an MDL annotation. diff --git a/mdl-examples/bug-tests/1640-stale-list-marker.mdl b/mdl-examples/bug-tests/1640-stale-list-marker.mdl new file mode 100644 index 0000000000..50e7ab947c --- /dev/null +++ b/mdl-examples/bug-tests/1640-stale-list-marker.mdl @@ -0,0 +1,67 @@ +-- ako/mxcli#1640: a list marker an older mxcli stored survived every rewrite. +-- +-- Symptom: a Data grid 2 written before SortBar.SortItems was measured kept +-- marker 3 through every `create or replace page`, also one that changed the +-- page for real, where Studio Pro and a page created from nothing store 2: +-- Home_Web mxcli=3 SP=2 <-- the old page +-- ZZ_A mxcli=2 SP=2 +-- mx check, mx diff, lint and `fix property-sets` were all clean, so only a +-- value-level diff against `mx convert -p` showed it. +-- +-- Cause: an unchanged pluggable widget passes its stored Type and Object +-- through byte for byte, and the stored list with them; the registered marker +-- only reached the lists the encoder built. Every write now gives a list with a +-- registered owner/key marker that marker (canon.NormalizeListMarkers). +-- +-- Also: a populated nanoflow parameter list (the button's call below and the +-- data view's nanoflow source) was written with marker 3, where Studio Pro +-- writes 2 — 78 of 78 call nanoflow actions and 21 of 21 nanoflow sources in +-- ako/TestApp and PedApp. +-- +-- To verify the stale marker: it takes a page stored by an older build, so it +-- is exercised in mdl/executor/issue1640_stale_list_marker_test.go, which +-- plants SortItems 3 and re-runs this page with a widget added. To verify the +-- nanoflow lists: run this script and dump Shop.Thing_View; both +-- ParameterMappings lists start with 2. + +mdl 1; + +create module Shop; + +create persistent entity Shop.Thing ( + Name: string(100) +); + +create nanoflow Shop.NF_Do ($Thing: Shop.Thing) +begin +end; + +create nanoflow Shop.NF_Get ($Thing: Shop.Thing) returns Shop.Thing +begin + return $Thing; +end; + +create or modify page Shop.Thing_Overview ( + Title: 'Things', + Layout: Atlas_Core.Atlas_Default +) { + dynamictext txtNew (Content: 'new') + container ctMain { + datagrid dgThings (DataSource: database from Shop.Thing sort by Name asc) { + column (Attribute: Name, Caption: 'Name') + } + } +}; + +create or modify page Shop.Thing_View ( + Title: 'Thing', + Layout: Atlas_Core.Atlas_Default, + Params: ($Thing: Shop.Thing) +) { + dataview dvThing (DataSource: $Thing) { + actionbutton btnDo (Caption: 'Do', Action: call nanoflow Shop.NF_Do(Thing = $currentObject)) + } + dataview dvGot (DataSource: nanoflow Shop.NF_Get(Thing = $Thing)) { + textbox tbName (Attribute: Name) + } +}; diff --git a/mdl/backend/modelsdk/issue1640_list_marker_census_test.go b/mdl/backend/modelsdk/issue1640_list_marker_census_test.go new file mode 100644 index 0000000000..5b7958e46b --- /dev/null +++ b/mdl/backend/modelsdk/issue1640_list_marker_census_test.go @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/modelsdk/canon" +) + +// ako/mxcli#1640: every write gives a list with a registered owner/key marker +// that marker, the stored lists a rewrite passes through included +// (canon.NormalizeListMarkers). That is only safe if each registration is what +// Studio Pro writes, so this census holds every one of them against the +// Studio Pro-authored fixtures: a document Studio Pro wrote must be a fixed +// point of the normalization. + +// studioProUnits lists the .mxunit files of the Studio Pro-authored fixtures. +func studioProUnits(t *testing.T) []string { + t.Helper() + var out []string + for _, root := range []string{ + "../../../testdata/testapp/TestApp/mprcontents", + "../../../testdata/pedapp/mprcontents", + } { + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && strings.HasSuffix(p, ".mxunit") { + out = append(out, p) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + } + if len(out) < 1000 { + t.Fatalf("found only %d fixture units", len(out)) + } + return out +} + +// markerCensus counts, per "Owner.Key", the markers stored for every list. +func markerCensus(t *testing.T) map[string]map[int32]int { + t.Helper() + census := map[string]map[int32]int{} + var walk func(any) + walk = func(v any) { + switch x := v.(type) { + case bson.D: + owner := "" + for _, e := range x { + if e.Key == "$Type" { + owner, _ = e.Value.(string) + } + } + for _, e := range x { + if a, ok := e.Value.(bson.A); ok && len(a) > 0 { + if m, ok := a[0].(int32); ok && owner != "" { + k := owner + "." + e.Key + if census[k] == nil { + census[k] = map[int32]int{} + } + census[k][m]++ + } + } + walk(e.Value) + } + case bson.A: + for _, e := range x { + walk(e) + } + } + } + for _, p := range studioProUnits(t) { + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + var doc bson.D + if err := bson.Unmarshal(raw, &doc); err != nil { + t.Fatalf("%s: %v", p, err) + } + walk(doc) + } + return census +} + +func TestIssue1640_RegisteredListMarkersAreWhatStudioProStores(t *testing.T) { + census := markerCensus(t) + var wrong, measured []string + for key, byMarker := range census { + owner, prop, _ := strings.Cut(key, ".") + want, ok := canon.PropertyListMarker(owner, prop) + if !ok { + continue + } + total := 0 + for _, n := range byMarker { + total += n + } + if byMarker[want] != total { + wrong = append(wrong, key) + t.Errorf("%s is registered as %d, but Studio Pro stores %v", key, want, byMarker) + continue + } + measured = append(measured, key) + } + sort.Strings(measured) + t.Logf("%d registered owner/key markers agree with every stored list: %s", len(measured), strings.Join(measured, ", ")) + + // And a Studio Pro document passes through the normalization unchanged. + changed := 0 + for _, p := range studioProUnits(t) { + raw, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if out := canon.NormalizeListMarkers(raw); string(out) != string(raw) { + changed++ + if changed <= 5 { + t.Errorf("normalizing changes the Studio Pro unit %s", p) + } + } + } + if changed > 0 { + t.Errorf("%d Studio Pro units change under the normalization", changed) + } +} diff --git a/mdl/backend/modelsdk/widget_write.go b/mdl/backend/modelsdk/widget_write.go index 20f9c1d832..3444312501 100644 --- a/mdl/backend/modelsdk/widget_write.go +++ b/mdl/backend/modelsdk/widget_write.go @@ -237,6 +237,11 @@ func init() { MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, NullFields: []string{"ProgressMessage", "ConfirmationInfo"}, }) + // Populated too: 78 of 78 Forms$CallNanoflowClientAction.ParameterMappings + // in ako/TestApp and PedApp are marker 2, six of them holding mappings. The + // child type (Forms$NanoflowParameterMapping) has no marker of its own, so + // a populated list took the default, 3 (ako/mxcli#1640). + codec.RegisterPropertyListMarker("Forms$CallNanoflowClientAction", "ParameterMappings", 2) // The workflow events' unset slots (ako/mxcli#832): a workflow call with // no confirmation, and an admin-page action with no default page. codec.RegisterTypeDefaults("Forms$CallWorkflowClientAction", codec.TypeDefaults{ @@ -258,6 +263,9 @@ func init() { codec.RegisterTypeDefaults("Forms$NanoflowSource", codec.TypeDefaults{ MandatoryListMarkers: map[string]int32{"ParameterMappings": 2}, }) + // And populated: 21 of 21 in ako/TestApp and PedApp, eight holding mappings + // (ako/mxcli#1640). + codec.RegisterPropertyListMarker("Forms$NanoflowSource", "ParameterMappings", 2) // TextBox: many null slots when unbound (attribute ref, screen-reader label, // source variable, label template, visibility/editability/native settings). codec.RegisterTypeDefaults("Forms$TextBox", codec.TypeDefaults{ diff --git a/mdl/executor/cmd_microflows_build.go b/mdl/executor/cmd_microflows_build.go index f37093ccad..964bd1e2a8 100644 --- a/mdl/executor/cmd_microflows_build.go +++ b/mdl/executor/cmd_microflows_build.go @@ -37,6 +37,10 @@ type buildFlowOpts struct { AllowCreate bool ResetLayout bool Quiet bool + // IgnoreRecordedLayout lays the flow out in the project's style even when its + // documentation records another (flowlayout_config.go). `mxcli layout flows` + // sets it: re-laying a flow out in the current style is what it is for. + IgnoreRecordedLayout bool } // builtFlow is a Microflow assembled from a statement, plus what the write @@ -279,6 +283,11 @@ func buildMicroflowFromStmt(ctx *ExecContext, s *ast.CreateMicroflowStmt, opts b if preserveDocumentation { mf.Documentation = carriedDocumentation(s.DocumentationSet, s.Documentation, existingDocumentation) } + layout, doc, err := resolveFlowLayout(ctx, mf.Documentation, !opts.IgnoreRecordedLayout) + if err != nil { + return nil, err + } + mf.Documentation = doc // A new flow starts with no allowed roles: access is the author's grant // (mendixlabs/mxcli#1474). if preserveAllowedRoles { @@ -432,7 +441,8 @@ func buildMicroflowFromStmt(ctx *ExecContext, s *ast.CreateMicroflowStmt, opts b spacing: HorizontalSpacing, varTypes: varTypes, declaredVars: declaredVars, - measurer: &layoutMeasurer{varTypes: varTypes}, + measurer: &layoutMeasurer{varTypes: varTypes, layout: layout}, + layout: layout, allowWrap: true, backend: flowBuildBackend(ctx.Backend), quiet: opts.Quiet, @@ -588,6 +598,11 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui if preserveDocumentation { nf.Documentation = carriedDocumentation(s.DocumentationSet, s.Documentation, existingDocumentation) } + layout, doc, err := resolveFlowLayout(ctx, nf.Documentation, !opts.IgnoreRecordedLayout) + if err != nil { + return nil, err + } + nf.Documentation = doc // Authored only; absent leaves "" and the backend carries the stored level // (mendixlabs/mxcli#1498). if s.ExportLevel != nil { @@ -734,7 +749,8 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui spacing: HorizontalSpacing, varTypes: varTypes, declaredVars: declaredVars, - measurer: &layoutMeasurer{varTypes: varTypes}, + measurer: &layoutMeasurer{varTypes: varTypes, layout: layout}, + layout: layout, allowWrap: true, backend: flowBuildBackend(ctx.Backend), hierarchy: hierarchy, diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 8533b4d698..5aa1fa728e 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -81,11 +81,14 @@ type flowBuilder struct { // textLang is the language a bare message/caption string is stored under // (mendixlabs/mxcli#970). Empty means en_US, which keeps a zero-value // flowBuilder — validateFlowBody builds one — behaving as before. - textLang string - declaredVars map[string]string // Declared primitive variables: name -> type (e.g., "$IsValid" -> "Boolean") - errors []string // Validation errors collected during build - measurer *layoutMeasurer // For measuring statement dimensions - nextConnectionPoint model.ID // For compound statements: the exit point differs from entry point + textLang string + declaredVars map[string]string // Declared primitive variables: name -> type (e.g., "$IsValid" -> "Boolean") + errors []string // Validation errors collected during build + measurer *layoutMeasurer // For measuring statement dimensions + // layout is the style the flow is laid out with (flowlayout.go); nil is the + // default, which is the layout mxcli has always drawn. + layout *flowLayout + nextConnectionPoint model.ID // For compound statements: the exit point differs from entry point // incomingRedirect, when set, instructs the next enclosing flow emission to // terminate at this merge/activity ID instead of the most recently emitted // activity. Used by retry-loop error handlers (where a merge is inserted @@ -227,6 +230,7 @@ func (fb *flowBuilder) nestedBuilder(x, y int) *flowBuilder { declaredVars: fb.declaredVars, // (fixes a nil map panic) textLang: fb.textLang, measurer: fb.measurer, + layout: fb.layout, // a loop body is drawn in the flow's style backend: fb.backend, quiet: fb.quiet, hierarchy: fb.hierarchy, diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index cf49ee0fd1..6b80f9874d 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -492,6 +492,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { SplitCondition: condition, ErrorHandlingType: fb.ehType(nil), } + splitIndex := len(fb.objects) fb.objects = append(fb.objects, split) splitID := split.ID // applyPendingAnnotations, not applyAnnotations: only it records the @@ -517,6 +518,22 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { branchWidth = w } } + // CaseSplitColumns: the first outcome on the line, the others as columns + // under the split (flowlayout.go). + caseCols := fb.layoutStyle().CaseSplit == CaseSplitColumns && len(branches) > 1 + var plan caseColumns + firstShift := 0 + if caseCols { + bodies := make([][]ast.MicroflowStatement, len(branches)) + for i, br := range branches { + bodies[i] = br.body + } + plan = fb.measurer.planCaseColumns(splitX, bodies) + if len(branches[0].values) > 1 { + firstShift = firstCaseGroupShift + } + branchWidth = caseColumnsBranchWidth(fb.measurer.measureBranch(branches[0].body).Width+firstShift, plan, SplitWidth) + } if branchWidth == 0 { branchWidth = HorizontalSpacing / 2 } @@ -563,6 +580,10 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } origins := enumSplitOriginAnchors(branchYs, centerY) + if caseCols { + branchYs[0], origins = centerY, nil + } + columnTop := 0 var caseAnchors []ast.CaseAnchor if s.Annotations != nil { caseAnchors = s.Annotations.CaseAnchors @@ -581,6 +602,40 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { fb.posX = splitX + SplitWidth + HorizontalSpacing/2 fb.posY = branchY fb.endsWithReturn = false + // Where a multi-value outcome's own merge goes, and how this outcome's + // first flows are drawn (sides for a column, the line for the first). + groupX, groupY := splitX+SplitWidth+HorizontalSpacing/4, branchY + var column *elseColumnCursor + if caseCols && i > 0 { + if columnTop == 0 { + // Every column starts under everything the first outcome drew. + _, bottom, _ := objectsExtent(fb.objects[splitIndex:]) + columnTop = max(centerY+branchHeights[0]/2, bottom) + BranchGap + } + top := columnTop + if len(br.values) > 1 { + groupX, groupY = plan.x[i], top+MergeSize/2 + top += MergeSize + BranchGap + } + column = fb.startElseColumn(plan.x[i], top) + } else if caseCols { + fb.posX += firstShift + } + caseFlowSides := func(from int) { + if !caseCols { + return + } + if column != nil { + setColumnFlowSides(fb.flows[from:], slot(i)) + return + } + for _, f := range fb.flows[from:] { + f.OriginConnectionIndex, f.DestinationConnectionIndex = AnchorRight, AnchorLeft + if ec, ok := f.CaseValue.(microflows.EnumerationCase); ok { + slot(i).applyCaseAnchor(f, ec.Value) + } + } + } lastID := model.ID("") pendingCase := "" @@ -589,7 +644,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { outerHandlers := fb.enterBranch() for j, stmt := range br.body { thisAnchor := stmtOwnAnchor(stmt) + column.begin(stmt) actID := fb.addStatement(stmt) + column.end() if fb.pendingJoin != nil { // `join L` as the FIRST statement of a case body means the split // itself goes to the merge, so the case flows are emitted with the @@ -599,7 +656,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { fb.pendingJoin = nil fb.labels().handled++ m := fb.mergeForLabel(label) - fb.addGroupedEnumSplitFlows(splitID, m.ID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) + from := len(fb.flows) + fb.addGroupedEnumSplitFlows(splitID, m.ID, br.values, slot(i), groupX, groupY) + caseFlowSides(from) } else { fb.takePendingJoin(lastID, pendingCase, prevAnchor) } @@ -613,7 +672,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { // statement's @curve for applyFlowCurves (mendixlabs/mxcli#1414). fb.applyPendingAnnotations(actID) if lastID == "" { - fb.addGroupedEnumSplitFlows(splitID, actID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) + from := len(fb.flows) + fb.addGroupedEnumSplitFlows(splitID, actID, br.values, slot(i), groupX, groupY) + caseFlowSides(from) // The first statement in a case can carry @anchor(from:…, // to:…) that should apply to the split→firstActivity flow. // addGroupedEnumSplitFlows appends one flow per case value; @@ -635,6 +696,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } else { flow = newHorizontalFlow(lastID, actID) } + if column != nil { + flow.OriginConnectionIndex, flow.DestinationConnectionIndex = AnchorBottom, AnchorTop + } applyUserAnchors(flow, prevAnchor, thisAnchor) fb.flows = append(fb.flows, flow) fb.settleBranchFlow(lastID, actID, stmt, br.body[j+1:], branchEnds) @@ -661,7 +725,17 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } allBranchesReturn = false if lastID == "" { - fb.addGroupedEnumSplitFlows(splitID, ensureMerge().ID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) + from := len(fb.flows) + fb.addGroupedEnumSplitFlows(splitID, ensureMerge().ID, br.values, slot(i), groupX, groupY) + caseFlowSides(from) + if column != nil { + // An empty column goes straight from the split's bottom to the merge's. + for _, f := range fb.flows[from:] { + if f.DestinationID == ensureMerge().ID { + f.DestinationConnectionIndex = AnchorBottom + } + } + } } else { var tail *microflows.SequenceFlow if pendingCase != "" { @@ -672,6 +746,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { if origins != nil { tail.DestinationConnectionIndex = mergeSideFor(fb.objectPosition(lastID), fb.objectPosition(ensureMerge().ID)) } + if column != nil { + tail.OriginConnectionIndex, tail.DestinationConnectionIndex = AnchorRight, AnchorBottom + } fb.flows = append(fb.flows, tail) fb.settleBranchFlow(lastID, ensureMerge().ID, nil, nil, false) } @@ -726,6 +803,7 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt ErrorHandlingType: microflows.ErrorHandlingTypeRollback, VariableName: s.Variable, } + splitIndex := len(fb.objects) fb.objects = append(fb.objects, split) splitID := split.ID // applyPendingAnnotations, not applyAnnotations: only it records the @@ -746,6 +824,26 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt branchWidth = w } } + // CaseSplitColumns (flowlayout.go): the first case on the line, every other + // case with a body as a column under the split. The (empty) branch always + // exists; without a body it stays a flow straight to the merge. + columnBodies := make([][]ast.MicroflowStatement, 0, len(s.Cases)+1) + for _, c := range s.Cases { + columnBodies = append(columnBodies, c.Body) + } + if len(s.ElseBody) > 0 { + columnBodies = append(columnBodies, s.ElseBody) + } + caseCols := fb.layoutStyle().CaseSplit == CaseSplitColumns && len(columnBodies) > 1 + var plan caseColumns + firstHeight := 0 + if caseCols { + plan = fb.measurer.planCaseColumns(splitX, columnBodies) + first := fb.measurer.measureStatements(columnBodies[0]) + firstHeight = max(first.Height, ActivityHeight) + branchWidth = caseColumnsBranchWidth(first.Width, plan, ActivityWidth) + } + columnTop := 0 if branchWidth == 0 { branchWidth = HorizontalSpacing / 2 } @@ -760,6 +858,9 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt order int anchor *ast.FlowAnchors handlers pendingHandlers + // column says the branch is a CaseSplitColumns column, which rejoins the + // merge from below. + column bool } var branchTails []branchTail @@ -780,6 +881,16 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt fb.posX = branchStartX fb.posY = branchY fb.endsWithReturn = false + var column *elseColumnCursor + if caseCols && branchNumber > 0 && branchNumber < len(plan.x) { + if columnTop == 0 { + _, bottom, _ := objectsExtent(fb.objects[splitIndex:]) + columnTop = max(centerY+firstHeight/2, bottom) + BranchGap + } + column = fb.startElseColumn(plan.x[branchNumber], columnTop) + } else if caseCols { + fb.posY = centerY + } var lastID model.ID var prevAnchor *ast.FlowAnchors @@ -788,7 +899,9 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt outerHandlers := fb.enterBranch() for j, stmt := range body { thisAnchor := stmtOwnAnchor(stmt) + column.begin(stmt) actID := fb.addStatement(stmt) + column.end() if fb.takeBranchJoin(lastID, splitID, caseValue, pendingCase, prevAnchor, nil) { pendingCase = "" continue @@ -809,19 +922,22 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt } else { flow = newDownwardFlowWithInheritanceCase(splitID, actID, caseValue) } + if column != nil { + flow.DestinationConnectionIndex = AnchorTop + } applyUserAnchors(flow, nil, thisAnchor) fb.flows = append(fb.flows, flow) } else { + flow := newHorizontalFlow(lastID, actID) if pendingCase != "" { - flow := newHorizontalFlowWithCase(lastID, actID, pendingCase) - applyUserAnchors(flow, prevAnchor, thisAnchor) - fb.flows = append(fb.flows, flow) + flow = newHorizontalFlowWithCase(lastID, actID, pendingCase) pendingCase = "" - } else { - flow := newHorizontalFlow(lastID, actID) - applyUserAnchors(flow, prevAnchor, thisAnchor) - fb.flows = append(fb.flows, flow) } + if column != nil { + flow.OriginConnectionIndex, flow.DestinationConnectionIndex = AnchorBottom, AnchorTop + } + applyUserAnchors(flow, prevAnchor, thisAnchor) + fb.flows = append(fb.flows, flow) fb.settleBranchFlow(lastID, actID, stmt, body[j+1:], branchEnds) } prevAnchor = thisAnchor @@ -844,7 +960,7 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt // The tail's flow into the merge is drawn once every branch is // built, and the handlers of its last activity are settled on // it then, so they wait with it. - branchTails = append(branchTails, branchTail{id: lastID, caseValue: pendingCase, anchor: prevAnchor, handlers: fb.takePendingErrorHandlers()}) + branchTails = append(branchTails, branchTail{id: lastID, caseValue: pendingCase, anchor: prevAnchor, handlers: fb.takePendingErrorHandlers(), column: column != nil}) } } fb.leaveBranch(outerHandlers) @@ -905,15 +1021,15 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt applyInheritanceSplitCaseOrder(flow, tail.order) fb.flows = append(fb.flows, flow) } else { + flow := newHorizontalFlow(tail.id, merge.ID) if tail.caseValue != "" { - flow := newHorizontalFlowWithCase(tail.id, merge.ID, tail.caseValue) - applyUserAnchors(flow, tail.anchor, nil) - fb.flows = append(fb.flows, flow) - } else { - flow := newHorizontalFlow(tail.id, merge.ID) - applyUserAnchors(flow, tail.anchor, nil) - fb.flows = append(fb.flows, flow) + flow = newHorizontalFlowWithCase(tail.id, merge.ID, tail.caseValue) } + if tail.column { + flow.OriginConnectionIndex, flow.DestinationConnectionIndex = AnchorRight, AnchorBottom + } + applyUserAnchors(flow, tail.anchor, nil) + fb.flows = append(fb.flows, flow) outerHandlers := fb.enterBranch() fb.adoptPendingErrorHandlers(tail.handlers) fb.settleBranchFlow(tail.id, merge.ID, nil, nil, false) diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 73f65951e2..7d2cfc5f07 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -25,6 +25,10 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // Calculate branch width (max of both branches) branchWidth := max(thenBounds.Width, elseBounds.Width) + elseColumn := fb.layoutStyle().IfElse == IfElseColumn && (s.HasElse || len(s.ElseBody) > 0) + if elseColumn { + branchWidth = elseColumnBranchWidth(thenBounds, fb.measurer.measureColumn(s.ElseBody)) + } if branchWidth == 0 { branchWidth = HorizontalSpacing / 2 } @@ -86,6 +90,10 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // Save current center line position splitX := fb.posX centerY := fb.posY // This is the center line for the happy path + // A TRUE-right guard in a branch moves that branch's happy path — and baseY + // with it — to a new line. That line ends with the branch: whatever follows + // this IF is back on the IF's own line. + savedBaseY := fb.baseY // Decide whether the IF condition is a rule call or a plain expression. // A rule-based split must be serialized as Microflows$RuleSplitCondition; @@ -104,8 +112,12 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { SplitCondition: splitCondition, ErrorHandlingType: fb.ehType(nil), } + splitIndex := len(fb.objects) fb.objects = append(fb.objects, split) splitID := split.ID + // A guard drawn TRUE-right keeps its branch on the centre line and sends the + // happy path down to a new line (GuardTrueRight). + guardRight := isGuard(s) && fb.layoutStyle().Guard == GuardTrueRight // Apply this IF's own annotations (e.g. @caption, @annotation) to the split // BEFORE recursing into branch bodies. Otherwise a nested statement's annotations @@ -290,6 +302,10 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { elseCenterY := centerY + thenH/2 + BranchGap + elseH/2 fb.posX = thenStartX fb.posY = elseCenterY + var column *elseColumnCursor + if elseColumn { + column = fb.startElseColumn(splitX, centerY+thenH/2+BranchGap) + } fb.endsWithReturn = false var lastElseID model.ID @@ -300,7 +316,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { for i, stmt := range s.ElseBody { thisAnchor := stmtOwnAnchor(stmt) fb.atBodyTail = tailWithoutMerge && i == len(s.ElseBody)-1 + column.begin(stmt) actID := fb.addStatement(stmt) + column.end() fb.atBodyTail = false if fb.takeBranchJoin(lastElseID, splitID, "false", pendingElseCase, prevElseAnchor, falseBranchAnchor) { pendingElseCase, pendingElseAnchor = "", nil @@ -312,6 +330,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // First statement in ELSE - connect from split going down (false path). // Same compositional rule as the THEN branch. flow := newDownwardFlowWithCase(splitID, actID, "false") + if column != nil { + flow.DestinationConnectionIndex = AnchorTop + } applyUserAnchors(flow, falseBranchAnchor, branchDestinationAnchor(falseBranchAnchor, thisAnchor)) fb.flows = append(fb.flows, flow) // Rejoin a pending empty custom error handler from the @@ -340,6 +361,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } else { flow = newHorizontalFlow(lastElseID, actID) } + if column != nil { + flow.OriginConnectionIndex, flow.DestinationConnectionIndex = AnchorBottom, AnchorTop + } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) fb.settleBranchFlow(lastElseID, actID, stmt, s.ElseBody[i+1:], elseReturns) @@ -453,6 +477,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // TRUE path: goes below the main line thenH := max(thenBounds.Height, ActivityHeight) thenCenterY := centerY + ActivityHeight/2 + BranchGap + thenH/2 + if guardRight { + thenCenterY = centerY + } fb.posX = thenStartX fb.posY = thenCenterY fb.endsWithReturn = false @@ -474,6 +501,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { if lastThenID == "" { // First statement in THEN - connect from split going down with "true" case flow := newDownwardFlowWithCase(splitID, actID, "true") + if guardRight { + flow = newHorizontalFlowWithCase(splitID, actID, "true") + } applyUserAnchors(flow, trueBranchAnchor, branchDestinationAnchor(trueBranchAnchor, thisAnchor)) fb.flows = append(fb.flows, flow) } else { @@ -549,6 +579,8 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } } + fb.baseY = savedBaseY + // If both branches end with RETURN, the flow terminates here if bothReturn { fb.endsWithReturn = true @@ -572,17 +604,28 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // Position after the split, past the downward branch's horizontal extent. afterSplit := splitX + HorizontalSpacing afterBranch := thenStartX + thenBounds.Width + HorizontalSpacing/2 - if !hasElseBody { + fb.posY = centerY + switch { + case guardRight: + // The branch took the centre line; the happy path drops from the split's + // bottom onto a new line, RowGap under everything the guard drew, starting + // in the split's column so the drop is straight. The new line has its own + // lane, so nothing is reserved. + _, bottom, _ := objectsExtent(fb.objects[splitIndex:]) + fb.posX = splitX + fb.posY = bottom + RowGap + ActivityHeight/2 + fb.baseY = fb.posY + noMergeExitAnchor = withDefaultSides(noMergeExitAnchor, AnchorBottom, AnchorTop) + case !hasElseBody: // A guard: the branch is in the lane below and ends there, so the main // line resumes right after the split — in the column the branch starts // in, so the two line up — and the lane is marked taken. fb.posX = thenStartX fb.reserveLowerLane(centerY, thenStartX-ActivityWidth/2+thenBounds.Width+laneGap) - } else { + default: fb.posX = max(afterSplit, afterBranch) fb.reserveLowerLane(centerY, thenStartX-ActivityWidth/2+branchWidth+laneGap) } - fb.posY = centerY if noMergeExitID != "" { fb.nextConnectionPoint = noMergeExitID fb.nextFlowCase = noMergeExitCase diff --git a/mdl/executor/cmd_microflows_builder_graph.go b/mdl/executor/cmd_microflows_builder_graph.go index b8d681a81b..76b6eb8fbb 100644 --- a/mdl/executor/cmd_microflows_builder_graph.go +++ b/mdl/executor/cmd_microflows_builder_graph.go @@ -19,6 +19,9 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a if fb.measurer == nil { fb.measurer = &layoutMeasurer{varTypes: fb.varTypes} } + if fb.measurer.layout == nil { + fb.measurer.layout = fb.layout + } if fb.declaredVars == nil { fb.declaredVars = make(map[string]string) } diff --git a/mdl/executor/cmd_microflows_builder_nested_test.go b/mdl/executor/cmd_microflows_builder_nested_test.go index 460f13947d..dc1c9b0a03 100644 --- a/mdl/executor/cmd_microflows_builder_nested_test.go +++ b/mdl/executor/cmd_microflows_builder_nested_test.go @@ -24,6 +24,7 @@ var nestedBuilderInherits = map[string]bool{ "declaredVars": true, "textLang": true, "measurer": true, + "layout": true, // a nested body is drawn in the flow's style (#1832) "backend": true, "quiet": true, "hierarchy": true, diff --git a/mdl/executor/cmd_microflows_handles.go b/mdl/executor/cmd_microflows_handles.go index 3c32bd2098..8b1bf842c3 100644 --- a/mdl/executor/cmd_microflows_handles.go +++ b/mdl/executor/cmd_microflows_handles.go @@ -126,6 +126,7 @@ func formatMicroflowActivitiesWithHandles( entityNames map[model.ID]string, microflowNames map[model.ID]string, ) []string { + defer ctx.describingFlowLayout(mf.Documentation)() cands, warnings, body, startLine := microflowTargets(ctx, mf, entityNames, microflowNames) handlesAt := map[int][]string{} diff --git a/mdl/executor/cmd_microflows_layout.go b/mdl/executor/cmd_microflows_layout.go index dcc2fe835d..4dbe5f7b92 100644 --- a/mdl/executor/cmd_microflows_layout.go +++ b/mdl/executor/cmd_microflows_layout.go @@ -50,11 +50,14 @@ type FlowLayoutResult struct { Objects int // objects in the flow Moved int // objects whose position or size changed Flows int // sequence flows whose anchors or curve changed - Refused string // why the flow was left alone; empty when it was laid out + // Restyled says the flow's @mxcli-layout documentation line changed: it is + // now laid out in a different style than the one it recorded. + Restyled bool + Refused string // why the flow was left alone; empty when it was laid out } // Changed reports whether laying the flow out changes anything. -func (r FlowLayoutResult) Changed() bool { return r.Moved > 0 || r.Flows > 0 } +func (r FlowLayoutResult) Changed() bool { return r.Moved > 0 || r.Flows > 0 || r.Restyled } // LayoutFlow lays out one microflow or nanoflow (kind "microflow" or // "nanoflow"). With dryRun, it computes the result and writes nothing. @@ -138,6 +141,14 @@ func layoutFlow(ctx *ExecContext, kind string, name ast.QualifiedName, dryRun bo } patched := plan.apply(doc) res.Moved, res.Flows = plan.movedObjects, plan.changedFlows + // The flow now records the style it was laid out in (flowlayout_config.go). + // Only the marker line changes; the rest of the documentation is the stored + // text, not the description's normal form of it. + style, err := ctx.ProjectFlowLayout() + if err != nil { + return res, fmt.Errorf("layout config: %w", err) + } + res.Restyled = setLayoutString(patched, "Documentation", withFlowLayoutMarker(stored.documentation, *style)) if !res.Changed() || dryRun { return res, nil } @@ -155,9 +166,10 @@ func layoutFlow(ctx *ExecContext, kind string, name ast.QualifiedName, dryRun bo // layoutGraph is the part of a flow the layout pairing needs, the same for a // microflow and a nanoflow. type layoutGraph struct { - id model.ID - parameters []*microflows.MicroflowParameter - objects *microflows.MicroflowObjectCollection + id model.ID + parameters []*microflows.MicroflowParameter + objects *microflows.MicroflowObjectCollection + documentation string } func storedFlow(ctx *ExecContext, kind string, name ast.QualifiedName) (*layoutGraph, error) { @@ -180,7 +192,7 @@ func storedFlow(ctx *ExecContext, kind string, name ast.QualifiedName) (*layoutG if !ok { return nil, mdlerrors.NewNotFound("microflow", name.String()) } - return &layoutGraph{id: mf.ID, parameters: mf.Parameters, objects: mf.ObjectCollection}, nil + return &layoutGraph{id: mf.ID, parameters: mf.Parameters, objects: mf.ObjectCollection, documentation: mf.Documentation}, nil case "nanoflow": all, err := ctx.Backend.ListNanoflows() if err != nil { @@ -192,7 +204,7 @@ func storedFlow(ctx *ExecContext, kind string, name ast.QualifiedName) (*layoutG if !ok { return nil, mdlerrors.NewNotFound("nanoflow", name.String()) } - return &layoutGraph{id: nf.ID, parameters: nf.Parameters, objects: nf.ObjectCollection}, nil + return &layoutGraph{id: nf.ID, parameters: nf.Parameters, objects: nf.ObjectCollection, documentation: nf.Documentation}, nil } return nil, fmt.Errorf("cannot lay out a %s", kind) } @@ -216,7 +228,9 @@ func rebuildFlowLayout(ctx *ExecContext, kind string, name ast.QualifiedName) (* return nil, fmt.Errorf("its description does not parse back (%v), so it does not round-trip through MDL", errs[0]) } - opts := buildFlowOpts{ResetLayout: true} + // Laid out in the project's style (or --style), not the one the flow records: + // re-styling a flow is what this is for. + opts := buildFlowOpts{ResetLayout: true, IgnoreRecordedLayout: true} for _, stmt := range prog.Statements { switch s := stmt.(type) { case *ast.CreateMicroflowStmt: diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index 8832b8a955..2327a7c336 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -483,6 +483,8 @@ func describeNanoflow(ctx *ExecContext, name ast.QualifiedName) error { wrapperMf := µflows.Microflow{ ReturnType: targetNf.ReturnType, ObjectCollection: targetNf.ObjectCollection, + // The describer reads the layout style the nanoflow records (#1832). + Documentation: targetNf.Documentation, } prevDescribingReturnValue := ctx.DescribingMicroflowHasReturnValue ctx.DescribingMicroflowHasReturnValue = microflowHasReturnValue(wrapperMf) @@ -499,6 +501,7 @@ func describeNanoflow(ctx *ExecContext, name ast.QualifiedName) error { ReturnType: targetNf.ReturnType, ReturnVariableName: targetNf.ReturnVariableName, ObjectCollection: targetNf.ObjectCollection, + Documentation: targetNf.Documentation, }, name, entityNames, microflowNames)() } activityLines := formatMicroflowActivities(ctx, wrapperMf, entityNames, microflowNames) @@ -819,6 +822,7 @@ func formatMicroflowActivities( if mf.ObjectCollection == nil { return []string{"-- debug: ObjectCollection is nil"} } + defer ctx.describingFlowLayout(mf.Documentation)() // Build activity map by ID for flow traversal activityMap := make(map[model.ID]microflows.MicroflowObject) @@ -1005,6 +1009,7 @@ func formatMicroflowBodyWithSourceMap( sourceMap map[string]elkSourceRange, headerLineCount int, ) (warnings, body []string) { + defer ctx.describingFlowLayout(mf.Documentation)() if mf.ObjectCollection == nil { return nil, []string{"-- debug: ObjectCollection is nil"} } diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index b2b7c2ee59..7a116d6d0f 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -1328,7 +1328,7 @@ func traverseFlow( } trueTerminates := branchFlowTerminatesBeforeMerge(trueFlow, mergeID, activityMap, flowsByOrigin, splitMergeMap) - isGuard := trueTerminates && flowLooksLikeGuardContinuation(falseFlow, obj, activityMap) && !hasExplicitFalseBranchAnchor(falseFlow) + isGuard := trueTerminates && flowLooksLikeGuardContinuation(ctx, falseFlow, obj, activityMap) && !hasExplicitFalseBranchAnchor(falseFlow) if isGuard { // A guard's THEN arm is followed by the false continuation. @@ -1551,7 +1551,7 @@ func traverseFlowUntilMerge( } trueTerminates := branchFlowTerminatesBeforeMerge(trueFlow, nestedMergeID, activityMap, flowsByOrigin, splitMergeMap) - isGuard := trueTerminates && flowLooksLikeGuardContinuation(falseFlow, obj, activityMap) && !hasExplicitFalseBranchAnchor(falseFlow) + isGuard := trueTerminates && flowLooksLikeGuardContinuation(ctx, falseFlow, obj, activityMap) && !hasExplicitFalseBranchAnchor(falseFlow) if isGuard { // A guard's THEN arm is followed by the false continuation. @@ -2214,7 +2214,7 @@ func emitEnumSplitStatement( branches := []enumBranch{} branchByDestination := map[model.ID]int{} var elseFlow *microflows.SequenceFlow - for _, flow := range orderedEnumSplitFlows(findNormalFlows(flowsByOrigin[currentID]), activityMap) { + for _, flow := range orderedEnumSplitFlowsFor(ctx, findNormalFlows(flowsByOrigin[currentID]), activityMap) { caseValue, ok := enumCaseValue(flow) if !ok { elseFlow = flow @@ -2279,7 +2279,7 @@ func emitInheritanceSplitStatement( } var elseFlow *microflows.SequenceFlow - for _, flow := range orderedInheritanceSplitFlows(findNormalFlows(flowsByOrigin[currentID])) { + for _, flow := range orderedInheritanceSplitFlowsFor(ctx, findNormalFlows(flowsByOrigin[currentID]), activityMap) { caseName, ok := inheritanceCaseName(flow, entityNames) if !ok { elseFlow = flow @@ -2427,6 +2427,59 @@ func orderedEnumSplitFlows(flows []*microflows.SequenceFlow, activityMap map[mod return ordered } +// orderedEnumSplitFlowsFor is orderedEnumSplitFlows for the flow being described. +// In a flow that records case-split columns (ako/mxcli#1832) every outcome after +// the first leaves the split's bottom for the top of its own column: one side +// pair and one depth, so the order is finished by how far right the column +// stands. The tie-break is kept to that style: any other drawing reads as before. +func orderedEnumSplitFlowsFor(ctx *ExecContext, flows []*microflows.SequenceFlow, activityMap map[model.ID]microflows.MicroflowObject) []*microflows.SequenceFlow { + ordered := orderedEnumSplitFlows(flows, activityMap) + if describesCaseColumns(ctx) { + sortColumnsLeftToRight(ordered, splitCaseOrder, activityMap) + } + return ordered +} + +// orderedInheritanceSplitFlowsFor is orderedInheritanceSplitFlows for the flow +// being described, finished left to right in a flow that records case-split +// columns (see orderedEnumSplitFlowsFor). +func orderedInheritanceSplitFlowsFor(ctx *ExecContext, flows []*microflows.SequenceFlow, activityMap map[model.ID]microflows.MicroflowObject) []*microflows.SequenceFlow { + ordered := orderedInheritanceSplitFlows(flows) + if describesCaseColumns(ctx) { + sortColumnsLeftToRight(ordered, inheritanceSplitCaseOrder, activityMap) + } + return ordered +} + +func describesCaseColumns(ctx *ExecContext) bool { + return ctx != nil && ctx.describedFlowLayout != nil && ctx.describedFlowLayout.CaseSplit == CaseSplitColumns +} + +// sortColumnsLeftToRight orders flows by their side pair's rank, then by how far +// right their destination stands, then how far down. Columns share a top edge, +// not a centre — a column starting with an end event has its centre higher than +// one starting with an activity — so x decides, not y. +func sortColumnsLeftToRight(flows []*microflows.SequenceFlow, rank func(*microflows.SequenceFlow) int, activityMap map[model.ID]microflows.MicroflowObject) { + at := func(f *microflows.SequenceFlow) model.Point { + if f != nil { + if obj := activityMap[f.DestinationID]; obj != nil { + return obj.GetPosition() + } + } + return model.Point{} + } + sort.SliceStable(flows, func(i, j int) bool { + if ri, rj := rank(flows[i]), rank(flows[j]); ri != rj { + return ri < rj + } + pi, pj := at(flows[i]), at(flows[j]) + if pi.X != pj.X { + return pi.X < pj.X + } + return pi.Y < pj.Y + }) +} + func orderedInheritanceSplitFlows(flows []*microflows.SequenceFlow) []*microflows.SequenceFlow { ordered := append([]*microflows.SequenceFlow(nil), flows...) sort.SliceStable(ordered, func(i, j int) bool { @@ -2563,6 +2616,7 @@ func findBranchFlows(flows []*microflows.SequenceFlow) (trueFlow, falseFlow *mic } func flowLooksLikeGuardContinuation( + ctx *ExecContext, flow *microflows.SequenceFlow, split microflows.MicroflowObject, activityMap map[model.ID]microflows.MicroflowObject, @@ -2583,9 +2637,22 @@ func flowLooksLikeGuardContinuation( // intentionally relies on mxcli's layout/anchor contract so a real false // branch whose activities happen to be aligned with the split is not // collapsed into a guard-style continuation during describe. - return dest.GetPosition().Y == split.GetPosition().Y && + if dest.GetPosition().Y == split.GetPosition().Y && flow.OriginConnectionIndex == AnchorRight && - flow.DestinationConnectionIndex == AnchorLeft + flow.DestinationConnectionIndex == AnchorLeft { + return true + } + // A flow drawn with `guard: true-right` (ako/mxcli#1832) continues straight + // down from the split's bottom instead. Only a flow that records that style is + // read this way: in any other the same shape is an ELSE column, and describing + // it as a guard would not rebuild where it is. + if ctx != nil && ctx.describedFlowLayout != nil && ctx.describedFlowLayout.Guard == GuardTrueRight { + return dest.GetPosition().X == split.GetPosition().X && + dest.GetPosition().Y > split.GetPosition().Y && + flow.OriginConnectionIndex == AnchorBottom && + flow.DestinationConnectionIndex == AnchorTop + } + return false } // findErrorHandlerFlow returns the error handler flow from an activity's outgoing flows. diff --git a/mdl/executor/exec_context.go b/mdl/executor/exec_context.go index edbf61afb0..1e30ba6599 100644 --- a/mdl/executor/exec_context.go +++ b/mdl/executor/exec_context.go @@ -103,6 +103,18 @@ type ExecContext struct { // ThemeRegistry holds cached theme design property definitions (lazy init). ThemeRegistry *ThemeRegistry + // FlowLayoutOverride, when set, is the layout style flows are laid out with + // instead of the project's layout-config.yaml (`mxcli layout flows --style`). + FlowLayoutOverride *flowLayout + // describedFlowLayout is the style the flow being described records in its + // documentation, nil when it records none (see describingFlowLayout). + describedFlowLayout *flowLayout + // projectFlowLayout caches the project's layout-config.yaml (see + // ProjectFlowLayout). + projectFlowLayout *flowLayout + projectFlowLayoutErr error + projectFlowLayoutLoaded bool + // widgetRegistry caches the widget definitions for this session. Loaded // once via GetWidgetRegistry, because DESCRIBE PAGE consults it per widget // and LoadWidgetRegistry reads .def.json files off disk. diff --git a/mdl/executor/executor.go b/mdl/executor/executor.go index 67c1e210d7..8e0bcfcde1 100644 --- a/mdl/executor/executor.go +++ b/mdl/executor/executor.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strconv" "sync" "time" @@ -281,9 +282,12 @@ type Executor struct { output io.Writer guard *outputGuard // line-limit wrapper around output mprPath string - scriptDir string // directory of the .mdl file being executed (see SetScriptDir) - langVersion langver.Version // `mdl ;` of the program being run (see enterLanguage) - describeLang *langver.Version // the language DESCRIBE writes, when set (SetDescribeLanguage) + // flowLayoutOverride is the style `mxcli layout flows --style` lays out + // with, in place of the project's layout-config.yaml. + flowLayoutOverride *flowLayout + scriptDir string // directory of the .mdl file being executed (see SetScriptDir) + langVersion langver.Version // `mdl ;` of the program being run (see enterLanguage) + describeLang *langver.Version // the language DESCRIBE writes, when set (SetDescribeLanguage) // forwardFlowCalls is set when the run is all-or-nothing (SetForwardFlowCalls); // scriptFlows then holds every flow the program being run declares. forwardFlowCalls bool @@ -336,6 +340,41 @@ func (e *Executor) SetQuiet(quiet bool) { e.quiet = quiet } +// SetFlowLayoutStyle lays flows out in the named preset instead of the +// project's layout-config.yaml. "" restores the project's style. +func (e *Executor) SetFlowLayoutStyle(preset string) error { + if preset == "" { + e.flowLayoutOverride = nil + return nil + } + l, err := FlowLayoutPreset(preset) + if err != nil { + return err + } + e.flowLayoutOverride = l + return nil +} + +// FlowLayoutStyle describes the style flows are laid out with and where it +// comes from: the --style override, the project's layout-config.yaml, or the +// default. +func (e *Executor) FlowLayoutStyle() (style, source string, err error) { + ctx := e.newExecContext(context.Background()) + l, err := ctx.ProjectFlowLayout() + if err != nil { + return "", "", err + } + switch { + case e.flowLayoutOverride != nil: + source = "--style" + case e.mprPath != "" && FindFlowLayoutConfig(filepath.Dir(e.mprPath)) != "": + source = FindFlowLayoutConfig(filepath.Dir(e.mprPath)) + default: + source = "default (no " + flowLayoutConfigName + ")" + } + return l.String(), source, nil +} + // SetFormat sets the output format (table or json). func (e *Executor) SetFormat(f OutputFormat) { e.format = f diff --git a/mdl/executor/executor_dispatch.go b/mdl/executor/executor_dispatch.go index 40ac9691a7..7e2f6b4304 100644 --- a/mdl/executor/executor_dispatch.go +++ b/mdl/executor/executor_dispatch.go @@ -93,32 +93,33 @@ func (e *Executor) newExecContext(ctx context.Context) *ExecContext { gen := e.catalogGen e.catalogMu.RUnlock() return &ExecContext{ - Context: ctx, - Backend: e.backend, - lastWriteStats: currentWriteStats(e.backend), - Output: e.output, - Format: e.format, - Quiet: e.quiet, - tally: e.tally, - accessRun: e.accessRun, - Logger: e.logger, - Fragments: e.fragments, - Catalog: cat, - Cache: e.cache, - MprPath: e.mprPath, - ScriptDir: e.scriptDir, - LanguageVersion: e.langVersion, - forwardFlowCalls: e.forwardFlowCalls, - scriptFlows: e.scriptFlows, - scriptTargets: e.scriptTargets, - describeLang: e.describeLang, - SqlMgr: e.sqlMgr, - ThemeRegistry: e.themeRegistry, - Settings: e.settings, - BackendFactory: e.backendFactory, - ExecuteFn: e.Execute, - ExecuteProgramFn: e.ExecuteProgram, - FinalizeFn: e.finalizeProgramExecution, + Context: ctx, + Backend: e.backend, + lastWriteStats: currentWriteStats(e.backend), + Output: e.output, + Format: e.format, + Quiet: e.quiet, + tally: e.tally, + accessRun: e.accessRun, + Logger: e.logger, + Fragments: e.fragments, + Catalog: cat, + Cache: e.cache, + MprPath: e.mprPath, + FlowLayoutOverride: e.flowLayoutOverride, + ScriptDir: e.scriptDir, + LanguageVersion: e.langVersion, + forwardFlowCalls: e.forwardFlowCalls, + scriptFlows: e.scriptFlows, + scriptTargets: e.scriptTargets, + describeLang: e.describeLang, + SqlMgr: e.sqlMgr, + ThemeRegistry: e.themeRegistry, + Settings: e.settings, + BackendFactory: e.backendFactory, + ExecuteFn: e.Execute, + ExecuteProgramFn: e.ExecuteProgram, + FinalizeFn: e.finalizeProgramExecution, SyncCatalog: func(cat *catalog.Catalog) { e.catalogMu.Lock() defer e.catalogMu.Unlock() diff --git a/mdl/executor/flowlayout.go b/mdl/executor/flowlayout.go new file mode 100644 index 0000000000..da00865bbf --- /dev/null +++ b/mdl/executor/flowlayout.go @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package executor - the layout style a flow is built with. +// +// The builder places every activity while it creates it (layout.go, +// layout_lanes.go, layout_rows.go). Teams do not agree on how a flow should look +// (ako/mxcli#1832, docs/11-proposals/PROPOSAL_configurable_flow_layout.md), so +// the decisions a style changes are read from a flowLayout instead of being +// fixed in the builder. +// +// The zero value is the default style, happy-line, and reproduces the layout +// mxcli has always drawn: a builder or measurer with no flowLayout lays out +// exactly as before. That is what keeps every existing layout test, and every +// DESCRIBE round trip, unchanged. +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// IfElseStyle is how an IF with an ELSE is drawn. +type IfElseStyle int + +const ( + // IfElseLanes: the TRUE body runs on the centre line, the ELSE body in a lane + // under it, left to right. The default. + IfElseLanes IfElseStyle = iota + // IfElseColumn: the TRUE body runs on the centre line, the ELSE body is stacked + // top to bottom in a column under the split, and rejoins the merge from below. + IfElseColumn +) + +// GuardStyle is how a guard — `if X then …; return; end if`, no ELSE — is drawn. +type GuardStyle int + +const ( + // GuardReturnBelow: the TRUE branch with its early return goes in the lane + // under the split; the happy path carries straight on along the FALSE exit. + // The default. + GuardReturnBelow GuardStyle = iota + // GuardTrueRight: the TRUE branch carries straight on to the right, on the + // centre line; the happy path leaves the split's bottom and continues on a + // new line below, starting in the split's column. + GuardTrueRight +) + +// CaseSplitStyle is how a CASE (enumeration split) or a split by type +// (inheritance split) is drawn. +type CaseSplitStyle int + +const ( + // CaseSplitLanes: the outcomes are stacked as lanes, each running left to + // right. The default. + CaseSplitLanes CaseSplitStyle = iota + // CaseSplitColumns: the first outcome, in the order written, carries on along + // the centre line; every other outcome is a column under the split, top to + // bottom, the columns side by side left to right in case order. Each column + // rejoins the merge from below. + CaseSplitColumns +) + +// WrapStyle is what the main line does when it gets long. +type WrapStyle int + +const ( + // WrapRows: past the row width the main line continues on a new row, back at + // the left (layout_rows.go). The default. + WrapRows WrapStyle = iota + // WrapOff: the main line never wraps, however long it gets. + WrapOff +) + +// flowLayout holds the layout decisions a style changes. Its zero value is the +// default style. +type flowLayout struct { + IfElse IfElseStyle + Guard GuardStyle + CaseSplit CaseSplitStyle + Wrap WrapStyle + // MaxRowWidth is where a row wraps; 0 is MaxRowWidth, the default. The + // default width is stored as 0, so a style says it the one way. + MaxRowWidth int +} + +// rowWidth is the width a row wraps at. +func (l flowLayout) rowWidth() int { + if l.MaxRowWidth > 0 { + return l.MaxRowWidth + } + return MaxRowWidth +} + +// layoutStyle returns the builder's layout style; a builder without one uses the +// default. +func (fb *flowBuilder) layoutStyle() flowLayout { + if fb == nil || fb.layout == nil { + return flowLayout{} + } + return *fb.layout +} + +// layoutStyle returns the measurer's layout style; a measurer without one uses the +// default. +func (m *layoutMeasurer) layoutStyle() flowLayout { + if m == nil || m.layout == nil { + return flowLayout{} + } + return *m.layout +} + +// elseColumnCursor places the statements of an ELSE body one under the other in +// the split's column (IfElseColumn). A nil cursor does nothing, which is how the +// default style's ELSE lane goes through the same loop untouched. +// +// Each statement is built where the cursor says and then moved, as a whole, +// straight down until its top edge is RowGap under the bottom of the one before. +// Measuring what was built, rather than estimating it, is what separateRows does +// for wrapped rows (layout_rows.go): a loop box is sized only after its body, and +// a note or an error handler's row adds its own height. +type elseColumnCursor struct { + fb *flowBuilder + x int + // top is where the next statement's top edge goes. + top int + // first is the index into fb.objects of the statement being built. + first int + // pinned says the statement places itself with @position; it is not moved, + // and the column carries on under it. + pinned bool +} + +func (fb *flowBuilder) startElseColumn(x, top int) *elseColumnCursor { + return &elseColumnCursor{fb: fb, x: x, top: top} +} + +func (c *elseColumnCursor) begin(stmt ast.MicroflowStatement) { + if c == nil { + return + } + c.first = len(c.fb.objects) + _, c.pinned = ownPositionX(stmt) + c.fb.posX = c.x + c.fb.posY = c.top + ActivityHeight/2 +} + +func (c *elseColumnCursor) end() { + if c == nil { + return + } + top, bottom, ok := objectsExtent(c.fb.objects[c.first:]) + if !ok { + return + } + if !c.pinned && top != c.top { + for _, o := range c.fb.objects[c.first:] { + if o == nil { + continue + } + p := o.GetPosition() + o.SetPosition(model.Point{X: p.X, Y: p.Y + c.top - top}) + } + bottom += c.top - top + } + c.top = bottom + RowGap + c.fb.posX = c.x + c.fb.posY = c.top + ActivityHeight/2 +} + +// objectsExtent returns the top and bottom edges of the objects' boxes. An +// object's Position is its centre. +func objectsExtent(objects []microflows.MicroflowObject) (top, bottom int, ok bool) { + for _, o := range objects { + if o == nil { + continue + } + p := o.GetPosition() + h := ActivityHeight + if ws, okSize := o.(interface{ GetSize() model.Size }); okSize { + if hh := ws.GetSize().Height; hh > 0 { + h = hh + } + } + t, b := p.Y-h/2, p.Y+h/2 + if !ok { + top, bottom, ok = t, b, true + continue + } + top, bottom = min(top, t), max(bottom, b) + } + return top, bottom, ok +} + +// measureColumn measures an ELSE body stacked as a column: as wide as its widest +// statement, as tall as all of them with RowGap between. A RETURN draws an end +// event at the bottom. +func (m *layoutMeasurer) measureColumn(stmts []ast.MicroflowStatement) Bounds { + width, height, n := 0, 0, 0 + for _, stmt := range stmts { + b := m.measureStatement(stmt) + w, h := b.Width, max(b.Height, ActivityHeight) + if w == 0 { + if _, isReturn := stmt.(*ast.ReturnStmt); !isReturn { + continue + } + w, h = EventSize, EventSize + } + if row := m.handlerRowWidth(stmt); row > 0 { + w = max(w, row) + h += ErrorHandlerPitch + } + width = max(width, w) + height += h + n++ + } + if n > 1 { + height += (n - 1) * RowGap + } + return Bounds{Width: width, Height: height} +} + +// elseColumnBranchWidth is the branch width addIfStatement places the merge +// after, for an ELSE column. The merge goes past the TRUE body as before, and +// past the column too: the column is centred under the split, so it begins half +// an activity left of the split's centre, where the TRUE body begins +// SplitWidth + HorizontalSpacing/2 to the right of it. +func elseColumnBranchWidth(thenBounds, column Bounds) int { + return max(thenBounds.Width, column.Width-(SplitWidth+HorizontalSpacing/2)) +} + +// withDefaultSides returns anchors whose unset sides are from and to. A side the +// script states is kept: an @anchor always wins over the style. +func withDefaultSides(a *ast.FlowAnchors, from, to int) *ast.FlowAnchors { + out := ast.FlowAnchors{From: ast.AnchorSide(from), To: ast.AnchorSide(to)} + if a != nil { + if a.From != ast.AnchorSideUnset { + out.From = a.From + } + if a.To != ast.AnchorSideUnset { + out.To = a.To + } + } + return &out +} + +// measureGuardTrueRight measures a statement list from a GuardTrueRight guard +// on: the guard's branch on this line, everything after it on a new line below, +// starting in the split's column. left is where the guard starts, lineHeight the +// height of what the line held before it. +// +// The height is reported as if centred on the line, as every Bounds is: the +// callers place things under a body at centre + Height/2, and the drop makes this +// body reach far further below its line than above it. Twice the depth below is +// what keeps them clear; the cost is room above the line that goes unused. +func (m *layoutMeasurer) measureGuardTrueRight(guard *ast.IfStmt, rest []ast.MicroflowStatement, left, extent, lineHeight int) Bounds { + // guardBranchInset is measured from the split's centre; what follows a body + // (a merge) is placed from its left edge, so the split's left half is added. + // Without it the merge after a TRUE-right guard sat on the guard's end event. + branch := m.measureBranch(guard.ThenBody) + extent = max(extent, left+SplitWidth/2+guardBranchInset+branch.Width) + line := max(lineHeight, branch.Height, ActivityHeight) + after := m.measureStatements(rest) + if after.Width == 0 { + return Bounds{Width: extent, Height: line} + } + // The new line starts with its first element centred under the split: half an + // activity left of the split's left edge, at most. + extent = max(extent, left+after.Width) + below := line/2 + RowGap + max(after.Height, ActivityHeight) + return Bounds{Width: extent, Height: 2 * below} +} + +// caseColumns plans the columns of a CaseSplitColumns split: outcome 0 is on the +// centre line, outcome i > 0 is the column centred at x[i]. The columns are +// placed by their MEASURED width, not by what turns out to be drawn, so the split +// is exactly as wide as the measurer says it is and an enclosing body that made +// room for it by measuring is not overrun. +type caseColumns struct { + x []int + // span is how far the columns reach right, measured as a branch is: from the + // left edge of an activity centred under the split. + span int +} + +func (m *layoutMeasurer) planCaseColumns(splitX int, bodies [][]ast.MicroflowStatement) caseColumns { + plan := caseColumns{x: make([]int, len(bodies))} + x := splitX + for i := 1; i < len(bodies); i++ { + w := max(m.measureColumn(bodies[i]).Width, ActivityWidth) + plan.x[i] = x + plan.span = x - splitX + w + x += w + BranchGap + } + return plan +} + +// caseColumnsBranchWidth is the branch width the split's merge is placed after: +// past the first outcome's line and past the columns, as for an ELSE column. +func caseColumnsBranchWidth(firstWidth int, plan caseColumns, splitWidth int) int { + return max(firstWidth, plan.span-(splitWidth+HorizontalSpacing/2)) +} + +// firstCaseGroupShift is how far a CaseSplitColumns split moves its first +// outcome's body right when that outcome has several values: their own merge +// stands on the line where the body would begin. (The default lanes draw the +// merge on top of the first activity; that is not changed here.) +const firstCaseGroupShift = MergeSize + (HorizontalSpacing - ActivityWidth) + +// measureCaseColumns measures a CaseSplitColumns split. splitWidth is the split +// object's own width (an inheritance split is activity-sized); firstShift is +// firstCaseGroupShift when the first outcome has several values. The height is +// reported as if centred on the line (see measureGuardTrueRight). +func (m *layoutMeasurer) measureCaseColumns(bodies [][]ast.MicroflowStatement, splitWidth, firstShift int) Bounds { + if len(bodies) == 0 { + return Bounds{Width: splitWidth + HorizontalSpacing/2 + HorizontalSpacing/2 + MergeSize, Height: ActivityHeight} + } + first := m.measureBranch(bodies[0]) + plan := m.planCaseColumns(0, bodies) + branchWidth := caseColumnsBranchWidth(first.Width+firstShift, plan, splitWidth) + if branchWidth == 0 { + branchWidth = HorizontalSpacing / 2 + } + columnHeight := 0 + for _, body := range bodies[1:] { + columnHeight = max(columnHeight, m.measureColumn(body).Height) + } + line := max(first.Height, ActivityHeight) + height := line + if len(bodies) > 1 { + height = 2 * (line/2 + BranchGap + max(columnHeight, ActivityHeight)) + } + return Bounds{Width: splitWidth + HorizontalSpacing/2 + branchWidth + MergeSize, Height: height} +} + +// setColumnFlowSides points the flows a split's outcome i > 0 starts with down +// into its column: out of the bottom, into the top. The CASE's own @anchor for +// the value is applied again on top, so a side the script states still wins. +func setColumnFlowSides(flows []*microflows.SequenceFlow, slot splitCaseSlot) { + for _, f := range flows { + f.OriginConnectionIndex, f.DestinationConnectionIndex = AnchorBottom, AnchorTop + if ec, ok := f.CaseValue.(microflows.EnumerationCase); ok { + slot.applyCaseAnchor(f, ec.Value) + } + } +} diff --git a/mdl/executor/flowlayout_casesplit_test.go b/mdl/executor/flowlayout_casesplit_test.go new file mode 100644 index 0000000000..a68287467d --- /dev/null +++ b/mdl/executor/flowlayout_casesplit_test.go @@ -0,0 +1,231 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "math/rand" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#1832: `case-split: first-right-rest-columns` keeps a CASE's first +// outcome on the centre line and lays every other outcome out as a column under +// the split, side by side, in case order. + +func caseColumnsLayout() *flowLayout { return &flowLayout{CaseSplit: CaseSplitColumns} } + +func enumCase(value string, body []ast.MicroflowStatement) ast.EnumSplitCase { + return ast.EnumSplitCase{Value: value, Body: body} +} + +func enumSplit(cases []ast.EnumSplitCase, elseBody []ast.MicroflowStatement) *ast.EnumSplitStmt { + return &ast.EnumSplitStmt{Variable: "Status", Cases: cases, ElseBody: elseBody} +} + +func TestCaseSplitColumns_FirstOnTheLineRestAsColumns(t *testing.T) { + s := enumSplit([]ast.EnumSplitCase{ + enumCase("Open", logStatements(2)), + enumCase("Closed", logStatements(2)), + enumCase("Archived", logStatements(3)), + }, logStatements(1)) + stmts := append([]ast.MicroflowStatement{s}, logStatements(1)...) + fb := buildWithLayout(t, stmts, caseColumnsLayout()) + + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + merge, ok := firstOfType[*microflows.ExclusiveMerge](fb.objects) + if !ok { + t.Fatal("no merge") + } + sp, mp := split.GetPosition(), merge.GetPosition() + acts := activityPositions(fb.objects) + if len(acts) != 9 { + t.Fatalf("want 9 activities, got %d", len(acts)) + } + first, closed, archived, other, after := acts[0:2], acts[2:4], acts[4:7], acts[7:8], acts[8] + + for _, p := range first { + if p.Y != sp.Y { + t.Errorf("first case at %v, want on the centre line y=%d", p, sp.Y) + } + } + columns := [][]model.Point{closed, archived, other} + prevX := 0 + for c, col := range columns { + for i, p := range col { + if p.X != col[0].X { + t.Errorf("column %d statement %d at x=%d, want the column's x=%d", c, i, p.X, col[0].X) + } + if i > 0 && p.Y <= col[i-1].Y { + t.Errorf("column %d statement %d at y=%d, not under the one before at y=%d", c, i, p.Y, col[i-1].Y) + } + } + if col[0].Y <= sp.Y { + t.Errorf("column %d starts at y=%d, not under the split at y=%d", c, col[0].Y, sp.Y) + } + if c == 0 && col[0].X != sp.X { + t.Errorf("second case's column at x=%d, want under the split x=%d", col[0].X, sp.X) + } + if c > 0 && col[0].X <= prevX { + t.Errorf("column %d at x=%d is not right of column %d at x=%d", c, col[0].X, c-1, prevX) + } + prevX = col[0].X + } + if mp.Y != sp.Y { + t.Errorf("merge at %v, want on the centre line", mp) + } + if after.Y != sp.Y || after.X <= mp.X { + t.Errorf("activity after the CASE at %v, want on the centre line past the merge at %v", after, mp) + } + + // The first case leaves the right; every other leaves the bottom into its + // column's top, and each column rejoins the merge from below. + for _, f := range fb.flows { + if f.OriginID != split.GetID() { + continue + } + dest := objectByIDIn(fb.objects, f.DestinationID).GetPosition() + if dest.Y == sp.Y { + if f.OriginConnectionIndex != AnchorRight || f.DestinationConnectionIndex != AnchorLeft { + t.Errorf("first case flow sides %d->%d, want right->left", f.OriginConnectionIndex, f.DestinationConnectionIndex) + } + } else if f.OriginConnectionIndex != AnchorBottom || f.DestinationConnectionIndex != AnchorTop { + t.Errorf("column flow to %v sides %d->%d, want bottom->top", dest, f.OriginConnectionIndex, f.DestinationConnectionIndex) + } + } + assertNoOverlap(t, fb.objects) +} + +func TestCaseSplitColumns_DefaultLayoutIsUnchanged(t *testing.T) { + s := enumSplit([]ast.EnumSplitCase{ + enumCase("Open", logStatements(2)), + enumCase("Closed", logStatements(2)), + }, nil) + want := buildRows(t, []ast.MicroflowStatement{s}) + fb := buildWithLayout(t, []ast.MicroflowStatement{s}, &flowLayout{}) + for i, o := range fb.objects { + if o.GetPosition() != want[i].GetPosition() { + t.Fatalf("object %d at %v, default builder puts it at %v", i, o.GetPosition(), want[i].GetPosition()) + } + } +} + +func TestCaseSplitColumns_MultiValueCaseAndNesting(t *testing.T) { + inner := ifElse(logStatements(1), logStatements(2)) + s := enumSplit([]ast.EnumSplitCase{ + {Values: []string{"Open", "New"}, Body: logStatements(1)}, + {Values: []string{"Closed", "Done"}, Body: []ast.MicroflowStatement{inner, logStatements(1)[0]}}, + enumCase("Archived", append(logStatements(1), &ast.ReturnStmt{})), + }, nil) + stmts := append([]ast.MicroflowStatement{s}, logStatements(1)...) + for _, l := range []*flowLayout{caseColumnsLayout(), {CaseSplit: CaseSplitColumns, IfElse: IfElseColumn}} { + fb := buildWithLayout(t, stmts, l) + assertNoOverlap(t, fb.objects) + } +} + +func TestCaseSplitColumns_InheritanceSplit(t *testing.T) { + s := &ast.InheritanceSplitStmt{ + Variable: "Animal", + Cases: []ast.InheritanceSplitCase{ + {Entity: ast.QualifiedName{Module: "M", Name: "Dog"}, Body: logStatements(2)}, + {Entity: ast.QualifiedName{Module: "M", Name: "Cat"}, Body: logStatements(2)}, + {Entity: ast.QualifiedName{Module: "M", Name: "Bird"}, Body: logStatements(1)}, + }, + } + stmts := append([]ast.MicroflowStatement{s}, logStatements(1)...) + fb := buildWithLayout(t, stmts, caseColumnsLayout()) + split, _ := firstOfType[*microflows.InheritanceSplit](fb.objects) + sp := split.GetPosition() + acts := activityPositions(fb.objects) + if acts[0].Y != sp.Y || acts[1].Y != sp.Y { + t.Errorf("first case %v, want on the centre line y=%d", acts[0:2], sp.Y) + } + if acts[2].X != sp.X || acts[3].X != sp.X || acts[3].Y <= acts[2].Y { + t.Errorf("second case %v, want a column under the split x=%d", acts[2:4], sp.X) + } + if acts[4].X <= sp.X || acts[4].Y != acts[2].Y { + t.Errorf("third case %v, want a column right of the second, level with it", acts[4]) + } + assertNoOverlap(t, fb.objects) +} + +func TestIfElseColumn_NestedColumnInTheTrueBodyIsCleared(t *testing.T) { + // The TRUE body holds an IF whose ELSE column hangs below the line: the outer + // ELSE column must start below it. The measurer reports the inner IF's height + // as if centred on the line, so this takes it doubling what reaches below. + // The outer ELSE holds an IF of its own whose TRUE body runs right, under the + // inner column, so starting the outer column too high puts the two on top of + // each other. + inner := ifElse(logStatements(1), logStatements(3)) + outerElse := []ast.MicroflowStatement{ifElse(logStatements(3), logStatements(1))} + stmts := []ast.MicroflowStatement{ifElse([]ast.MicroflowStatement{inner}, outerElse)} + fb := buildWithLayout(t, stmts, columnLayout()) + assertNoOverlap(t, fb.objects) +} + +// The stored flow order does not survive serialization, so DESCRIBE reads a +// CASE's order back from the drawing. Columns share one side pair and one top, +// so in a flow recording case-split columns the order is finished left to right. +func TestCaseSplitColumns_DescribeReadsColumnsLeftToRight(t *testing.T) { + cases := []ast.EnumSplitCase{enumCase("V1", logStatements(1))} + for i := 2; i <= 5; i++ { + cases = append(cases, enumCase(fmt.Sprintf("V%d", i), logStatements(i%3+1))) + } + fb := buildWithLayout(t, []ast.MicroflowStatement{enumSplit(cases, nil)}, caseColumnsLayout()) + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + objects := map[model.ID]microflows.MicroflowObject{} + for _, o := range fb.objects { + objects[o.GetID()] = o + } + var flows []*microflows.SequenceFlow + for _, f := range fb.flows { + if f.OriginID == split.GetID() { + flows = append(flows, f) + } + } + ctx := &ExecContext{describedFlowLayout: caseColumnsLayout()} + for seed := int64(1); seed <= 5; seed++ { + rand.New(rand.NewSource(seed)).Shuffle(len(flows), func(i, j int) { flows[i], flows[j] = flows[j], flows[i] }) + for i, f := range orderedEnumSplitFlowsFor(ctx, flows, objects) { + if v, _ := enumCaseValue(f); v != fmt.Sprintf("V%d", i+1) { + t.Fatalf("seed %d: position %d holds %s", seed, i, v) + } + } + } +} + +func TestCaseSplitColumns_DescribeReadsInheritanceColumnsLeftToRight(t *testing.T) { + s := &ast.InheritanceSplitStmt{Variable: "Animal"} + names := []string{"Dog", "Cat", "Bird", "Fish"} + for i, n := range names { + s.Cases = append(s.Cases, ast.InheritanceSplitCase{Entity: ast.QualifiedName{Module: "M", Name: n}, Body: logStatements(i%2 + 1)}) + } + fb := buildWithLayout(t, []ast.MicroflowStatement{s}, caseColumnsLayout()) + split, _ := firstOfType[*microflows.InheritanceSplit](fb.objects) + objects := map[model.ID]microflows.MicroflowObject{} + for _, o := range fb.objects { + objects[o.GetID()] = o + } + var flows []*microflows.SequenceFlow + for _, f := range fb.flows { + if f.OriginID == split.GetID() { + if _, isMerge := objects[f.DestinationID].(*microflows.ExclusiveMerge); !isMerge { + flows = append(flows, f) // the (empty) flow straight to the merge orders itself by its pair + } + } + } + ctx := &ExecContext{describedFlowLayout: caseColumnsLayout()} + for seed := int64(1); seed <= 5; seed++ { + rand.New(rand.NewSource(seed)).Shuffle(len(flows), func(i, j int) { flows[i], flows[j] = flows[j], flows[i] }) + for i, f := range orderedInheritanceSplitFlowsFor(ctx, flows, objects) { + ic, _ := f.CaseValue.(*microflows.InheritanceCase) + if ic == nil || ic.EntityQualifiedName != "M."+names[i] { + t.Fatalf("seed %d: position %d holds %+v", seed, i, f.CaseValue) + } + } + } +} diff --git a/mdl/executor/flowlayout_config.go b/mdl/executor/flowlayout_config.go new file mode 100644 index 0000000000..18949fd80e --- /dev/null +++ b/mdl/executor/flowlayout_config.go @@ -0,0 +1,384 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package executor - where a flow's layout style comes from. +// +// Two places, in this order (ako/mxcli#1832): +// +// 1. The flow's own documentation, when it carries an `@mxcli-layout:` line. +// That line records the style the flow was last laid out with, so a +// describe → exec round trip rebuilds the same geometry whatever the project +// says now, and canonical DESCRIBE judges a position "derived" against the +// style that actually drew it. +// 2. Otherwise the project's committed layout-config.yaml, looked up the way +// lint-config.yaml is. No file is the default style. +// +// A flow laid out under a style other than the default gets the line; under the +// default it loses it. So a project with no config file writes exactly what it +// always wrote. +package executor + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + + "gopkg.in/yaml.v3" +) + +// minRowWidth and maxRowWidthLimit bound max-row-width: narrower than a few +// activities every statement starts a row of its own, and past the limit a +// typo of an extra digit would quietly mean "never wrap" — which is `wrap: off`. +const ( + minRowWidth = 4 * HorizontalSpacing + maxRowWidthLimit = 20000 +) + +// flowLayoutMarker starts the documentation line that records a flow's style. +// It is plain documentation text: the visitor keeps it inside `/** … */` +// verbatim and never reads it as an annotation. +const flowLayoutMarker = "@mxcli-layout:" + +// flowLayoutConfigName is the project file holding the layout style. +const flowLayoutConfigName = "layout-config.yaml" + +// FindFlowLayoutConfig returns the project's layout-config.yaml, or "" when there +// is none. Searched like lint-config.yaml: .claude/ first, then the project +// directory — both committed, unlike .mxcli/, which holds secrets and is +// git-ignored. +func FindFlowLayoutConfig(projectDir string) string { + for _, p := range []string{ + filepath.Join(projectDir, ".claude", flowLayoutConfigName), + filepath.Join(projectDir, flowLayoutConfigName), + } { + if _, err := os.Stat(p); err == nil { + return p + } + } + return "" +} + +// flowLayoutPresets are the named styles. A preset only names options; it is +// never what the marker records, so a preset can grow options later without +// changing how an already laid-out flow is read back. +var flowLayoutPresets = map[string]flowLayout{ + "happy-line": {}, + "true-right-false-down": {IfElse: IfElseColumn, CaseSplit: CaseSplitColumns}, +} + +// flowLayoutOption is one option key: its values and how each sets a flowLayout. +// An option with a number for a value has parse instead of values. +type flowLayoutOption struct { + values map[string]func(*flowLayout) + parse func(*flowLayout, string) error + // value returns the option's spelling for a layout. + value func(flowLayout) string + // defaultValue is the spelling of the default style's value. + defaultValue string +} + +var flowLayoutOptions = map[string]flowLayoutOption{ + "if-else": { + values: map[string]func(*flowLayout){ + "lanes": func(l *flowLayout) { l.IfElse = IfElseLanes }, + "true-right-else-column": func(l *flowLayout) { l.IfElse = IfElseColumn }, + }, + value: func(l flowLayout) string { + if l.IfElse == IfElseColumn { + return "true-right-else-column" + } + return "lanes" + }, + defaultValue: "lanes", + }, + "case-split": { + values: map[string]func(*flowLayout){ + "lanes": func(l *flowLayout) { l.CaseSplit = CaseSplitLanes }, + "first-right-rest-columns": func(l *flowLayout) { l.CaseSplit = CaseSplitColumns }, + }, + value: func(l flowLayout) string { + if l.CaseSplit == CaseSplitColumns { + return "first-right-rest-columns" + } + return "lanes" + }, + defaultValue: "lanes", + }, + "wrap": { + values: map[string]func(*flowLayout){ + "rows": func(l *flowLayout) { l.Wrap = WrapRows }, + "off": func(l *flowLayout) { l.Wrap = WrapOff }, + }, + value: func(l flowLayout) string { + if l.Wrap == WrapOff { + return "off" + } + return "rows" + }, + defaultValue: "rows", + }, + "max-row-width": { + parse: func(l *flowLayout, v string) error { + n, err := strconv.Atoi(v) + if err != nil || n < minRowWidth || n > maxRowWidthLimit { + return fmt.Errorf("%q is not a width in px from %d to %d", v, minRowWidth, maxRowWidthLimit) + } + if n == MaxRowWidth { + n = 0 + } + l.MaxRowWidth = n + return nil + }, + value: func(l flowLayout) string { return strconv.Itoa(l.rowWidth()) }, + defaultValue: strconv.Itoa(MaxRowWidth), + }, + "guard": { + values: map[string]func(*flowLayout){ + "return-below": func(l *flowLayout) { l.Guard = GuardReturnBelow }, + "true-right": func(l *flowLayout) { l.Guard = GuardTrueRight }, + }, + value: func(l flowLayout) string { + if l.Guard == GuardTrueRight { + return "true-right" + } + return "return-below" + }, + defaultValue: "return-below", + }, +} + +func sortedLayoutKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// setFlowLayoutOption applies key=value to l, refusing an unknown key or value: +// a typo silently falling back to the default is exactly what the file exists +// to prevent. +func setFlowLayoutOption(l *flowLayout, key, value string) error { + opt, ok := flowLayoutOptions[key] + if !ok { + return fmt.Errorf("unknown layout option %q (known: %s)", key, strings.Join(sortedLayoutKeys(flowLayoutOptions), ", ")) + } + if opt.parse != nil { + if err := opt.parse(l, value); err != nil { + return fmt.Errorf("layout option %s: %w", key, err) + } + return nil + } + set, ok := opt.values[value] + if !ok { + return fmt.Errorf("layout option %s: unknown value %q (known: %s)", key, value, strings.Join(sortedLayoutKeys(opt.values), ", ")) + } + set(l) + return nil +} + +// FlowLayoutPreset resolves a preset name, for `--style`. +func FlowLayoutPreset(name string) (*flowLayout, error) { + l, ok := flowLayoutPresets[name] + if !ok { + return nil, fmt.Errorf("unknown layout preset %q (known: %s)", name, strings.Join(sortedLayoutKeys(flowLayoutPresets), ", ")) + } + return &l, nil +} + +// LoadFlowLayoutConfig reads a layout-config.yaml. An empty path is the default +// style. +func LoadFlowLayoutConfig(path string) (*flowLayout, error) { + if path == "" { + return &flowLayout{}, nil + } + data, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var file struct { + Flows map[string]string `yaml:"flows"` + } + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + if err := dec.Decode(&file); err != nil && !errors.Is(err, io.EOF) { + return nil, fmt.Errorf("%s: %w", path, err) + } + l := flowLayout{} + if name, ok := file.Flows["preset"]; ok { + p, err := FlowLayoutPreset(name) + if err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + l = *p + } + for _, key := range sortedLayoutKeys(file.Flows) { + if key == "preset" { + continue + } + if err := setFlowLayoutOption(&l, key, file.Flows[key]); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + } + return &l, nil +} + +// markerValue is what the marker line records for l: every option that differs +// from the default, `key=value`, in key order — so the same style always gives +// the same line and re-laying out a flow changes nothing. "" for the default. +func (l flowLayout) markerValue() string { + var parts []string + for _, key := range sortedLayoutKeys(flowLayoutOptions) { + opt := flowLayoutOptions[key] + if v := opt.value(l); v != opt.defaultValue { + parts = append(parts, key+"="+v) + } + } + return strings.Join(parts, "; ") +} + +// String names the style for messages: its marker value, or happy-line. +func (l flowLayout) String() string { + if v := l.markerValue(); v != "" { + return v + } + return "happy-line" +} + +// parseFlowLayoutMarker reads a marker value back. +func parseFlowLayoutMarker(value string) (flowLayout, error) { + l := flowLayout{} + for _, part := range strings.Split(value, ";") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + key, v, ok := strings.Cut(part, "=") + if !ok { + return flowLayout{}, fmt.Errorf("layout marker: %q is not key=value", part) + } + if err := setFlowLayoutOption(&l, strings.TrimSpace(key), strings.TrimSpace(v)); err != nil { + return flowLayout{}, err + } + } + return l, nil +} + +// isFlowLayoutMarkerLine reports whether a documentation line is the marker. +func isFlowLayoutMarkerLine(line string) bool { + return strings.HasPrefix(strings.TrimSpace(line), flowLayoutMarker) +} + +// flowLayoutFromDocumentation returns the style a flow's documentation records. +// found is false when there is no marker line. A marker that does not parse is +// reported as err, with found true; the caller warns and treats the flow as +// unmarked. +func flowLayoutFromDocumentation(doc string) (l flowLayout, found bool, err error) { + for line := range strings.SplitSeq(doc, "\n") { + if !isFlowLayoutMarkerLine(line) { + continue + } + value := strings.TrimPrefix(strings.TrimSpace(line), flowLayoutMarker) + l, err = parseFlowLayoutMarker(value) + return l, true, err + } + return flowLayout{}, false, nil +} + +// withFlowLayoutMarker returns doc with its marker line set for l: any marker +// line is removed, and one is appended for a style other than the default. The +// rest of the documentation is kept byte for byte; only blank lines the removed +// marker leaves at the end go with it. +func withFlowLayoutMarker(doc string, l flowLayout) string { + _, found, _ := flowLayoutFromDocumentation(doc) + value := l.markerValue() + if !found && value == "" { + return doc + } + out := doc + if found { + var kept []string + for line := range strings.SplitSeq(doc, "\n") { + if !isFlowLayoutMarkerLine(line) { + kept = append(kept, line) + } + } + out = strings.TrimRight(strings.Join(kept, "\n"), "\r\n\t ") + } + if value == "" { + return out + } + line := flowLayoutMarker + " " + value + if out == "" { + return line + } + return out + "\n" + line +} + +// ProjectFlowLayout returns the style new flows are laid out with: the +// override if one is set, else the project's layout-config.yaml, else the +// default. A config that does not load is an error for every flow built while it +// stands, never a silent fallback. +func (ctx *ExecContext) ProjectFlowLayout() (*flowLayout, error) { + if ctx == nil { + return &flowLayout{}, nil + } + if ctx.FlowLayoutOverride != nil { + return ctx.FlowLayoutOverride, nil + } + if !ctx.projectFlowLayoutLoaded { + ctx.projectFlowLayoutLoaded = true + path := "" + if ctx.MprPath != "" { + path = FindFlowLayoutConfig(filepath.Dir(ctx.MprPath)) + } + ctx.projectFlowLayout, ctx.projectFlowLayoutErr = LoadFlowLayoutConfig(path) + } + return ctx.projectFlowLayout, ctx.projectFlowLayoutErr +} + +// resolveFlowLayout decides the style a flow is built with from its final +// documentation, and returns the documentation with the marker set for that +// style. A marked flow keeps its recorded style; an unmarked one gets the +// project's. A marker that does not parse is warned about and treated as absent. +// +// keepRecorded false ignores the marker: `mxcli layout flows` re-lays a flow out +// in the project style (or --style), which is the point of running it. +func resolveFlowLayout(ctx *ExecContext, doc string, keepRecorded bool) (*flowLayout, string, error) { + if keepRecorded { + l, found, err := flowLayoutFromDocumentation(doc) + if found && err == nil { + return &l, withFlowLayoutMarker(doc, l), nil + } + if found && err != nil && ctx != nil && ctx.Output != nil { + fmt.Fprintf(ctx.Output, "Warning: ignoring the flow's %s line: %v\n", flowLayoutMarker, err) + } + } + l, err := ctx.ProjectFlowLayout() + if err != nil { + return nil, doc, fmt.Errorf("layout config: %w", err) + } + return l, withFlowLayoutMarker(doc, *l), nil +} + +// describingFlowLayout records, for the description of one flow, the style its +// documentation says it was laid out in, and returns the function that restores +// what was recorded before. The describer reads a shape only that style draws +// as the statement that drew it (flowLooksLikeGuardContinuation). +func (ctx *ExecContext) describingFlowLayout(doc string) func() { + if ctx == nil { + return func() {} + } + prev := ctx.describedFlowLayout + ctx.describedFlowLayout = nil + if l, found, err := flowLayoutFromDocumentation(doc); found && err == nil { + ctx.describedFlowLayout = &l + } + return func() { ctx.describedFlowLayout = prev } +} diff --git a/mdl/executor/flowlayout_config_test.go b/mdl/executor/flowlayout_config_test.go new file mode 100644 index 0000000000..6cc288ae7f --- /dev/null +++ b/mdl/executor/flowlayout_config_test.go @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// ako/mxcli#1832: where a flow's layout style comes from. + +func writeLayoutConfig(t *testing.T, dir, body string) string { + t.Helper() + p := filepath.Join(dir, flowLayoutConfigName) + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +func TestLoadFlowLayoutConfig(t *testing.T) { + for _, c := range []struct { + name, body string + want flowLayout + wantErr string + }{ + {"empty file is the default", "", flowLayout{}, ""}, + {"preset", "flows:\n preset: true-right-false-down\n", flowLayout{IfElse: IfElseColumn, CaseSplit: CaseSplitColumns}, ""}, + {"option", "flows:\n if-else: true-right-else-column\n", flowLayout{IfElse: IfElseColumn}, ""}, + {"option overrides preset", "flows:\n preset: true-right-false-down\n if-else: lanes\n", flowLayout{CaseSplit: CaseSplitColumns}, ""}, + {"unknown preset", "flows:\n preset: zigzag\n", flowLayout{}, `unknown layout preset "zigzag"`}, + {"unknown option", "flows:\n if-els: lanes\n", flowLayout{}, `unknown layout option "if-els"`}, + {"unknown value", "flows:\n if-else: column\n", flowLayout{}, `unknown value "column"`}, + {"unknown top-level key", "flow:\n preset: happy-line\n", flowLayout{}, "field flow not found"}, + } { + t.Run(c.name, func(t *testing.T) { + got, err := LoadFlowLayoutConfig(writeLayoutConfig(t, t.TempDir(), c.body)) + if c.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), c.wantErr) { + t.Fatalf("err = %v, want it to mention %q", err, c.wantErr) + } + return + } + if err != nil { + t.Fatal(err) + } + if *got != c.want { + t.Fatalf("got %+v, want %+v", *got, c.want) + } + }) + } +} + +func TestFindFlowLayoutConfig_PrefersClaudeDir(t *testing.T) { + dir := t.TempDir() + if got := FindFlowLayoutConfig(dir); got != "" { + t.Fatalf("no file: got %q", got) + } + root := writeLayoutConfig(t, dir, "") + if got := FindFlowLayoutConfig(dir); got != root { + t.Fatalf("got %q, want %q", got, root) + } + if err := os.Mkdir(filepath.Join(dir, ".claude"), 0o755); err != nil { + t.Fatal(err) + } + claude := writeLayoutConfig(t, filepath.Join(dir, ".claude"), "") + if got := FindFlowLayoutConfig(dir); got != claude { + t.Fatalf("got %q, want %q", got, claude) + } +} + +func TestFlowLayoutMarker(t *testing.T) { + column := flowLayout{IfElse: IfElseColumn} + const line = "@mxcli-layout: if-else=true-right-else-column" + + for _, c := range []struct { + name string + doc string + layout flowLayout + want string + }{ + {"default leaves undocumented flow alone", "", flowLayout{}, ""}, + {"default leaves documentation byte for byte", "Submits.\r\n trailing ", flowLayout{}, "Submits.\r\n trailing "}, + {"style on an undocumented flow", "", column, line}, + {"style appended", "Submits the order.", column, "Submits the order.\n" + line}, + {"style replaced, not duplicated", "Submits.\n" + line, column, "Submits.\n" + line}, + {"older marker replaced", "Submits.\n@mxcli-layout: if-else=lanes", column, "Submits.\n" + line}, + {"default removes the marker", "Submits.\n" + line, flowLayout{}, "Submits."}, + {"default removes a lone marker", line, flowLayout{}, ""}, + } { + t.Run(c.name, func(t *testing.T) { + got := withFlowLayoutMarker(c.doc, c.layout) + if got != c.want { + t.Fatalf("got %q, want %q", got, c.want) + } + // What is written reads back as the style it records. + back, found, err := flowLayoutFromDocumentation(got) + if err != nil || back != c.layout || found != (c.layout != flowLayout{}) { + t.Fatalf("read back %+v found=%v err=%v, want %+v", back, found, err, c.layout) + } + // Writing it again changes nothing (ADR-0008). + if again := withFlowLayoutMarker(got, c.layout); again != got { + t.Fatalf("second write %q differs from first %q", again, got) + } + }) + } +} + +func TestResolveFlowLayout(t *testing.T) { + dir := t.TempDir() + writeLayoutConfig(t, dir, "flows:\n preset: true-right-false-down\n") + ctx := &ExecContext{MprPath: filepath.Join(dir, "app.mpr"), Output: &strings.Builder{}} + + // Unmarked flow: the project's style, and the marker is written. + l, doc, err := resolveFlowLayout(ctx, "Submits.", true) + if err != nil || l.IfElse != IfElseColumn || !strings.HasSuffix(doc, "if-else=true-right-else-column") { + t.Fatalf("unmarked: %+v %q %v", l, doc, err) + } + + // A flow recording the default (no line can say so) is unmarked; one recording + // a style keeps it whatever the project says. + ctx2 := &ExecContext{MprPath: filepath.Join(t.TempDir(), "app.mpr"), Output: &strings.Builder{}} + l, doc, err = resolveFlowLayout(ctx2, "Submits.\n@mxcli-layout: if-else=true-right-else-column", true) + if err != nil || l.IfElse != IfElseColumn || !strings.Contains(doc, flowLayoutMarker) { + t.Fatalf("recorded style in a default project: %+v %q %v", l, doc, err) + } + + // layout flows ignores the record and re-styles. + l, doc, err = resolveFlowLayout(ctx2, "Submits.\n@mxcli-layout: if-else=true-right-else-column", false) + if err != nil || l.IfElse != IfElseLanes || doc != "Submits." { + t.Fatalf("re-style to default: %+v %q %v", l, doc, err) + } + + // An unreadable marker is warned about and treated as absent — never an error. + out := &strings.Builder{} + ctx3 := &ExecContext{MprPath: filepath.Join(t.TempDir(), "app.mpr"), Output: out} + l, doc, err = resolveFlowLayout(ctx3, "Submits.\n@mxcli-layout: zigzag=yes", true) + if err != nil || l.IfElse != IfElseLanes || doc != "Submits." || !strings.Contains(out.String(), "Warning") { + t.Fatalf("bad marker: %+v %q %v, output %q", l, doc, err, out.String()) + } + + // A broken project config is an error, not a silent default. + bad := t.TempDir() + writeLayoutConfig(t, bad, "flows:\n preset: zigzag\n") + if _, _, err := resolveFlowLayout(&ExecContext{MprPath: filepath.Join(bad, "app.mpr")}, "", true); err == nil { + t.Fatal("broken config: no error") + } +} diff --git a/mdl/executor/flowlayout_describe_test.go b/mdl/executor/flowlayout_describe_test.go new file mode 100644 index 0000000000..cded7f2505 --- /dev/null +++ b/mdl/executor/flowlayout_describe_test.go @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + "testing" +) + +// ako/mxcli#1832: a flow laid out in a style records it, and a canonical +// DESCRIBE judges its positions against that style — whatever style the project +// is in when it is described. With the record gone the same description keeps +// every position it can no longer derive, which is the control. + +const styledFlowsMDL = ` +create microflow MyFirstModule.ACT_Styled ($N: Integer) returns Boolean +begin + log info 'start'; + if $N < 0 then + log info 'negative'; + return false; + end if; + if $N > 100 then + log info 'too big'; + return false; + end if; + if $N > 10 then + log info 'big'; + else + log info 'small 1'; + log info 'small 2'; + end if; + log info 'ok'; + return true; +end; + +create nanoflow MyFirstModule.NAV_Styled ($N: Integer) returns Boolean +begin + if $N < 0 then + return false; + end if; + if $N > 10 then + log info 'big'; + else + log info 'small 1'; + log info 'small 2'; + end if; + return true; +end; +` + +func TestDescribeUsesTheRecordedLayoutStyle(t *testing.T) { + for _, preset := range []string{"true-right-false-down"} { + for _, guard := range []bool{false, true} { + style := preset + if guard { + style += "+guard" + } + t.Run(style, func(t *testing.T) { + exec, out := describeExecutor(t) + if err := exec.SetFlowLayoutStyle(preset); err != nil { + t.Fatal(err) + } + if guard { + l := *exec.flowLayoutOverride + l.Guard = GuardTrueRight + exec.flowLayoutOverride = &l + } + run(t, exec, styledFlowsMDL) + // The project is back on the default style; the flows record theirs. + if err := exec.SetFlowLayoutStyle(""); err != nil { + t.Fatal(err) + } + for _, d := range []string{"describe microflow MyFirstModule.ACT_Styled", "describe nanoflow MyFirstModule.NAV_Styled"} { + mdl := describeText(t, exec, out, d) + if !strings.Contains(mdl, flowLayoutMarker) { + t.Fatalf("%s: no %s line:\n%s", d, flowLayoutMarker, mdl) + } + if got := layoutLines(mdl); len(got) > 0 { + t.Errorf("%s printed layout annotations under the recorded style: %v\n%s", d, got, mdl) + } + } + }) + } + } +} + +const styledCaseSplitsMDL = ` +create or modify persistent entity MyFirstModule.Animal ( "Label": String(50) ); +create or modify persistent entity MyFirstModule.Dog generalization MyFirstModule.Animal (); +create or modify persistent entity MyFirstModule.Cat generalization MyFirstModule.Animal (); + +create microflow MyFirstModule.ACT_Cases ($P: NanoflowCommons.GeocodingProvider, $A: MyFirstModule.Animal) returns String +begin + case $P + when MapQuest then + log info 'map 1'; + log info 'map 2'; + when Google then + log info 'google'; + when LocationIQ then + log info 'iq 1'; + log info 'iq 2'; + log info 'iq 3'; + when Geocodio then + log info 'geocodio'; + when (empty) then + log info 'none'; + end case; + split type $A + when MyFirstModule.Dog then + log info 'dog'; + when MyFirstModule.Cat then + log info 'cat 1'; + log info 'cat 2'; + when MyFirstModule.Animal then + log info 'animal'; + end split; + return 'done'; +end; +` + +func TestDescribeKeepsCaseOrderInCaseColumns(t *testing.T) { + exec, out := describeExecutor(t) + if err := exec.SetFlowLayoutStyle("true-right-false-down"); err != nil { + t.Fatal(err) + } + run(t, exec, styledCaseSplitsMDL) + if err := exec.SetFlowLayoutStyle(""); err != nil { + t.Fatal(err) + } + mdl := describeText(t, exec, out, "describe microflow MyFirstModule.ACT_Cases") + if got := layoutLines(mdl); len(got) > 0 { + t.Errorf("printed layout annotations under the recorded style: %v\n%s", got, mdl) + } + // The cases come back in the order they were written, not the order a + // column's sides would sort them in. + last := -1 + for _, want := range []string{"when MapQuest", "when Google", "when LocationIQ", "when Geocodio", "when (empty)", "when MyFirstModule.Dog", "when MyFirstModule.Cat", "when MyFirstModule.Animal"} { + i := strings.Index(mdl, want) + if i < 0 || i < last { + t.Fatalf("%q missing or out of order in:\n%s", want, mdl) + } + last = i + } +} + +func TestDescribeUsesTheRecordedWrap(t *testing.T) { + var body strings.Builder + for i := 0; i < 30; i++ { + fmt.Fprintf(&body, " log info 'step %d';\n", i) + } + mdl := "create microflow MyFirstModule.ACT_Long () returns Boolean begin\n" + body.String() + " return true;\nend;\n" + for _, l := range []flowLayout{{}, {Wrap: WrapOff}, {MaxRowWidth: 1200}} { + t.Run(l.String(), func(t *testing.T) { + exec, out := describeExecutor(t) + style := l + exec.flowLayoutOverride = &style + run(t, exec, mdl) + exec.flowLayoutOverride = nil + got := describeText(t, exec, out, "describe microflow MyFirstModule.ACT_Long") + if l.markerValue() != "" && !strings.Contains(got, flowLayoutMarker+" "+l.markerValue()) { + t.Fatalf("no marker for %s:\n%s", l, got) + } + if lines := layoutLines(got); len(lines) > 0 { + t.Errorf("printed layout annotations under the recorded style: %v", lines) + } + }) + } +} diff --git a/mdl/executor/flowlayout_guard_test.go b/mdl/executor/flowlayout_guard_test.go new file mode 100644 index 0000000000..1c8aa5d3ee --- /dev/null +++ b/mdl/executor/flowlayout_guard_test.go @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#1832: `guard: true-right` sends a guard's TRUE branch — the early +// return — straight on to the right, and the happy path down from the split's +// bottom onto a new line starting in the split's column. + +func guardRightLayout() *flowLayout { return &flowLayout{Guard: GuardTrueRight} } + +func TestGuardTrueRight_ReturnOnTheLineHappyPathBelow(t *testing.T) { + stmts := append(logStatements(1), guardStmt()) + stmts = append(stmts, logStatements(2)...) + fb := buildWithLayout(t, stmts, guardRightLayout()) + + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + sp := split.GetPosition() + acts := activityPositions(fb.objects) + if len(acts) != 4 { + t.Fatalf("want 4 activities, got %d", len(acts)) + } + before, branch, after := acts[0], acts[1], acts[2:] + + if before.Y != sp.Y || branch.Y != sp.Y || branch.X <= sp.X { + t.Errorf("guard branch at %v, want on the centre line y=%d right of the split at x=%d", branch, sp.Y, sp.X) + } + if after[0].X != sp.X { + t.Errorf("first activity after the guard at x=%d, want the split's column x=%d", after[0].X, sp.X) + } + if after[1].Y != after[0].Y || after[1].X <= after[0].X { + t.Errorf("happy path after the guard %v is not a horizontal line", after) + } + // The new line clears everything the guard drew, end event included. + var lowest int + for _, o := range fb.objects { + if o.GetPosition().Y == sp.Y { + lowest = max(lowest, o.GetPosition().Y+sizeOf(o).Height/2) + } + } + if top := after[0].Y - ActivityHeight/2; top-lowest != RowGap { + t.Errorf("new line's top y=%d is %d px under the guard's line (bottom %d), want RowGap=%d", top, top-lowest, lowest, RowGap) + } + + ids := activityIDs(fb.objects) + if f := flowBetween(fb.flows, split.GetID(), ids[1]); f == nil || f.OriginConnectionIndex != AnchorRight || f.DestinationConnectionIndex != AnchorLeft { + t.Errorf("TRUE flow = %+v, want split right -> branch left", f) + } + f := flowBetween(fb.flows, split.GetID(), ids[2]) + if f == nil || f.OriginConnectionIndex != AnchorBottom || f.DestinationConnectionIndex != AnchorTop { + t.Errorf("FALSE flow = %+v, want split bottom -> top of the next activity", f) + } + if ec, ok := f.CaseValue.(*microflows.ExpressionCase); f != nil && (!ok || ec.Expression != "false") { + t.Errorf("FALSE flow case = %+v, want false", f.CaseValue) + } + assertNoOverlap(t, fb.objects) +} + +func TestGuardTrueRight_DefaultLayoutIsUnchanged(t *testing.T) { + // Control: without the option the branch is in the lane below and the happy + // path stays on the centre line. + stmts := append([]ast.MicroflowStatement{guardStmt()}, logStatements(1)...) + fb := buildWithLayout(t, stmts, &flowLayout{}) + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + acts := activityPositions(fb.objects) + if acts[0].Y <= split.GetPosition().Y || acts[1].Y != split.GetPosition().Y { + t.Fatalf("default guard: branch at %v, happy path at %v, split at %v", acts[0], acts[1], split.GetPosition()) + } +} + +func TestGuardTrueRight_InsideAnIfBranchKeepsTheElseLaneClear(t *testing.T) { + // A guard in a THEN body moves the rest of that body down; the ELSE lane under + // it must be placed below all of it, which takes the measurer knowing. + thenBody := append([]ast.MicroflowStatement{guardStmt()}, logStatements(2)...) + stmts := []ast.MicroflowStatement{ifElse(thenBody, logStatements(2))} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, guardRightLayout()) + assertNoOverlap(t, fb.objects) + + // The guard's new line belongs to the THEN body: after the IF the flow is + // back on the centre line, its end event too. + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + acts := activityPositions(fb.objects) + if last := acts[len(acts)-1]; last.Y != split.GetPosition().Y { + t.Errorf("activity after the IF at %v, want the centre line y=%d", last, split.GetPosition().Y) + } + end := fb.objects[len(fb.objects)-1] + if _, ok := end.(*microflows.EndEvent); !ok || end.GetPosition().Y != split.GetPosition().Y { + t.Errorf("flow's end event %T at %v, want an end event on the centre line y=%d", end, end.GetPosition(), split.GetPosition().Y) + } +} + +func TestGuardTrueRight_SeveralGuardsStepDown(t *testing.T) { + stmts := []ast.MicroflowStatement{guardStmt(), guardStmt(), guardStmt()} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, guardRightLayout()) + var ys []int + for _, o := range fb.objects { + if _, ok := o.(*microflows.ExclusiveSplit); ok { + ys = append(ys, o.GetPosition().Y) + } + } + if len(ys) != 3 || !(ys[0] < ys[1] && ys[1] < ys[2]) { + t.Fatalf("guard splits at y=%v, want each on its own line below the last", ys) + } + assertNoOverlap(t, fb.objects) +} + +func TestGuardTrueRight_InsideALoopFitsTheBox(t *testing.T) { + body := append([]ast.MicroflowStatement{guardStmt()}, logStatements(2)...) + loop := &ast.LoopStmt{LoopVariable: "Item", ListVariable: "Items", Body: body} + fb := buildWithLayout(t, []ast.MicroflowStatement{loop}, guardRightLayout()) + l, ok := firstOfType[*microflows.LoopedActivity](fb.objects) + if !ok { + t.Fatal("no loop") + } + ls := sizeOf(l) + for _, o := range l.ObjectCollection.Objects { + p, s := o.GetPosition(), sizeOf(o) + if p.Y+s.Height/2 > ls.Height || p.X+s.Width/2 > ls.Width || p.Y-s.Height/2 < 0 { + t.Errorf("%T at %v sticks out of the loop box %v", o, p, ls) + } + } + assertNoOverlap(t, l.ObjectCollection.Objects) +} + +func TestGuardTrueRight_CombinesWithElseColumn(t *testing.T) { + elseBody := append([]ast.MicroflowStatement{guardStmt()}, logStatements(2)...) + stmts := []ast.MicroflowStatement{ifElse(logStatements(1), elseBody)} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, &flowLayout{IfElse: IfElseColumn, Guard: GuardTrueRight}) + assertNoOverlap(t, fb.objects) +} diff --git a/mdl/executor/flowlayout_ifelse_column_test.go b/mdl/executor/flowlayout_ifelse_column_test.go new file mode 100644 index 0000000000..20c7463650 --- /dev/null +++ b/mdl/executor/flowlayout_ifelse_column_test.go @@ -0,0 +1,335 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ako/mxcli#1832: `if-else: true-right-else-column` keeps the TRUE body on the +// centre line and stacks the ELSE body top to bottom in a column under the split. + +// buildWithLayout lays statements out the way buildRows does, under a style. +func buildWithLayout(t *testing.T, stmts []ast.MicroflowStatement, layout *flowLayout) *flowBuilder { + t.Helper() + fb := &flowBuilder{ + posX: 200, + posY: 200, + baseY: 200, + spacing: HorizontalSpacing, + allowWrap: true, + varTypes: map[string]string{}, + declaredVars: map[string]string{}, + measurer: &layoutMeasurer{varTypes: map[string]string{}}, + layout: layout, + } + fb.buildFlowGraph(stmts, nil) + return fb +} + +func columnLayout() *flowLayout { return &flowLayout{IfElse: IfElseColumn} } + +func ifElse(thenBody, elseBody []ast.MicroflowStatement) *ast.IfStmt { + return &ast.IfStmt{ + Condition: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}, + ThenBody: thenBody, + ElseBody: elseBody, + HasElse: true, + } +} + +func objectByIDIn(objects []microflows.MicroflowObject, id model.ID) microflows.MicroflowObject { + for _, o := range objects { + if o.GetID() == id { + return o + } + } + return nil +} + +func sizeOf(o microflows.MicroflowObject) model.Size { + if ws, ok := o.(interface{ GetSize() model.Size }); ok { + if s := ws.GetSize(); s.Width > 0 && s.Height > 0 { + return s + } + } + return model.Size{Width: ActivityWidth, Height: ActivityHeight} +} + +// flowsWithCase returns the flows leaving origin with the given boolean case. +func flowFrom(flows []*microflows.SequenceFlow, origin model.ID, caseValue string) *microflows.SequenceFlow { + for _, f := range flows { + if f.OriginID != origin { + continue + } + ec, ok := f.CaseValue.(*microflows.ExpressionCase) + if caseValue == "" && f.CaseValue == nil { + return f + } + if ok && ec.Expression == caseValue { + return f + } + } + return nil +} + +func flowBetween(flows []*microflows.SequenceFlow, origin, dest model.ID) *microflows.SequenceFlow { + for _, f := range flows { + if f.OriginID == origin && f.DestinationID == dest { + return f + } + } + return nil +} + +func firstOfType[T microflows.MicroflowObject](objects []microflows.MicroflowObject) (T, bool) { + for _, o := range objects { + if v, ok := o.(T); ok { + return v, true + } + } + var zero T + return zero, false +} + +// assertNoOverlap fails when two placed objects' boxes intersect. A loop box +// contains its body, so an object inside a loop box is not an overlap; anything +// else inside another box is. +func assertNoOverlap(t *testing.T, objects []microflows.MicroflowObject) { + t.Helper() + type box struct{ l, t, r, b int } + boxOf := func(o microflows.MicroflowObject) box { + p, s := o.GetPosition(), sizeOf(o) + return box{p.X - s.Width/2, p.Y - s.Height/2, p.X + s.Width/2, p.Y + s.Height/2} + } + contains := func(a, b box) bool { return a.l <= b.l && a.t <= b.t && a.r >= b.r && a.b >= b.b } + for i := range objects { + for j := i + 1; j < len(objects); j++ { + a, b := boxOf(objects[i]), boxOf(objects[j]) + _, iLoop := objects[i].(*microflows.LoopedActivity) + _, jLoop := objects[j].(*microflows.LoopedActivity) + if (iLoop && contains(a, b)) || (jLoop && contains(b, a)) { + continue + } + if a.l < b.r && b.l < a.r && a.t < b.b && b.t < a.b { + t.Errorf("%T at %v overlaps %T at %v", objects[i], objects[i].GetPosition(), objects[j], objects[j].GetPosition()) + } + } + } +} + +func TestIfElseColumn_ElseBodyIsAColumnUnderTheSplit(t *testing.T) { + stmts := []ast.MicroflowStatement{ifElse(logStatements(1), logStatements(3))} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, columnLayout()) + + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + merge, ok := firstOfType[*microflows.ExclusiveMerge](fb.objects) + if !ok { + t.Fatal("no merge") + } + acts := activityPositions(fb.objects) + if len(acts) != 5 { + t.Fatalf("want 5 activities, got %d", len(acts)) + } + then, column, after := acts[0], acts[1:4], acts[4] + sp := split.GetPosition() + + if then.Y != sp.Y { + t.Errorf("TRUE body at y=%d, want the centre line y=%d", then.Y, sp.Y) + } + for i, p := range column { + if p.X != sp.X { + t.Errorf("ELSE statement %d at x=%d, want the split's column x=%d", i, p.X, sp.X) + } + } + if top := column[0].Y - ActivityHeight/2; top < then.Y+ActivityHeight/2+BranchGap { + t.Errorf("first ELSE statement's top y=%d is not clear of the TRUE body (bottom %d)", top, then.Y+ActivityHeight/2) + } + for i := 1; i < len(column); i++ { + if gap := (column[i].Y - ActivityHeight/2) - (column[i-1].Y + ActivityHeight/2); gap != RowGap { + t.Errorf("ELSE statements %d and %d are %d px apart edge to edge, want RowGap=%d", i-1, i, gap, RowGap) + } + } + mp := merge.GetPosition() + if mp.Y != sp.Y { + t.Errorf("merge at y=%d, want the centre line y=%d", mp.Y, sp.Y) + } + if mp.X-MergeSize/2 <= column[0].X+ActivityWidth/2 || mp.X-MergeSize/2 <= then.X+ActivityWidth/2 { + t.Errorf("merge at x=%d is not right of both branches", mp.X) + } + if after.Y != sp.Y || after.X <= mp.X { + t.Errorf("activity after the IF at %v, want on the centre line past the merge at %v", after, mp) + } + + // Exits: TRUE right, FALSE bottom into the top of the column; the column runs + // bottom to top; its last element rejoins the merge from below. + if f := flowFrom(fb.flows, split.GetID(), "true"); f == nil || f.OriginConnectionIndex != AnchorRight { + t.Errorf("TRUE flow = %+v, want it to leave the split's right side", f) + } + f := flowFrom(fb.flows, split.GetID(), "false") + if f == nil || f.OriginConnectionIndex != AnchorBottom || f.DestinationConnectionIndex != AnchorTop { + t.Errorf("FALSE flow = %+v, want bottom of split -> top of the first ELSE statement", f) + } + ids := activityIDs(fb.objects) + for i := 1; i < 3; i++ { + f := flowBetween(fb.flows, ids[i], ids[i+1]) + if f == nil || f.OriginConnectionIndex != AnchorBottom || f.DestinationConnectionIndex != AnchorTop { + t.Errorf("column flow %d = %+v, want bottom -> top", i, f) + } + } + if f := flowBetween(fb.flows, ids[3], merge.GetID()); f == nil || f.OriginConnectionIndex != AnchorRight || f.DestinationConnectionIndex != AnchorBottom { + t.Errorf("column -> merge flow = %+v, want right -> bottom", f) + } + assertNoOverlap(t, fb.objects) +} + +func TestIfElseColumn_DefaultLayoutIsUnchanged(t *testing.T) { + // Control: without the style the ELSE body is a horizontal lane, as before. + stmts := []ast.MicroflowStatement{ifElse(logStatements(1), logStatements(3))} + for _, layout := range []*flowLayout{nil, {}} { + fb := buildWithLayout(t, stmts, layout) + acts := activityPositions(fb.objects) + if acts[1].Y != acts[2].Y || acts[2].Y != acts[3].Y || acts[1].X >= acts[2].X { + t.Fatalf("layout %v: ELSE body %v is not a horizontal lane", layout, acts[1:]) + } + want := buildRows(t, stmts) + for i, o := range fb.objects { + if o.GetPosition() != want[i].GetPosition() { + t.Fatalf("layout %v: object %d at %v, default builder puts it at %v", layout, i, o.GetPosition(), want[i].GetPosition()) + } + } + } +} + +func TestIfElseColumn_EmptyThenRunsStraightToTheMerge(t *testing.T) { + fb := buildWithLayout(t, []ast.MicroflowStatement{ifElse(nil, logStatements(2))}, columnLayout()) + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + merge, ok := firstOfType[*microflows.ExclusiveMerge](fb.objects) + if !ok { + t.Fatal("no merge") + } + if merge.GetPosition().Y != split.GetPosition().Y { + t.Errorf("merge at %v is not on the centre line of the split at %v", merge.GetPosition(), split.GetPosition()) + } + f := flowBetween(fb.flows, split.GetID(), merge.GetID()) + if f == nil || f.OriginConnectionIndex != AnchorRight || f.DestinationConnectionIndex != AnchorLeft { + t.Errorf("TRUE flow = %+v, want split right -> merge left", f) + } + assertNoOverlap(t, fb.objects) +} + +func TestIfElseColumn_ElseReturnEndsAtTheBottomOfTheColumn(t *testing.T) { + elseBody := append(logStatements(2), &ast.ReturnStmt{}) + fb := buildWithLayout(t, []ast.MicroflowStatement{ifElse(logStatements(1), elseBody)}, columnLayout()) + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + acts := activityPositions(fb.objects) + last := acts[len(acts)-1] + var end model.Point + found := false + for _, o := range fb.objects { + if e, ok := o.(*microflows.EndEvent); ok && e.GetPosition().Y > split.GetPosition().Y { + end, found = e.GetPosition(), true + } + } + if !found { + t.Fatal("no end event under the centre line") + } + if end.X != split.GetPosition().X || end.Y <= last.Y { + t.Errorf("ELSE's end event at %v, want in the column under the last ELSE activity at %v", end, last) + } + assertNoOverlap(t, fb.objects) +} + +func TestIfElseColumn_NestedIfInTheColumnDoesNotOverlap(t *testing.T) { + inner := ifElse(logStatements(2), logStatements(2)) + elseBody := []ast.MicroflowStatement{inner} + elseBody = append(elseBody, logStatements(2)...) + stmts := []ast.MicroflowStatement{ifElse(logStatements(1), elseBody)} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, columnLayout()) + assertNoOverlap(t, fb.objects) + + // The outer merge clears the inner IF, which reaches right of the column. + var splits []microflows.MicroflowObject + var merges []microflows.MicroflowObject + for _, o := range fb.objects { + switch o.(type) { + case *microflows.ExclusiveSplit: + splits = append(splits, o) + case *microflows.ExclusiveMerge: + merges = append(merges, o) + } + } + if len(splits) != 2 || len(merges) != 2 { + t.Fatalf("want 2 splits and 2 merges, got %d and %d", len(splits), len(merges)) + } + outerMerge := merges[0] + for _, o := range fb.objects { + if o == outerMerge { + continue + } + if o.GetPosition().Y > splits[0].GetPosition().Y && o.GetPosition().X+sizeOf(o).Width/2 >= outerMerge.GetPosition().X { + t.Errorf("%T at %v in the column reaches past the outer merge at %v", o, o.GetPosition(), outerMerge.GetPosition()) + } + } +} + +func activityIDs(objects []microflows.MicroflowObject) []model.ID { + var out []model.ID + for _, o := range objects { + if _, ok := o.(*microflows.ActionActivity); ok { + out = append(out, o.GetID()) + } + } + return out +} + +func TestIfElseColumn_LoopAndGuardInTheColumnDoNotOverlap(t *testing.T) { + loop := &ast.LoopStmt{LoopVariable: "Item", ListVariable: "Items", Body: logStatements(2)} + elseBody := []ast.MicroflowStatement{loop, guardStmt()} + elseBody = append(elseBody, logStatements(1)...) + inner := ifElse(logStatements(1), logStatements(2)) // a column inside the column + elseBody = append(elseBody, inner) + stmts := []ast.MicroflowStatement{ifElse(logStatements(2), elseBody)} + stmts = append(stmts, logStatements(1)...) + fb := buildWithLayout(t, stmts, columnLayout()) + assertNoOverlap(t, fb.objects) + + // Every statement of the column starts under the previous one. + split, _ := firstOfType[*microflows.ExclusiveSplit](fb.objects) + loopObj, ok := firstOfType[*microflows.LoopedActivity](fb.objects) + if !ok { + t.Fatal("no loop") + } + if loopObj.GetPosition().Y <= split.GetPosition().Y { + t.Errorf("loop at %v is not under the split at %v", loopObj.GetPosition(), split.GetPosition()) + } +} + +func TestIfElseColumn_AppliesInsideALoopBody(t *testing.T) { + // A loop body is built by a nested builder; it must draw in the flow's style. + loop := &ast.LoopStmt{LoopVariable: "Item", ListVariable: "Items", + Body: []ast.MicroflowStatement{ifElse(logStatements(1), logStatements(2))}} + fb := buildWithLayout(t, []ast.MicroflowStatement{loop}, columnLayout()) + l, ok := firstOfType[*microflows.LoopedActivity](fb.objects) + if !ok { + t.Fatal("no loop") + } + body := l.ObjectCollection.Objects + split, _ := firstOfType[*microflows.ExclusiveSplit](body) + acts := activityPositions(body) + if len(acts) != 3 || acts[1].X != split.GetPosition().X || acts[2].X != split.GetPosition().X || acts[2].Y <= acts[1].Y { + t.Fatalf("ELSE body in the loop at %v, want a column under the split at %v", acts, split.GetPosition()) + } + ls := sizeOf(l) + for _, p := range acts { + if p.Y+ActivityHeight/2 > ls.Height || p.X+ActivityWidth/2 > ls.Width { + t.Errorf("activity at %v sticks out of the loop box %v", p, ls) + } + } +} diff --git a/mdl/executor/flowlayout_wrap_test.go b/mdl/executor/flowlayout_wrap_test.go new file mode 100644 index 0000000000..7b31f8d69b --- /dev/null +++ b/mdl/executor/flowlayout_wrap_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "path/filepath" + "testing" +) + +// ako/mxcli#1832: `wrap: off` keeps the main line on one row however long it +// gets; `max-row-width` moves where a row wraps. + +func rowYs(t *testing.T, layout *flowLayout, n int) map[int]int { + t.Helper() + fb := buildWithLayout(t, logStatements(n), layout) + ys := map[int]int{} + for _, p := range activityPositions(fb.objects) { + ys[p.Y]++ + } + return ys +} + +func TestWrap_DefaultWrapsALongFlow(t *testing.T) { + // Control: 40 activities are far past MaxRowWidth and wrap by default. + if ys := rowYs(t, nil, 40); len(ys) < 2 { + t.Fatalf("default layout put 40 activities on %d row(s), want them wrapped", len(ys)) + } +} + +func TestWrap_OffKeepsOneRow(t *testing.T) { + if ys := rowYs(t, &flowLayout{Wrap: WrapOff}, 40); len(ys) != 1 { + t.Fatalf("wrap: off put 40 activities on %d rows, want 1", len(ys)) + } +} + +func TestWrap_MaxRowWidthMovesTheBreak(t *testing.T) { + def := rowYs(t, nil, 40) + narrow := rowYs(t, &flowLayout{MaxRowWidth: 1200}, 40) + if len(narrow) <= len(def) { + t.Fatalf("max-row-width 1200 gave %d rows, default %d: want more rows", len(narrow), len(def)) + } + fb := buildWithLayout(t, logStatements(40), &flowLayout{MaxRowWidth: 1200}) + acts := activityPositions(fb.objects) + rowStart := acts[0].X + for _, p := range acts { + if p.X-rowStart > 1200+RowOverhang { + t.Errorf("activity at x=%d is %d px into its row, past max-row-width 1200", p.X, p.X-rowStart) + } + } + assertNoOverlap(t, fb.objects) +} + +func TestWrap_Config(t *testing.T) { + dir := t.TempDir() + l, err := LoadFlowLayoutConfig(writeLayoutConfig(t, dir, "flows:\n wrap: off\n max-row-width: 2000\n")) + if err != nil { + t.Fatal(err) + } + if l.Wrap != WrapOff || l.MaxRowWidth != 2000 { + t.Fatalf("got %+v", *l) + } + if got, want := l.markerValue(), "max-row-width=2000; wrap=off"; got != want { + t.Fatalf("marker %q, want %q", got, want) + } + back, err := parseFlowLayoutMarker(l.markerValue()) + if err != nil || back != *l { + t.Fatalf("marker reads back as %+v, %v", back, err) + } + // The default width is the default: it is not recorded. + if v := (flowLayout{MaxRowWidth: MaxRowWidth}).markerValue(); v != "" { + t.Fatalf("explicit default width recorded as %q", v) + } + for _, bad := range []string{"wide", "-5", "100"} { + if _, err := LoadFlowLayoutConfig(writeLayoutConfig(t, filepath.Join(t.TempDir()), "flows:\n max-row-width: "+bad+"\n")); err == nil { + t.Errorf("max-row-width %q accepted", bad) + } + } +} diff --git a/mdl/executor/issue1640_stale_list_marker_test.go b/mdl/executor/issue1640_stale_list_marker_test.go new file mode 100644 index 0000000000..50491ce5b1 --- /dev/null +++ b/mdl/executor/issue1640_stale_list_marker_test.go @@ -0,0 +1,260 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + modelsdkbackend "github.com/mendixlabs/mxcli/mdl/backend/modelsdk" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" +) + +// ako/mxcli#1640: a Data grid 2 that an older mxcli stored with +// SortBar.SortItems marker 3 kept it through every rewrite, where Studio Pro +// and today's writer both write 2: +// +// Home_Web mxcli=3 SP=2 <-- the old page +// ZZ_A mxcli=2 SP=2 +// +// "drop page + re-run the same create or replace page MDL" gave 2, so only an +// existing document kept it — also when the rewrite changed the page for real +// (a dynamictext added). An unchanged pluggable widget passes its stored Type +// and Object through byte for byte, and the stored list with them. + +const issue1640Setup = `create persistent entity MyFirstModule.Thing (Name: String(100));` + +const issue1640Page = `create or modify page MyFirstModule.Things1640 (Title: 'Things', Layout: Atlas_Core.Atlas_Default) { + container ctMain { + datagrid dgThings (DataSource: database from MyFirstModule.Thing sort by Name asc) { + column (Attribute: Name, Caption: 'Name') + } + } +};` + +// listMarkers returns the marker of every stored list under key. +func listMarkers(doc bson.D, key string) []int32 { + var got []int32 + var walk func(any) + walk = func(v any) { + switch n := v.(type) { + case bson.D: + for _, e := range n { + if a, ok := e.Value.(bson.A); ok && e.Key == key && len(a) > 0 { + if m, ok := a[0].(int32); ok { + got = append(got, m) + } + } + walk(e.Value) + } + case bson.A: + for _, e := range n { + walk(e) + } + } + } + walk(doc) + return got +} + +// setListMarkers sets the marker of every list under key to m. +func setListMarkers(doc bson.D, key string, m int32) int { + n := 0 + var walk func(any) + walk = func(v any) { + switch d := v.(type) { + case bson.D: + for i := range d { + if a, ok := d[i].Value.(bson.A); ok && d[i].Key == key && len(a) > 0 { + a[0] = m + n++ + } + walk(d[i].Value) + } + case bson.A: + for _, e := range d { + walk(e) + } + } + } + walk(doc) + return n +} + +// markerFixture is pageFixtureExecutor keeping the project path, so a test can +// change a stored unit behind the writer's back. +func markerFixture(t *testing.T) (*Executor, *bytes.Buffer, string) { + t.Helper() + var out bytes.Buffer + path := projectFixture(t) + exec := New(&out) + exec.SetBackendFactory(func() backend.FullBackend { return modelsdkbackend.New() }) + t.Cleanup(func() { exec.Close() }) + run(t, exec, "CONNECT LOCAL '"+visitor.QuoteString(path)+"'") + out.Reset() + return exec, &out, path +} + +// plantStoredUnit replaces a unit's file with doc, as an older mxcli left it. +// Every write gives a registered list its marker, so the write path cannot +// plant a stale one; the file is written directly, then the project reopened. +func plantStoredUnit(t *testing.T, exec *Executor, path, unitID string, doc bson.D) { + t.Helper() + stored, err := exec.backend.GetRawUnitBytes(model.ID(unitID)) + if err != nil { + t.Fatal(err) + } + raw, err := bson.Marshal(doc) + if err != nil { + t.Fatal(err) + } + var file string + err = filepath.WalkDir(filepath.Join(filepath.Dir(path), "mprcontents"), func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(p, ".mxunit") { + return err + } + if b, err := os.ReadFile(p); err == nil && bytes.Equal(b, stored) { + file = p + } + return nil + }) + if err != nil || file == "" { + t.Fatalf("no file holds unit %s (%v)", unitID, err) + } + if err := os.WriteFile(file, raw, 0o644); err != nil { + t.Fatal(err) + } + if err := exec.Execute(&ast.DisconnectStmt{}); err != nil { + t.Fatalf("disconnect: %v", err) + } + if err := exec.Execute(&ast.ConnectStmt{Path: path}); err != nil { + t.Fatalf("reconnect: %v", err) + } +} + +// plantStaleSortItems stores the page's SortItems with marker 3. +func plantStaleSortItems(t *testing.T, exec *Executor, path, id string) { + t.Helper() + doc := storedUnitDoc(t, exec, id) + if got := listMarkers(doc, "SortItems"); len(got) != 1 || got[0] != 2 { + t.Fatalf("a fresh page stores SortItems %v, want [2]", got) + } + setListMarkers(doc, "SortItems", 3) + plantStoredUnit(t, exec, path, id, doc) + if got := listMarkers(storedUnitDoc(t, exec, id), "SortItems"); len(got) != 1 || got[0] != 3 { + t.Fatalf("the planted page stores SortItems %v, want [3]", got) + } +} + +// The report's scenario: the page is changed for real, the grid is not. +func TestIssue1640_ARewriteGivesAStoredListStudioProsMarker(t *testing.T) { + exec, _, path := markerFixture(t) + run(t, exec, issue1640Setup) + run(t, exec, issue1640Page) + id := unitIDByName(t, exec, "page", "Things1640") + plantStaleSortItems(t, exec, path, id) + + run(t, exec, strings.Replace(issue1640Page, " container ctMain {", " dynamictext txtNew (Content: 'new')\n container ctMain {", 1)) + doc := storedUnitDoc(t, exec, id) + namedDoc(t, doc, "txtNew") // the rewrite landed + if got := listMarkers(doc, "SortItems"); len(got) != 1 || got[0] != 2 { + t.Errorf("after the rewrite SortItems is stored %v, want [2] as Studio Pro writes", got) + } +} + +// Executing describe's output corrects it once, then is unchanged. +func TestIssue1640_ReExecutingDescribeCorrectsTheMarkerOnce(t *testing.T) { + exec, out, path := markerFixture(t) + run(t, exec, issue1640Setup) + run(t, exec, issue1640Page) + id := unitIDByName(t, exec, "page", "Things1640") + plantStaleSortItems(t, exec, path, id) + + out.Reset() + run(t, exec, "describe page MyFirstModule.Things1640") + described := out.String() + run(t, exec, described) + if got := listMarkers(storedUnitDoc(t, exec, id), "SortItems"); len(got) != 1 || got[0] != 2 { + t.Errorf("after describe -> exec SortItems is stored %v, want [2]", got) + } + out.Reset() + run(t, exec, described) + if !strings.Contains(out.String(), "Unchanged page") { + t.Errorf("the second run wrote the page again:\n%s", out.String()) + } +} + +// The comment on the issue: a populated nanoflow parameter list is marker 2 +// in every Studio Pro document (78 of 78 call nanoflow actions and 21 of 21 +// nanoflow data sources across ako/TestApp and PedApp); the writer used the +// child type's default, 3. +func TestIssue1640_NanoflowParameterListsAreWrittenAsStudioProWritesThem(t *testing.T) { + exec, _ := pageFixtureExecutor(t) + run(t, exec, issue1640Setup) + run(t, exec, `create nanoflow MyFirstModule.NF_Do1640 ($Thing: MyFirstModule.Thing) +begin +end;`) + run(t, exec, `create nanoflow MyFirstModule.NF_Get1640 ($Thing: MyFirstModule.Thing) returns MyFirstModule.Thing +begin + return $Thing; +end;`) + run(t, exec, `create or modify page MyFirstModule.Thing1640 (Title: 'Thing', Layout: Atlas_Core.Atlas_Default, Params: ($Thing: MyFirstModule.Thing)) { + dataview dvThing (DataSource: $Thing) { + actionbutton btnDo (Caption: 'Do', Action: call nanoflow MyFirstModule.NF_Do1640(Thing = $currentObject)) + } + dataview dvGot (DataSource: nanoflow MyFirstModule.NF_Get1640(Thing = $Thing)) { + textbox tbName (Attribute: Name) + } +};`) + doc := storedUnitDoc(t, exec, unitIDByName(t, exec, "page", "Thing1640")) + for _, c := range []struct{ widget, owner string }{ + {"btnDo", "Forms$CallNanoflowClientAction"}, + {"dvGot", "Forms$NanoflowSource"}, + } { + owner := storedElementOfType(t, namedDoc(t, doc, c.widget), c.owner) + list, _ := bsonValue(owner, "ParameterMappings").(bson.A) + if len(list) != 2 { + t.Fatalf("%s: %s.ParameterMappings is %v, want a marker and one mapping", c.widget, c.owner, list) + } + if m, _ := list[0].(int32); m != 2 { + t.Errorf("%s: %s.ParameterMappings is stored with marker %v, want 2 as Studio Pro writes", c.widget, c.owner, list[0]) + } + } +} + +// storedElementOfType returns the first element of $Type typ inside doc. +func storedElementOfType(t *testing.T, doc bson.D, typ string) bson.D { + t.Helper() + var found bson.D + var walk func(any) + walk = func(v any) { + switch n := v.(type) { + case bson.D: + if found == nil && bsonString(n, "$Type") == typ { + found = n + return + } + for _, e := range n { + walk(e.Value) + } + case bson.A: + for _, e := range n { + walk(e) + } + } + } + walk(doc) + if found == nil { + t.Fatalf("no %s", typ) + } + return found +} diff --git a/mdl/executor/layout.go b/mdl/executor/layout.go index 55e2ec4598..0c5e5fbfc3 100644 --- a/mdl/executor/layout.go +++ b/mdl/executor/layout.go @@ -59,6 +59,10 @@ type Bounds struct { // layoutMeasurer calculates dimensions of microflow statements type layoutMeasurer struct { varTypes map[string]string + // layout is the style the flow is measured for; nil is the default + // (flowlayout.go). It must be the builder's, or the measured sizes are those + // of a different drawing. + layout *flowLayout } // measureStatements calculates the total bounds for a list of statements. @@ -77,7 +81,7 @@ func (m *layoutMeasurer) measureStatements(stmts []ast.MicroflowStatement) Bound maxHeight := ActivityHeight var prev ast.MicroflowStatement - for _, stmt := range stmts { + for i, stmt := range stmts { bounds := m.measureStatement(stmt) maxHeight = max(maxHeight, bounds.Height) if bounds.Width == 0 { @@ -90,6 +94,9 @@ func (m *layoutMeasurer) measureStatements(stmts []ast.MicroflowStatement) Bound if left < laneBusy && reachesBelowMainLine(stmt, bounds) { left = laneBusy } + if guard, ok := stmt.(*ast.IfStmt); ok && isGuard(guard) && m.layoutStyle().Guard == GuardTrueRight { + return m.measureGuardTrueRight(guard, stmts[i+1:], left, extent, maxHeight) + } if guard, ok := stmt.(*ast.IfStmt); ok && isGuard(guard) { branchRight := left + guardBranchInset + m.measureBranch(guard.ThenBody).Width cursor = left + guardMainWidth @@ -238,6 +245,22 @@ func (m *layoutMeasurer) measureStatement(stmt ast.MicroflowStatement) Bounds { } func (m *layoutMeasurer) measureEnumSplitStatement(s *ast.EnumSplitStmt) Bounds { + if m.layoutStyle().CaseSplit == CaseSplitColumns { + bodies := make([][]ast.MicroflowStatement, 0, len(s.Cases)+1) + for _, c := range s.Cases { + bodies = append(bodies, c.Body) + } + if len(s.ElseBody) > 0 { + bodies = append(bodies, s.ElseBody) + } + if len(bodies) > 1 { + shift := 0 + if len(s.Cases) > 0 && len(s.Cases[0].Values) > 1 { + shift = firstCaseGroupShift + } + return m.measureCaseColumns(bodies, SplitWidth, shift) + } + } maxBranchWidth := 0 var branchHeights []int for _, c := range s.Cases { @@ -268,6 +291,18 @@ func (m *layoutMeasurer) measureEnumSplitStatement(s *ast.EnumSplitStmt) Bounds } func (m *layoutMeasurer) measureInheritanceSplitStatement(s *ast.InheritanceSplitStmt) Bounds { + if m.layoutStyle().CaseSplit == CaseSplitColumns { + bodies := make([][]ast.MicroflowStatement, 0, len(s.Cases)+1) + for _, c := range s.Cases { + bodies = append(bodies, c.Body) + } + if len(s.ElseBody) > 0 { + bodies = append(bodies, s.ElseBody) + } + if len(bodies) > 1 { + return m.measureCaseColumns(bodies, ActivityWidth, 0) + } + } maxBranchWidth := 0 branchCount := len(s.Cases) for _, c := range s.Cases { @@ -309,6 +344,16 @@ func (m *layoutMeasurer) measureIfStatement(s *ast.IfStmt) Bounds { branchWidth = HorizontalSpacing / 2 } + elseColumn := m.layoutStyle().IfElse == IfElseColumn && (s.HasElse || len(s.ElseBody) > 0) + var column Bounds + if elseColumn { + column = m.measureColumn(s.ElseBody) + branchWidth = elseColumnBranchWidth(thenBounds, column) + if branchWidth == 0 { + branchWidth = HorizontalSpacing / 2 + } + } + totalWidth := SplitWidth + HorizontalSpacing/2 + branchWidth + MergeSize // A guard — `if X then ...; return; end if` with no ELSE — has no merge: its // branch ends in an end event and the main line resumes straight after the @@ -321,7 +366,12 @@ func (m *layoutMeasurer) measureIfStatement(s *ast.IfStmt) Bounds { // Height depends on layout strategy var totalHeight int - if len(s.ElseBody) > 0 { + if elseColumn { + // The column hangs under the split, below the TRUE body. + // Reported as if centred on the line, as every Bounds is read: twice + // what reaches below it (see measureGuardTrueRight). + totalHeight = 2 * (max(thenBounds.Height, ActivityHeight)/2 + BranchGap + max(column.Height, ActivityHeight)) + } else if len(s.ElseBody) > 0 { // IF WITH ELSE: TRUE path horizontal (main line), FALSE path below // Height = THEN branch height + gap + ELSE branch height thenHeight := max(thenBounds.Height, ActivityHeight) diff --git a/mdl/executor/layout_rows.go b/mdl/executor/layout_rows.go index 621f81a4c6..732871de62 100644 --- a/mdl/executor/layout_rows.go +++ b/mdl/executor/layout_rows.go @@ -108,9 +108,10 @@ func (fb *flowBuilder) rowBottom() int { // lone end event on a row of its own, with a line running back across the diagram // to reach it. func (fb *flowBuilder) shouldWrap(stmt ast.MicroflowStatement, rest []ast.MicroflowStatement) bool { - if !fb.allowWrap { + if !fb.allowWrap || fb.layoutStyle().Wrap == WrapOff { return false } + rowWidth := fb.layoutStyle().rowWidth() width := ActivityWidth if fb.measurer != nil { w := fb.measurer.measureStatement(stmt).Width @@ -122,12 +123,12 @@ func (fb *flowBuilder) shouldWrap(stmt ast.MicroflowStatement, rest []ast.Microf // The whole element has to fit, not just its starting column: a loop box 810px // wide begun at 1290 past the row start ran the row to 2100, well past a screen. used := fb.posX - fb.row.startX - if used+width <= MaxRowWidth { + if used+width <= rowWidth { return false } // Past the limit — unless everything that is left fits in the overhang, in which // case finishing on this row reads better than a stub of a row underneath. - if fb.measurer != nil && used+fb.measurer.measureStatements(rest).Width <= MaxRowWidth+RowOverhang { + if fb.measurer != nil && used+fb.measurer.measureStatements(rest).Width <= rowWidth+RowOverhang { return false } return true diff --git a/mdl/linter/rules/legacy_image_widget.go b/mdl/linter/rules/legacy_image_widget.go index 8a7f36258b..03f2e4d81a 100644 --- a/mdl/linter/rules/legacy_image_widget.go +++ b/mdl/linter/rules/legacy_image_widget.go @@ -13,8 +13,8 @@ import ( // // # Why this is correctness, not style // -// The React client arrived in Mendix 10.7 and is the only client on 11, and -// mxbuild reports CE0582 on each of these widgets whenever it is enabled — +// The React client arrived in Mendix 10.7, and mxbuild reports CE0582 on each +// of these widgets whenever it is enabled — // measured on 11.12.1, where a page carrying one is a build ERROR: // // [error] [CE0582] "Widget static image is not supported in React client. @@ -100,6 +100,7 @@ func LegacyImageWidget(widgetType string) (LegacyImage, bool) { func (r *LegacyImageWidgetRule) Check(ctx *linter.LintContext) []linter.Violation { var violations []linter.Violation native := ctx.NativePages() + classic := runsClassicClient(ctx) for w := range ctx.Widgets() { if ctx.IsExcluded(w.ModuleName) { @@ -120,12 +121,20 @@ func (r *LegacyImageWidgetRule) Check(ctx *linter.LintContext) []linter.Violatio docType = "snippet" } + severity := r.DefaultSeverity() + message := fmt.Sprintf( + "%s '%s' in %s is not supported by the React client, which this app uses — mxbuild reports CE0582", + legacy.MendixTerm, w.Name, w.ContainerQualifiedName) + if classic { + severity = linter.SeverityInfo + message = fmt.Sprintf( + "%s '%s' in %s will need converting before the app switches to the React client (it builds on the client this app uses now)", + legacy.MendixTerm, w.Name, w.ContainerQualifiedName) + } violations = append(violations, linter.Violation{ RuleID: r.ID(), - Severity: r.DefaultSeverity(), - Message: fmt.Sprintf( - "%s '%s' in %s is not supported by the React client (Mendix 10.7+, the only client on 11) — mxbuild reports CE0582", - legacy.MendixTerm, w.Name, w.ContainerQualifiedName), + Severity: severity, + Message: message, Location: linter.Location{ Module: w.ModuleName, DocumentType: docType, @@ -140,3 +149,25 @@ func (r *LegacyImageWidgetRule) Check(ctx *linter.LintContext) []linter.Violatio return violations } + +// runsClassicClient reports whether the project's Web UI setting is anything +// but the React client outright. A Mendix 11 app can still run the classic +// client, and there these widgets build clean: 57 warnings that each said +// "mxbuild reports CE0582" on an app at 0 errors (mendixlabs/mxcli#1226). +// Migration mode is not the React client either — measured on 11.14.0, the same +// CE0582 on a classic drop-down is an error under "Yes" and clean under "No" +// and "MigrationMode" (usesReactClient in mdl/executor). +// +// Settings that cannot be read keep the warning: lint always has them, so an +// unreadable value means something is wrong, not that the app is classic. +func runsClassicClient(ctx *linter.LintContext) bool { + reader := ctx.Reader() + if reader == nil { + return false + } + ps, err := reader.GetProjectSettings() + if err != nil || ps == nil || ps.WebUI == nil || ps.WebUI.UseOptimizedClient == "" { + return false + } + return ps.WebUI.UseOptimizedClient != "Yes" +} diff --git a/mdl/linter/rules/legacy_image_widget_client_1287_test.go b/mdl/linter/rules/legacy_image_widget_client_1287_test.go new file mode 100644 index 0000000000..a133b9a197 --- /dev/null +++ b/mdl/linter/rules/legacy_image_widget_client_1287_test.go @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" +) + +// settingsReader serves the project settings; everything else is empty. +type settingsReader struct { + rawUnitReader + ps *model.ProjectSettings +} + +func (r settingsReader) GetProjectSettings() (*model.ProjectSettings, error) { return r.ps, nil } + +// CE0582 is the React client's error. A Mendix 11 app can still run the classic +// client (Web UI → UseOptimizedClient = No), and there mxbuild builds the legacy +// image widgets clean: 57 MPR012 warnings, each saying "mxbuild reports CE0582", +// on an app at 0 errors (mendixlabs/mxcli#1226, ako/mxcli#1287). Measured on 11.14.0 for the same CE0582 on a classic +// drop-down: an error under "Yes", clean under "No" and "MigrationMode" +// (usesReactClient in mdl/executor). +func TestLegacyImageWidgetRule_FollowsTheProjectsWebClient(t *testing.T) { + cat, err := catalog.New() + if err != nil { + t.Fatal(err) + } + defer cat.Close() + for _, stmt := range []string{ + `INSERT INTO modules_data (Id, Name, QualifiedName, ModuleName, Source) VALUES + ('m1', 'MyFirstModule', 'MyFirstModule', 'MyFirstModule', '')`, + `INSERT INTO pages_data (Id, Name, QualifiedName, ModuleName, LayoutRef) VALUES + ('p1', 'P_Images', 'MyFirstModule.P_Images', 'MyFirstModule', 'Atlas_Core.Atlas_Default')`, + `INSERT INTO widgets_data (Id, Name, WidgetType, ContainerId, ContainerQualifiedName, ContainerType, ModuleName) VALUES + ('w1', 'img1', 'Forms$StaticImageViewer', 'p1', 'MyFirstModule.P_Images', 'PAGE', 'MyFirstModule')`, + } { + if _, err := cat.CatalogDB().Exec(stmt); err != nil { + t.Fatalf("%s: %v", stmt, err) + } + } + check := func(reader linter.LintReader) linter.Violation { + t.Helper() + vs := NewLegacyImageWidgetRule().Check(linter.NewLintContext(cat, reader)) + if len(vs) != 1 { + t.Fatalf("want one MPR012 for img1, got %d: %v", len(vs), vs) + } + return vs[0] + } + client := func(v string) linter.LintReader { + return settingsReader{ps: &model.ProjectSettings{WebUI: &model.WebUISettings{UseOptimizedClient: v}}} + } + + // The React client: CE0582 is real, so the warning stands. + if v := check(client("Yes")); v.Severity != linter.SeverityWarning || !strings.Contains(v.Message, "CE0582") { + t.Errorf("React client: got %s %q, want a warning naming CE0582", v.Severity, v.Message) + } + + // The classic client and migration mode build clean: an advisory, and no + // claim that mxbuild reports anything. + for _, setting := range []string{"No", "MigrationMode"} { + v := check(client(setting)) + if v.Severity != linter.SeverityInfo { + t.Errorf("UseOptimizedClient %s: severity %s, want info", setting, v.Severity) + } + if strings.Contains(v.Message, "reports CE0582") || strings.Contains(v.Message, "only client") { + t.Errorf("UseOptimizedClient %s: message claims an error mxbuild does not report: %q", setting, v.Message) + } + if !strings.Contains(v.Message, "React client") { + t.Errorf("UseOptimizedClient %s: message no longer says why it matters: %q", setting, v.Message) + } + } + + // Settings that cannot be read leave the rule as it was. + if v := check(nil); v.Severity != linter.SeverityWarning { + t.Errorf("no reader: severity %s, want the warning unchanged", v.Severity) + } +} diff --git a/mdl/roundtrip/testapp_allowlist_test.go b/mdl/roundtrip/testapp_allowlist_test.go index 09db9b7dba..70f706bfa3 100644 --- a/mdl/roundtrip/testapp_allowlist_test.go +++ b/mdl/roundtrip/testapp_allowlist_test.go @@ -145,14 +145,11 @@ var testAppKnownFailures = map[string]knownFailure{ "snippet WorkflowCommons.Snip_TaskAssignmentHelper_TargetUser_Search": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes; executes since a widget bound to a snippet parameter is described as $Param.Attr — getput not yet triaged"}, "snippet WorkflowCommons.Snip_TaskAssignment_UserTask_Assignees": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "snippet: breaks getput and putget on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_TaskAssignment_UserTask_TargetUsers": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "snippet: breaks getput and putget on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "snippet WorkflowCommons.Snip_TaskCount_RefreshBTN": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_TimelineViewer_Timeline_Full": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_TimelineViewer_Timeline_Tasks": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "snippet WorkflowCommons.Snip_UserTask_Detail": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: exec passes since mendixlabs/mxcli#1403 (a show page passing the snippet parameter); getput then rewrites conditional-visibility ModuleRoles, a date picker ReadOnlyStyle and a data view source mapping marker, none of them show page mappings — not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_UserTask_DueDate_Warning": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp (#743); putget passes since a template parameter written as an expression is stored as one (#823 option C)"}, "snippet WorkflowCommons.Snip_UserTask_NameColumn": {laws: []law{lawGetPut, lawPutGet}, issue: "#721 #826", why: "executes since describe prints snippet call Params (#826); an association-path template parameter read from a snippet parameter loses its AttributeRef and SourceVariable (#721)"}, "snippet WorkflowCommons.Snip_UserTask_NotificationArea": {laws: []law{lawGetPut, lawPutGet}, issue: "#721", why: "snippet: breaks getput and putget on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "snippet WorkflowCommons.Snip_UserTask_UnassignedMessage": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_WorkflowActivityRecord_ActivityIcon": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_WorkflowActivityRecord_Details": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes; executes since a widget bound to a snippet parameter is described as $Param.Attr — getput not yet triaged"}, "snippet WorkflowCommons.Snip_WorkflowActivityRecord_Header": {laws: []law{lawGetPut, lawPutGet}, issue: "#721 #826", why: "executes since describe prints snippet call Params (#826); association-path template parameters and an expression parameter drift (#721)"}, @@ -168,8 +165,6 @@ var testAppKnownFailures = map[string]knownFailure{ "snippet WorkflowCommons.Snip_WorkflowUserTaskView_Details": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_WorkflowUserTaskView_DueDate_Warning": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp (#743); putget passes since a template parameter written as an expression is stored as one (#823 option C)"}, "snippet WorkflowCommons.Snip_WorkflowUserTaskView_NameColumn": {laws: []law{lawGetPut, lawPutGet}, issue: "#721 #826", why: "executes since describe prints snippet call Params (#826); an association-path template parameter read from a snippet parameter loses its AttributeRef and SourceVariable (#721)"}, - "snippet WorkflowCommons.Snip_Workflow_ActivityTimelineOnly_Full": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "snippet WorkflowCommons.Snip_Workflow_ActivityTimelineOnly_Tasks": {laws: []law{lawGetPut}, issue: "#721", why: "snippet: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_Workflow_AuditTrail": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_Workflow_CommentsAndAttachments": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "snippet WorkflowCommons.Snip_Workflow_CommentsAndAttachments_Admin": {laws: []law{lawExec}, issue: "#721", why: "snippet: breaks exec on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, diff --git a/modelsdk/canon/listmarkers.go b/modelsdk/canon/listmarkers.go new file mode 100644 index 0000000000..4c820461a9 --- /dev/null +++ b/modelsdk/canon/listmarkers.go @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "go.mongodb.org/mongo-driver/v2/bson" +) + +// List markers (ako/mxcli#1640). +// +// Mendix stores every list as a BSON array whose first entry is an int32 typed- +// array marker, then the items. Studio Pro writes 2 for some lists and 3 for +// others, and which one is a property of the list, not of anything a script +// says: it is measured per list and registered here, and the encoder writes the +// registered marker for every list it builds. +// +// A list the encoder does NOT build keeps the marker it had. That happens on +// every rewrite of a stored document: an unchanged pluggable widget's Type and +// Object pass through byte for byte, an existing element's clean properties are +// copied as raw bytes, a mutator edits the stored document in place. So a marker +// an older mxcli wrote before the list was measured survived every later +// rewrite, while a fresh document got Studio Pro's. NormalizeListMarkers is the +// write-side half of the same rule: every write gives a registered list its +// registered marker, whoever built the list. +// +// Only a marker REGISTERED for the owner $Type and key is applied. The marker +// the encoder writes for an unregistered list (3) is a default, not a +// measurement, so a stored marker there is left as it is; it is either Studio +// Pro's or unknown. The child-$Type registrations are not applied either: they +// stand in for the list's real rule only on the lists the encoder builds, which +// all start with one of the registered children, and are not a statement about +// every list that does. + +// childListMarkers maps a child element $Type to the marker the encoder writes +// for a list whose first item is of that type. +var childListMarkers = map[string]int32{} + +// propertyListMarkers maps an owning $Type and list key to its marker. +var propertyListMarkers = map[string]map[string]int32{} + +// RegisterChildListMarker records the marker of a list whose first item is of +// childType. It is the encoder's rule for a list it builds; NormalizeListMarkers +// does not apply it. +func RegisterChildListMarker(childType string, marker int32) { + childListMarkers[childType] = marker +} + +// ChildListMarker returns the marker registered for a list whose first item is +// of childType. +func ChildListMarker(childType string) (int32, bool) { + m, ok := childListMarkers[childType] + return m, ok +} + +// RegisterPropertyListMarker records the marker Studio Pro writes for the list +// stored under key on elements of ownerType, empty or not. +func RegisterPropertyListMarker(ownerType, key string, marker int32) { + if propertyListMarkers[ownerType] == nil { + propertyListMarkers[ownerType] = map[string]int32{} + } + propertyListMarkers[ownerType][key] = marker +} + +// PropertyListMarker returns the marker registered for the list under key on +// elements of ownerType. +func PropertyListMarker(ownerType, key string) (int32, bool) { + m, ok := propertyListMarkers[ownerType][key] + return m, ok +} + +// NormalizeListMarkers gives every list with a registered owner/key marker that +// marker, wherever it sits in the unit. It changes nothing else, and returns +// contents unchanged (the same slice) when no marker differs or contents is not +// a document. +func NormalizeListMarkers(contents []byte) []byte { + if len(propertyListMarkers) == 0 { + return contents + } + var doc bson.D + if err := bson.Unmarshal(contents, &doc); err != nil { + return contents + } + if !normalizeListMarkersIn(doc) { + return contents + } + out, err := bson.Marshal(doc) + if err != nil { + return contents + } + return out +} + +func normalizeListMarkersIn(v any) bool { + changed := false + switch x := v.(type) { + case bson.D: + owner, _ := lookupString(x, "$Type") + keys := propertyListMarkers[owner] + for i := range x { + if m, ok := keys[x[i].Key]; ok { + if a, isList := x[i].Value.(bson.A); isList && len(a) > 0 { + if cur, isMarker := a[0].(int32); isMarker && cur != m { + a[0] = m + changed = true + } + } + } + if normalizeListMarkersIn(x[i].Value) { + changed = true + } + } + case bson.A: + for _, e := range x { + if normalizeListMarkersIn(e) { + changed = true + } + } + } + return changed +} diff --git a/modelsdk/canon/listmarkers_test.go b/modelsdk/canon/listmarkers_test.go new file mode 100644 index 0000000000..81fadaea3c --- /dev/null +++ b/modelsdk/canon/listmarkers_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// ako/mxcli#1640: a registered list's stored marker is the one Studio Pro +// writes, wherever the list sits; nothing else moves. + +func init() { + RegisterPropertyListMarker("Test1640$Bar", "Items", 2) +} + +// unit1640 is a unit holding a registered list (marker given) deep inside an +// unregistered one, beside an unregistered list on the same owner. +func unit1640(items, other int32) bson.D { + bar := bson.D{ + {Key: "$Type", Value: "Test1640$Bar"}, + {Key: "Items", Value: bson.A{items, bson.D{{Key: "$Type", Value: "Test1640$Item"}}}}, + {Key: "Others", Value: bson.A{other}}, + } + return bson.D{ + {Key: "$Type", Value: "Test1640$Unit"}, + {Key: "Widgets", Value: bson.A{int32(3), bson.D{ + {Key: "$Type", Value: "Test1640$Widget"}, + {Key: "Value", Value: bson.D{{Key: "$Type", Value: "Test1640$Value"}, {Key: "SortBar", Value: bar}}}, + }}}, + } +} + +func TestNormalizeListMarkers_GivesARegisteredListItsMarker(t *testing.T) { + got := NormalizeListMarkers(marshal(t, unit1640(3, 3))) + want := marshal(t, unit1640(2, 3)) + if string(got) != string(want) { + t.Errorf("normalized:\n%v\nwant:\n%v", bson.Raw(got), bson.Raw(want)) + } +} + +func TestNormalizeListMarkers_LeavesAMatchingUnitAlone(t *testing.T) { + in := marshal(t, unit1640(2, 1)) + got := NormalizeListMarkers(in) + if &got[0] != &in[0] { + t.Error("a unit with nothing to normalize was re-encoded") + } +} + +func TestNormalizeListMarkers_LeavesAnUnregisteredListAlone(t *testing.T) { + // Others is not registered, so 1 stays 1 although the encoder would + // write 3 for a list it built. + got := NormalizeListMarkers(marshal(t, unit1640(3, 1))) + if string(got) != string(marshal(t, unit1640(2, 1))) { + t.Errorf("normalized: %v", bson.Raw(got)) + } +} + +func TestNormalizeListMarkers_IsTheEncodersTable(t *testing.T) { + if m, ok := PropertyListMarker("Test1640$Bar", "Items"); !ok || m != 2 { + t.Errorf("PropertyListMarker = %d, %v", m, ok) + } + if _, ok := PropertyListMarker("Test1640$Bar", "Others"); ok { + t.Error("an unregistered list reads as registered") + } +} diff --git a/modelsdk/codec/defaults.go b/modelsdk/codec/defaults.go index dd3c07b526..68cdc9a617 100644 --- a/modelsdk/codec/defaults.go +++ b/modelsdk/codec/defaults.go @@ -2,6 +2,8 @@ package codec +import "github.com/mendixlabs/mxcli/modelsdk/canon" + // TypeDefaults captures the serialization defaults Studio Pro applies to a // freshly-created element of a given $Type but that the gen New() // constructors do not yet set — the "applyDefaults" gap (engalar Fix 4 TODO). @@ -80,36 +82,33 @@ func FreshGUIDRegistrations() map[string][]string { return out } -// listMarkers maps a child element $Type to the leading typed-array marker -// Studio Pro writes for a PartList of that element type. Most domain-model -// member lists use 3 (the encoder default); some — notably the IndexedAttribute -// list inside an EntityIndex — use a different version marker. Confirmed against -// real Studio-Pro 11.x BSON (mx-test-projects/test7-app: IdxProbe index uses 2). -var listMarkers = map[string]int32{} - // RegisterListMarker declares the typed-array marker for PartLists whose child -// elements are of childType. Without registration the encoder emits 3. +// elements are of childType. Without registration the encoder emits 3. Most +// domain-model member lists use 3; some — notably the IndexedAttribute list +// inside an EntityIndex — use a different version marker. Confirmed against +// real Studio-Pro 11.x BSON (mx-test-projects/test7-app: IdxProbe index uses 2). +// +// The table lives in canon (canon.RegisterChildListMarker), beside the +// owner/key table every write applies. func RegisterListMarker(childType string, marker int32) { - listMarkers[childType] = marker + canon.RegisterChildListMarker(childType, marker) } -// propertyListMarkers maps an owning $Type and PartList key to its marker. It +// RegisterPropertyListMarker declares the typed-array marker for the PartList +// stored under key on elements of ownerType. It takes precedence over a +// child-type RegisterListMarker and also covers the list when it is empty. It // exists for lists whose marker depends on WHERE the list sits rather than on // what it holds: a Forms$DesignPropertyValue list is marker 3 as a // Forms$Appearance's DesignProperties and marker 2 as a // Forms$CompoundDesignPropertyValue's Properties (measured: 1821/1821 and // 373/373 across Studio Pro-authored documents in a Mendix 11.13.0 app), so a // RegisterListMarker on the child type cannot express it. -var propertyListMarkers = map[string]map[string]int32{} - -// RegisterPropertyListMarker declares the typed-array marker for the PartList -// stored under key on elements of ownerType. It takes precedence over a -// child-type RegisterListMarker and also covers the list when it is empty. +// +// Register only a measured marker: every write applies it to the stored lists +// the encoder does not rebuild too (canon.NormalizeListMarkers, +// ako/mxcli#1640), where a wrong one would rewrite what Studio Pro stored. func RegisterPropertyListMarker(ownerType, key string, marker int32) { - if propertyListMarkers[ownerType] == nil { - propertyListMarkers[ownerType] = map[string]int32{} - } - propertyListMarkers[ownerType][key] = marker + canon.RegisterPropertyListMarker(ownerType, key, marker) } // mandatoryPropertyLists marks the owner/key pairs Studio Pro serializes on @@ -136,12 +135,11 @@ func isMandatoryPropertyList(ownerType, key string) bool { } func lookupPropertyListMarker(ownerType, key string) (int32, bool) { - m, ok := propertyListMarkers[ownerType][key] - return m, ok + return canon.PropertyListMarker(ownerType, key) } func lookupListMarker(childType string) int32 { - if m, ok := listMarkers[childType]; ok { + if m, ok := canon.ChildListMarker(childType); ok { return m } return 3 diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index 393ab264bf..12ffebbabc 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -564,7 +564,7 @@ func (w *Writer) insertUnit(unitID, containerID, containmentName, unitType strin return fmt.Errorf("invalid container ID (not a valid UUID): %q", containerID) } - contents, normErr := w.normalizePropertySets(unitID, contents) + contents, normErr := w.normalizeForStorage(unitID, contents) if normErr != nil { return normErr } @@ -732,7 +732,7 @@ func (w *Writer) reconcileAgainst(unitID string, contents []byte, readStored fun w.writesOffered++ // Normalised before comparing, so a rebuild of a Studio Pro document that // differed from it only by the properties the writer leaves out is elided. - contents, err = w.normalizePropertySets(unitID, contents) + contents, err = w.normalizeForStorage(unitID, contents) if err != nil { return nil, false, err } @@ -1035,7 +1035,7 @@ func (w *Writer) UpdateUnitContainer(unitID, newContainerID string) error { return nil } -// normalizePropertySets gives every element exactly the property set Studio +// normalizeForStorage gives every element exactly the property set Studio // Pro writes for its $Type on this project's version: keys the version does not // declare yet are stripped when empty and refused when they hold a value // (canon.StripUndeclaredProperties), then missing ones are added @@ -1044,6 +1044,17 @@ func (w *Writer) UpdateUnitContainer(unitID, newContainerID string) error { // because a key too many or too few from ANY writer makes the document // unopenable or unmergeable (mendixlabs/mxcli#1373), and a fix per writer is // the list that never ends. -func (w *Writer) normalizePropertySets(unitID string, contents []byte) ([]byte, error) { - return normalizePropertySetsFor(w.reader.ProjectVersion(), unitID, contents) +// +// Every registered list then gets the marker Studio Pro writes for it +// (canon.NormalizeListMarkers). The encoder writes that marker for a list it +// builds; a list a rewrite passes through as stored bytes — an unchanged +// pluggable widget, an existing element's clean property, a mutator's in-place +// edit — kept whatever an older build wrote, on every later rewrite +// (ako/mxcli#1640). +func (w *Writer) normalizeForStorage(unitID string, contents []byte) ([]byte, error) { + out, err := normalizePropertySetsFor(w.reader.ProjectVersion(), unitID, contents) + if err != nil { + return nil, err + } + return canon.NormalizeListMarkers(out), nil } diff --git a/modelsdk/mpr/writer_property_sets.go b/modelsdk/mpr/writer_property_sets.go index e16ccc6731..9f1c499286 100644 --- a/modelsdk/mpr/writer_property_sets.go +++ b/modelsdk/mpr/writer_property_sets.go @@ -51,7 +51,7 @@ func (r *Reader) StoredPropertySetGaps(unitID string) (PropertySetRepair, error) // // # Why this is not just another write // -// Every write already normalizes (normalizePropertySets), so a unit that a +// Every write already normalizes (normalizeForStorage), so a unit that a // statement rewrites is completed on the way. The units nothing rewrites are // not: a project built by an older mxcli keeps every gap in documents no // later statement touches — a module's Projects$ModuleSettings, a microflow