Windows build: layout grouping fix + style manager fix + nested groups - #1
Open
vicquick wants to merge 14 commits into
Open
Windows build: layout grouping fix + style manager fix + nested groups#1vicquick wants to merge 14 commits into
vicquick wants to merge 14 commits into
Conversation
styleClicked() was connected to QAbstractItemView::clicked with a direct connection. When the user clicks a saved style in the Layer Styling panel's Style Manager tab, styleClicked() calls setCurrentStyle() synchronously from inside QListView::mouseReleaseEvent(). setCurrentStyle() emits currentStyleChanged(), which triggers currentStyleChanged() → mStyleList->setCurrentIndex(). This modifies the view's selection model while mouseReleaseEvent still holds a QPersistentModelIndex (d->pressedIndex). On Qt 6, the invalidated persistent index causes a use-after-free when mouseReleaseEvent continues after the clicked() handler returns. On Windows: access violation (0xbaadf00d = freed heap). On Linux: SIGSEGV → SIGABRT. The crash does NOT happen when switching styles via: - Right-click layer → Styles submenu (QAction::triggered fires from the menu event loop, outside any view event handler) - Layer Properties dialog (QComboBox::currentIndexChanged, also outside a view event handler) Fix: change the clicked → styleClicked connection to Qt::QueuedConnection, so styleClicked() runs on the next event loop iteration — after mouseReleaseEvent has fully completed and released d->pressedIndex. Assisted-by: Claude (Anthropic)
…te rows QgsLayoutItemsListView::updateSelection() was connected to QItemSelectionModel::selectionChanged with a direct connection. When the source QgsLayoutModel begins a row insertion (for example when creating a new group via QgsLayoutView::groupSelectedItems -> addLayoutItem -> rebuildZList -> beginInsertRows) and there are persistent indices held by the items list view's selection model, those indices shift and QItemSelectionModel emits selectionChanged synchronously, from inside the still-open beginInsertRows transaction. updateSelection() then runs while the source model is mid-transaction and calls setSelected() on layout items, which emits dataChanged() on the source model. Emitting dataChanged from inside a beginInsertRows/endInsertRows bracket is a QAbstractItemModel contract violation. The observable consequences differed by Qt version: - Qt 6.8: QTreeView::dataChanged eagerly touches the selection model, which re-emits selectionChanged, re-enters updateSelection(), and the cycle recurses until the stack overflows. - Qt 6.9+: The mid-transaction dataChanged corrupts QSortFilterProxyModel's internal source-to-proxy mapping cache. The newly inserted row ends up with two proxy rows mapping to the same source row, so the Items list panel visibly shows a duplicate entry for the newly created group. Closing and reopening the layout rebuilds the proxy from scratch and the duplicate disappears. Both symptoms share the same root cause: updateSelection() running synchronously inside an open source-model transaction. Making the selectionChanged -> updateSelection connection a Qt::QueuedConnection defers the slot to the next event loop iteration, by which time endInsertRows() has fired and the proxy is in a consistent state. The mUpdatingSelection re-entry guard is also added to updateSelection()'s early return, matching the guard that onItemFocused() already uses. With Qt::QueuedConnection this is defense in depth, but it is cheap and protects against future refactors that might reintroduce a synchronous path into the slot. Fixes qgis#61702 Assisted-by: Claude (Anthropic)
- QgsLayoutModel: implement parent(), restructure index()/rowCount() so child items of a QgsLayoutItemGroup appear nested under their group instead of flat at the root. Two helpers, topLevelItemsInScene() and childItemsInScene(group), filter the existing scene-order list by parentGroup() membership. The root sentinel at top-level row 0 is preserved (proxy filters hide it from the user-facing panel). - QgsLayoutModel::indexForItem(): walk up parentGroup() chain to build the hierarchical index, instead of the flat row+1 lookup. - QgsLayoutItemsListView: setIndentation(16), setRootIsDecorated(true), setAnimated(true) so the QTreeView actually renders the hierarchy with disclosure arrows now that the model exposes it. Drag-and-drop reorder still rejects non-root parents pending per-group local z-stack support in QgsLayoutItemGroup. Assisted-by: Claude Opus 4.7
- QgsLayoutItemGroup gains reorderItemUp/Down/ToTop/ToBottom that mutate the group's mItems QList (which now has documented semantics: index 0 = topmost member, last index = bottommost). Persistence to ComposerItemGroupElement was already iteration-order based, so the local z-stack survives save/reload with no schema change. - QgsLayoutModel::childItemsInScene now defers to QgsLayoutItemGroup::items() for ordering, so reorder methods are immediately reflected in the layout items panel tree. - QgsLayout::groupItems sorts incoming selection by current global zValue (descending) before populating the new group, so the visually topmost selected item becomes the top of the group's local stack — matching the user's mental model when grouping a stack of items. Assisted-by: Claude Opus 4.7
QgsLayoutViewToolSelect: - The existing CTRL stack-cycle now also skips the "promote to top-level group" step, letting users click straight onto a nested member instead of always grabbing the enclosing group. Mirrors Adobe Illustrator's Group Selection / Direct Selection semantics with a single modifier. - Double-click on a group (or on any item inside one) enters isolation mode: every other layout item is dimmed to 25 % opacity so editing happens visually scoped to the group. Pages stay full opacity. - Esc, double-click on empty space, or tool deactivation restores every dimmed item to its original opacity. Original opacities are cached in mDimmedItems so a user-set opacity is preserved. - mIsolatedGroup tracked via QPointer so a deleted group is harmless. Assisted-by: Claude Opus 4.7
QgsLayoutItemsListView::updateSelection used to walk parentGroup() up to the outermost group and add it to the layout selection alongside the clicked child. That was useful when the panel was the only way to grab the group at all, but now that the canvas tool handles the click → group promotion (and Ctrl-click handles drill-in), the panel becomes the precise / direct selection surface — Adobe Layers panel semantics. A click on a child row now selects exactly that child. Group rows still work the same: clicking the group row selects the group. Assisted-by: Claude Opus 4.7
The inline accessor returned QgsLayoutItemGroup* from a QPointer<QgsLayoutItemGroup> member, which requires the complete QgsLayoutItemGroup type at every translation unit that includes the header. The header only forward-declares the class to keep its include surface minimal, so a translation unit that included the selection tool header without also pulling in qgslayoutitemgroup.h failed to compile (qgslayoutdesignerdialog.cpp was the first such unit hit during a clean build). Move the body to the .cpp so the QPointer-to-pointer conversion is instantiated where the full type is already available. Assisted-by: Claude Opus 4.7
Three follow-up issues from the first round of nested-grouping testing in the layout designer: 1. Visibility column overflow under nesting QTreeView puts the disclosure arrow + indent in column 0 by default, which is the visibility checkbox column. The fixed-width visibility column was getting eaten by the indent on nested rows. Switch tree position to the name column so vis / lock stay flush left at their fixed widths regardless of depth. 2. Last grouped member missing + ungroup wipes other rows QgsLayout::groupItems and ungroupItems mutate parentGroup() on members directly without telling QAbstractItemModel about the parent change. With the items model now hierarchical via parent() / index() / rowCount(), these mutations leave the tree reading stale state — a member appears under top-level when it should be under the new group, or vice versa. Easiest correct fix: emit a modelReset around the operation. Added QgsLayoutModel::emitModelReset() (a tiny public wrapper around the protected begin/endResetModel) and call it from groupItems and ungroupItems after the structural change. 3. New groups stay collapsed After modelReset the tree forgets expansion state. Wire expandAll() to modelReset so every group is visible by default — mirrors layer tree / browser tree behavior elsewhere in QGIS. Assisted-by: Claude Opus 4.7
Replace setTreePosition() (which moved the disclosure arrow into the name column) with a depth-aware width adjustment on the visibility column itself. The disclosure arrow now lives in the visibility column where it belongs visually (the Adobe-like nested look the user actually wants), and the column auto-grows by one indentation step per nesting level so the checkbox always has clear space. Recomputed on every modelReset (i.e. every group/ungroup) via adjustVisibilityColumnWidth(); base width when no groups exist is the original 4 'x' character widths, unchanged from upstream. Assisted-by: Claude Opus 4.7
Assisted-by: Claude Opus 4.7
It was placed next to the other hierarchy helpers in the private section, but unlike them it is called from QgsLayout::groupItems and ungroupItems which sit outside the class — the build failure on qgslayout.cpp was the access check. The other helpers stay private because they are only invoked from QgsLayoutModel members. Assisted-by: Claude Opus 4.7
QgsLayoutItemsListViewModel::filterAcceptsRow used to drop sourceRow 0 unconditionally to hide the source model's top-level paper / null sentinel. With the source model now hierarchical, sourceRow 0 of a group parent is a real item — the topmost member — and was being silently dropped from the panel. Made the check parent-aware so only the root sentinel is hidden. Also widen the visibility column to base * 2 + depth * indentation when any group exists, so the disclosure arrow plus indented checkbox have actual room at deeper nesting levels. Assisted-by: Claude Opus 4.7
Wires QgsMessageLog::logMessage("LayoutPerf", Info) on:
- QgsLayout::groupItems / ungroupItems — total wall time, plus
the split between mUndoStack work and the post-op
emitModelReset.
- QgsLayoutModel::emitModelReset — wall time and pre-reset
top-level + scene counts so we can see how big the model is.
- QgsLayoutItemsListView modelReset slot — expandAll vs total
panel work after a reset.
- QgsLayoutItemsListView::adjustVisibilityColumnWidth — depth
walk vs total column resize.
User can filter the QGIS Message Log panel to "LayoutPerf"
to see numbers per group/ungroup operation. No behavior change.
Assisted-by: Claude Opus 4.7
LayoutPerf logging used both without their headers; build failed on qgslayout.cpp.o. Assisted-by: Claude Opus 4.7
🍎
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integration branch for local Windows builds. Not for upstream.
Contains on top of upstream master:
Opened only to trigger the Windows Qt6 build workflow so the artifact can be downloaded.
Known: the nested-groups commits add public methods to QgsLayoutItemGroup, QgsLayoutModel and QgsLayoutViewToolSelect without regenerating the SIP bindings, so the Code Layout check will fail. Does not affect the Windows build.