From 91804d5f5e6eb7c4a04ce5fe765be78ab4242ecb Mon Sep 17 00:00:00 2001 From: ispyisail Date: Mon, 28 Sep 2026 09:19:59 +1300 Subject: [PATCH] Align: a group reduced to one shape lines up on its own centre A group whose only unlocked member is a shape (its other members locked) kept a centre of (0,0), so Centrer horizontalement and Centrer verticalement pulled every item toward the folio origin and the shape itself landed off the line. The rule that turns a group's members into one item now lives in Alignment::combined(), where every member, shapes included, brings its own centre, and tst_alignment covers it. Also comments why unitFor()'s returned reference is safe, as asked in the review of #1087. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G2d2Zi8BfrYRPX88zhaoFG --- sources/alignment.h | 18 +++++++++++++ sources/undocommand/alignselectioncommand.cpp | 20 +++++--------- tests/qttest/tst_alignment.cpp | 27 +++++++++++++++++++ 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/sources/alignment.h b/sources/alignment.h index adc533877..c3a320c55 100644 --- a/sources/alignment.h +++ b/sources/alignment.h @@ -54,6 +54,24 @@ namespace Alignment QPointF ref; }; + /** + @return @a items taken as one piece, the way a group lines up: + their edges together, and the middle of that box as its centre. + A single item keeps its own centre. + */ + inline Item combined(const QList &items) + { + Item result; + for (const Item &item : items) + result.edges = result.edges.isNull() ? item.edges + : result.edges.united(item.edges); + if (items.size() == 1) + result.ref = items.first().ref; + else + result.ref = result.edges.center(); + return result; + } + /** @return true if aligning on @a edge moves items along x */ diff --git a/sources/undocommand/alignselectioncommand.cpp b/sources/undocommand/alignselectioncommand.cpp index 1efa08dfd..f3a63c4b7 100644 --- a/sources/undocommand/alignselectioncommand.cpp +++ b/sources/undocommand/alignselectioncommand.cpp @@ -97,12 +97,14 @@ AlignSelectionCommand::AlignSelectionCommand(Diagram *diagram, Mode mode, QUndoC //group come along; shapes outside one are left out, as above. struct Unit { QList members; - Alignment::Item geometry; + QList parts; QGraphicsObject *snap_item = nullptr; ///< lands on its grid qreal divisor = 1; }; QList units; QHash group_units; + //Returns a reference into units, which a later call can grow: + //use it before calling again, never keep it auto unitFor = [&](QGraphicsObject *item) -> Unit & { const QUuid group = ItemGroups::groupOf(item); @@ -120,10 +122,7 @@ AlignSelectionCommand::AlignSelectionCommand(Diagram *diagram, Mode mode, QUndoC { Unit &unit = unitFor(entry.item); unit.members << entry.item; - unit.geometry.edges = unit.geometry.edges.isNull() - ? entry.geometry.edges - : unit.geometry.edges.united(entry.geometry.edges); - unit.geometry.ref = entry.geometry.ref; + unit.parts << entry.geometry; //Symbols come first in entries, so a group with one snaps on it if (!unit.snap_item) { unit.snap_item = entry.item; @@ -136,16 +135,11 @@ AlignSelectionCommand::AlignSelectionCommand(Diagram *diagram, Mode mode, QUndoC continue; Unit &unit = unitFor(shape); unit.members << shape; - unit.geometry.edges = unit.geometry.edges.isNull() - ? shape->sceneBoundingRect() - : unit.geometry.edges.united(shape->sceneBoundingRect()); + const QRectF rect = shape->sceneBoundingRect(); + unit.parts << Alignment::Item{rect, rect.center()}; if (!unit.snap_item) unit.snap_item = shape; } - for (Unit &unit : units) { - if (unit.members.size() > 1) - unit.geometry.ref = unit.geometry.edges.center(); - } m_item_count = units.size(); //Lining up a single item on itself would only snap it @@ -165,7 +159,7 @@ AlignSelectionCommand::AlignSelectionCommand(Diagram *diagram, Mode mode, QUndoC QList geometry; for (const Unit &unit : std::as_const(units)) - geometry << unit.geometry; + geometry << Alignment::combined(unit.parts); const QList offsets = Alignment::alignOffsets(geometry, edge); for (int i = 0 ; i < units.size() ; ++i) diff --git a/tests/qttest/tst_alignment.cpp b/tests/qttest/tst_alignment.cpp index f491ec439..5b8a8410a 100644 --- a/tests/qttest/tst_alignment.cpp +++ b/tests/qttest/tst_alignment.cpp @@ -155,6 +155,33 @@ private slots: QCOMPARE(offset.y(), 0.0); } + // A group lines up on the middle of its members' box; a unit with a + // single member, such as a shape whose group-mate is locked, keeps + // that member's own centre instead of the origin of the folio. + void combinedUnits() + { + const Alignment::Item a{QRectF(100, 200, 40, 20), QPointF(110, 210)}; + const Alignment::Item b{QRectF(300, 260, 20, 60), QPointF(310, 270)}; + + const Alignment::Item one = Alignment::combined({a}); + QCOMPARE(one.edges, a.edges); + QCOMPARE(one.ref, a.ref); + + const Alignment::Item both = Alignment::combined({a, b}); + QCOMPARE(both.edges, QRectF(100, 200, 220, 120)); + QCOMPARE(both.ref, QPointF(210, 260)); + + // a lone shape centred on x = 200 and a symbol at x = 400 meet + // half way, at 300; with the folio origin as the shape's centre + // they would meet at 200 and the shape would move 200 px + const QRectF shape(180, 50, 40, 40); + const QList offsets = Alignment::alignOffsets( + {Alignment::combined({{shape, shape.center()}}), {QRectF(390, 0, 20, 20), QPointF(400, 10)}}, + Alignment::HCenter); + QCOMPARE(offsets.at(0), QPointF(100, 0)); + QCOMPARE(offsets.at(1), QPointF(-100, 0)); + } + void emptySelection() { QVERIFY(Alignment::alignOffsets({}, Alignment::Left).isEmpty());