From 50cf336ffe6d2003ff0715ef01ef624f5da9ec57 Mon Sep 17 00:00:00 2001 From: stagknee <241821830+stagknee@users.noreply.github.com> Date: Tue, 13 Oct 2026 11:06:10 -0500 Subject: [PATCH 1/3] fix: view, external and contract-imported entities use the grid placement CREATE ENTITY places an entity without an @Position on dmlayout.GridSlot, but view entities, external entities and entities imported from a contract kept the old single-row formula, whose 150px step is narrower than an entity box. All three now take the next grid slot after the entities the module already holds; the contract import also counted only its own entities before. Co-Authored-By: Claude Sonnet 5.5 --- ...contract-entities-single-row-position.json | 1 + CHANGELOG.md | 4 + mdl/executor/cmd_contract.go | 6 +- mdl/executor/cmd_entities.go | 3 +- mdl/executor/cmd_odata.go | 6 +- mdl/executor/entity_default_position_test.go | 207 ++++++++++++++++++ 6 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-13-view-external-contract-entities-single-row-position.json create mode 100644 mdl/executor/entity_default_position_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-view-external-contract-entities-single-row-position.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-view-external-contract-entities-single-row-position.json new file mode 100644 index 0000000000..ab25b332ca --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-view-external-contract-entities-single-row-position.json @@ -0,0 +1 @@ +{"date": "2026-10-13", "area": "mdl/executor", "symptom": "View entities, external entities and entities imported with `create external entities from` were placed on one row (X = 100 + n*150, Y = 100), 150px apart: a 40-entity module was a 6,000px line and the entity boxes touched. CREATE ENTITY had already moved to a wrapping grid.", "cause": "dmlayout.GridSlot replaced the single-row formula in the CREATE ENTITY handler only; three other creation paths kept a private copy of the old formula, and the contract import counted only the entities it had created itself (created+updated), ignoring the entities the module already held.", "file": "mdl/executor/cmd_entities.go (execCreateViewEntity), mdl/executor/cmd_odata.go (execCreateExternalEntity), mdl/executor/cmd_contract.go (createExternalEntities)", "fix": "All three call dmlayout.GridSlot(n) with n = entities already in the domain model; the contract import uses the count read before its loop plus the entities created so far (a created entity is not in that domain model value).", "test": "TestCreateViewEntity_DefaultPositionIsTheGridSlot; TestCreateExternalEntity_DefaultPositionIsTheGridSlot; TestCreateExternalEntities_DefaultPositionIsTheGridSlot (mdl/executor/entity_default_position_test.go)", "insight": "**When a default moves into a shared helper, grep for the old formula's literal constants, not the old function**: `grep -rn '\\* *150' mdl/executor --include=*.go` finds the stragglers in seconds, while a search for the call site of the helper never will. The mock backend appends to dm.Entities on CreateEntity but the real one does not mutate the value it handed out, so a count taken inside the loop is right in the test and wrong in production; read it once before the loop."} diff --git a/CHANGELOG.md b/CHANGELOG.md index ab8083df83..29cb181417 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **View entities, external entities and entities imported from a contract take the grid position `create entity` does** — all three still used the old single-row default (`X: 100 + n*150, Y: 100`) that the grid replaced because 150px is narrower than an entity box: a module of 40 of them was a 6,000px line with the boxes touching. They now take the next `GridSlot` after the entities the module already holds, and an `@Position` is still honoured. Before, `create external entities from` counted only the entities it had created itself, so the first imported entity could land on one the module already had. + ## [0.26.0] - 2026-10-11 Headline: **Documents mxcli writes now survive Studio Pro's merge and diff: every element carries exactly the property names Studio Pro writes for the project's Mendix version.** Before, a key mxcli left out, or wrote for a newer version, made a document unmergeable the first time Studio Pro saved it, although `mx check`, the build and the runtime stayed green. `mxcli fix property-sets` repairs projects an older mxcli wrote. Alongside it: a Claude Code companion mod with a Project pane, `describe` → `exec` round trips that keep loop back-edges, shared error handlers and page design properties, `check -p` resolving script sets and security references the way `exec` does, `mxcli run --local` as a background service, and a run of Windows fixes for the local dev loop. The [language poster](https://www.mxcli.org/poster/mxcli-language-poster.html) puts all of mxcli and MDL on one page. diff --git a/mdl/executor/cmd_contract.go b/mdl/executor/cmd_contract.go index eb1620251f..0188b41e70 100644 --- a/mdl/executor/cmd_contract.go +++ b/mdl/executor/cmd_contract.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/dmlayout" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" @@ -525,6 +526,7 @@ func createExternalEntities(ctx *ExecContext, s *ast.CreateExternalEntitiesStmt) return mdlerrors.NewBackend("get domain model", err) } + existingCount := len(dm.Entities) // before the loop: a created entity is not in this dm // Index existing entities by name for upsert existing := make(map[string]*domainmodel.Entity) for _, ent := range dm.Entities { @@ -817,7 +819,9 @@ func createExternalEntities(ctx *ExecContext, s *ast.CreateExternalEntitiesStmt) continue } - location := model.Point{X: 100 + (created+updated)*150, Y: 100} + // The next grid slot after the entities the module already held and the + // ones this import has created so far, as CREATE ENTITY places them. + location := dmlayout.GridSlot(existingCount + created) newEntity := &domainmodel.Entity{ Name: mendixName, Location: location, diff --git a/mdl/executor/cmd_entities.go b/mdl/executor/cmd_entities.go index 8b95c9b836..1fffc7efb9 100644 --- a/mdl/executor/cmd_entities.go +++ b/mdl/executor/cmd_entities.go @@ -1026,7 +1026,8 @@ func execCreateViewEntity(ctx *ExecContext, s *ast.CreateViewEntityStmt) error { } else if existingEntity != nil { location = existingEntity.Location } else { - location = model.Point{X: 100 + len(dm.Entities)*150, Y: 100} + // The next grid slot, as CREATE ENTITY does (see there for why not a row). + location = dmlayout.GridSlot(len(dm.Entities)) } // Create or update ViewEntitySourceDocument (separate document for OQL query). diff --git a/mdl/executor/cmd_odata.go b/mdl/executor/cmd_odata.go index 3b27a6ba16..762cf55cdd 100644 --- a/mdl/executor/cmd_odata.go +++ b/mdl/executor/cmd_odata.go @@ -16,6 +16,7 @@ import ( "github.com/mendixlabs/mxcli/internal/pathutil" "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/dmlayout" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" @@ -947,8 +948,9 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt) return mdlerrors.NewValidation("EntitySet property is required when creating an external entity") } - // Auto-position based on existing entities - location := model.Point{X: 100 + len(dm.Entities)*150, Y: 100} + // No @Position exists for an external entity: take the next grid slot, as + // CREATE ENTITY does. + location := dmlayout.GridSlot(len(dm.Entities)) newEntity := &domainmodel.Entity{ Name: s.Name.Name, diff --git a/mdl/executor/entity_default_position_test.go b/mdl/executor/entity_default_position_test.go new file mode 100644 index 0000000000..2f936a93d6 --- /dev/null +++ b/mdl/executor/entity_default_position_test.go @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/dmlayout" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// CREATE ENTITY places an entity without an @Position on dmlayout.GridSlot(n), +// n being the number of entities already in the domain model. View entities, +// external entities and entities imported from a contract used to keep the +// pre-grid formula `X: 100 + n*150, Y: 100`: one row, and 150px is narrower +// than an entity box, so neighbouring boxes touched. + +// assertGridSlots checks that created[i] sits on GridSlot(first+i) and that no +// two of them share a slot. +func assertGridSlots(t *testing.T, kind string, first int, created []*domainmodel.Entity) { + t.Helper() + seen := map[model.Point]string{} + for i, e := range created { + want := dmlayout.GridSlot(first + i) + if e.Location != want { + t.Errorf("%s %s (#%d in the domain model) is at %+v, want grid slot %+v", kind, e.Name, first+i, e.Location, want) + } + if prev, dup := seen[e.Location]; dup { + t.Errorf("%s %s and %s share the position %+v", kind, prev, e.Name, e.Location) + } + seen[e.Location] = e.Name + } + // Eight entities on top of two existing ones wrap the six-column grid: the + // old formula kept every one of them at y=100. + if len(created) > dmlayout.GridColumns && created[len(created)-1].Location.Y == created[0].Location.Y { + t.Errorf("%s: %d entities are still on one row (y=%d)", kind, len(created), created[0].Location.Y) + } +} + +// domainModelWith returns a domain model that already holds n entities. +func domainModelWith(mod *model.Module, n int) *domainmodel.DomainModel { + dm := &domainmodel.DomainModel{} + dm.ID = nextID("dm") + dm.ContainerID = mod.ID + for i := 0; i < n; i++ { + dm.Entities = append(dm.Entities, mkEntity(mod.ID, fmt.Sprintf("Existing%d", i))) + } + return dm +} + +func TestCreateViewEntity_DefaultPositionIsTheGridSlot(t *testing.T) { + const existing, views = 2, 8 + mod := mkModule("Shop") + dm := domainModelWith(mod, existing) + var created []*domainmodel.Entity + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + CreateEntityFunc: func(_ model.ID, e *domainmodel.Entity) error { + created = append(created, e) + dm.Entities = append(dm.Entities, e) + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + + for i := 0; i < views; i++ { + s := parseViewStmt(t, fmt.Sprintf("create view entity Shop.View%d (\n A: Integer\n) as (\n select 1 as A\n);", i)) + if err := execCreateViewEntity(ctx, s); err != nil { + t.Fatalf("view %d: %v", i, err) + } + } + if len(created) != views { + t.Fatalf("created %d view entities, want %d", len(created), views) + } + assertGridSlots(t, "view entity", existing, created) +} + +// A view entity given an @Position keeps it: the grid slot is only a default. +func TestCreateViewEntity_ExplicitPositionIsKept(t *testing.T) { + mod := mkModule("Shop") + dm := domainModelWith(mod, 3) + var created []*domainmodel.Entity + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + CreateEntityFunc: func(_ model.ID, e *domainmodel.Entity) error { + created = append(created, e) + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + s := parseViewStmt(t, "create view entity Shop.Pinned (\n A: Integer\n) as (\n select 1 as A\n);") + s.Position = &ast.Position{X: 777, Y: 333} + if err := execCreateViewEntity(ctx, s); err != nil { + t.Fatal(err) + } + if len(created) != 1 || created[0].Location != (model.Point{X: 777, Y: 333}) { + t.Fatalf("explicit @Position was not kept: %+v", created) + } +} + +func TestCreateExternalEntity_DefaultPositionIsTheGridSlot(t *testing.T) { + const existing, externals = 2, 8 + mod := mkModule("Clients") + h := mkHierarchy(mod) + svc := &model.ConsumedODataService{BaseElement: model.BaseElement{ID: nextID("cos")}, ContainerID: mod.ID, Name: "OrderODataClient"} + withContainer(h, svc.ContainerID, mod.ID) + dm := domainModelWith(mod, existing) + var created []*domainmodel.Entity + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + ListConsumedODataServicesFunc: func() ([]*model.ConsumedODataService, error) { return []*model.ConsumedODataService{svc}, nil }, + CreateEntityFunc: func(_ model.ID, e *domainmodel.Entity) error { + created = append(created, e) + dm.Entities = append(dm.Entities, e) + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + + for i := 0; i < externals; i++ { + set := fmt.Sprintf("Orders%d", i) + s := &ast.CreateExternalEntityStmt{ + Name: ast.QualifiedName{Module: "Clients", Name: set}, + ServiceRef: ast.QualifiedName{Module: "Clients", Name: "OrderODataClient"}, + EntitySet: &set, + Attributes: []ast.Attribute{{Name: "OrderId", Type: ast.DataType{Kind: ast.TypeLong}}}, + } + assertNoError(t, execCreateExternalEntity(ctx, s)) + } + if len(created) != externals { + t.Fatalf("created %d external entities, want %d", len(created), externals) + } + assertGridSlots(t, "external entity", existing, created) +} + +// manyTypesMetadata is a contract with n entity types and no associations. +func manyTypesMetadata(n int) string { + var entityTypes, sets strings.Builder + for i := 0; i < n; i++ { + fmt.Fprintf(&entityTypes, ``, i) + fmt.Fprintf(&sets, ``, i, i) + } + return ` + +` + entityTypes.String() + + `` + sets.String() + `` +} + +// Entities imported from a contract go on the grid after the entities the +// module already holds — the running count is the module's, not just the +// import's, or the first imported entity would land on top of an existing one. +func TestCreateExternalEntities_DefaultPositionIsTheGridSlot(t *testing.T) { + const existing, imported = 2, 8 + mod := mkModule("CustomModule") + svc := &model.ConsumedODataService{ + BaseElement: model.BaseElement{ID: nextID("cos")}, + ContainerID: mod.ID, + Name: "App", + MetadataUrl: "https://example.com/$metadata", + ODataVersion: "4.0", + Metadata: manyTypesMetadata(imported), + } + h := mkHierarchy(mod) + withContainer(h, svc.ContainerID, mod.ID) + dm := domainModelWith(mod, existing) + + var created []*domainmodel.Entity + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListConsumedODataServicesFunc: func() ([]*model.ConsumedODataService, error) { + return []*model.ConsumedODataService{svc}, nil + }, + GetDomainModelFunc: func(model.ID) (*domainmodel.DomainModel, error) { return dm, nil }, + CreateEntityFunc: func(_ model.ID, e *domainmodel.Entity) error { + created = append(created, e) + dm.Entities = append(dm.Entities, e) + return nil + }, + ProjectVersionFunc: func() *types.ProjectVersion { + return &types.ProjectVersion{MajorVersion: 11, MinorVersion: 12, ProductVersion: "11.12.0"} + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + stmt := &ast.CreateExternalEntitiesStmt{ + ServiceRef: ast.QualifiedName{Module: "CustomModule", Name: "App"}, + TargetModule: "CustomModule", + CreateOrModify: true, + } + assertNoError(t, createExternalEntities(ctx, stmt)) + if len(created) != imported { + t.Fatalf("imported %d entities, want %d", len(created), imported) + } + assertGridSlots(t, "contract entity", existing, created) +} From 2d4007a1a1e60a352905d37b7b8201d1e6c05900 Mon Sep 17 00:00:00 2001 From: stagknee <241821830+stagknee@users.noreply.github.com> Date: Tue, 13 Oct 2026 11:26:42 -0500 Subject: [PATCH 2/3] feat: mxcli layout --only-overlapping moves only entities whose boxes collide The default grid has a fixed row pitch, but a box is as tall as its attribute list, so an entity with many attributes runs into the one below it. The flag moves just the entities whose estimated boxes (or an annotation) collide, to the nearest free spot, and keeps every other position. Deterministic and idempotent: a second run reports no overlapping entities. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 4 + cmd/mxcli/cmd_layout.go | 120 +++++-- cmd/mxcli/cmd_layout_test.go | 104 ++++++ docs-site/src/tools/domain-model-layout.md | 57 +++- mdl/dmlayout/dmlayout_test.go | 21 +- mdl/dmlayout/overlap.go | 285 ++++++++++++++++ mdl/dmlayout/overlap_test.go | 362 +++++++++++++++++++++ 7 files changed, 901 insertions(+), 52 deletions(-) create mode 100644 mdl/dmlayout/overlap.go create mode 100644 mdl/dmlayout/overlap_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 29cb181417..2c28d6997a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **`mxcli layout --only-overlapping` moves only the entities whose boxes collide** — the default grid gives every entity the same 280px row, but a box is as tall as its attribute list, so an entity with about 17 or more attributes ran into the one below it, and the only repair was `mxcli layout`, which replaces every position in the module, including the ones set by hand. With the flag, the command finds the entities whose estimated boxes (the size estimate the full layout uses) intersect each other or a domain-model annotation, keeps the one that comes first in the model, and moves each other colliding entity to the nearest free spot (a 40px margin from every box, never above or left of the origin). Every entity that collides with nothing keeps its position exactly. `--dry-run` lists each collision and each move; a second run reports `no overlapping entities` and writes nothing. `--module` and `--include-marketplace` work as before. + ### Fixed - **View entities, external entities and entities imported from a contract take the grid position `create entity` does** — all three still used the old single-row default (`X: 100 + n*150, Y: 100`) that the grid replaced because 150px is narrower than an entity box: a module of 40 of them was a 6,000px line with the boxes touching. They now take the next `GridSlot` after the entities the module already holds, and an `@Position` is still honoured. Before, `create external entities from` counted only the entities it had created itself, so the first imported entity could land on one the module already had. diff --git a/cmd/mxcli/cmd_layout.go b/cmd/mxcli/cmd_layout.go index 6e821f54ca..8f703b7e3b 100644 --- a/cmd/mxcli/cmd_layout.go +++ b/cmd/mxcli/cmd_layout.go @@ -4,12 +4,14 @@ package main import ( "fmt" + "io" "os" "sort" "strings" "github.com/mendixlabs/mxcli/mdl/dmlayout" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" "github.com/spf13/cobra" ) @@ -31,6 +33,7 @@ var ( layoutModules []string layoutDryRun bool layoutIncludeMarketplace bool + layoutOnlyOverlapping bool ) var layoutCmd = &cobra.Command{ @@ -53,10 +56,21 @@ This REPLACES the positions of every entity in the modules it touches, including any you arranged by hand. Use --dry-run to see the moves first. Marketplace modules and System are never touched. +With --only-overlapping nothing is re-arranged. The command finds the entities +whose boxes collide (a box is estimated from the entity's name and attribute +count, as in the full layout; a domain-model annotation counts as an obstacle), +keeps the one that comes first in the model, and moves each other colliding +entity to the nearest free spot. Every entity that collides with nothing keeps +its position exactly, so a diagram arranged by hand is repaired, not replaced. +Running it again finds nothing to move. This is the fix for the default grid, +where an entity with many attributes is taller than a grid row and runs into +the one below. + To re-arrange microflows and nanoflows, use 'mxcli layout flows'.`, Example: ` mxcli layout -p app.mpr mxcli layout -p app.mpr --module CapTrack - mxcli layout -p app.mpr --dry-run`, + mxcli layout -p app.mpr --dry-run + mxcli layout -p app.mpr --module Sales --only-overlapping --dry-run`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return runLayout(cmd) @@ -70,6 +84,8 @@ func init() { "report the moves without writing") layoutCmd.Flags().BoolVar(&layoutIncludeMarketplace, "include-marketplace", false, "also lay out Marketplace modules (a module update replaces them, so this is normally pointless)") + layoutCmd.Flags().BoolVar(&layoutOnlyOverlapping, "only-overlapping", false, + "move only entities whose boxes collide with another entity or annotation (to the nearest free spot); all other positions are kept") rootCmd.AddCommand(layoutCmd) } @@ -118,38 +134,18 @@ func runLayout(cmd *cobra.Command) error { continue } - plan := dmlayout.Plan(dm) - moved := 0 - var moves []string - for _, e := range dm.Entities { - p, ok := plan[e.ID] - if !ok || p == e.Location { - continue - } - moves = append(moves, fmt.Sprintf(" %s.%s (%d, %d) -> (%d, %d)", - m.Name, e.Name, e.Location.X, e.Location.Y, p.X, p.Y)) - if !layoutDryRun { - e.Location = p + update := func(dm *domainmodel.DomainModel) error { + if err := b.UpdateDomainModel(dm); err != nil { + return fmt.Errorf("write domain model of %s: %w", m.Name, err) } - moved++ + return nil + } + moved, err := layoutModule(cmd.OutOrStdout(), m.Name, dm, layoutDryRun, layoutOnlyOverlapping, update) + if err != nil { + return err } - sort.Strings(moves) - if moved == 0 { unchangedTotal += len(dm.Entities) - fmt.Fprintf(cmd.OutOrStdout(), "%s: already laid out (%d entities)\n", m.Name, len(dm.Entities)) - continue - } - for _, line := range moves { - fmt.Fprintln(cmd.OutOrStdout(), line) - } - if layoutDryRun { - fmt.Fprintf(cmd.OutOrStdout(), "%s: %d of %d entities would move\n", m.Name, moved, len(dm.Entities)) - } else { - if err := b.UpdateDomainModel(dm); err != nil { - return fmt.Errorf("write domain model of %s: %w", m.Name, err) - } - fmt.Fprintf(cmd.OutOrStdout(), "%s: moved %d of %d entities\n", m.Name, moved, len(dm.Entities)) } movedTotal += moved } @@ -157,12 +153,76 @@ func runLayout(cmd *cobra.Command) error { switch { case movedTotal == 0 && unchangedTotal == 0: fmt.Fprintln(cmd.OutOrStdout(), "Nothing to lay out.") - case layoutDryRun: + case layoutDryRun && (movedTotal > 0 || !layoutOnlyOverlapping): fmt.Fprintf(cmd.OutOrStdout(), "Dry run: %d entities would move. Re-run without --dry-run to apply.\n", movedTotal) } return nil } +// layoutModule positions the entities of one domain model, prints what it did +// and, unless dryRun, hands the changed model to update. It returns how many +// entities moved. +// +// The full layout (Plan) positions every entity from the association graph. +// With onlyOverlapping it instead moves only the entities whose estimated boxes +// collide (dmlayout.ResolveOverlaps) and leaves every other Location untouched. +func layoutModule(out io.Writer, module string, dm *domainmodel.DomainModel, dryRun, onlyOverlapping bool, + update func(*domainmodel.DomainModel) error) (int, error) { + to := map[model.ID]model.Point{} + if onlyOverlapping { + plan := dmlayout.ResolveOverlaps(dm) + for _, p := range plan.Pairs { + with := module + "." + p.With + if p.WithAnnotation { + with = fmt.Sprintf("annotation %q", p.With) + } + fmt.Fprintf(out, " %s.%s overlaps %s\n", module, p.Entity, with) + } + for _, mv := range plan.Moves { + to[mv.ID] = mv.To + } + } else { + to = dmlayout.Plan(dm) + } + + moved := 0 + var moves []string + for _, e := range dm.Entities { + p, ok := to[e.ID] + if !ok || p == e.Location { + continue + } + moves = append(moves, fmt.Sprintf(" %s.%s (%d, %d) -> (%d, %d)", + module, e.Name, e.Location.X, e.Location.Y, p.X, p.Y)) + if !dryRun { + e.Location = p + } + moved++ + } + sort.Strings(moves) + + if moved == 0 { + if onlyOverlapping { + fmt.Fprintf(out, "%s: no overlapping entities (%d entities)\n", module, len(dm.Entities)) + } else { + fmt.Fprintf(out, "%s: already laid out (%d entities)\n", module, len(dm.Entities)) + } + return 0, nil + } + for _, line := range moves { + fmt.Fprintln(out, line) + } + if dryRun { + fmt.Fprintf(out, "%s: %d of %d entities would move\n", module, moved, len(dm.Entities)) + return moved, nil + } + if err := update(dm); err != nil { + return 0, err + } + fmt.Fprintf(out, "%s: moved %d of %d entities\n", module, moved, len(dm.Entities)) + return moved, nil +} + // layoutTargets picks the modules to touch. // // A Marketplace module is excluded by default for the same reason CREATE LAYOUT diff --git a/cmd/mxcli/cmd_layout_test.go b/cmd/mxcli/cmd_layout_test.go index c05c36abe3..43796774cf 100644 --- a/cmd/mxcli/cmd_layout_test.go +++ b/cmd/mxcli/cmd_layout_test.go @@ -3,10 +3,13 @@ package main import ( + "bytes" + "fmt" "strings" "testing" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" ) func mod(name string, marketplace bool) *model.Module { @@ -132,3 +135,104 @@ func TestLayoutTargets_IsSorted(t *testing.T) { t.Errorf("targets = %v, want them sorted", names) } } + +// overlapFixture is a domain model as CREATE ENTITY leaves it when one entity is +// longer than a grid row: Order (30 attributes) runs into Invoice below it. +// Pinned sits far from both, off any grid. +func overlapFixture() *domainmodel.DomainModel { + attrs := func(n int) []*domainmodel.Attribute { + out := make([]*domainmodel.Attribute, n) + for i := range out { + out[i] = &domainmodel.Attribute{Name: fmt.Sprintf("Attr%d", i)} + } + return out + } + mk := func(name string, n, x, y int) *domainmodel.Entity { + e := &domainmodel.Entity{Name: name, Attributes: attrs(n), Location: model.Point{X: x, Y: y}} + e.ID = model.ID("id-" + name) + return e + } + return &domainmodel.DomainModel{Entities: []*domainmodel.Entity{ + mk("Order", 30, 100, 100), + mk("Invoice", 3, 100, 380), + mk("Pinned", 4, 4321, 1234), + }} +} + +func runLayoutModule(t *testing.T, dm *domainmodel.DomainModel, dryRun, only bool) (string, int, int) { + t.Helper() + var out bytes.Buffer + writes := 0 + moved, err := layoutModule(&out, "Sales", dm, dryRun, only, func(*domainmodel.DomainModel) error { + writes++ + return nil + }) + if err != nil { + t.Fatal(err) + } + return out.String(), moved, writes +} + +// --dry-run lists the colliding pair and the move, and neither writes nor +// changes the model. +func TestLayoutOnlyOverlapping_DryRunListsPairsAndMoves(t *testing.T) { + dm := overlapFixture() + before := fmt.Sprint(dm.Entities[0].Location, dm.Entities[1].Location, dm.Entities[2].Location) + out, moved, writes := runLayoutModule(t, dm, true, true) + + for _, want := range []string{ + "Sales.Invoice overlaps Sales.Order", + "Sales.Invoice (100, 380) -> (", + "Sales: 1 of 3 entities would move", + } { + if !strings.Contains(out, want) { + t.Errorf("output lacks %q:\n%s", want, out) + } + } + if strings.Contains(out, "Sales.Pinned (") || strings.Contains(out, "Sales.Order (") { + t.Errorf("an entity that keeps its place was listed as a move:\n%s", out) + } + if moved != 1 || writes != 0 { + t.Errorf("moved=%d writes=%d, want 1 and 0 on a dry run", moved, writes) + } + if after := fmt.Sprint(dm.Entities[0].Location, dm.Entities[1].Location, dm.Entities[2].Location); after != before { + t.Errorf("a dry run changed the model: %s -> %s", before, after) + } +} + +// A real run moves only the colliding entity, writes once, and the next run has +// nothing to do. +func TestLayoutOnlyOverlapping_MovesOnlyTheCollidingEntityAndIsIdempotent(t *testing.T) { + dm := overlapFixture() + order, pinned := dm.Entities[0].Location, dm.Entities[2].Location + invoice := dm.Entities[1].Location + + out, moved, writes := runLayoutModule(t, dm, false, true) + if moved != 1 || writes != 1 || !strings.Contains(out, "Sales: moved 1 of 3 entities") { + t.Fatalf("first run: moved=%d writes=%d\n%s", moved, writes, out) + } + if dm.Entities[0].Location != order || dm.Entities[2].Location != pinned { + t.Error("an entity that collides with nothing was moved") + } + if dm.Entities[1].Location == invoice { + t.Error("Invoice was not moved") + } + + out, moved, writes = runLayoutModule(t, dm, false, true) + if moved != 0 || writes != 0 || !strings.Contains(out, "Sales: no overlapping entities (3 entities)") { + t.Errorf("second run: moved=%d writes=%d\n%s", moved, writes, out) + } +} + +// CONTROL: without the flag the command is the full layout, as before. +func TestLayoutWithoutOnlyOverlapping_StillArrangesEverything(t *testing.T) { + dm := overlapFixture() + _, moved, writes := runLayoutModule(t, dm, false, false) + if moved != 3 || writes != 1 { + t.Errorf("full layout moved %d of 3 (writes=%d), want all three", moved, writes) + } + out, moved, writes := runLayoutModule(t, dm, false, false) + if moved != 0 || writes != 0 || !strings.Contains(out, "already laid out") { + t.Errorf("second full layout: moved=%d writes=%d\n%s", moved, writes, out) + } +} diff --git a/docs-site/src/tools/domain-model-layout.md b/docs-site/src/tools/domain-model-layout.md index 4e776cc0c9..de452318af 100644 --- a/docs-site/src/tools/domain-model-layout.md +++ b/docs-site/src/tools/domain-model-layout.md @@ -50,6 +50,56 @@ Administration comes from the Marketplace, and a module update replaces it — pass --include-marketplace to lay it out anyway ``` +## Repairing only the overlaps + +A full layout is more than you want when the diagram is mostly fine. The default +grid gives every entity the same 280px row, but a box is as tall as its +attribute list, so an entity with about 30 or more attributes (fewer when the +one below it is long too) runs into the one below. `--only-overlapping` repairs +exactly that and leaves the rest alone: + +```bash +mxcli layout -p app.mpr --module Sales --only-overlapping --dry-run +mxcli layout -p app.mpr --module Sales --only-overlapping +``` + +``` + Sales.Category overlaps Sales.Order + Sales.Warehouse overlaps Sales.Invoice + Sales.Employee overlaps Sales.Payment + Sales.Ticket overlaps Sales.Category + Sales.Category (360, 380) -> (560, 250) + Sales.Employee (1140, 380) -> (1600, 420) + Sales.Warehouse (880, 380) -> (1080, 380) +Sales: 3 of 16 entities would move +Dry run: 3 entities would move. Re-run without --dry-run to apply. +``` + +`Ticket` collides with `Category`, which is itself moving away, so `Ticket` stays. + +The rules: + +- **A collision is an intersection of estimated boxes.** The estimate is the one + the full layout uses: width from the longest name, height from the number of + attributes. Boxes that only touch, or sit close, are not a collision. A + domain-model annotation counts as something an entity can collide with. +- **The earlier entity in the model stays.** Of two colliding entities the one + created first keeps its position, and the other moves (unless the first has + itself been moved away by an earlier collision). An entity that collides + with nothing is never moved, so a position you set by hand is kept unless its + box collides with an entity that comes before it in the model. +- **A mover goes to the nearest free spot.** It is put at the closest position + where its box, with a 40px margin, touches nothing else, and never above or to + the left of the canvas origin. Annotations never move. +- **Running it again changes nothing.** The second run prints + `no overlapping entities` and does not write. + +`--dry-run` lists every collision found (`A overlaps B`, where `A` comes later in +the model) and every move; a collision with an entity that itself moves away +needs no move of its own. The box size is an estimate: Studio Pro's real +drawing is not known to mxcli, so an entity can still look crowded without +counting as a collision. + ## Running it more than once Positions are a function of the model alone, so the command is safe to leave in @@ -68,6 +118,7 @@ a build script: | `--module ` | Lay out this module (repeatable). Default: every module the project owns. | | `--dry-run` | Print the moves, write nothing. | | `--include-marketplace` | Also lay out Marketplace modules. A module update replaces them, so this is normally pointless. | +| `--only-overlapping` | Move only entities whose boxes collide with another entity or an annotation, to the nearest free spot; keep every other position. | ## When you do want explicit positions @@ -80,8 +131,10 @@ Two things to know if you place entities yourself: - The coordinate is the box's **centre**, not its top-left corner. - An entity created with no position takes the next slot in a wrapping grid. - That is a default, not a layout: it keeps a large model on screen and stops - boxes overlapping, but it knows nothing about which entities are related. + That is a default, not a layout: it keeps a large model on screen and keeps + boxes of ordinary size apart, but it knows nothing about which entities are + related, and an entity with many attributes is taller than a grid row. Run + `mxcli layout --only-overlapping` to move just the ones that collide. ## Default position for an entity with no `@Position` diff --git a/mdl/dmlayout/dmlayout_test.go b/mdl/dmlayout/dmlayout_test.go index 51b310bdbc..c78594b4a9 100644 --- a/mdl/dmlayout/dmlayout_test.go +++ b/mdl/dmlayout/dmlayout_test.go @@ -173,26 +173,7 @@ func TestPlan_LayersTheModel(t *testing.T) { // ignoring the single row: a 150px step is narrower than a box. func TestPlan_NoOverlap(t *testing.T) { dm := capTrack() - pos := Plan(dm) - - type rect struct { - name string - x1, y1, x2, y2 int - } - var boxes []rect - for _, e := range dm.Entities { - p := pos[e.ID] - w, h := boxSize(e) - boxes = append(boxes, rect{e.Name, p.X - w/2, p.Y - h/2, p.X + w/2, p.Y + h/2}) - } - for i := range boxes { - for j := i + 1; j < len(boxes); j++ { - a, b := boxes[i], boxes[j] - if a.x1 < b.x2 && b.x1 < a.x2 && a.y1 < b.y2 && b.y1 < a.y2 { - t.Errorf("%s and %s overlap: %+v vs %+v", a.name, b.name, a, b) - } - } - } + assertNoOverlap(t, dm, Plan(dm)) } // Unconnected entities are not lookups. Putting them in layer 0 would mix the diff --git a/mdl/dmlayout/overlap.go b/mdl/dmlayout/overlap.go new file mode 100644 index 0000000000..c47fd507a8 --- /dev/null +++ b/mdl/dmlayout/overlap.go @@ -0,0 +1,285 @@ +// SPDX-License-Identifier: Apache-2.0 + +package dmlayout + +import ( + "sort" + "strings" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// The default grid is a fixed pitch, and a box is as tall as its attribute list: +// from 16 attributes up an entity is taller than GridRowPitch and runs into the +// one below it. Plan fixes that by laying the whole module out again, which also +// throws away every position somebody chose. ResolveOverlaps is the narrow +// repair: it moves the entities whose boxes collide and nothing else. +// +// # Strategy +// +// Boxes are the estimates boxSize gives (the same ones Plan uses), centred on the +// entity's Location, the convention this package states. A collision is a +// strict intersection: boxes that merely touch, or sit close, are left alone. +// +// Which entity gives way. Entities are taken in the order they appear in the +// domain model, which is creation order. An entity stays if its box clears every +// entity that has already been kept and every annotation; otherwise it is a +// mover. So of two colliding entities the earlier one stays (it is the one more +// likely to have been placed on purpose), a chain of collisions costs the fewest +// moves a greedy pass can manage, and an entity nothing collides with is always +// kept, because there is nothing it could fail against. Annotations are notes the +// author put on the canvas on purpose: they are obstacles, never movers. +// +// Where a mover goes. Each mover, in the same order, is put on the nearest free +// point to where it was. "Free" means its box, grown by placementGutter on every +// side, intersects no kept entity, no annotation and no mover placed before it. +// The search walks outwards from the old position in square rings of +// placementStep pixels and takes, within the first ring that has a free point, +// the one closest to the old position (ties: smaller y, then smaller x). Nothing +// in it depends on map order, so the same model always gives the same moves. +// +// A mover is never placed left of or above the canvas origin, where the +// entities of a generated model start. +// +// Idempotence follows from the two rules: kept entities clear each other by +// construction and every mover clears everything placed before it with a margin, +// so the result has no collision, and a model with no collision has no mover. +// +// # Annotations +// +// An annotation stores a position and a width but no height or origin convention +// this package can rely on, so its obstacle is a deliberately generous estimate: +// the width as stored (defaultAnnotationWidth when absent), a height from the +// caption's line count, and a rectangle that covers the box whether Location is +// its centre or its top-left corner. + +// placementGutter is the clear space a relocated entity keeps to every other +// box. It is the gutter Plan keeps between stacked entities. +const placementGutter = rowGutter + +const ( + // placementStep is the search lattice, in pixels. + placementStep = 10 + + defaultAnnotationWidth = 440 // what CREATE ANNOTATION stores when no width is given +) + +// Overlap is one pair of colliding boxes found in a domain model. +type Overlap struct { + // Entity is the name of the entity that gives way. + Entity string + // With is what it collides with: another entity's name, or the first line of + // an annotation's caption when WithAnnotation is set. + With string + WithAnnotation bool +} + +// Move is one entity that ResolveOverlaps relocates. +type Move struct { + ID model.ID + Name string + From, To model.Point +} + +// OverlapPlan is the result of ResolveOverlaps. +type OverlapPlan struct { + // Pairs lists every collision in the model as it was handed in. For two + // entities, Entity is the later one in model order — the one that gives way + // unless an earlier collision already moved the other. + Pairs []Overlap + // Moves lists the entities to relocate, in the order they were placed. + Moves []Move +} + +// rect is a half-open box: x1 <= x < x2, y1 <= y < y2. +type rect struct{ x1, y1, x2, y2 int } + +// intersects reports whether the two boxes share area once a is grown by pad on +// every side. Boxes that only touch do not intersect. +func (a rect) intersects(b rect, pad int) bool { + return a.x1-pad < b.x2 && b.x1 < a.x2+pad && a.y1-pad < b.y2 && b.y1 < a.y2+pad +} + +// entityRect is the estimated box of e when its Location is at. +func entityRect(e *domainmodel.Entity, at model.Point) rect { + w, h := boxSize(e) + return rect{at.X - w/2, at.Y - h/2, at.X - w/2 + w, at.Y - h/2 + h} +} + +// annotationRect is the generous obstacle for a note; see "Annotations" above. +func annotationRect(a *domainmodel.Annotation) rect { + w := a.Width + if w <= 0 { + w = defaultAnnotationWidth + } + perLine := (w - 2*boxPadding) / pixelsPerChar + if perLine < 1 { + perLine = 1 + } + lines := 0 + for _, l := range strings.Split(strings.ReplaceAll(a.Caption, "\r\n", "\n"), "\n") { + lines += 1 + (len(l)-1)/perLine // an empty line still takes one + } + h := 2*boxPadding + lines*memberHeight + if h < minBoxHeight { + h = minBoxHeight + } + // Covers Location as the centre and Location as the top-left corner. + return rect{a.Location.X - w/2, a.Location.Y - h/2, a.Location.X + w, a.Location.Y + h} +} + +// annotationTitle is the first non-empty line of a caption, which is how a note +// is called everywhere else. +func annotationTitle(caption string) string { + for _, l := range strings.Split(strings.ReplaceAll(caption, "\r\n", "\n"), "\n") { + if t := strings.TrimSpace(l); t != "" { + return t + } + } + return "" +} + +// ResolveOverlaps finds the entities whose estimated boxes collide and plans a +// new position for the ones that have to give way; see the strategy above. It +// does not modify dm. +func ResolveOverlaps(dm *domainmodel.DomainModel) OverlapPlan { + var plan OverlapPlan + if dm == nil { + return plan + } + var ents []*domainmodel.Entity + for _, e := range dm.Entities { + if e != nil { + ents = append(ents, e) + } + } + var notes []*domainmodel.Annotation + var noteRects []rect + for _, a := range dm.Annotations { + if a != nil { + notes = append(notes, a) + noteRects = append(noteRects, annotationRect(a)) + } + } + boxes := make([]rect, len(ents)) + for i, e := range ents { + boxes[i] = entityRect(e, e.Location) + } + + // Every collision, for reporting. + for j := range ents { + for i := 0; i < j; i++ { + if boxes[i].intersects(boxes[j], 0) { + plan.Pairs = append(plan.Pairs, Overlap{Entity: ents[j].Name, With: ents[i].Name}) + } + } + for k, n := range notes { + if boxes[j].intersects(noteRects[k], 0) { + plan.Pairs = append(plan.Pairs, Overlap{Entity: ents[j].Name, With: annotationTitle(n.Caption), WithAnnotation: true}) + } + } + } + if len(plan.Pairs) == 0 { + return plan + } + + // Who gives way: greedy over model order, against what has been kept. + obstacles := append([]rect(nil), noteRects...) + var movers []int + for i := range ents { + clear := true + for _, o := range obstacles { + if boxes[i].intersects(o, 0) { + clear = false + break + } + } + if clear { + obstacles = append(obstacles, boxes[i]) + } else { + movers = append(movers, i) + } + } + + // Where they go: nearest free point, each mover seeing those placed before it. + for _, i := range movers { + w, h := boxSize(ents[i]) + to := nearestFree(w, h, ents[i].Location, obstacles) + obstacles = append(obstacles, entityRect(ents[i], to)) + plan.Moves = append(plan.Moves, Move{ID: ents[i].ID, Name: ents[i].Name, From: ents[i].Location, To: to}) + } + return plan +} + +// nearestFree returns the point closest to from at which a w x h box centred +// there, grown by placementGutter, intersects none of obstacles. +func nearestFree(w, h int, from model.Point, obstacles []rect) model.Point { + free := func(p model.Point) bool { + // A mover never goes left of or above the canvas origin. + if p.X < w/2 || p.Y < h/2 { + return false + } + b := rect{p.X - w/2, p.Y - h/2, p.X - w/2 + w, p.Y - h/2 + h} + for _, o := range obstacles { + if b.intersects(o, placementGutter) { + return false + } + } + return true + } + if free(from) { + return from + } + start := model.Point{X: max(from.X, w/2), Y: max(from.Y, h/2)} + + // Past every obstacle there is always room, so the search is bounded: the + // first ring that reaches beyond the rightmost edge has a free point. + right := from.X + for _, o := range obstacles { + if o.x2 > right { + right = o.x2 + } + } + maxRing := (right-from.X+w+h+2*placementGutter)/placementStep + 2 + + type cand struct { + p model.Point + d2 int + } + for r := 1; r <= maxRing; r++ { + var ring []cand + for dy := -r; dy <= r; dy++ { + for dx := -r; dx <= r; dx++ { + if abs(dx) != r && abs(dy) != r { + continue + } + x, y := dx*placementStep, dy*placementStep + ring = append(ring, cand{model.Point{X: from.X + x, Y: from.Y + y}, x*x + y*y}) + } + } + sort.Slice(ring, func(a, b int) bool { + if ring[a].d2 != ring[b].d2 { + return ring[a].d2 < ring[b].d2 + } + if ring[a].p.Y != ring[b].p.Y { + return ring[a].p.Y < ring[b].p.Y + } + return ring[a].p.X < ring[b].p.X + }) + for _, c := range ring { + if free(c.p) { + return c.p + } + } + } + // Unreachable by the bound above; the point is clear of everything anyway. + return model.Point{X: right + w/2 + placementGutter + placementStep, Y: start.Y} +} + +func abs(v int) int { + if v < 0 { + return -v + } + return v +} diff --git a/mdl/dmlayout/overlap_test.go b/mdl/dmlayout/overlap_test.go new file mode 100644 index 0000000000..ef70d658fc --- /dev/null +++ b/mdl/dmlayout/overlap_test.go @@ -0,0 +1,362 @@ +// SPDX-License-Identifier: Apache-2.0 + +package dmlayout + +import ( + "fmt" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// assertNoOverlap fails for every pair of entities whose estimated boxes +// intersect when each sits at pos[id]. It is the checker TestPlan_NoOverlap +// uses, shared so the two layouts are held to the same rule. +func assertNoOverlap(t *testing.T, dm *domainmodel.DomainModel, pos map[model.ID]model.Point) { + t.Helper() + type rect struct { + name string + x1, y1, x2, y2 int + } + var boxes []rect + for _, e := range dm.Entities { + p := pos[e.ID] + w, h := boxSize(e) + boxes = append(boxes, rect{e.Name, p.X - w/2, p.Y - h/2, p.X + w/2, p.Y + h/2}) + } + for i := range boxes { + for j := i + 1; j < len(boxes); j++ { + a, b := boxes[i], boxes[j] + if a.x1 < b.x2 && b.x1 < a.x2 && a.y1 < b.y2 && b.y1 < a.y2 { + t.Errorf("%s and %s overlap: %+v vs %+v", a.name, b.name, a, b) + } + } + } +} + +func locations(dm *domainmodel.DomainModel) map[model.ID]model.Point { + out := make(map[model.ID]model.Point, len(dm.Entities)) + for _, e := range dm.Entities { + out[e.ID] = e.Location + } + return out +} + +// withLocation is ent() with a position. +func withLocation(e *domainmodel.Entity, x, y int) *domainmodel.Entity { + e.Location = model.Point{X: x, Y: y} + return e +} + +// attrs returns n attribute names. +func attrs(n int) []string { + out := make([]string, n) + for i := range out { + out[i] = fmt.Sprintf("A%d", i) + } + return out +} + +// apply returns the positions after the plan's moves. +func apply(dm *domainmodel.DomainModel, plan OverlapPlan) map[model.ID]model.Point { + pos := locations(dm) + for _, m := range plan.Moves { + pos[m.ID] = m.To + } + return pos +} + +// withPositions returns a copy of dm whose entities sit at pos, so a second +// pass can be run over the outcome of the first. +func withPositions(dm *domainmodel.DomainModel, pos map[model.ID]model.Point) *domainmodel.DomainModel { + out := &domainmodel.DomainModel{Annotations: dm.Annotations} + for _, e := range dm.Entities { + c := *e + c.Location = pos[e.ID] + out.Entities = append(out.Entities, &c) + } + return out +} + +// gridModel is what CREATE ENTITY leaves behind: n entities on GridSlot, every +// third one tall. Boxes are centred on the grid points, so from about 30 +// attributes (34+16*attrs high) a box reaches into the box one row down — the +// reported symptom, and the reason a fixed pitch cannot be the whole answer. +func gridModel(n int) *domainmodel.DomainModel { + dm := &domainmodel.DomainModel{} + for i := 0; i < n; i++ { + na := 3 + if i%3 == 0 { + na = 20 + (i*7)%31 // 20..50 attributes + } + p := GridSlot(i) + dm.Entities = append(dm.Entities, withLocation(ent(fmt.Sprintf("E%02d", i), attrs(na)...), p.X, p.Y)) + } + return dm +} + +func movedIDs(plan OverlapPlan) map[model.ID]bool { + out := map[model.ID]bool{} + for _, m := range plan.Moves { + out[m.ID] = true + } + return out +} + +// A box is as tall as its attribute list, so two entities one grid row apart +// collide only when the upper one is long. +func TestResolveOverlaps_DetectsCollisionsByAttributeCount(t *testing.T) { + short := &domainmodel.DomainModel{Entities: []*domainmodel.Entity{ + withLocation(ent("Upper", attrs(3)...), 100, 100), + withLocation(ent("Lower", attrs(3)...), 100, 380), + }} + if plan := ResolveOverlaps(short); len(plan.Pairs) != 0 || len(plan.Moves) != 0 { + t.Fatalf("two short entities one row apart collide: %+v", plan) + } + + tall := &domainmodel.DomainModel{Entities: []*domainmodel.Entity{ + withLocation(ent("Upper", attrs(40)...), 100, 100), // h = 34+40*16 = 674 + withLocation(ent("Lower", attrs(3)...), 100, 380), + }} + plan := ResolveOverlaps(tall) + if len(plan.Pairs) != 1 || plan.Pairs[0].Entity != "Lower" || plan.Pairs[0].With != "Upper" { + t.Fatalf("want the one Upper/Lower pair, got %+v", plan.Pairs) + } + if len(plan.Moves) != 1 || plan.Moves[0].Name != "Lower" { + t.Fatalf("want only Lower to move (Upper comes first), got %+v", plan.Moves) + } +} + +// Boxes that only touch do not collide: the rule is intersection, not nearness. +func TestResolveOverlaps_TouchingBoxesAreNotACollision(t *testing.T) { + a := ent("A", attrs(3)...) + b := ent("B", attrs(3)...) + wa, ha := boxSize(a) + withLocation(a, 100, 100) + withLocation(b, 100+wa, 100) // exactly one box width to the right + _ = ha + dm := &domainmodel.DomainModel{Entities: []*domainmodel.Entity{a, b}} + if plan := ResolveOverlaps(dm); len(plan.Pairs) != 0 { + t.Errorf("touching boxes reported as a collision: %+v", plan.Pairs) + } + b.Location.X-- // one pixel closer + if plan := ResolveOverlaps(dm); len(plan.Pairs) != 1 { + t.Errorf("one pixel of overlap not found: %+v", plan.Pairs) + } +} + +// The point of --only-overlapping: an entity nothing collides with keeps its +// position to the pixel, however odd, and a collision moves only the entity that +// comes later in the model. +func TestResolveOverlaps_MovesOnlyCollidingEntities(t *testing.T) { + dm := gridModel(14) + // A hand-placed entity far from everything, and not on any grid. + pinned := withLocation(ent("Pinned", attrs(4)...), 4321, 1234) + dm.Entities = append(dm.Entities, pinned) + + before := locations(dm) + plan := ResolveOverlaps(dm) + if len(plan.Pairs) == 0 || len(plan.Moves) == 0 { + t.Fatalf("fixture has no collisions to resolve: %+v", plan) + } + + inPair := map[string]bool{} + for _, p := range plan.Pairs { + inPair[p.Entity] = true + inPair[p.With] = true + } + moved := movedIDs(plan) + for _, e := range dm.Entities { + switch { + case !inPair[e.Name] && moved[e.ID]: + t.Errorf("%s collides with nothing but was moved", e.Name) + case !moved[e.ID] && apply(dm, plan)[e.ID] != before[e.ID]: + t.Errorf("%s was not moved but its position changed", e.Name) + } + } + if moved[pinned.ID] { + t.Error("the hand-placed entity was moved") + } + if len(plan.Moves) >= len(dm.Entities) { + t.Errorf("%d of %d entities moved — that is a re-layout, not a repair", len(plan.Moves), len(dm.Entities)) + } + // Every move leaves a position, and says where it came from. + for _, m := range plan.Moves { + if m.From != before[m.ID] || m.To == m.From { + t.Errorf("bad move %+v (stored %+v)", m, before[m.ID]) + } + } +} + +// ResolveOverlaps plans; it must not write into the model it was handed. +func TestResolveOverlaps_DoesNotModifyTheModel(t *testing.T) { + dm := gridModel(14) + before := locations(dm) + ResolveOverlaps(dm) + for id, p := range locations(dm) { + if before[id] != p { + t.Fatalf("%s was moved in the model itself", id) + } + } +} + +// After the moves no two boxes intersect — the same check TestPlan_NoOverlap +// applies to the full layout — and a mover keeps the package's gutter to every +// neighbour, so the result is not boxes an exact pixel apart. +func TestResolveOverlaps_LeavesNoOverlap(t *testing.T) { + for _, n := range []int{14, 40, 90} { + dm := gridModel(n) + plan := ResolveOverlaps(dm) + assertNoOverlap(t, dm, apply(dm, plan)) + + pos := apply(dm, plan) + moved := movedIDs(plan) + for _, a := range dm.Entities { + if !moved[a.ID] { + continue + } + wa, ha := boxSize(a) + for _, b := range dm.Entities { + if a == b { + continue + } + wb, hb := boxSize(b) + pa, pb := pos[a.ID], pos[b.ID] + gapX := abs(pa.X-pb.X) - (wa+wb)/2 + gapY := abs(pa.Y-pb.Y) - (ha+hb)/2 + if gapX < placementGutter-1 && gapY < placementGutter-1 { + t.Errorf("n=%d: moved %s is within the gutter of %s (gap %d,%d)", n, a.Name, b.Name, gapX, gapY) + } + } + } + } +} + +// A mover is put NEAR where it was, not sent to the end of the diagram. +func TestResolveOverlaps_MovesToTheNearestFreeSpot(t *testing.T) { + a := withLocation(ent("A", attrs(3)...), 400, 300) + b := withLocation(ent("B", attrs(3)...), 400, 320) // sits almost on top of A + dm := &domainmodel.DomainModel{Entities: []*domainmodel.Entity{a, b}} + plan := ResolveOverlaps(dm) + if len(plan.Moves) != 1 || plan.Moves[0].Name != "B" { + t.Fatalf("want B to move, got %+v", plan.Moves) + } + m := plan.Moves[0] + if d := abs(m.To.X-m.From.X) + abs(m.To.Y-m.From.Y); d > 160 { + t.Errorf("B moved %dpx (%+v -> %+v) to clear a box it overlaps by a few pixels", d, m.From, m.To) + } +} + +// Of two colliding entities the one earlier in the model stays: it is the older, +// likelier hand-placed one, and the rule needs no information beyond the model. +func TestResolveOverlaps_TheEarlierEntityStays(t *testing.T) { + first := withLocation(ent("Zeta", attrs(3)...), 100, 100) + second := withLocation(ent("Alpha", attrs(3)...), 110, 100) + dm := &domainmodel.DomainModel{Entities: []*domainmodel.Entity{first, second}} + plan := ResolveOverlaps(dm) + if len(plan.Moves) != 1 || plan.Moves[0].Name != "Alpha" { + t.Fatalf("model order decides, not name: got %+v", plan.Moves) + } +} + +// Several entities on one point: all but the first leave, and none lands on +// another mover. +func TestResolveOverlaps_StackedEntitiesAreSeparated(t *testing.T) { + dm := &domainmodel.DomainModel{} + for i := 0; i < 5; i++ { + dm.Entities = append(dm.Entities, withLocation(ent(fmt.Sprintf("S%d", i), attrs(5)...), 500, 500)) + } + plan := ResolveOverlaps(dm) + if len(plan.Moves) != 4 { + t.Fatalf("want 4 of 5 to move, got %d: %+v", len(plan.Moves), plan.Moves) + } + assertNoOverlap(t, dm, apply(dm, plan)) +} + +// Same model, same answer — Go randomises map iteration, so an unsorted walk +// anywhere in the search would show here. +func TestResolveOverlaps_IsDeterministic(t *testing.T) { + want := fmt.Sprint(ResolveOverlaps(gridModel(40))) + for i := 0; i < 20; i++ { + if got := fmt.Sprint(ResolveOverlaps(gridModel(40))); got != want { + t.Fatalf("run %d planned differently", i) + } + } +} + +// A second pass over the result moves nothing and finds nothing: the command can +// sit in a build script. +func TestResolveOverlaps_IsIdempotent(t *testing.T) { + for _, n := range []int{14, 40, 90} { + dm := gridModel(n) + first := ResolveOverlaps(dm) + if len(first.Moves) == 0 { + t.Fatalf("n=%d: nothing to resolve in the fixture", n) + } + again := ResolveOverlaps(withPositions(dm, apply(dm, first))) + if len(again.Pairs) != 0 || len(again.Moves) != 0 { + t.Errorf("n=%d: second pass found %d collisions and planned %d moves", n, len(again.Pairs), len(again.Moves)) + } + } +} + +// A note on the canvas is an obstacle: an entity under it moves, the note stays. +func TestResolveOverlaps_AnnotationIsAnObstacle(t *testing.T) { + note := &domainmodel.Annotation{Caption: "Billing\nEverything about invoices", Location: model.Point{X: 300, Y: 300}, Width: 440} + under := withLocation(ent("Under", attrs(3)...), 300, 300) + clear := withLocation(ent("Clear", attrs(3)...), 3000, 3000) + dm := &domainmodel.DomainModel{ + Entities: []*domainmodel.Entity{under, clear}, + Annotations: []*domainmodel.Annotation{note}, + } + plan := ResolveOverlaps(dm) + if len(plan.Pairs) != 1 || !plan.Pairs[0].WithAnnotation || plan.Pairs[0].Entity != "Under" || plan.Pairs[0].With != "Billing" { + t.Fatalf("want the one Under/Billing pair, got %+v", plan.Pairs) + } + if len(plan.Moves) != 1 || plan.Moves[0].Name != "Under" { + t.Fatalf("want only Under to move, got %+v", plan.Moves) + } + again := ResolveOverlaps(withPositions(dm, apply(dm, plan))) + if len(again.Pairs) != 0 { + t.Errorf("Under still collides with the note after moving: %+v", again.Pairs) + } + if note.Location != (model.Point{X: 300, Y: 300}) { + t.Error("the annotation was moved") + } +} + +// CONTROL: nothing to do on an empty, nil or already-clean model. +func TestResolveOverlaps_NothingToDo(t *testing.T) { + for name, dm := range map[string]*domainmodel.DomainModel{ + "nil": nil, + "empty": {}, + "one": {Entities: []*domainmodel.Entity{withLocation(ent("Only", "x"), 10, 10)}}, + "clean": {Entities: []*domainmodel.Entity{ + withLocation(ent("A", "x"), 100, 100), + withLocation(ent("B", "x"), 1000, 100), + }}, + } { + if plan := ResolveOverlaps(dm); len(plan.Pairs) != 0 || len(plan.Moves) != 0 { + t.Errorf("%s: %+v", name, plan) + } + } +} + +// A mover is not pushed off the top or left of the canvas, where a generated +// model starts: the entity in the first grid column is the one that would be. +func TestResolveOverlaps_NeverPlacesBeforeTheOrigin(t *testing.T) { + for _, n := range []int{14, 40, 90} { + dm := gridModel(n) + byID := map[model.ID]*domainmodel.Entity{} + for _, e := range dm.Entities { + byID[e.ID] = e + } + for _, m := range ResolveOverlaps(dm).Moves { + w, h := boxSize(byID[m.ID]) + if m.To.X-w/2 < 0 || m.To.Y-h/2 < 0 { + t.Errorf("n=%d: %s moved to %+v, whose box starts before the origin", n, m.Name, m.To) + } + } + } +} From 8bc865ae706a595e3fa9025c446f952b1d7cc0a8 Mon Sep 17 00:00:00 2001 From: stagknee <241821830+stagknee@users.noreply.github.com> Date: Tue, 13 Oct 2026 12:57:20 -0500 Subject: [PATCH 3/3] fix: an entity's position is the top-left corner of its box, not its centre dmlayout treated an entity's Location as the centre of its box (citing RelativeMiddlePoint, which is the microflow-object field). Studio Pro 11.12.5 draws an entity's Location as the box's top-left corner: entities created at the same Y with 4, 30 and 45 attributes share one top edge. Plan therefore placed every box half its size lower and further right than planned, and a tall box ran into a short one under it; the overlap check missed the same collisions. Both now measure from the corner. Co-Authored-By: Claude Opus 5.5 --- ...ntity-location-is-top-left-not-centre.json | 1 + CHANGELOG.md | 1 + docs-site/src/tools/domain-model-layout.md | 2 +- mdl/dmlayout/dmlayout.go | 14 ++- mdl/dmlayout/dmlayout_test.go | 24 +++++ mdl/dmlayout/overlap.go | 22 ++--- mdl/dmlayout/overlap_test.go | 90 ++++++++++++++++--- 7 files changed, 122 insertions(+), 32 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-13-entity-location-is-top-left-not-centre.json diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-entity-location-is-top-left-not-centre.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-entity-location-is-top-left-not-centre.json new file mode 100644 index 0000000000..333359d85e --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-13-entity-location-is-top-left-not-centre.json @@ -0,0 +1 @@ +{"date": "2026-10-13", "area": "mdl/dmlayout", "symptom": "After `mxcli layout`, entity boxes overlapped in Studio Pro: a tall box stacked above a short one ran into it, and `mxcli layout --only-overlapping` reported nothing to move for real collisions (a 22-attribute entity at (100,380) running into one at (100,660); a 45-attribute entity at (880,100) into one at (880,660)).", "cause": "dmlayout assumed an entity's Location is the CENTRE of its box (the package comment cited RelativeMiddlePoint). That field belongs to microflow objects; a domain-model entity stores Location and Studio Pro 11.12.5 draws it as the box's TOP-LEFT corner (entities created at the same Y with 4, 30 and 45 attributes share one top edge). Plan added half a box to every position and the overlap check centred each box, so both were half a box off in x and y.", "file": "mdl/dmlayout/dmlayout.go (Plan), mdl/dmlayout/overlap.go (entityRect, nearestFree)", "fix": "Plan places each entity at the corner of its slot (no half-box); entityRect and nearestFree treat Location as the top-left corner, and a mover is rejected only for negative coordinates. Annotation obstacles keep their both-conventions cover because that convention is unverified.", "test": "TestResolveOverlaps_TopLeftConventionFixture; TestPlan_TallEntityAboveShortOneDoesNotOverlap (mdl/dmlayout)", "insight": "**A coordinate convention is a fact about the file format, so verify it against the application, not against a similarly named field**: `RelativeMiddlePoint` (centre) is the microflow-object field, an entity's `Location` is its top-left. Cheap check: create entities at the same Location.Y with different attribute counts and look at whether the top edges or the centres line up; a test that only agrees with the code's own convention proves nothing, so the regression test sizes boxes independently with the measured geometry (150 wide, 34 + 15 per attribute)."} diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c28d6997a..544efe4498 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`mxcli layout` no longer stacks a tall entity into the one below it** — the layout treated an entity's position as the centre of its box, but Studio Pro draws it as the top-left corner (the centre convention, `RelativeMiddlePoint`, belongs to microflow objects), so every placed box sat half its size lower and further right than planned and a tall box ran into a short one under it. - **View entities, external entities and entities imported from a contract take the grid position `create entity` does** — all three still used the old single-row default (`X: 100 + n*150, Y: 100`) that the grid replaced because 150px is narrower than an entity box: a module of 40 of them was a 6,000px line with the boxes touching. They now take the next `GridSlot` after the entities the module already holds, and an `@Position` is still honoured. Before, `create external entities from` counted only the entities it had created itself, so the first imported entity could land on one the module already had. ## [0.26.0] - 2026-10-11 diff --git a/docs-site/src/tools/domain-model-layout.md b/docs-site/src/tools/domain-model-layout.md index de452318af..68c977cd83 100644 --- a/docs-site/src/tools/domain-model-layout.md +++ b/docs-site/src/tools/domain-model-layout.md @@ -129,7 +129,7 @@ layout into MDL. Two things to know if you place entities yourself: -- The coordinate is the box's **centre**, not its top-left corner. +- The coordinate is the box's **top-left corner**. - An entity created with no position takes the next slot in a wrapping grid. That is a default, not a layout: it keeps a large model on screen and keeps boxes of ordinary size apart, but it knows nothing about which entities are diff --git a/mdl/dmlayout/dmlayout.go b/mdl/dmlayout/dmlayout.go index c3077b67e2..8d3e448ce3 100644 --- a/mdl/dmlayout/dmlayout.go +++ b/mdl/dmlayout/dmlayout.go @@ -28,8 +28,9 @@ // model, deliberately generous: too much space is a diagram that scrolls, too // little is the overlap this package exists to remove. // -// A Mendix position is the box's CENTRE (RelativeMiddlePoint), not its top-left -// corner, which is why the placement code adds half a box rather than none. +// An entity's Location is its box's TOP-LEFT corner (verified in Studio Pro +// 11.12.5). RelativeMiddlePoint, which is a centre, belongs to microflow objects, +// not to entities, so placement uses the box's corner directly. package dmlayout import ( @@ -122,7 +123,7 @@ func Plan(dm *domainmodel.DomainModel) map[model.ID]model.Point { if w > widest { widest = w } - out[id] = model.Point{X: x + w/2, Y: y + h/2} + out[id] = model.Point{X: x, Y: y} y += h + rowGutter } if y > maxY { @@ -136,13 +137,8 @@ func Plan(dm *domainmodel.DomainModel) map[model.ID]model.Point { if len(isolated) > 0 { bandY := maxY + bandGutter for i, id := range isolated { - e := g.byID[id] - w, h := boxSize(e) slot := GridSlot(i) - out[id] = model.Point{ - X: slot.X + w/2, - Y: bandY + (slot.Y - OriginY) + h/2, - } + out[id] = model.Point{X: slot.X, Y: bandY + (slot.Y - OriginY)} } } return out diff --git a/mdl/dmlayout/dmlayout_test.go b/mdl/dmlayout/dmlayout_test.go index c78594b4a9..972efd9d84 100644 --- a/mdl/dmlayout/dmlayout_test.go +++ b/mdl/dmlayout/dmlayout_test.go @@ -176,6 +176,30 @@ func TestPlan_NoOverlap(t *testing.T) { assertNoOverlap(t, dm, Plan(dm)) } +// Location is the box's top-left corner, so a tall box stacked above a short one +// must end above it, not run into it. Centring each box on its position put the +// pair half a box too low and let them collide. +func TestPlan_TallEntityAboveShortOneDoesNotOverlap(t *testing.T) { + // Layer members are ordered by name, so A_Tall is stacked above B_Short. + dm := &domainmodel.DomainModel{ + Entities: []*domainmodel.Entity{ent("A_Tall", attrs(40)...), ent("B_Short", "x"), ent("Target", "x")}, + Associations: []*domainmodel.Association{ + assoc("Tall_Target", "A_Tall", "Target"), + assoc("Short_Target", "B_Short", "Target"), + }, + } + pos := Plan(dm) + tall, short := pos["id-A_Tall"], pos["id-B_Short"] + if tall.X != short.X || tall.Y >= short.Y { + t.Fatalf("want A_Tall stacked above B_Short in one column, got %+v and %+v", tall, short) + } + _, h := boxSize(dm.Entities[0]) + if short.Y < tall.Y+h { + t.Errorf("B_Short at y=%d starts inside A_Tall, which spans y=%d..%d", short.Y, tall.Y, tall.Y+h) + } + assertNoOverlap(t, dm, pos) +} + // Unconnected entities are not lookups. Putting them in layer 0 would mix the // non-persistent helpers in with the real reference data. func TestPlan_IsolatedEntitiesGoInTheirOwnBand(t *testing.T) { diff --git a/mdl/dmlayout/overlap.go b/mdl/dmlayout/overlap.go index c47fd507a8..c1cb1795f4 100644 --- a/mdl/dmlayout/overlap.go +++ b/mdl/dmlayout/overlap.go @@ -18,9 +18,10 @@ import ( // // # Strategy // -// Boxes are the estimates boxSize gives (the same ones Plan uses), centred on the -// entity's Location, the convention this package states. A collision is a -// strict intersection: boxes that merely touch, or sit close, are left alone. +// Boxes are the estimates boxSize gives (the same ones Plan uses), hanging from +// the entity's Location, which is the box's top-left corner, the convention this +// package states. A collision is a strict intersection: boxes that merely touch, +// or sit close, are left alone. // // Which entity gives way. Entities are taken in the order they appear in the // domain model, which is creation order. An entity stays if its box clears every @@ -104,7 +105,7 @@ func (a rect) intersects(b rect, pad int) bool { // entityRect is the estimated box of e when its Location is at. func entityRect(e *domainmodel.Entity, at model.Point) rect { w, h := boxSize(e) - return rect{at.X - w/2, at.Y - h/2, at.X - w/2 + w, at.Y - h/2 + h} + return rect{at.X, at.Y, at.X + w, at.Y + h} } // annotationRect is the generous obstacle for a note; see "Annotations" above. @@ -212,15 +213,16 @@ func ResolveOverlaps(dm *domainmodel.DomainModel) OverlapPlan { return plan } -// nearestFree returns the point closest to from at which a w x h box centred -// there, grown by placementGutter, intersects none of obstacles. +// nearestFree returns the point closest to from at which a w x h box whose +// top-left corner is there, grown by placementGutter, intersects none of +// obstacles. func nearestFree(w, h int, from model.Point, obstacles []rect) model.Point { free := func(p model.Point) bool { // A mover never goes left of or above the canvas origin. - if p.X < w/2 || p.Y < h/2 { + if p.X < 0 || p.Y < 0 { return false } - b := rect{p.X - w/2, p.Y - h/2, p.X - w/2 + w, p.Y - h/2 + h} + b := rect{p.X, p.Y, p.X + w, p.Y + h} for _, o := range obstacles { if b.intersects(o, placementGutter) { return false @@ -231,7 +233,7 @@ func nearestFree(w, h int, from model.Point, obstacles []rect) model.Point { if free(from) { return from } - start := model.Point{X: max(from.X, w/2), Y: max(from.Y, h/2)} + start := model.Point{X: max(from.X, 0), Y: max(from.Y, 0)} // Past every obstacle there is always room, so the search is bounded: the // first ring that reaches beyond the rightmost edge has a free point. @@ -274,7 +276,7 @@ func nearestFree(w, h int, from model.Point, obstacles []rect) model.Point { } } // Unreachable by the bound above; the point is clear of everything anyway. - return model.Point{X: right + w/2 + placementGutter + placementStep, Y: start.Y} + return model.Point{X: right + placementGutter + placementStep, Y: start.Y} } func abs(v int) int { diff --git a/mdl/dmlayout/overlap_test.go b/mdl/dmlayout/overlap_test.go index ef70d658fc..f3d25560ce 100644 --- a/mdl/dmlayout/overlap_test.go +++ b/mdl/dmlayout/overlap_test.go @@ -11,7 +11,7 @@ import ( ) // assertNoOverlap fails for every pair of entities whose estimated boxes -// intersect when each sits at pos[id]. It is the checker TestPlan_NoOverlap +// intersect when each has its top-left corner at pos[id]. It is the checker TestPlan_NoOverlap // uses, shared so the two layouts are held to the same rule. func assertNoOverlap(t *testing.T, dm *domainmodel.DomainModel, pos map[model.ID]model.Point) { t.Helper() @@ -23,7 +23,7 @@ func assertNoOverlap(t *testing.T, dm *domainmodel.DomainModel, pos map[model.ID for _, e := range dm.Entities { p := pos[e.ID] w, h := boxSize(e) - boxes = append(boxes, rect{e.Name, p.X - w/2, p.Y - h/2, p.X + w/2, p.Y + h/2}) + boxes = append(boxes, rect{e.Name, p.X, p.Y, p.X + w, p.Y + h}) } for i := range boxes { for j := i + 1; j < len(boxes); j++ { @@ -80,8 +80,8 @@ func withPositions(dm *domainmodel.DomainModel, pos map[model.ID]model.Point) *d } // gridModel is what CREATE ENTITY leaves behind: n entities on GridSlot, every -// third one tall. Boxes are centred on the grid points, so from about 30 -// attributes (34+16*attrs high) a box reaches into the box one row down — the +// third one tall. Boxes hang from the grid points (Location is the top-left +// corner), so from about 30 attributes (34+16*attrs high) a box reaches into the box one row down — the // reported symptom, and the reason a fixed pitch cannot be the whole answer. func gridModel(n int) *domainmodel.DomainModel { dm := &domainmodel.DomainModel{} @@ -223,8 +223,8 @@ func TestResolveOverlaps_LeavesNoOverlap(t *testing.T) { } wb, hb := boxSize(b) pa, pb := pos[a.ID], pos[b.ID] - gapX := abs(pa.X-pb.X) - (wa+wb)/2 - gapY := abs(pa.Y-pb.Y) - (ha+hb)/2 + gapX := max(pb.X-(pa.X+wa), pa.X-(pb.X+wb)) + gapY := max(pb.Y-(pa.Y+ha), pa.Y-(pb.Y+hb)) if gapX < placementGutter-1 && gapY < placementGutter-1 { t.Errorf("n=%d: moved %s is within the gutter of %s (gap %d,%d)", n, a.Name, b.Name, gapX, gapY) } @@ -348,15 +348,81 @@ func TestResolveOverlaps_NothingToDo(t *testing.T) { func TestResolveOverlaps_NeverPlacesBeforeTheOrigin(t *testing.T) { for _, n := range []int{14, 40, 90} { dm := gridModel(n) - byID := map[model.ID]*domainmodel.Entity{} - for _, e := range dm.Entities { - byID[e.ID] = e - } for _, m := range ResolveOverlaps(dm).Moves { - w, h := boxSize(byID[m.ID]) - if m.To.X-w/2 < 0 || m.To.Y-h/2 < 0 { + if m.To.X < 0 || m.To.Y < 0 { t.Errorf("n=%d: %s moved to %+v, whose box starts before the origin", n, m.Name, m.To) } } } } + +// The real case that exposed the coordinate convention: an entity's Location is +// the top-left corner of its box in Studio Pro (verified in 11.12.5), not the +// centre. Laid out on the default grid, a 22-attribute entity at (100,380) ran +// into the one at (100,660) and a 45-attribute one at (880,100) into the one at +// (880,660), and a centred estimate saw neither. +func TestResolveOverlaps_TopLeftConventionFixture(t *testing.T) { + counts := []struct { + name string + attrs int + }{ + {"Customer", 4}, {"Order", 30}, {"OrderLine", 3}, {"Invoice", 45}, {"Payment", 5}, + {"Shipment", 6}, {"Product", 22}, {"Category", 2}, {"Supplier", 4}, {"Warehouse", 3}, + {"Employee", 50}, {"Address", 5}, {"Contact", 4}, {"Ticket", 35}, + } + dm := &domainmodel.DomainModel{} + for i, c := range counts { + p := GridSlot(i) + dm.Entities = append(dm.Entities, withLocation(ent(c.name, attrs(c.attrs)...), p.X, p.Y)) + } + dm.Entities = append(dm.Entities, withLocation(ent("Pinned", attrs(1)...), 4000, 3000)) + p := GridSlot(15) // (880,660), under Invoice + dm.Entities = append(dm.Entities, withLocation(ent("OrderSummary", attrs(1)...), p.X, p.Y)) + + plan := ResolveOverlaps(dm) + + // (a) both real collisions are found. + found := map[string]bool{} + for _, o := range plan.Pairs { + found[o.With+"/"+o.Entity] = true + } + for _, want := range []string{"Product/Contact", "Invoice/OrderSummary"} { + if !found[want] { + t.Errorf("collision %s not found; pairs: %+v", want, plan.Pairs) + } + } + + // (b) after the moves no two boxes intersect at the measured Studio Pro + // geometry: 150 wide, 34 + 15 per attribute tall, Location = top-left. Worked + // out here on purpose, not through boxSize. + pos := apply(dm, plan) + type box struct { + name string + x1, y1, x2, y2 int + } + var boxes []box + for _, e := range dm.Entities { + at := pos[e.ID] + boxes = append(boxes, box{e.Name, at.X, at.Y, at.X + 150, at.Y + 34 + 15*len(e.Attributes)}) + } + for i := range boxes { + for j := i + 1; j < len(boxes); j++ { + a, b := boxes[i], boxes[j] + if a.x1 < b.x2 && b.x1 < a.x2 && a.y1 < b.y2 && b.y1 < a.y2 { + t.Errorf("%s and %s overlap at the measured geometry: %+v vs %+v", a.name, b.name, a, b) + } + } + } + + // (c) a second pass has nothing left to do. + if again := ResolveOverlaps(withPositions(dm, pos)); len(again.Moves) != 0 { + t.Errorf("second pass still moves %d entities: %+v", len(again.Moves), again.Moves) + } + + // (d) nobody was sent off the top or left of the canvas. + for _, m := range plan.Moves { + if m.To.X < 0 || m.To.Y < 0 { + t.Errorf("%s moved to negative coordinates %+v", m.Name, m.To) + } + } +}