diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 4965c4295..3a491f569 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -662,6 +662,7 @@ {"area": "mdl/backend/pagemutator", "date": "2026-09-20", "symptom": "`alter page … { set DataSource = DATABASE Mod.Entity on dvCust; }` passes `check`, prints `Altered page …` with exit 0, and leaves the DataView with no datasource: `describe page` renders `dataview dvCust {` with the property gone, and the only other signal is CE7007 at `mx check`", "cause": "`serializeDataSourceBson` mapped every `*pages.DatabaseSource` to a `Forms$DataViewSource` — the *context* source — with the entity in `EntityRef` and `SourceVariable` left null. A DATABASE source has no single stored shape: the widget holding it decides (`Forms$ListViewXPathSource` on a list view, `CustomWidgets$CustomWidgetXPathSource` on a pluggable widget, `Forms$GridXPathSource` on a grid), and a DATA VIEW has no database form at all — which is why CREATE PAGE's `dataViewSourceToGen` already refused that pairing while SET wrote it silently", "file": "`mdl/backend/pagemutator/mutator.go` (`SetWidgetDataSource`, new `databaseSourceRefusal`, `serializeDataSourceBson`)", "insight": "**The \"gone entirely\" in the report was DESCRIBE, not the document.** The DataSource was present and well-formed BSON; `parseContextSource` returns nil for a `Forms$DataViewSource` with no `SourceVariable`, so the reader rendered nothing. Chasing a deleted property would have been the wrong hunt — diff the stored BSON before believing a describe-shaped symptom. **The refusal belongs in the mutator, not the validator**: `validateAlterSetProperties` dry-runs the real setter against a `pagemutator.Probe()` copy, so one refusal makes `check -p --references` and `exec` agree by construction; a second copy of the rule in the validator is the duplicate-resolver drift CLAUDE.md warns about. **Refusing beat rebuilding the shapes here** — writing ListViewXPathSource in raw BSON would duplicate `listViewSourceToGen` in a second currency, and REPLACE already reaches the real builder. **Two remedies, not one**: on a data view `use REPLACE` is a dead end (CREATE PAGE refuses it too), so the message names the sources a data view can take; on a list view REPLACE genuinely works, so it names REPLACE. Getting that backwards sends the author in a circle. **Same generalisable shape as #855/#1101**: when SET and REPLACE express different vocabularies for one property, SET is a whitelist extended one bug report at a time. **`make check-mdl` runs `mxcli check` WITHOUT `-p`**, so a document-dependent refusal cannot be a `.fail.mdl` — it would be reported as a negative test that unexpectedly passed. Write the passing shape and comment the refused statements, as #1063 does. Measured on two copies of a real 11.13.0 app: faulty → `Check passed!`, exit 0, `mx check` 1 error CE7007 at Data view 'dvCust'; fixed → both refuse with exit 1, datasource unchanged, `mx check` 0 errors. Tests `mdl/backend/pagemutator/mutator_datasource_test.go`, `mdl/executor/validate_alter_set_test.go`; example `mdl-examples/bug-tests/1032-alter-page-set-database-datasource.mdl`. upstream #1032", "refs": ["#855", "#1032"], "ce": ["CE7007"]} {"area": "mdl/executor", "date": "2026-09-20", "symptom": "`describe page` → `exec` over a **Studio Pro-authored** page silently drops six things, `mx check` 0 errors throughout. The one that matters: `IsPasswordBox True → False` — a **password field round-trips into a plaintext text box**, and describe → rename → exec is mxcli's copy operation. Also `Validation.Expression` blanked, a DataView's `ReadOnlyStyle Text → Control`, `PopupCloseAction` wiped, and two typed-array markers", "cause": "Four different causes behind one symptom, which is why triage came first: (1) IsPasswordBox — model and writer carried it, nothing parsed it, nothing emitted it; (2) Validation — `widgetValidationToGen()` wrote a DEFAULT EMPTY Forms$WidgetValidation over whatever was stored, on five widget types; (3) ReadOnlyStyle — wired for CheckBox only, and a DataView's draws no MDL-WIDGET07 warning because `staticWidgetKnownProps` is deliberately a union across widget types; (4) PopupCloseAction — `pageToGen` wrote \"\" unconditionally. Plus ParameterMappings/OutputMappings markers", "file": "`mdl/executor/cmd_pages_describe_parse.go` + `_output.go` (extract/emit), `cmd_pages_builder_v3_widgets.go` (consume), `cmd_pages_builder_v3.go`, `mdl/visitor/visitor_page_v3.go`, `mdl/ast/ast_page_v3.go`, `sdk/pages/*`, `mdl/backend/modelsdk/widget_write.go` + `page_write.go`, `mdl/executor/validate_widgets.go` (describe vocabulary)", "insight": "**Triage the layer before writing anything** — describer / grammar / builder have different fixes and this one issue had all three. The quickest probe is to run the property through `mxcli check`: MDL-WIDGET07 names an unrecognised one, and *silence is not acceptance* — the known-props list is a union across widget types, so a DataView's ReadOnlyStyle passed check and was dropped anyway. **Emit an expression QUOTED, not bracketed**: `[...]` is the XPath-constraint spelling and `propertyValueV3` parses it as an ARRAY, so `GetStringProp` yields \"\" — the emitter's own unit test was green while the real round trip still lost the value (storage form is not input form). **Measure the default before keeping it**: a DataView's ReadOnlyStyle is Control on 47 of 56, never Inherit, so the 'obvious' Inherit that every other input widget uses would have been wrong. Markers likewise measured, not assumed: ParameterMappings is marker 2 on 220 of 220 lists in every parent type, OutputMappings present on 91 of 91 — and an EMPTY list needs `MandatoryListMarkers` since `RegisterListMarker` keys on a child that is not there. Result 17 → 9 differences, the 9 being ako/mxcli#549", "refs": ["#550", "#541", "#549", "#490"]} {"area": "mdl/executor", "date": "2026-09-20", "symptom": "MDL-PAGEARG01 refused a list widget's OWN row action: `datagrid dg (DataSource: DATABASE M.E, onClick: SHOW_PAGE M.Edit(E: $currentObject))` was rejected at `check` with \"widget `dg` is not inside a data view, list view or grid row\" \u2014 and since exec refuses a script whose check errors, the slice could not be applied at all. On a `listview` the message contradicted itself. mxbuild 11.14.0 accepts the stored pages at 0 errors.", "cause": "The #1029 guard judged EVERY widget's own action in the context its PARENT supplies: `argContextForSubtreeOf` returns the parent context for a childless widget and `validate_widgets.go` passed the inherited `argCtx` to `validateShowPageArguments`. Right for a button, wrong for the widget that ESTABLISHES the context \u2014 a list widget's onClick is row-scoped, so the row it renders is the context object. Added `argContextForOwnAction`: a widget that binds a source of its own supplies the context for its own action; a source in a shape the pass cannot read (the bare-entity shorthand) degrades to UNKNOWN so the guard stands down rather than refusing what it cannot prove is discarded.", "file": "`mdl/executor/cmd_pages_showpage_args.go` (argContextForOwnAction, argContextForSubtreeOf), `mdl/executor/validate_widgets.go`", "insight": "**A false refusal costs more than a missing rule now that exec refuses on a check error** \u2014 the blast radius is 'this project cannot be built with this mxcli', not 'a warning is noisy'. Two things would have caught it before release: judging the rule against the widget kinds it NAMES in its own message (the listview refusal reads 'lvA is not inside a \u2026 list view'), and running it against mxbuild rather than against intuition. The mxbuild run paid for itself twice: it also showed that `DataSource: M.E` (bare-entity shorthand) on a datagrid is silently dropped, so that case is CE0488 + a REAL CE1571 \u2014 the stand-down is still correct, but the shorthand case must not be written into a bug test as mxbuild-clean (#576). Control the fix with the widget kinds STILL refused (a foreign variable, a sibling button beside the grid), or it is indistinguishable from deleting the rule.", "refs": ["#552", "#576", "mendixlabs/mxcli#1029", "#939"]} +{"area": "mdl/executor", "date": "2026-09-22", "symptom": "Microflows written with no @position are hard to read in Studio Pro although `mx check` is clean and nothing overlaps (mendixlabs/mxcli#1154). Measured on a generated app of 41 microflows: the widest flow is 6930x160 px on one row; every guard (`if X then ...; return; end if`) leaves 370px of bare main line above its own branch where two activities stand 40px apart; a merge stands 120px after its branch; a 7-case enumeration split sends case 4 out of the split's LEFT corner and cases 5+ onto the TOP of their activities, so the lines cross each other; a note on a loop is drawn inside the loop box, its connector attached to the top of the note.", "cause": "Five separate things. (1) The builder is a single x-cursor: every top-level statement advances posX and nothing ever moves down. (2) After a no-merge IF the cursor was set past the branch's far end (`max(afterSplit, afterBranch)`) although a guard's branch is in the lane BELOW and never rejoins. (3) mergeX added HorizontalSpacing/2 to a branch width that is already edge to edge, and a branch ending in RETURN was measured without the end event it draws. (4) The anchor pair on an enum case flow is not geometry: 0e692431 made it the storage for CASE order (`splitCaseOrderAnchors`), one distinct pair per case, and past the third pair the table runs out of sides a drawing would choose. (5) A note was offset 100px from its target's CENTRE, which is inside any box taller than 200px, and `annotationFlowToGen` wrote connection indices 0/0 (top to top).", "file": "`mdl/executor/layout_rows.go` (wrapRow, separateRows, takeWrapAnchors), `mdl/executor/layout_lanes.go` (reserveLowerLane, clearLowerLane), `mdl/executor/layout.go` (measureStatements, measureBranch, gapBetween), `mdl/executor/cmd_microflows_builder_actions.go` (enumSplitOriginAnchors), `mdl/executor/cmd_microflows_show_helpers.go` (orderedEnumSplitFlows), `mdl/executor/cmd_microflows_builder_annotations.go` (defaultAnnotationGeometry), `mdl/backend/modelsdk/microflow_write.go` (annotationFlowToGen)", "insight": "**Nothing automatic sees this class** - no check, no build error, no test: it took a person opening 41 flows in Studio Pro, so measure instead of looking. A 30-line script over `describe` output (extent per flow, pairwise box overlap that knows a loop's children are in the loop's own coordinate space) turned 'complex flows look bad' into numbers, and disproved the first diagnosis: there were ZERO overlaps, the defect was unbounded width. Two things cost time. **An anchor that looks like layout may be storage**: changing the enum case anchors without reading `splitCaseOrder` would have silently reordered CASE branches on every describe - grep for readers of a field before treating it as cosmetic. The replacement key is side, then the branch's Y, which the old table already ranks identically for its first three pairs, so one reading path serves models written either way. **Do not estimate what you can measure afterwards**: a row's depth is unknown when the row begins (a loop box is fitted AFTER its body, 250 estimated against 310 built), and every estimate added to wrapRow was later made redundant by one pass over the finished objects (separateRows) - the control experiment showed the estimate's own test passing with the estimate removed. A measurer that mirrors the builder must mirror its LANES too: once the main line stops waiting for a guard's branch, a run's width is max(main line, lane), not a sum.", "refs": ["mendixlabs/mxcli#1154", "mendixlabs/mxcli#884", "mendixlabs/mxcli#724", "0e692431"]} {"area": "mdl/executor", "date": "2026-09-21", "symptom": "A page's image-collection reference passed `mxcli check --references` and failed the build. Reported as \"no MDL syntax for a StaticImageViewer inside a Selection helper custom state\" — the authoring half was already closed by #1057; what was left is that nothing RESOLVED the name it made writable. Measured on a blank Mendix 11.14.0 project: `staticimage imgAll (Image: 'Atlas_UI_Resources.Atlas_Icons.checkbox_checked')` in a custom state -> check passed, exec created the page, `mx check` -> 3x CE1613 \"The selected image … no longer exists.\"", "cause": "TWO independent holes, and either alone leaves the reported script unchecked. (1) widgetRefCollector keyed the image reference on the widget TYPE — `if w.Type == \"image\"` — so the pluggable widget was collected and `staticimage` (which #1057 had just given the SAME `Image:` property) and `dynamicimage`'s `DefaultImage` were not; replaced with an imageRefProps table. (2) A page's widgets live in two AST fields: `Widgets` is the bare body, `Placeholders` holds `placeholder X { … }` content (#532). validate.go passed `s.Widgets` alone to validateWidgetReferences, validatePageContextTree AND validateFlowArguments, so EVERY reference inside a placeholder block — microflow, nanoflow, page, snippet, entity, image — was validated by nothing; added allPageWidgets to collect both roots once.", "file": "`mdl/executor/helpers.go` (widgetRefCollector.collectFromWidget, imageRefProps), `mdl/executor/validate.go` (allPageWidgets)", "insight": "**When a capability gets a new spelling, grep for who RESOLVES the old one.** #1057 added `Image:` to a second and third widget and moved on; the resolver keyed on the type name, so the new spellings were unchecked from the day they shipped. A property list and a resolver list that describe the same property are two copies — `validate_widgets.go` already accepted `Image`/`DefaultImage` for these widgets and DESCRIBE already emitted them, and only the resolver disagreed. **The placeholder hole is the more useful lesson: it was the THIRD copy of one walk.** validateIconRefs (#1008) and forEachWidget had each grown the `Placeholders` arm separately, with a comment saying a missed walk is silent both ways — and the three validators next door still had not. When a fix is 'add the missing arm to this walker', the question is how many walkers there are; collect the roots once instead. **Do not reason about a bug report from the issue text alone when the version is older than the fix** — the reported symptom did not reproduce on main at all, and running the reporter's own script end to end is what turned 'already fixed, close it' into two real defects. **Control both directions**: a reference that resolves must stay silent, because a walker that can suddenly see a whole new region of the tree is as likely to report correct scripts as broken ones.", "refs": ["mendixlabs/mxcli#1149", "mendixlabs/mxcli#1057", "mendixlabs/mxcli#1008", "#532"]} {"area": "mdl/executor", "date": "2026-09-21", "symptom": "A pluggable widget's textTemplate property took only literal text, so it rendered the same string on every row. `treenode tn (headerType: 'text', headerCaption: 'Name')` passed check, exec and `mx check` and printed the word \"Name\" on every node; the companion the report reached for did not exist — \"widget `tn` (treenode) has no property `headerCaptionParams` — did you mean `headerCaption`? [MDL-WIDGET01]\".", "cause": "The `Params` convention was already how an object-list ITEM bound its text templates (#956, buildObjectListItem looks up matchedAlias+\"Params\"), and it simply stopped at the item boundary. At the WIDGET level the engine read parameters from ONE place: the widget-wide `contentparams:`. Three separate holes: (1) `resolveMapping` case \"TextTemplate\" consulted only `w.GetContentParams()`; (2) a texttemplate mapping addressed by its def.json SOURCE name (an Image's `ImageUrl:`, schema key `imageUrl`) fell through to the `default:` branch, which set no ClientParams at all — not even contentparams, so #928's fix covered only the schema-key spelling; (3) `allowedWidgetProperties` did not know the companion, so writing it was MDL-WIDGET01 and exec refused the script. (4) on the READ side `extractExplicitProperties` — the GENERIC describe path, used by every pluggable widget with no dedicated extractor, TreeNode and Timeline included — read AttributeRef and PrimitiveValue only, so every text-template property was absent from `describe page` whether bound or literal, and describe → exec dropped the caption outright. Added `textTemplateParams` (own companion, then contentparams as fallback), `namedPropValueWithKey`/`templateParamNames` so the companion is found under whichever spelling the template was written in, `addTemplateParamsNames` in the validator, `validatePluggableTemplateParams` (MDL-WIDGET21) for an orphaned companion, and the DESCRIBE side for the Image's two templates.", "file": "`mdl/executor/widget_engine.go` (textTemplateParams, namedPropValueWithKey, templateParamNames, resolveMapping TextTemplate + default branches), `mdl/executor/validate_widgets.go` (addTemplateParamsNames), `mdl/executor/validate_widget_contentparams.go` (validatePluggableTemplateParams), `mdl/executor/cmd_pages_describe_pluggable.go` + `cmd_pages_describe_output.go`, `mdl/executor/cmd_pages_describe.go` (rawExplicitProp.Params)", "insight": "**There IS a generic describe path for pluggable widgets and it has a hole, which is not the same as there being no path** — `extractExplicitProperties` handles every widget outside `isKnownCustomWidgetType`'s list of nine, and it read AttributeRef and PrimitiveValue only. Reading the dispatcher rather than running it gives the wrong answer here: the describe LOOKS complete (it emits ten primitives for a TreeNode) and is silently missing the caption. Run `describe page` on the widget before concluding anything about its read side. **Also do not assume the reported widget needs a Marketplace download**: TreeNode.mpk and Timeline.mpk both ship in a blank 11.14.0 app's `widgets/`, and their defs are generated by `mxcli init`, so the exact widgets from a report are usually reproducible directly — check `/widgets/` first. The silent control is what makes the write-side bug visible: with the companions deleted and one shared `contentparams: [{1} = PictureUrl]` left, `check`, `exec` and `mx check` are all clean and BOTH of an Image's stored ClientTemplates bind PictureUrl. Do not treat the MDL-WIDGET01 message as the whole bug — that is the loud half, and fixing only it leaves the shared-list aliasing intact. Last trap: a texttemplate mapping's def.json `source` is sometimes a source KIND (\"TextTemplate\", as on TreeNode/Timeline) and sometimes a real MDL property name (\"ImageUrl\"), and the two take different branches of resolveMapping — a fix applied to one branch looks complete and covers half the widgets.", "refs": ["ako/mxcli#575", "mendixlabs/mxcli#928", "mendixlabs/mxcli#956"], "rules": ["MDL-WIDGET01", "MDL-WIDGET21"], "ce": ["CE0720"]} {"area": "mdl/executor", "date": "2026-09-21", "symptom": "MDL031's pass-through length rule refused a view entity column over a `System.*` string attribute AND suggested exactly the declaration it had just rejected: \"declared as String(200) but pass-through column 'u.Name' inherits length 0 from source attribute System.User.Name \u2026 Fix: change to 'EngineerName: String(200)'\". Worse than self-contradictory: it refused EVERY declaration except `String` (unlimited), which is the one spelling that fails the build.", "cause": "The System metadata carries no attribute lengths (0 of 115, #584), so the inferred length is 0, and the rule treated 0 as a length to match. `formatDataTypeForMDL` then substituted a hardcoded `String(200)` for any unknown length, which is correct for a DERIVED column (String(200) by rule) and wrong here, where the number is the source's. Fixed by having the predicate decline an unknown length; the message collapses to the one case it can judge.", "file": "`mdl/executor/oql_type_inference.go` (passthroughStringLengthMismatch, passthroughLengthError)", "insight": "**Ask mxbuild for the number rather than trusting either side's belief about it.** The field report said System.User.Name is String(200) and I repeated it in the issue; two one-column view entities settled it in two builds \u2014 String(200) is CE6770, String(100) is 0 errors, so it is String(100). That single measurement overturned the report, my issue text, AND my first proposed fix. The decisive third probe was checking what `check` did to the CORRECT declaration: it refused that too, which turned 'confusing message' into 'the rule blocks the right answer and waves through the wrong one' and changed the fix from re-wording to not judging. When a rule compares against a value your own metadata supplies, test the case where that value is MISSING \u2014 a 0 that means 'unknown' silently becomes a 0 that means 'zero'. Probe recipe, reusable and needing no decompiler: one small view entity per attribute, two builds, bisect.", "refs": ["#585", "#584", "#583"]} diff --git a/.claude/skills/mendix/write-microflows/reference/control-flow.md b/.claude/skills/mendix/write-microflows/reference/control-flow.md index 76fdb7ac8..bc39d74ee 100644 --- a/.claude/skills/mendix/write-microflows/reference/control-flow.md +++ b/.claude/skills/mendix/write-microflows/reference/control-flow.md @@ -307,6 +307,7 @@ commit $Product; - `@annotation` before activity-binding metadata such as `@position`, `@caption`, `@color`, `@excluded`, or `@anchor` stays free-floating when later metadata binds the following activity - `@annotation` at the end (no following activity) creates a free-floating note - Escape single quotes by doubling: `@annotation 'Don''t forget'` +- **Leave `@position` out unless you are reproducing a hand-made diagram.** Without it the builder lays the flow out itself: the main line wraps onto rows past two canvas widths, a guard's branch drops into the lane below while the main line carries on above it, and a `case` of four or more branches leaves the decision in three groups so its lines do not cross. A statement with `@position` is never moved and is not measured against what is placed around it, so a few hand-placed statements in an otherwise automatic flow are what produces overlaps (mendixlabs/mxcli#1154) - `@position` always appears in DESCRIBE output; `@caption` only when custom; `@color` only when not Default - DESCRIBE MICROFLOW shows `@` annotations before their activities - `@start(x, y)` positions the **start event** and goes on the first statement, because the start has no statement of its own. Omit it and the start is derived — one spacing unit (160) left of the first activity, on its centre line — and a rewrite re-derives it so the start follows the activities when they move. A start that is not at the derived spot was placed by hand (in Studio Pro or with `@start`): it survives a rewrite that does not mention it, and DESCRIBE emits `@start` for it. An explicit `@start` overrides both (#951) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb64e012c..32dc27fa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Changed + +- **Microflows written without `@position` are laid out to be read, not just to be valid** (mendixlabs/mxcli#1154). Geometry and connection sides only: no MDL syntax changes, a statement carrying `@position` is never moved, and `describe` → `exec` still reports `Unchanged microflow`. Measured on a generated app of 41 microflows, none with an `@position`: the widest flow went from 6930×160 px on one row to 3220 px, with 0 overlapping elements and `mx check` at 0 errors before and after. + + - **A long main line wraps onto rows.** Past 2880 px (two Studio Pro canvases) the next statement starts a new row under everything the current row occupies — branch lanes, loop boxes and notes included, measured after they are built rather than estimated. The line joining two rows leaves the bottom of the last element and arrives on top of the first, so it runs through the band between the rows instead of back across the activities. A tail short enough to finish within two more activities stays on its row rather than becoming a stub underneath. + - **The main line no longer waits for a guard's branch.** `if … then …; return; end if` draws its branch in the lane below and ends it there, yet the next element used to be placed past the branch's far end: 370 px from the split, against 40 px between any two activities. It now stands in the column the branch starts in, and only an element that reaches down into that lane — another decision, a loop, an activity with an error handler — waits for the lane to clear. A merge stands one ordinary gap after its branch (was 120 px), a branch ending in `return` is measured with the end event it draws, and a loop box keeps the same 40 px gap from its neighbours as an activity. + - **An enumeration split of four or more cases draws its lines in three groups** — the upper third from the split's top corner, the middle from its right, the lower from its bottom, every one arriving on the left of its activity, mirrored on the merge. The anchor pair on these flows is where the `case` order is stored, and past the third case the table behind it used sides no drawing would choose: case 4 left the split's left corner, cases 5–8 arrived on top of their activity. `describe` now reads the order as side, then position on the canvas; a model written with one pair per case reads exactly as before, and a split of up to three cases is unchanged. + - **A note sits above its element, not inside it.** Notes were offset from the element's centre, which is inside any loop box; they are now placed above its top edge, and the connector runs from the bottom of the note to the top of the element instead of top to top. + ### Fixed - **A page's image-collection reference passed `mxcli check --references` and failed the build** (mendixlabs/mxcli#1149) — `staticimage imgAll (Image: 'Atlas_UI_Resources.Atlas_Icons.checkbox_checked')` in a Selection helper's custom state checked clean, exec'd cleanly and then came back as `[error] [CE1613] "The selected image … no longer exists."`, once per state. The report asks for syntax, but the syntax landed with #1057 — describe emits the three `staticimage` lines and re-running the description reports `Unchanged page`, measured on a blank 11.14.0 project. What was missing is that nothing resolved the name #1057 had made writable. diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index b3de1d437..1d962152f 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -500,6 +500,15 @@ func init() { "pixel offset from its end of the line. (0, 0) at both ends is straight.\n" + "@position on a split belongs to the SPLIT, so its end-if join has its own\n" + "annotation. Container Size is still computed, not authorable.\n\n" + + "WITHOUT @position the builder places everything. The main line runs left to\n" + + "right and wraps onto a new row past 2880px; the line joining two rows leaves\n" + + "the bottom of one and arrives on top of the next. A guard (if … return; end\n" + + "if) drops its branch into the lane below and the main line carries straight\n" + + "on over it. A CASE of four or more branches leaves the split in three groups\n" + + "— top, right, bottom — so its lines do not cross. A statement that carries\n" + + "@position is never moved, and starts the row for what follows it. Prefer no\n" + + "@position at all to a few: hand-placed statements are not measured against\n" + + "what the builder puts around them. (#1154)\n\n" + "@start and @merge position the two nodes that have no statement of their\n" + "own, so each is written on the statement it belongs to. Omit @start and the\n" + "start is placed one spacing unit left of the first activity, on its centre\n" + diff --git a/docs-site/src/language/microflow-structure.md b/docs-site/src/language/microflow-structure.md index aedc1311c..2e85e8d9d 100644 --- a/docs-site/src/language/microflow-structure.md +++ b/docs-site/src/language/microflow-structure.md @@ -138,6 +138,15 @@ Set the canvas position of the next activity: $Order = CREATE Sales.Order (Status = 'New'); ``` +Positions are optional. A microflow written without them is laid out by mxcli: the +main line runs left to right and wraps onto a new row once it passes two canvas +widths (2880 px); a guard — `if … then …; return; end if` — drops its branch into the +lane below while the main line carries on above it; and a `case` of four or more +branches leaves the decision in three groups (top, right, bottom) so its lines do not +cross. A statement that carries `@position` is never moved, and becomes the start of +the row for the statements after it — so either place everything or nothing: a few +hand-placed statements are not measured against what is laid out around them. + ### Start event The start event has no statement of its own, so `@start` goes on the **first** diff --git a/mdl-examples/bug-tests/microflow-layout-rows-lanes-cases.mdl b/mdl-examples/bug-tests/microflow-layout-rows-lanes-cases.mdl new file mode 100644 index 000000000..9fc944b84 --- /dev/null +++ b/mdl-examples/bug-tests/microflow-layout-rows-lanes-cases.mdl @@ -0,0 +1,112 @@ +-- Auto-layout of a microflow written with no @position: long main lines, guards +-- and wide enumeration splits. +-- +-- Measured on a generated app of 41 microflows (Mendix 11.12.1, mxcli v0.23.0), none +-- of them carrying an @position: nothing overlapped, and the widest flow was +-- 6930x160 px - four screens of sideways scrolling. Every guard left 370px of bare +-- main line above its own branch (40px between any two activities), a merge stood +-- 120px after its branch, and a 7-case split sent its fourth case out of the split's +-- LEFT corner and its fifth onwards onto the TOP of their activities, because the +-- anchor pair was storing the case order. +-- +-- Open the three flows in Studio Pro after exec: +-- MF_LayoutLong wraps onto rows; the joining line runs between the rows +-- MF_LayoutGuards each activity after a guard stands in the branch's column +-- MF_LayoutCases case lines leave the split top / right / bottom, 2-3-2 +-- +-- mendixlabs/mxcli#1154 + +CREATE ENUMERATION MyFirstModule.LayoutStatus ( + NewOrder 'New', Confirmed 'Confirmed', InProgress 'In progress', Shipped 'Shipped', + Delivered 'Delivered', Invoiced 'Invoiced' +); + +CREATE MICROFLOW MyFirstModule.MF_LayoutLong () +BEGIN + LOG INFO 'step 1'; + LOG INFO 'step 2'; + LOG INFO 'step 3'; + LOG INFO 'step 4'; + LOG INFO 'step 5'; + LOG INFO 'step 6'; + LOG INFO 'step 7'; + LOG INFO 'step 8'; + LOG INFO 'step 9'; + LOG INFO 'step 10'; + LOG INFO 'step 11'; + LOG INFO 'step 12'; + LOG INFO 'step 13'; + LOG INFO 'step 14'; + LOG INFO 'step 15'; + LOG INFO 'step 16'; + LOG INFO 'step 17'; + LOG INFO 'step 18'; + LOG INFO 'step 19'; + LOG INFO 'step 20'; + LOG INFO 'step 21'; + LOG INFO 'step 22'; + LOG INFO 'step 23'; + LOG INFO 'step 24'; + LOG INFO 'step 25'; + LOG INFO 'step 26'; + LOG INFO 'step 27'; + LOG INFO 'step 28'; + LOG INFO 'step 29'; + LOG INFO 'step 30'; + LOG INFO 'step 31'; + LOG INFO 'step 32'; + LOG INFO 'step 33'; + LOG INFO 'step 34'; + LOG INFO 'step 35'; + LOG INFO 'step 36'; + LOG INFO 'step 37'; + LOG INFO 'step 38'; + LOG INFO 'step 39'; + LOG INFO 'step 40'; +END; +/ + +CREATE MICROFLOW MyFirstModule.MF_LayoutGuards ($Amount: Integer, $Limit: Integer) +RETURNS Boolean AS $IsAccepted +BEGIN + IF $Amount = 0 THEN + LOG WARNING 'nothing to place'; + RETURN false; + END IF; + LOG INFO 'amount present'; + IF $Amount > $Limit THEN + LOG WARNING 'over the limit'; + RETURN false; + END IF; + IF $Limit < 0 THEN + LOG WARNING 'no credit at all'; + RETURN false; + END IF; + LOG INFO 'accepted'; + RETURN true; +END; +/ + +CREATE MICROFLOW MyFirstModule.MF_LayoutCases ($Status: MyFirstModule.LayoutStatus) +RETURNS String AS $Next +BEGIN + DECLARE $Next String = ''; + CASE $Status + WHEN NewOrder THEN + SET $Next = 'Confirmed'; + WHEN Confirmed THEN + SET $Next = 'InProgress'; + WHEN InProgress THEN + SET $Next = 'Shipped'; + WHEN Shipped THEN + SET $Next = 'Delivered'; + WHEN Delivered THEN + SET $Next = 'Invoiced'; + WHEN Invoiced THEN + SET $Next = 'Invoiced'; + WHEN (empty) THEN + SET $Next = 'NewOrder'; + END CASE; + RETURN $Next; +END; +/ diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index 117552c6a..a81a4df8c 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -421,7 +421,10 @@ func annotationFlowToGen(af *microflows.AnnotationFlow, major int) element.Eleme g.SetID(element.ID(af.ID)) g.SetOriginID(element.ID(af.OriginID)) g.SetDestinationID(element.ID(af.DestinationID)) - g.SetOriginConnectionIndex(0) + // A note sits ABOVE the element it documents, so its line leaves the note's + // bottom edge and enters the element's top. Both indexes were 0 (top), which + // drew the line out of the top of the note and back down around it. + g.SetOriginConnectionIndex(2) g.SetDestinationConnectionIndex(0) if major <= 9 { g.SetOriginBezierVector("0;0") diff --git a/mdl/executor/bugfix_regression_test.go b/mdl/executor/bugfix_regression_test.go index 6508f7a91..60e5de19a 100644 --- a/mdl/executor/bugfix_regression_test.go +++ b/mdl/executor/bugfix_regression_test.go @@ -97,7 +97,7 @@ func TestAddLoopStatement_PreservesAnnotatedPosition(t *testing.T) { if loop.Position.X != 350 || loop.Position.Y != 200 { t.Fatalf("got loop position (%d, %d), want (350, 200)", loop.Position.X, loop.Position.Y) } - wantNextX := 350 + loop.Size.Width/2 + HorizontalSpacing + wantNextX := 350 + loop.Size.Width/2 + ActivityWidth/2 + (HorizontalSpacing - ActivityWidth) // the next activity sits one activity-gap past the box if fb.posX != wantNextX { t.Fatalf("got next posX %d, want %d", fb.posX, wantNextX) } @@ -136,7 +136,7 @@ func TestAddWhileStatement_PreservesAnnotatedPosition(t *testing.T) { if loop.Position.X != 420 || loop.Position.Y != 180 { t.Fatalf("got while position (%d, %d), want (420, 180)", loop.Position.X, loop.Position.Y) } - wantNextX := 420 + loop.Size.Width/2 + HorizontalSpacing + wantNextX := 420 + loop.Size.Width/2 + ActivityWidth/2 + (HorizontalSpacing - ActivityWidth) // the next activity sits one activity-gap past the box if fb.posX != wantNextX { t.Fatalf("got next posX %d, want %d", fb.posX, wantNextX) } diff --git a/mdl/executor/cmd_microflows_build.go b/mdl/executor/cmd_microflows_build.go index a77fc68cc..332b88456 100644 --- a/mdl/executor/cmd_microflows_build.go +++ b/mdl/executor/cmd_microflows_build.go @@ -407,6 +407,7 @@ func buildMicroflowFromStmt(ctx *ExecContext, s *ast.CreateMicroflowStmt, opts b varTypes: varTypes, declaredVars: declaredVars, measurer: &layoutMeasurer{varTypes: varTypes}, + allowWrap: true, backend: ctx.Backend, hierarchy: hierarchy, restServices: restServices, @@ -685,6 +686,7 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui varTypes: varTypes, declaredVars: declaredVars, measurer: &layoutMeasurer{varTypes: varTypes}, + allowWrap: true, backend: ctx.Backend, hierarchy: hierarchy, restServices: restServices, diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index af0e0b090..6f18acd96 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -113,6 +113,16 @@ type flowBuilder struct { // pendingJoin is the `join` addStatement just saw, waiting for the enclosing // body loop to say which activity the path had reached. pendingJoin *ast.JoinStmt + // lowerLane is how far right the lane under each main line (keyed by the line's + // y) is occupied by a guard's branch. See layout_lanes.go. + lowerLane map[int]int + // allowWrap turns on row wrapping for this builder: the main line breaks onto a + // new row past MaxRowWidth instead of running off the canvas (layout_rows.go). + // Only the builders that lay out a whole microflow set it; a loop body builds in + // its own coordinate space inside a box that is sized to fit, so wrapping there + // would fight the box rather than help the reader. + allowWrap bool + row rowTracker } type flowBuilderVariableState struct { diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 37eb86793..ec6316e90 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -425,7 +425,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { branchWidth := 0 for _, br := range branches { - w := fb.measurer.measureStatements(br.body).Width + w := fb.measurer.measureBranch(br.body).Width if w > branchWidth { branchWidth = w } @@ -433,7 +433,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { if branchWidth == 0 { branchWidth = HorizontalSpacing / 2 } - mergeX := splitX + SplitWidth + HorizontalSpacing/2 + branchWidth + HorizontalSpacing/2 + mergeX := splitX + SplitWidth + HorizontalSpacing/2 + branchWidth mergeX, mergeY := mergePosition(s.Annotations, mergeX, centerY) var merge *microflows.ExclusiveMerge ensureMerge := func() *microflows.ExclusiveMerge { @@ -475,6 +475,14 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } } + origins := enumSplitOriginAnchors(branchYs, centerY) + slot := func(i int) splitCaseSlot { + if origins == nil { + return splitCaseSlot{order: i, origin: -1} + } + return splitCaseSlot{order: i, origin: origins[i]} + } + savedEndsWithReturn := fb.endsWithReturn allBranchesReturn := len(branches) > 0 for i, br := range branches { @@ -498,7 +506,7 @@ 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, i, splitX+SplitWidth+HorizontalSpacing/4, branchY) + fb.addGroupedEnumSplitFlows(splitID, m.ID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) } else { fb.takePendingJoin(lastID, pendingCase, prevAnchor) } @@ -513,7 +521,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { fb.pendingAnnotations = nil } if lastID == "" { - fb.addGroupedEnumSplitFlows(splitID, actID, br.values, i, splitX+SplitWidth+HorizontalSpacing/4, branchY) + fb.addGroupedEnumSplitFlows(splitID, actID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) // 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; @@ -552,13 +560,18 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } allBranchesReturn = false if lastID == "" { - fb.addGroupedEnumSplitFlows(splitID, ensureMerge().ID, br.values, i, splitX+SplitWidth+HorizontalSpacing/4, branchY) + fb.addGroupedEnumSplitFlows(splitID, ensureMerge().ID, br.values, slot(i), splitX+SplitWidth+HorizontalSpacing/4, branchY) } else { + var tail *microflows.SequenceFlow if pendingCase != "" { - fb.flows = append(fb.flows, newHorizontalFlowWithCase(lastID, ensureMerge().ID, pendingCase)) + tail = newHorizontalFlowWithCase(lastID, ensureMerge().ID, pendingCase) } else { - fb.flows = append(fb.flows, newHorizontalFlow(lastID, ensureMerge().ID)) + tail = newHorizontalFlow(lastID, ensureMerge().ID) + } + if origins != nil { + tail.DestinationConnectionIndex = mergeSideFor(fb.objectPosition(lastID), fb.objectPosition(ensureMerge().ID)) } + fb.flows = append(fb.flows, tail) } } @@ -793,9 +806,9 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt return splitID } -func (fb *flowBuilder) addGroupedEnumSplitFlows(originID, destinationID model.ID, values []string, order int, mergeX, mergeY int) { +func (fb *flowBuilder) addGroupedEnumSplitFlows(originID, destinationID model.ID, values []string, slot splitCaseSlot, mergeX, mergeY int) { if len(values) <= 1 { - fb.addEnumSplitFlows(originID, destinationID, values, order) + fb.addEnumSplitFlows(originID, destinationID, values, slot) return } branchMerge := µflows.ExclusiveMerge{ @@ -806,24 +819,120 @@ func (fb *flowBuilder) addGroupedEnumSplitFlows(originID, destinationID model.ID }, } fb.objects = append(fb.objects, branchMerge) - fb.addEnumSplitFlows(originID, branchMerge.ID, values, order) + fb.addEnumSplitFlows(originID, branchMerge.ID, values, slot) fb.flows = append(fb.flows, newHorizontalFlow(branchMerge.ID, destinationID)) } -func (fb *flowBuilder) addEnumSplitFlows(originID, destinationID model.ID, values []string, order int) { +func (fb *flowBuilder) addEnumSplitFlows(originID, destinationID model.ID, values []string, slot splitCaseSlot) { + split, target := fb.objectPosition(originID), fb.objectPosition(destinationID) if len(values) == 0 { flow := newHorizontalFlow(originID, destinationID) - applySplitCaseOrder(flow, order) + slot.apply(flow, split, target) fb.flows = append(fb.flows, flow) return } for _, value := range values { flow := newHorizontalFlowWithEnumCase(originID, destinationID, value) - applySplitCaseOrder(flow, order) + slot.apply(flow, split, target) fb.flows = append(fb.flows, flow) } } +// splitCaseSlot says how one case's flow leaves the split. origin is the side of +// the split it leaves from, or -1 for a split of up to three cases, which keeps the +// pair table below (top, right, bottom — the same sides, as it happens). +type splitCaseSlot struct { + order int + origin int +} + +// apply sets the flow's sides. split and target are where the two ends actually are, +// which is not where the stack was planned once a branch carries @position: a line is +// only sent out of the top corner towards something above the split, and out of the +// bottom towards something below, whatever third the case was counted into. +func (slot splitCaseSlot) apply(flow *microflows.SequenceFlow, split, target model.Point) { + if flow == nil { + return + } + if slot.origin < 0 { + applySplitCaseOrder(flow, slot.order) + return + } + origin := slot.origin + if origin == AnchorTop && target.Y >= split.Y || origin == AnchorBottom && target.Y <= split.Y { + origin = AnchorRight + } + flow.OriginConnectionIndex = origin + flow.DestinationConnectionIndex = AnchorLeft +} + +// objectPosition returns where an already-built object stands. +func (fb *flowBuilder) objectPosition(id model.ID) model.Point { + for i := len(fb.objects) - 1; i >= 0; i-- { + if o := fb.objects[i]; o != nil && o.GetID() == id { + return o.GetPosition() + } + } + return model.Point{} +} + +// enumSplitOriginAnchors picks the side of the split each case leaves from, for a +// split of four or more cases: the upper third from the top corner, the middle +// third from the right, the lower third from the bottom. It returns nil for three +// cases or fewer, which the pair table already draws that way. +// +// The pair table was written to store the case ORDER (see splitCaseOrder), and past +// the third case it does so with sides no drawing would choose: the fourth case +// leaves the split's LEFT corner, the fifth to eighth arrive on top of their +// activity, the ninth onwards on its far side. Seven cases drawn that way cross each +// other and the activities they pass. Grouped, no two lines cross: the branches are +// stacked top to bottom in case order, so a line from the top corner only ever goes +// up and one from the bottom only down. +// +// A third is by count, the remainder going to the middle (7 cases: 2/3/2) or, when +// it is two, one each to top and bottom (8 cases: 3/2/3). A branch is only given the +// top corner if it really is above the split, and the bottom only if below, so a +// stack made lopsided by one tall branch never gets a line that leaves upwards to +// reach something underneath. +func enumSplitOriginAnchors(branchYs []int, centerY int) []int { + n := len(branchYs) + if n <= 3 { + return nil + } + top, bottom := n/3, n/3 + if n%3 == 2 { + top++ + bottom++ + } + origins := make([]int, n) + for i, y := range branchYs { + switch { + case i < top && y < centerY: + origins[i] = AnchorTop + case i >= n-bottom && y > centerY: + origins[i] = AnchorBottom + default: + origins[i] = AnchorRight + } + } + return origins +} + +// mergeSideFor is the side of the closing merge a branch arrives on: an upper branch +// comes down onto its top corner and a lower one up onto its bottom, instead of all of +// them converging on the left corner. It goes by where the branch's last element +// actually stands, so a branch moved with @position still arrives from its own side. +func mergeSideFor(last, merge model.Point) int { + switch { + case last.Y < merge.Y: + return AnchorTop + case last.Y > merge.Y: + return AnchorBottom + default: + return AnchorLeft + } +} + type splitCaseOrderAnchor struct { origin int destination int diff --git a/mdl/executor/cmd_microflows_builder_annotations.go b/mdl/executor/cmd_microflows_builder_annotations.go index f30fd5e34..591a203b7 100644 --- a/mdl/executor/cmd_microflows_builder_annotations.go +++ b/mdl/executor/cmd_microflows_builder_annotations.go @@ -256,8 +256,30 @@ var DefaultAnnotationSize = model.Size{Width: 200, Height: 50} // failure is silent in the worst way: the describer omits a position the builder // then re-derives differently, so the note creeps further on every round trip. // TestAnnotationGeometryDefaultIsSharedByBothSides pins that. -func defaultAnnotationGeometry(activityPos model.Point, index int) (model.Point, model.Size) { - return model.Point{X: activityPos.X, Y: activityPos.Y - 100 - index*(DefaultAnnotationSize.Height+10)}, DefaultAnnotationSize +// targetHeight is the height of the element the note belongs to. A note is placed +// above that element's TOP EDGE, not a fixed distance above its centre: a loop is a +// box several hundred pixels tall, so the old centre-relative offset dropped its +// note inside the box, on top of the body it was describing. An ordinary activity +// is ActivityHeight tall and keeps the geometry it has always had. +func defaultAnnotationGeometry(activityPos model.Point, index int, targetHeight int) (model.Point, model.Size) { + if targetHeight < ActivityHeight { + targetHeight = ActivityHeight + } + above := targetHeight/2 - ActivityHeight/2 + return model.Point{ + X: activityPos.X, + Y: activityPos.Y - above - 100 - index*(DefaultAnnotationSize.Height+10), + }, DefaultAnnotationSize +} + +// objectHeight is an element's stored height, defaulting to an activity's. +func objectHeight(obj microflows.MicroflowObject) int { + if withSize, ok := obj.(interface{ GetSize() model.Size }); ok { + if h := withSize.GetSize().Height; h > 0 { + return h + } + } + return ActivityHeight } // attachAnnotation attaches one note to an activity. @@ -291,13 +313,15 @@ func (fb *flowBuilder) attachAnnotation(note ast.MicroflowAnnotation, activityID } var activityPos model.Point + activityHeight := ActivityHeight for _, obj := range fb.objects { if obj.GetID() == activityID { activityPos = obj.GetPosition() + activityHeight = objectHeight(obj) break } } - pos, size := defaultAnnotationGeometry(activityPos, index) + pos, size := defaultAnnotationGeometry(activityPos, index, activityHeight) if note.Position != nil { pos = model.Point{X: note.Position.X, Y: note.Position.Y} } diff --git a/mdl/executor/cmd_microflows_builder_annotations_geometry_test.go b/mdl/executor/cmd_microflows_builder_annotations_geometry_test.go new file mode 100644 index 000000000..dd7750ad3 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_annotations_geometry_test.go @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" +) + +func TestNoteOnATallElementSitsAboveIt(t *testing.T) { + // A note is placed above its element's TOP EDGE. Offset from the centre, a + // note on a loop — a box several hundred pixels tall — landed inside the box, + // on top of the body it describes. + loopHeight := 400 + pos, size := defaultAnnotationGeometry(model.Point{X: 500, Y: 300}, 0, loopHeight) + boxTop := 300 - loopHeight/2 + if pos.Y+size.Height/2 >= boxTop { + t.Fatalf("note bottom at %d, box top at %d: the note is inside the box", + pos.Y+size.Height/2, boxTop) + } + // An ordinary activity keeps the geometry it has always had. + plain, _ := defaultAnnotationGeometry(model.Point{X: 500, Y: 300}, 0, ActivityHeight) + if plain.Y != 200 { + t.Fatalf("note on an activity moved to y=%d, want 200", plain.Y) + } +} diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 67d6464d8..bd26a1bf4 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -20,8 +20,8 @@ import ( // connect to the merge. When both branches end with RETURN, no merge is created. func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { // First, measure the branches to know how much space they need - thenBounds := fb.measurer.measureStatements(s.ThenBody) - elseBounds := fb.measurer.measureStatements(s.ElseBody) + thenBounds := fb.measurer.measureBranch(s.ThenBody) + elseBounds := fb.measurer.measureBranch(s.ElseBody) // Calculate branch width (max of both branches) branchWidth := max(thenBounds.Width, elseBounds.Width) @@ -111,7 +111,11 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } // Calculate merge position (after the longest branch) - mergeX := splitX + SplitWidth + HorizontalSpacing/2 + branchWidth + HorizontalSpacing/2 + // thenStartX (below) is the CENTRE of the branch's first activity and branchWidth + // is measured edge to edge, so the branch ends at thenStartX - ActivityWidth/2 + + // branchWidth. The merge goes one ordinary gap past that; a further half pitch used + // to be added here, which left 120px before every merge against 40px elsewhere. + mergeX := splitX + SplitWidth + HorizontalSpacing/2 + branchWidth // Determine if the merge would have 2+ incoming edges (non-redundant). // Skip merge when only one branch flows into it (the other returns). @@ -496,12 +500,17 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } else { // No merge: the split's continuing branch connects directly to the next activity. // Position after the split, past the downward branch's horizontal extent. - afterSplit := splitX + SplitWidth + HorizontalSpacing + afterSplit := splitX + HorizontalSpacing afterBranch := thenStartX + thenBounds.Width + HorizontalSpacing/2 if !hasElseBody { - fb.posX = max(afterSplit, afterBranch) + // 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 { fb.posX = max(afterSplit, afterBranch) + fb.reserveLowerLane(centerY, thenStartX-ActivityWidth/2+branchWidth+laneGap) } fb.posY = centerY if noMergeExitID != "" { @@ -592,10 +601,19 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { // Inner positioning: activities start after the iterator icon on the left, // and are centred vertically within the loop box so that branching content // (IF/CASE) centred on innerStartY stays within [0, loopHeight]. - innerStartX := LoopPadding + iteratorSpace + // + // A position is a CENTRE, so the first activity has to start half its own + // width past the iterator column; at LoopPadding+iteratorSpace its left edge + // landed 60px inside that column, on top of the iterator's icon and the list + // variable's label. + innerStartX := LoopPadding + iteratorSpace + ActivityWidth/2 innerStartY := loopHeight / 2 - loopLeftX := fb.posX + // posX is where the builder would CENTRE the next element. A loop box placed + // with its left edge there started 100px after the previous activity, where a + // neighbouring activity would have started 40px after it; half an activity to + // the left puts the box's edge where an activity's edge would be. + loopLeftX := fb.posX - ActivityWidth/2 loopCenterX := loopLeftX + loopWidth/2 if s.Annotations != nil && s.Annotations.Position != nil { loopCenterX = s.Annotations.Position.X @@ -739,7 +757,11 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { fb.applyAnnotations(loop.ID, savedLoopAnnotations) } - fb.posX = loopLeftX + loopWidth + HorizontalSpacing + // The next element is centred on posX, so an activity placed a full + // HorizontalSpacing past the box's right edge sat 100px away from it, against + // the 40px that separates two activities. Half an activity plus that same gap + // puts it where a neighbour on the main line would be. + fb.posX = loopLeftX + loopWidth + ActivityWidth/2 + (HorizontalSpacing - ActivityWidth) return loop.ID } @@ -944,7 +966,11 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { innerStartX := LoopPadding innerStartY := LoopPadding + ActivityHeight/2 - loopLeftX := fb.posX + // posX is where the builder would CENTRE the next element. A loop box placed + // with its left edge there started 100px after the previous activity, where a + // neighbouring activity would have started 40px after it; half an activity to + // the left puts the box's edge where an activity's edge would be. + loopLeftX := fb.posX - ActivityWidth/2 loopCenterX := loopLeftX + loopWidth/2 if s.Annotations != nil && s.Annotations.Position != nil { loopCenterX = s.Annotations.Position.X @@ -1066,7 +1092,11 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { fb.applyAnnotations(loop.ID, savedWhileAnnotations) } - fb.posX = loopLeftX + loopWidth + HorizontalSpacing + // The next element is centred on posX, so an activity placed a full + // HorizontalSpacing past the box's right edge sat 100px away from it, against + // the 40px that separates two activities. Half an activity plus that same gap + // puts it where a neighbour on the main line would be. + fb.posX = loopLeftX + loopWidth + ActivityWidth/2 + (HorizontalSpacing - ActivityWidth) return loop.ID } diff --git a/mdl/executor/cmd_microflows_builder_enum_split_layout_test.go b/mdl/executor/cmd_microflows_builder_enum_split_layout_test.go new file mode 100644 index 000000000..4463e2685 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_enum_split_layout_test.go @@ -0,0 +1,168 @@ +// 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" +) + +func buildEnumSplit(t *testing.T, cases int) (*flowBuilder, model.ID) { + t.Helper() + fb := &flowBuilder{posX: 200, posY: 200, baseY: 200, spacing: HorizontalSpacing, + varTypes: map[string]string{}, declaredVars: map[string]string{}, + measurer: &layoutMeasurer{varTypes: map[string]string{}}} + fb.buildFlowGraph([]ast.MicroflowStatement{enumSplitWithBranchCount(cases)}, nil) + for _, o := range fb.objects { + if _, ok := o.(*microflows.ExclusiveSplit); ok { + return fb, o.GetID() + } + } + t.Fatal("no split built") + return nil, "" +} + +func TestEnumCaseLinesLeaveTheSplitInThreeGroups(t *testing.T) { + // Seven cases: the two upper branches leave the split's top corner, the three in + // the middle its right, the two lower its bottom, and every one arrives at the + // left of its activity. The order table this replaces sent the fourth case out of + // the split's LEFT corner and the fifth onwards onto the top of their activities, + // so the lines crossed each other and the activities in between. + fb, splitID := buildEnumSplit(t, 7) + want := map[string]int{ + "Value1": AnchorTop, "Value2": AnchorTop, + "Value3": AnchorRight, "Value4": AnchorRight, "Value5": AnchorRight, + "Value6": AnchorBottom, "Value7": AnchorBottom, + } + seen := 0 + for _, flow := range fb.flows { + if flow.OriginID != splitID { + continue + } + value, ok := enumCaseValue(flow) + if !ok { + continue + } + seen++ + if flow.OriginConnectionIndex != want[value] || flow.DestinationConnectionIndex != AnchorLeft { + t.Errorf("%s leaves side %d and arrives side %d, want %d and left(%d)", + value, flow.OriginConnectionIndex, flow.DestinationConnectionIndex, want[value], AnchorLeft) + } + // A line from the top corner only ever goes up, one from the bottom only down. + split, _ := byIDPosition(fb.objects, splitID) + dest, _ := byIDPosition(fb.objects, flow.DestinationID) + if want[value] == AnchorTop && dest.Y >= split.Y || want[value] == AnchorBottom && dest.Y <= split.Y { + t.Errorf("%s leaves side %d towards y=%d with the split at y=%d", value, want[value], dest.Y, split.Y) + } + } + if seen != 7 { + t.Fatalf("found %d case flows, want 7", seen) + } +} + +func TestEnumCaseGroupSizes(t *testing.T) { + for n, want := range map[int][3]int{4: {1, 2, 1}, 5: {2, 1, 2}, 6: {2, 2, 2}, 7: {2, 3, 2}, 8: {3, 2, 3}, 9: {3, 3, 3}} { + ys := make([]int, n) + for i := range ys { + ys[i] = (i*2 - (n - 1)) * 50 // stacked symmetrically around 0 + } + var got [3]int + for _, side := range enumSplitOriginAnchors(ys, 0) { + got[side]++ // AnchorTop=0, AnchorRight=1, AnchorBottom=2 + } + if got != want { + t.Errorf("%d cases grouped %v, want %v", n, got, want) + } + } + if enumSplitOriginAnchors([]int{-100, 0, 100}, 0) != nil { + t.Error("three cases must keep the pair table: it already draws top, right, bottom") + } +} + +func TestDescribeReadsCaseOrderFromGroupedLines(t *testing.T) { + // The pair table existed to store the case order, which the stored flow order + // does not survive. Grouped lines store it as side, then depth on the canvas. + for _, n := range []int{4, 7, 12} { + fb, splitID := buildEnumSplit(t, n) + objects := map[model.ID]microflows.MicroflowObject{} + for _, o := range fb.objects { + objects[o.GetID()] = o + } + var flows []*microflows.SequenceFlow + for _, flow := range fb.flows { + if flow.OriginID == splitID { + flows = append(flows, flow) + } + } + rand.New(rand.NewSource(int64(n))).Shuffle(len(flows), func(i, j int) { flows[i], flows[j] = flows[j], flows[i] }) + for i, flow := range orderedEnumSplitFlows(flows, objects) { + if value, _ := enumCaseValue(flow); value != fmt.Sprintf("Value%d", i+1) { + t.Fatalf("%d cases: position %d holds %s", n, i, value) + } + } + } +} + +func TestDescribeStillReadsThePairTable(t *testing.T) { + // A model written before lines were grouped stores the order as one pair per case, + // and nothing else: here every branch has the same Y, so only the pair can order + // them. + var flows []*microflows.SequenceFlow + for i := 0; i < 9; i++ { + flow := newHorizontalFlowWithEnumCase("split", model.ID(fmt.Sprintf("a%d", i)), fmt.Sprintf("Value%d", i+1)) + applySplitCaseOrder(flow, i) + flows = append(flows, flow) + } + rand.New(rand.NewSource(9)).Shuffle(len(flows), func(i, j int) { flows[i], flows[j] = flows[j], flows[i] }) + for i, flow := range orderedEnumSplitFlows(flows, nil) { + if value, _ := enumCaseValue(flow); value != fmt.Sprintf("Value%d", i+1) { + t.Fatalf("position %d holds %s", i, value) + } + } +} + +func TestHandPlacedCaseBranchesAreReadInCanvasOrder(t *testing.T) { + // Deliberate, and a change from the pair table: with four or more cases the order + // inside a group is the branch's place on the canvas, so branches a user has moved + // with @position come back from DESCRIBE top to bottom, not in the order they were + // typed. The model is unaffected — CASE branches have no order at run time — and a + // second describe -> exec is a fixed point. Here the four branches are placed + // bottom-up; none of them is on the side of the split its third was planned for, so + // all four leave from the right and are read purely by Y. + split := enumSplitWithBranchCount(4) + for i := range split.Cases { + log := split.Cases[i].Body[0].(*ast.LogStmt) + log.Annotations = &ast.ActivityAnnotations{Position: &ast.Position{X: 700, Y: 500 - i*200}} + } + out := describeBuiltEnumSplitBody(t, []ast.MicroflowStatement{split}) + assertOrder(t, out, "when Value4", "when Value3", "when Value2", "when Value1") +} + +func TestCaseLineNeverLeavesTheSplitAwayFromItsBranch(t *testing.T) { + // The first case is counted into the upper third, but @position put its branch + // under the split. Leaving from the top corner it would loop over the split to get + // down there; it leaves from the right instead. + split := enumSplitWithBranchCount(7) + log := split.Cases[0].Body[0].(*ast.LogStmt) + log.Annotations = &ast.ActivityAnnotations{Position: &ast.Position{X: 700, Y: 900}} + + fb := &flowBuilder{posX: 200, posY: 200, baseY: 200, spacing: HorizontalSpacing, + varTypes: map[string]string{}, declaredVars: map[string]string{}, + measurer: &layoutMeasurer{varTypes: map[string]string{}}} + fb.buildFlowGraph([]ast.MicroflowStatement{split}, nil) + for _, flow := range fb.flows { + if value, ok := enumCaseValue(flow); ok && value == "Value1" { + if flow.OriginConnectionIndex != AnchorRight { + t.Fatalf("Value1's branch is below the split but its line leaves side %d, want right(%d)", + flow.OriginConnectionIndex, AnchorRight) + } + return + } + } + t.Fatal("no flow for Value1") +} diff --git a/mdl/executor/cmd_microflows_builder_graph.go b/mdl/executor/cmd_microflows_builder_graph.go index e8e89a8c8..3dc1b8237 100644 --- a/mdl/executor/cmd_microflows_builder_graph.go +++ b/mdl/executor/cmd_microflows_builder_graph.go @@ -77,6 +77,9 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a lastID := startEvent.ID fb.posX += fb.spacing + // The main line starts here, at the first statement's column. Rows wrap back to + // it (layout_rows.go); a flow that never reaches MaxRowWidth never notices. + fb.noteRowStart() // Process each statement // pendingCase holds the case value for the NEXT flow (set by merge-less splits) @@ -98,6 +101,18 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a // and wiring it produced a duplicate flow into the merge. pathOpen := !fb.endsWithReturn + // Wrap the main line before placing a statement that would run past the + // readable width. A statement carrying its own @position places itself, so + // it is left alone and becomes the start of the current row instead. + if x, ok := ownPositionX(stmt); ok { + // The statement places itself, so this is where the current row now + // begins; wrapping measures from here on. + fb.row.startX = x + fb.row.firstObject = len(fb.objects) + } else if fb.shouldWrap(stmt, stmts[i:]) { + fb.wrapRow(startsWithNote(stmt)) + } + activityID := fb.addStatement(stmt) if fb.takePendingJoin(lastID, pendingCase, fb.previousStmtAnchor) { pendingCase = "" @@ -142,6 +157,14 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a // overrides its own To. originAnchor, destAnchor := pendingFlowAnchors(fb.previousStmtAnchor, pendingFlowAnchor, stmtAnchor) pendingFlowAnchor = nil + // An edge that crosses from one row to the next leaves the bottom of + // the last element and enters the left of the first — anchored right + // to left it is drawn straight back across the row above, through + // whatever sits between the two columns. + if from, to, wraps := fb.takeWrapAnchors(); wraps && originAnchor == nil && destAnchor == nil { + flow.OriginConnectionIndex = from + flow.DestinationConnectionIndex = to + } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) fb.addPendingErrorHandlerFlowForStatement(lastID, activityID, stmt, statementsReferenceVar(stmts[i+1:], fb.errorHandlerSkipVar)) @@ -238,6 +261,10 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a // end event, so it cannot create the over-connection that pass fixes. fb.resolveJoins() + // Rows last: every object exists and every container has its final size, so + // this is the first point where a row's real depth is known. + fb.separateRows() + return µflows.MicroflowObjectCollection{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, Objects: fb.objects, @@ -571,6 +598,8 @@ func (fb *flowBuilder) addStatement(stmt ast.MicroflowStatement) model.ID { if fb.pendingAnnotations != nil && fb.pendingAnnotations.Position != nil { fb.posX = fb.pendingAnnotations.Position.X fb.posY = fb.pendingAnnotations.Position.Y + } else { + fb.clearLowerLane(stmt) } if fb.pendingAnnotations != nil { for _, note := range fb.pendingAnnotations.FreeNotes { diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index f1c895180..07274f92e 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -158,7 +158,7 @@ func (e *annotationEmitter) labelFor(id model.ID) (label string, first bool) { // exactly as before. Only a note that is shared, or that has been moved or // resized on the canvas, pays for the longer form — so this fix does not churn // the output of every microflow that has a note in it. -func (e *annotationEmitter) lines(target model.ID, activityPos model.Point, indentStr string) []string { +func (e *annotationEmitter) lines(target model.ID, activityPos model.Point, targetHeight int, indentStr string) []string { if e == nil { return nil } @@ -178,7 +178,7 @@ func (e *annotationEmitter) lines(target model.ID, activityPos model.Point, inde continue } - defPos, defSize := defaultAnnotationGeometry(activityPos, i) + defPos, defSize := defaultAnnotationGeometry(activityPos, i, targetHeight) var params []string if note.Position != defPos { params = append(params, fmt.Sprintf("position: (%d, %d)", note.Position.X, note.Position.Y)) @@ -802,7 +802,7 @@ func emitObjectAnnotations( } // @annotation (attached Annotation objects) - *lines = append(*lines, annotationsByTarget.lines(currentID, pos, indentStr)...) + *lines = append(*lines, annotationsByTarget.lines(currentID, pos, objectHeight(obj), indentStr)...) } // emitActivityStatement appends the formatted activity statement (with error handling) @@ -1776,7 +1776,7 @@ func emitEnumSplitStatement( branches := []enumBranch{} branchByDestination := map[model.ID]int{} var elseFlow *microflows.SequenceFlow - for _, flow := range orderedEnumSplitFlows(findNormalFlows(flowsByOrigin[currentID])) { + for _, flow := range orderedEnumSplitFlows(findNormalFlows(flowsByOrigin[currentID]), activityMap) { caseValue, ok := enumCaseValue(flow) if !ok { elseFlow = flow @@ -1929,10 +1929,34 @@ func inheritanceCaseName(flow *microflows.SequenceFlow, entityNames map[model.ID return "", false } -func orderedEnumSplitFlows(flows []*microflows.SequenceFlow) []*microflows.SequenceFlow { +// orderedEnumSplitFlows puts a split's case flows back in the order they were +// written. Stored flow order does not survive serialization, so the order is read +// from what does: the anchor pair (splitCaseOrder), then how far down the canvas the +// branch sits. +// +// The pair alone used to carry the whole order, one distinct pair per case. A split +// of four or more cases now shares three pairs — top, right and bottom of the split, +// each arriving on the left (enumSplitOriginAnchors) — which the table already ranks +// in that order, and the branches inside a group are stacked top to bottom in case +// order, so their Y finishes the job. A model written with one pair per case never +// reaches the tie-break and reads exactly as before; a split drawn by hand in Studio +// Pro reads side first, then top to bottom. +func orderedEnumSplitFlows(flows []*microflows.SequenceFlow, activityMap map[model.ID]microflows.MicroflowObject) []*microflows.SequenceFlow { ordered := append([]*microflows.SequenceFlow(nil), flows...) + branchY := func(flow *microflows.SequenceFlow) int { + if flow == nil { + return 0 + } + if obj := activityMap[flow.DestinationID]; obj != nil { + return obj.GetPosition().Y + } + return 0 + } sort.SliceStable(ordered, func(i, j int) bool { - return splitCaseOrder(ordered[i]) < splitCaseOrder(ordered[j]) + if ri, rj := splitCaseOrder(ordered[i]), splitCaseOrder(ordered[j]); ri != rj { + return ri < rj + } + return branchY(ordered[i]) < branchY(ordered[j]) }) return ordered } @@ -2319,7 +2343,7 @@ func collectErrorHandlerStatements( // note and the read path drops it, which is the same round-trip loss #1077 // is about, one nesting level down. notes := func(obj microflows.MicroflowObject, indentStr string) { - statements = append(statements, annotationsByTarget.lines(obj.GetID(), obj.GetPosition(), indentStr)...) + statements = append(statements, annotationsByTarget.lines(obj.GetID(), obj.GetPosition(), objectHeight(obj), indentStr)...) } splitMergeMap := findErrorHandlerSplitMergePoints(ctx, activityMap, flowsByOrigin) diff --git a/mdl/executor/cmd_microflows_show_helpers_test.go b/mdl/executor/cmd_microflows_show_helpers_test.go index 750c7fe30..2cf881784 100644 --- a/mdl/executor/cmd_microflows_show_helpers_test.go +++ b/mdl/executor/cmd_microflows_show_helpers_test.go @@ -553,6 +553,6 @@ func TestFormatErrorHandlingSuffix_RollbackIsNotEmitted(t *testing.T) { // note, so this test exercises the SHORT emit form rather than accidentally // asserting escaping on the parameterised one. func mustDefaultAnnotationPos(activity model.Point, index int) model.Point { - pos, _ := defaultAnnotationGeometry(activity, index) + pos, _ := defaultAnnotationGeometry(activity, index, ActivityHeight) return pos } diff --git a/mdl/executor/layout.go b/mdl/executor/layout.go index b63cf693d..1ec4907c7 100644 --- a/mdl/executor/layout.go +++ b/mdl/executor/layout.go @@ -58,22 +58,98 @@ func (m *layoutMeasurer) measureStatements(stmts []ast.MicroflowStatement) Bound return Bounds{Width: 0, Height: 0} } - totalWidth := 0 + // cursor is the right edge of what sits on the main line; laneBusy is how far the + // lane underneath it is taken by a guard's branch (see lowerLane in + // layout_lanes.go), which the main line does not wait for. + cursor, laneBusy, extent := 0, 0, 0 maxHeight := ActivityHeight + var prev ast.MicroflowStatement for _, stmt := range stmts { bounds := m.measureStatement(stmt) maxHeight = max(maxHeight, bounds.Height) if bounds.Width == 0 { continue } - if totalWidth > 0 { - totalWidth += HorizontalSpacing + left := cursor + if prev != nil { + left += gapBetween(prev, stmt) } - totalWidth += bounds.Width + if left < laneBusy && reachesBelowMainLine(stmt, bounds) { + left = laneBusy + } + if guard, ok := stmt.(*ast.IfStmt); ok && isGuard(guard) { + branchRight := left + guardBranchInset + m.measureBranch(guard.ThenBody).Width + cursor = left + guardMainWidth + laneBusy = branchRight + laneGap + extent = max(extent, branchRight) + } else { + cursor = left + bounds.Width + } + extent = max(extent, cursor) + prev = stmt + } + + return Bounds{Width: extent, Height: maxHeight} +} + +// measureBranch measures a branch body including the end event a trailing RETURN +// draws. measureStatements gives RETURN no width, because on the main line the end +// event is the flow's own; in a branch it is an extra element one pitch past the last +// activity, and a merge placed by the activities alone lands on top of it. +func (m *layoutMeasurer) measureBranch(stmts []ast.MicroflowStatement) Bounds { + b := m.measureStatements(stmts) + if !lastStmtIsReturn(stmts) { + return b } + if b.Width == 0 { + // A bare RETURN: the end event stands where the first activity would. + b.Width = ActivityWidth/2 + EventSize/2 + } else { + b.Width += HorizontalSpacing - ActivityWidth/2 + EventSize/2 + } + return b +} - return Bounds{Width: totalWidth, Height: maxHeight} +// gapBetween is the empty space the builder actually leaves between two consecutive +// elements of a run, edge to edge. +// +// It used to be HorizontalSpacing for every pair. But HorizontalSpacing is a +// centre-to-centre pitch — the builder does `posX += spacing` and centres each +// activity on posX — so adding it on top of both widths counted the activity twice: +// two activities were measured 400 wide and occupy 280. Every IF sized its branch +// from that, so a merge sat a whole activity further right per extra statement in +// the branch, and the main line ran empty underneath it. +// +// Only the pairs whose arithmetic is known exactly are tightened; anything else +// keeps the old, generous gap, because a merge placed short of its branch's content +// is worse than a merge placed long. +func gapBetween(prev, next ast.MicroflowStatement) int { + // Distance from prev's measured right edge to the centre the builder gives next. + var toNextCentre int + switch prev.(type) { + case *ast.IfStmt: + // After an IF, posX = (measured right edge) + HorizontalSpacing/2. + toNextCentre = HorizontalSpacing / 2 + case *ast.EnumSplitStmt, *ast.InheritanceSplitStmt: + return HorizontalSpacing + default: + // A simple activity, or a loop box: after either, the builder centres the + // next element one activity-gap plus half an activity past the right edge. + // A simple activity: centre-to-centre pitch, less its own right half. + toNextCentre = HorizontalSpacing - ActivityWidth/2 + } + // How far next reaches left of the centre it is given. + var leftHalf int + switch next.(type) { + case *ast.IfStmt, *ast.EnumSplitStmt: + leftHalf = SplitWidth / 2 + default: + // An activity — and a loop box, whose left edge the builder now puts where + // an activity's would be. + leftHalf = ActivityWidth / 2 + } + return max(toNextCentre-leftHalf, 20) } // measureStatementsSpan returns the horizontal extent a statement run actually @@ -139,12 +215,12 @@ func (m *layoutMeasurer) measureEnumSplitStatement(s *ast.EnumSplitStmt) Bounds maxBranchWidth := 0 var branchHeights []int for _, c := range s.Cases { - bounds := m.measureStatements(c.Body) + bounds := m.measureBranch(c.Body) maxBranchWidth = max(maxBranchWidth, bounds.Width) branchHeights = append(branchHeights, max(bounds.Height, ActivityHeight)) } if len(s.ElseBody) > 0 { - bounds := m.measureStatements(s.ElseBody) + bounds := m.measureBranch(s.ElseBody) maxBranchWidth = max(maxBranchWidth, bounds.Width) branchHeights = append(branchHeights, max(bounds.Height, ActivityHeight)) } @@ -161,7 +237,7 @@ func (m *layoutMeasurer) measureEnumSplitStatement(s *ast.EnumSplitStmt) Bounds } totalHeight += (len(branchHeights) - 1) * BranchGap - width := SplitWidth + HorizontalSpacing/2 + maxBranchWidth + HorizontalSpacing/2 + MergeSize + width := SplitWidth + HorizontalSpacing/2 + maxBranchWidth + MergeSize return Bounds{Width: width, Height: totalHeight} } @@ -195,10 +271,10 @@ func (m *layoutMeasurer) measureInheritanceSplitStatement(s *ast.InheritanceSpli // - IF without ELSE: FALSE path horizontal, TRUE path below func (m *layoutMeasurer) measureIfStatement(s *ast.IfStmt) Bounds { // Measure THEN branch - thenBounds := m.measureStatements(s.ThenBody) + thenBounds := m.measureBranch(s.ThenBody) // Measure ELSE branch - elseBounds := m.measureStatements(s.ElseBody) + elseBounds := m.measureBranch(s.ElseBody) // Width: split + max(then, else) + merge + spacing branchWidth := max(thenBounds.Width, elseBounds.Width) @@ -207,7 +283,15 @@ func (m *layoutMeasurer) measureIfStatement(s *ast.IfStmt) Bounds { branchWidth = HorizontalSpacing / 2 } - totalWidth := SplitWidth + HorizontalSpacing/2 + branchWidth + HorizontalSpacing/2 + MergeSize + 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 + // branch (addIfStatement's no-merge exit). Measuring a merge and its spacing + // that are never drawn left 120px of empty main line after every guard nested + // in a branch. + if isGuard(s) { + totalWidth = SplitWidth + HorizontalSpacing/2 + thenBounds.Width + } // Height depends on layout strategy var totalHeight int diff --git a/mdl/executor/layout_helpers_test.go b/mdl/executor/layout_helpers_test.go new file mode 100644 index 000000000..4e699936b --- /dev/null +++ b/mdl/executor/layout_helpers_test.go @@ -0,0 +1,65 @@ +// 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" +) + +// logStatements returns n log activities, which the builder lays out as a straight +// horizontal run of ActivityWidth boxes one HorizontalSpacing apart. +func logStatements(n int) []ast.MicroflowStatement { + stmts := make([]ast.MicroflowStatement, 0, n) + for i := 0; i < n; i++ { + stmts = append(stmts, &ast.LogStmt{Level: ast.LogInfo, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "x"}}) + } + return stmts +} + +// buildRows lays out statements through a builder configured the way a whole +// microflow is built, and returns the activities in the order they were placed. +func buildRows(t *testing.T, stmts []ast.MicroflowStatement) []microflows.MicroflowObject { + 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{}}, + } + fb.buildFlowGraph(stmts, nil) + return fb.objects +} + +func activityPositions(objects []microflows.MicroflowObject) []model.Point { + var out []model.Point + for _, o := range objects { + if _, ok := o.(*microflows.ActionActivity); ok { + out = append(out, o.GetPosition()) + } + } + return out +} + +func byIDPosition(objects []microflows.MicroflowObject, id model.ID) (model.Point, bool) { + for _, o := range objects { + if o.GetID() == id { + return o.GetPosition(), true + } + } + return model.Point{}, false +} + +func abs(v int) int { + if v < 0 { + return -v + } + return v +} diff --git a/mdl/executor/layout_lanes.go b/mdl/executor/layout_lanes.go new file mode 100644 index 000000000..827efc98b --- /dev/null +++ b/mdl/executor/layout_lanes.go @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package executor - the lane under the main line. +// +// A guard — `if X then ...; return; end if`, no ELSE — draws its branch in the lane +// below the main line and ends it there, in an end event. Nothing comes back up. The +// main line nevertheless used to wait for it: the next element was placed past the +// branch's far end, so every guard left a stretch of bare line above its own branch. +// Measured on a flow of three guards, each split stood 370px from the activity after +// it, with a 40px gap everywhere else. +// +// The main line now resumes one pitch after the split, over the branch, and the lane +// remembers how far it is taken. Only an element that reaches down into that lane — +// another decision, a loop box, an activity with an error handler under it — has to +// wait for it to clear; a plain activity does not. +package executor + +import "github.com/mendixlabs/mxcli/mdl/ast" + +const ( + // guardBranchInset is where a guard's branch starts, measured the way the + // measurer measures: from the split's x to the left edge of the branch's first + // activity (thenStartX - ActivityWidth/2). + guardBranchInset = SplitWidth + HorizontalSpacing/2 - ActivityWidth/2 + + // guardMainWidth is what a guard takes of the main line, in the measurer's terms: + // gapBetween adds HorizontalSpacing/2 after an IF, and the builder puts the next + // element at thenStartX = split x + SplitWidth + HorizontalSpacing/2. + guardMainWidth = SplitWidth + + // laneGap is the clear space kept after a branch before the lane is used again: + // the same edge-to-edge gap two activities have on the main line. + laneGap = HorizontalSpacing - ActivityWidth +) + +// isGuard reports whether an IF is drawn as a guard: no ELSE, and a THEN that ends +// the flow, so there is no merge and the branch never rejoins the main line. +func isGuard(s *ast.IfStmt) bool { + return len(s.ElseBody) == 0 && !s.HasElse && lastStmtIsReturn(s.ThenBody) +} + +// reachesBelowMainLine reports whether a statement draws anything under the main +// line's own row of activities. bounds is its measured size. +func reachesBelowMainLine(stmt ast.MicroflowStatement, bounds Bounds) bool { + if bounds.Height > ActivityHeight { + return true + } + // A custom error handler's body is laid out under its activity and is not part + // of the activity's measured size. + return len(getErrorHandlerBody(stmt)) > 0 +} + +// leftHalfOf is the distance from a statement's x to its left edge, as gapBetween +// counts it. +func leftHalfOf(stmt ast.MicroflowStatement) int { + switch stmt.(type) { + case *ast.IfStmt, *ast.EnumSplitStmt: + return SplitWidth / 2 + } + return ActivityWidth / 2 +} + +// reserveLowerLane records that the lane under the main line at centerY is taken up +// to untilX (a left-edge limit: the next element reaching into the lane starts there +// or later). +func (fb *flowBuilder) reserveLowerLane(centerY, untilX int) { + if fb.lowerLane == nil { + fb.lowerLane = map[int]int{} + } + if untilX > fb.lowerLane[centerY] { + fb.lowerLane[centerY] = untilX + } +} + +// clearLowerLane moves the cursor right until the statement about to be placed is +// clear of whatever already occupies the lane under this main line. A statement that +// stays on the main line is left where it is. +func (fb *flowBuilder) clearLowerLane(stmt ast.MicroflowStatement) { + busy, ok := fb.lowerLane[fb.posY] + if !ok || fb.measurer == nil { + return + } + if !reachesBelowMainLine(stmt, fb.measurer.measureStatement(stmt)) { + return + } + if half := leftHalfOf(stmt); fb.posX-half < busy { + fb.posX = busy + half + } +} diff --git a/mdl/executor/layout_lanes_test.go b/mdl/executor/layout_lanes_test.go new file mode 100644 index 000000000..88eb6e6d0 --- /dev/null +++ b/mdl/executor/layout_lanes_test.go @@ -0,0 +1,148 @@ +// 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" +) + +func guardStmt() *ast.IfStmt { + return &ast.IfStmt{ + Condition: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}, + ThenBody: []ast.MicroflowStatement{ + &ast.LogStmt{Level: ast.LogInfo, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "refused"}}, + &ast.ReturnStmt{}, + }, + } +} + +func splitsOf(objects []microflows.MicroflowObject) []model.Point { + var out []model.Point + for _, o := range objects { + if _, ok := o.(*microflows.ExclusiveSplit); ok { + out = append(out, o.GetPosition()) + } + } + return out +} + +func TestMainLineDoesNotWaitForAGuardsBranch(t *testing.T) { + // The guard's branch is in the lane below and ends there. The activity after the + // guard used to be placed past the branch's far end — 370px from the split, with + // bare line over the branch; it belongs one pitch after the split. + stmts := []ast.MicroflowStatement{guardStmt()} + stmts = append(stmts, logStatements(1)...) + objects := buildRows(t, stmts) + + split := splitsOf(objects)[0] + var next, inBranch model.Point + for _, p := range activityPositions(objects) { + if p.Y == split.Y { + next = p + } else { + inBranch = p + } + } + if next.X != inBranch.X { + t.Fatalf("activity after the guard at x=%d, branch starts at x=%d: they share a column", next.X, inBranch.X) + } + if got, want := next.X-split.X, SplitWidth+HorizontalSpacing/2; got != want { + t.Fatalf("activity after the guard is %d px from the split, want %d", got, want) + } + if inBranch.Y-ActivityHeight/2 < next.Y+ActivityHeight/2 { + t.Fatalf("branch activity at y=%d touches the main line activity at y=%d", inBranch.Y, next.Y) + } +} + +func TestSecondGuardWaitsForTheLaneToClear(t *testing.T) { + // Two guards in a row both drop a branch into the same lane. The second split may + // not start until the first branch — end event included — is out of the way. + objects := buildRows(t, []ast.MicroflowStatement{guardStmt(), guardStmt()}) + splits := splitsOf(objects) + if len(splits) != 2 { + t.Fatalf("expected 2 splits, got %d", len(splits)) + } + firstBranchRight := 0 + for _, o := range objects { + p := o.GetPosition() + if p.Y == splits[0].Y || p.X > splits[1].X { + continue + } + w := ActivityWidth + if ws, ok := o.(interface{ GetSize() model.Size }); ok && ws.GetSize().Width > 0 { + w = ws.GetSize().Width + } + firstBranchRight = max(firstBranchRight, p.X+w/2) + } + if firstBranchRight == 0 { + t.Fatal("found nothing in the first guard's branch") + } + if left := splits[1].X - SplitWidth/2; left < firstBranchRight { + t.Fatalf("second split starts at x=%d, inside the first guard's branch (ends x=%d)", left, firstBranchRight) + } + // And the two branches themselves may not share space. + var branch []model.Point + for _, p := range activityPositions(objects) { + if p.Y != splits[0].Y { + branch = append(branch, p) + } + } + if len(branch) != 2 || abs(branch[0].X-branch[1].X) < ActivityWidth { + t.Fatalf("branch activities overlap: %v", branch) + } +} + +func TestMergeSitsOneGapAfterItsBranch(t *testing.T) { + // 120px used to separate a branch's last activity from the merge, against 40px + // between any two activities. + objects := buildRows(t, []ast.MicroflowStatement{&ast.IfStmt{ + Condition: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}, + ThenBody: logStatements(2), + ElseBody: logStatements(1), + HasElse: true, + }}) + var mergeLeft, lastRight int + for _, o := range objects { + switch o.(type) { + case *microflows.ExclusiveMerge: + mergeLeft = o.GetPosition().X - MergeSize/2 + case *microflows.ActionActivity: + lastRight = max(lastRight, o.GetPosition().X+ActivityWidth/2) + } + } + if gap := mergeLeft - lastRight; gap != HorizontalSpacing-ActivityWidth { + t.Fatalf("merge is %d px after its branch, want %d", gap, HorizontalSpacing-ActivityWidth) + } +} + +func TestReturningBranchIsMeasuredWithItsEndEvent(t *testing.T) { + // The merge is placed by the widest branch. A branch that ends in RETURN draws an + // end event one pitch past its last activity; measured without it, a tightened + // merge lands on that event. + split := enumSplitWithBranchCount(3) + split.Cases[1].Body = append(split.Cases[1].Body, &ast.ReturnStmt{}) // the branch on the centre line + objects := buildRows(t, []ast.MicroflowStatement{split}) + + var merge, end microflows.MicroflowObject + for _, o := range objects { + switch o.(type) { + case *microflows.ExclusiveMerge: + merge = o + case *microflows.EndEvent: + if end == nil { + end = o + } + } + } + if merge == nil || end == nil { + t.Fatal("expected a merge and a branch end event") + } + if end.GetPosition().Y == merge.GetPosition().Y && + merge.GetPosition().X-MergeSize/2 < end.GetPosition().X+EventSize/2 { + t.Fatalf("merge at x=%d sits on the branch's end event at x=%d", merge.GetPosition().X, end.GetPosition().X) + } +} diff --git a/mdl/executor/layout_rows.go b/mdl/executor/layout_rows.go new file mode 100644 index 000000000..ab3fc1c2a --- /dev/null +++ b/mdl/executor/layout_rows.go @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package executor - wrapping a long main line onto several rows. +// +// The builder lays the happy path left to right and never stops: every top-level +// statement advances posX by one HorizontalSpacing, so a flow is exactly as wide as +// it is long. Measured on a generated app of 41 microflows, with no `@position` +// anywhere, nothing overlapped — and the widest flow was 6930x160 px, 43:1, four +// screens of horizontal scrolling for something 160 px tall. A reviewer cannot see +// such a flow; Studio Pro draws what is stored and does not re-arrange. +// +// Wrapping breaks that line into rows of bounded width. The next row starts below +// everything the current row occupies — branch lanes and loop boxes included, which +// is what a fixed vertical step gets wrong — so a row can be as deep as it needs and +// the one after it still clears it. +// +// It applies to the main flow only, and only past MaxRowWidth: a flow that already +// fits on a screen keeps the coordinates it has today, which is what keeps the +// existing layout tests (and every round trip through DESCRIBE) unchanged. +package executor + +import ( + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" +) + +const ( + // MaxRowWidth is how far the main line may run before the next statement starts + // a new row, measured from the row's first element. Studio Pro's canvas on a + // 1920px screen is about 1500px wide once the toolbox and properties pane are + // open; two of those is what a reader will scroll sideways without losing the + // thread, and it keeps a pair of loops — or a flow of eighteen activities — on + // one line, where one screen's worth stacked them into rows joined by a long + // diagonal. Past that, the flow wraps. + MaxRowWidth = 2880 + + // RowGap is the empty space between the bottom of one row and the top of the + // next, edge to edge. Studio Pro prints an activity's output variable and its type + // under the box, about 35px of text, and the line that joins two rows has to cross + // the whole diagram in this band: at BranchGap (40) the text of one row touched the + // boxes of the next and the joining line ran through both. + RowGap = VerticalSpacing + + // RowOverhang is how far past MaxRowWidth a row may run when that finishes the + // flow. Without it a flow a little over the limit wrapped its last two activities + // and the end event onto a row of their own — a second row three elements long + // under one of sixteen, reached by a line back across the whole diagram. + RowOverhang = 2 * HorizontalSpacing +) + +// rowTracker remembers where the current row began, so wrapRow can put the next row +// under the full extent of this one rather than a fixed step below its centre line. +type rowTracker struct { + // startX is the x every row begins at: the first statement's x, not the start + // event's, so a flow whose first statement carries an @position wraps back to + // that column instead of to the canvas origin. + startX int + // firstObject is the index into fb.objects of the first object of this row. + // Everything from there on contributes to the row's depth. + firstObject int + // pendingWrapEdge marks that the next flow created on the main line crosses + // from one row to the next, so it needs the anchors that keep it out of the + // row above (takeWrapAnchors). + pendingWrapEdge bool + // wrapNoteAbove says the element starting the new row has a note over it, which + // is where the joining line would otherwise arrive. + wrapNoteAbove bool + // starts is the index into fb.objects where each row begins, in order. The + // final pass walks it to push rows apart where the estimate fell short. + starts []int +} + +// rowBottom returns the lowest edge occupied by the objects placed in the current +// row. An object's Position is its centre (RelativeMiddlePoint), so its bottom is +// the centre plus half its height; a loop box contributes its own full height, +// which is how a row containing a loop pushes the next row far enough down. +func (fb *flowBuilder) rowBottom() int { + bottom := fb.posY + ActivityHeight/2 + if fb.row.firstObject > len(fb.objects) { + return bottom + } + for _, o := range fb.objects[fb.row.firstObject:] { + if o == nil { + continue + } + p := o.GetPosition() + height := ActivityHeight + if withSize, ok := o.(interface{ GetSize() model.Size }); ok { + if h := withSize.GetSize().Height; h > 0 { + height = h + } + } + if b := p.Y + height/2; b > bottom { + bottom = b + } + } + return bottom +} + +// shouldWrap reports whether the next top-level statement would take the main line +// past MaxRowWidth. It is asked before the statement is placed, so posX is where +// that statement's centre would go. +// +// rest is the statement and everything after it on the main line. +// +// A statement that draws nothing — a RETURN, which the builder turns into the end +// event already on the happy path — never starts a row: wrapping there stranded a +// 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 { + return false + } + width := ActivityWidth + if fb.measurer != nil { + w := fb.measurer.measureStatement(stmt).Width + if w == 0 { + return false + } + width = w + } + // 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 { + 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 { + return false + } + return true +} + +// startsWithNote reports whether a statement carries a note, which is drawn above it. +func startsWithNote(stmt ast.MicroflowStatement) bool { + ann := getStatementAnnotations(stmt) + return ann != nil && (len(ann.Notes) > 0 || len(ann.FreeNotes) > 0) +} + +// wrapRow moves the cursor to the start of a new row, below everything the current +// row occupies. noteAbove says the element starting the new row carries a note, which +// decides where the joining line arrives (takeWrapAnchors). +// +// The new centre line is placed as if an activity started the row. That is only a +// first position: a loop box hangs half its height above its centre line, a note sits +// above that, and neither size is known until the element is built. separateRows +// measures the finished rows and moves them apart, so no estimate is made here. +func (fb *flowBuilder) wrapRow(noteAbove bool) { + next := fb.rowBottom() + RowGap + ActivityHeight/2 + fb.posX = fb.row.startX + fb.posY = next + fb.baseY = next + fb.row.firstObject = len(fb.objects) + fb.row.pendingWrapEdge = true + fb.row.wrapNoteAbove = noteAbove + fb.row.starts = append(fb.row.starts, fb.row.firstObject) +} + +// takeWrapAnchors is asked once per flow on the main line and answers only for the +// one that crosses from the last element of a row to the first of the next — the one +// flow that travels backwards. It leaves the BOTTOM of its origin and arrives on TOP +// of its destination, so it runs through the empty band between the rows. +// +// Anchored right-to-left, as every other flow is, it is drawn straight back across +// the row it just left. Bottom-to-left cleared that row but still arrived from the +// right, over the first elements of the new row, to reach the far side of the first +// one. Only an element with a note above it keeps the left side: the note stands +// exactly where the line would come down. +func (fb *flowBuilder) takeWrapAnchors() (origin, destination int, ok bool) { + if !fb.row.pendingWrapEdge { + return 0, 0, false + } + fb.row.pendingWrapEdge = false + if fb.row.wrapNoteAbove { + return AnchorBottom, AnchorLeft, true + } + return AnchorBottom, AnchorTop, true +} + +// noteRowStart records where the main line begins, once the first top-level +// statement's column is known. +func (fb *flowBuilder) noteRowStart() { + fb.row.startX = fb.posX + fb.row.firstObject = len(fb.objects) + fb.row.starts = []int{fb.row.firstObject} +} + +// separateRows pushes each row down until it clears the one above it by RowGap, +// measuring what was actually built. +// +// A row's depth cannot be known when the row is begun: a loop box is sized from its +// body AFTER the body is laid out (fitContainerSize) — the measurer's guess for a +// one-branch body is 250 against 310 fitted — and a note adds its own height on top. +// Rather than estimate, wrapRow starts each row at an activity's depth and this pass +// measures the finished geometry once and shifts whole rows. Notes are objects too, +// so they are part of what is measured. +func (fb *flowBuilder) separateRows() { + if !fb.allowWrap || len(fb.row.starts) < 2 { + return + } + bounds := func(from, to int) (top, bottom int, ok bool) { + for i := from; i < to && i < len(fb.objects); i++ { + o := fb.objects[i] + 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 + } + shift := func(from int, dy int) { + for i := from; i < len(fb.objects); i++ { + o := fb.objects[i] + if o == nil { + continue + } + p := o.GetPosition() + o.SetPosition(model.Point{X: p.X, Y: p.Y + dy}) + } + } + for r := 1; r < len(fb.row.starts); r++ { + end := len(fb.objects) + if r+1 < len(fb.row.starts) { + end = fb.row.starts[r+1] + } + _, prevBottom, okPrev := bounds(fb.row.starts[r-1], fb.row.starts[r]) + top, _, okRow := bounds(fb.row.starts[r], end) + if !okPrev || !okRow { + continue + } + if gap := top - prevBottom; gap < RowGap { + shift(fb.row.starts[r], RowGap-gap) + } + } +} + +// ownPositionX returns the x of a statement that places itself with @position. Such +// a statement is never moved: its coordinates round-trip through DESCRIBE, so +// wrapping it would make a describe→exec cycle rewrite the model it just read. The +// row re-anchors to it instead, so the statements after it wrap to its column. +func ownPositionX(stmt ast.MicroflowStatement) (int, bool) { + ann := getStatementAnnotations(stmt) + if ann == nil || ann.Position == nil { + return 0, false + } + return ann.Position.X, true +} diff --git a/mdl/executor/layout_rows_test.go b/mdl/executor/layout_rows_test.go new file mode 100644 index 000000000..cabebbf45 --- /dev/null +++ b/mdl/executor/layout_rows_test.go @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A long main line used to run off the canvas: every top-level statement advanced +// posX and nothing ever moved down. Measured on a generated app of 41 microflows +// with no @position anywhere, the widest flow was 6930x160 px — 43:1, four screens +// of horizontal scrolling. Nothing overlapped; it simply could not be read. +package executor + +import ( + "sort" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestShortFlowIsNotWrapped(t *testing.T) { + // Eight activities span 200..1320, inside MaxRowWidth: the coordinates every + // existing layout test pins must not move. + positions := activityPositions(buildRows(t, logStatements(8))) + if len(positions) != 8 { + t.Fatalf("expected 8 activities, got %d", len(positions)) + } + for i, p := range positions { + wantX := 360 + i*HorizontalSpacing + if p.X != wantX || p.Y != 200 { + t.Fatalf("activity %d at (%d,%d), want (%d,200) — a flow that fits must lay out exactly as before", + i, p.X, p.Y, wantX) + } + } +} + +func TestLongFlowWrapsOntoRowsThatDoNotTouch(t *testing.T) { + positions := activityPositions(buildRows(t, logStatements(30))) + if len(positions) != 30 { + t.Fatalf("expected 30 activities, got %d", len(positions)) + } + + var maxX, rows int + lastY := positions[0].Y + for _, p := range positions { + if p.X > maxX { + maxX = p.X + } + if p.Y != lastY { + rows++ + lastY = p.Y + } + } + if rows == 0 { + t.Fatalf("30 activities stayed on one row %d px wide; the flow should have wrapped", maxX) + } + // The row starts at the first activity's column, one spacing past the start event. + rowStart := positions[0].X + if maxX-rowStart > MaxRowWidth { + t.Fatalf("row runs from x=%d to x=%d, %d px wide, past MaxRowWidth=%d", + rowStart, maxX, maxX-rowStart, MaxRowWidth) + } + + // No two activities may share space, and rows keep RowGap between them. + for i := 0; i < len(positions); i++ { + for j := i + 1; j < len(positions); j++ { + dx := abs(positions[i].X - positions[j].X) + dy := abs(positions[i].Y - positions[j].Y) + if dx < ActivityWidth && dy < ActivityHeight { + t.Fatalf("activities %d and %d overlap: (%d,%d) and (%d,%d)", + i, j, positions[i].X, positions[i].Y, positions[j].X, positions[j].Y) + } + } + } +} + +func TestWrapEdgeLeavesTheBottomOfTheRow(t *testing.T) { + // The edge from the last element of a row to the first of the next is the one + // flow that travels backwards. Anchored right-to-left it is drawn straight + // across the row it just left; arriving on the left it still comes in from the + // right, over the first activities of the new row. Bottom to top keeps it in the + // band between the rows. + objects := buildRows(t, logStatements(30)) + positions := activityPositions(objects) + byID := map[model.ID]model.Point{} + for _, o := range objects { + byID[o.GetID()] = o.GetPosition() + } + _ = positions + + 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{}}, + } + fb.buildFlowGraph(logStatements(30), nil) + + var wrapEdges int + for _, flow := range fb.flows { + from, okFrom := byIDPosition(fb.objects, flow.OriginID) + to, okTo := byIDPosition(fb.objects, flow.DestinationID) + if !okFrom || !okTo || to.Y <= from.Y || to.X >= from.X { + continue // not a backwards, downwards edge + } + wrapEdges++ + if flow.OriginConnectionIndex != AnchorBottom || flow.DestinationConnectionIndex != AnchorTop { + t.Fatalf("wrap edge (%d,%d)->(%d,%d) anchored %d->%d, want bottom(%d)->top(%d)", + from.X, from.Y, to.X, to.Y, + flow.OriginConnectionIndex, flow.DestinationConnectionIndex, AnchorBottom, AnchorTop) + } + } + if wrapEdges == 0 { + t.Fatal("no row-crossing edge found; the flow did not wrap") + } +} + +// loopOverBranch is a loop whose body is an IF with both branches: a box the measurer +// sizes smaller than fitContainerSize makes it once the body is laid out. +func loopOverBranch() *ast.LoopStmt { + return &ast.LoopStmt{LoopVariable: "Item", ListVariable: "List", Body: []ast.MicroflowStatement{ + &ast.IfStmt{ + Condition: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}, + ThenBody: logStatements(2), + ElseBody: logStatements(2), + HasElse: true, + }, + }} +} + +// rowBoxes returns the top and bottom edge of every loop box, grouped by row (boxes +// sharing a centre line), rows in top-to-bottom order. +func rowBoxes(objects []microflows.MicroflowObject) (tops, bottoms []int) { + byRow := map[int][2]int{} + var rows []int + for _, o := range objects { + if _, ok := o.(*microflows.LoopedActivity); !ok { + continue + } + h := o.(interface{ GetSize() model.Size }).GetSize().Height + y := o.GetPosition().Y + edges, seen := byRow[y] + if !seen { + rows = append(rows, y) + edges = [2]int{y - h/2, y + h/2} + } + byRow[y] = [2]int{min(edges[0], y-h/2), max(edges[1], y+h/2)} + } + sort.Ints(rows) + for _, y := range rows { + tops = append(tops, byRow[y][0]) + bottoms = append(bottoms, byRow[y][1]) + } + return tops, bottoms +} + +func TestTallElementStartingARowClearsTheRowAbove(t *testing.T) { + // A loop box hangs half its height above its row's centre line, so a row of loops + // begun at an activity's depth reaches into the row above — two loops on top of + // each other — unless the finished rows are moved apart. + stmts := make([]ast.MicroflowStatement, 0, 12) + for i := 0; i < 12; i++ { + stmts = append(stmts, loopOverBranch()) + } + 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{}}} + fb.buildFlowGraph(stmts, nil) + tops, bottoms := rowBoxes(fb.objects) + if len(tops) < 2 { + t.Fatalf("expected the loops to wrap onto several rows, got %d", len(tops)) + } + for r := 1; r < len(tops); r++ { + if tops[r] < bottoms[r-1]+RowGap { + t.Fatalf("row %d starts at y=%d, %d px under the row above (want at least %d)", + r, tops[r], tops[r]-bottoms[r-1], RowGap) + } + } +} + +func TestRowsKeepTheirGapAfterContainersAreSized(t *testing.T) { + // A loop box is sized from its body after the body is laid out, and the + // measurer's estimate for the same loop is smaller, so a row placed on the + // estimate alone ends up closer to the row above than RowGap. separateRows + // measures what was built. Calling it on a flow whose rows were deliberately + // pulled together shows it is the pass that restores the gap. + stmts := make([]ast.MicroflowStatement, 0, 12) + for i := 0; i < 12; i++ { + stmts = append(stmts, loopOverBranch()) + } + 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{}}} + fb.buildFlowGraph(stmts, nil) + if len(fb.row.starts) < 2 { + t.Fatalf("expected several rows, got %d", len(fb.row.starts)) + } + // Pull the second row up into the first, as an under-estimated reservation would. + for _, o := range fb.objects[fb.row.starts[1]:] { + p := o.GetPosition() + o.SetPosition(model.Point{X: p.X, Y: p.Y - 150}) + } + if tops, bottoms := rowBoxes(fb.objects); tops[1] >= bottoms[0]+RowGap { + t.Fatal("test setup: the rows still clear each other, so there is nothing to repair") + } + fb.separateRows() + tops, bottoms := rowBoxes(fb.objects) + for r := 1; r < len(tops); r++ { + if gap := tops[r] - bottoms[r-1]; gap < RowGap { + t.Fatalf("row %d is %d px under the row above after separateRows (want at least %d)", r, gap, RowGap) + } + } +} + +func TestFlowSlightlyOverTheLimitFinishesOnItsRow(t *testing.T) { + // 20 activities run 80px past MaxRowWidth. Wrapping there left two activities and + // the end event on a row of their own; the row is allowed to overhang instead. + positions := activityPositions(buildRows(t, logStatements(20))) + for i, p := range positions { + if p.Y != positions[0].Y { + t.Fatalf("activity %d of 20 wrapped to y=%d; the tail is short enough to stay on the row", i, p.Y) + } + } + if span := positions[len(positions)-1].X - positions[0].X + ActivityWidth; span <= MaxRowWidth { + t.Fatalf("test flow spans %d, inside MaxRowWidth=%d: it does not exercise the overhang", span, MaxRowWidth) + } +} diff --git a/mdl/executor/layout_span_test.go b/mdl/executor/layout_span_test.go index b0d616e00..408cab5b0 100644 --- a/mdl/executor/layout_span_test.go +++ b/mdl/executor/layout_span_test.go @@ -39,9 +39,13 @@ func TestMeasureStatementsSpan_SimpleRun(t *testing.T) { t.Errorf("span of %d activities = %d, want %d", tc.n, got, tc.want) } if tc.n > 1 { - // The old measure is the one that over-sized the loop box. - if old := m.measureStatements(simpleStmts(tc.n)).Width; old <= got { - t.Errorf("expected measureStatements (%d) to exceed the true span (%d)", old, got) + // measureStatements used to count HorizontalSpacing on top of both + // widths and over-measure a simple run by (n-1)*ActivityWidth — the + // reason this function exists. gapBetween made it exact, so the two + // now agree on a simple run; the span stays as the loop box's measure + // because it bails out to the general one for compound bodies. + if general := m.measureStatements(simpleStmts(tc.n)).Width; general != got { + t.Errorf("measureStatements (%d) should now equal the true span (%d) for a simple run", general, got) } } } diff --git a/mdl/executor/microflow_annotation_sharing_test.go b/mdl/executor/microflow_annotation_sharing_test.go index d7e5bbe0e..042b1961d 100644 --- a/mdl/executor/microflow_annotation_sharing_test.go +++ b/mdl/executor/microflow_annotation_sharing_test.go @@ -216,7 +216,7 @@ func TestTwoNotesOnOneActivity_BothSurvive(t *testing.T) { // and it is only sound because both sides go through // defaultAnnotationGeometry — see the test below. func TestUnsharedNoteAtTheDefault_KeepsTheShortForm(t *testing.T) { - pos, size := defaultAnnotationGeometry(model.Point{X: 100, Y: 100}, 0) + pos, size := defaultAnnotationGeometry(model.Point{X: 100, Y: 100}, 0, ActivityHeight) mf := mfWithNotes([]*microflows.AnnotationFlow{ {BaseElement: model.BaseElement{ID: "af1"}, OriginID: "n1", DestinationID: "a1"}, }, note("n1", "plain note", pos, size)) @@ -264,7 +264,7 @@ func TestMovedNote_CarriesItsGeometryThroughTheRoundTrip(t *testing.T) { func TestAnnotationGeometryDefaultIsSharedByBothSides(t *testing.T) { activity := model.Point{X: 640, Y: 320} for index := 0; index < 3; index++ { - wantPos, wantSize := defaultAnnotationGeometry(activity, index) + wantPos, wantSize := defaultAnnotationGeometry(activity, index, ActivityHeight) fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing, varTypes: map[string]string{}, declaredVars: map[string]string{}}