Shortcuts v2. Added display of sections in UI - #192
Conversation
cacc7aa to
2793ec4
Compare
📝 WalkthroughWalkthroughThe change adds grouped section support to 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qml`:
- Line 37: Update the ValueList.selection mapping to route selection through
SectionProxyModel rather than directly through sortFilterProxyModel, while
preserving the active header item in the computed sourceSelection when
applicable. Use the existing selectionModel, sectionProxyModel, and
sortFilterProxyModel symbols and keep the current selection behavior for
non-header items.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp`:
- Line 59: Update the RoleGroup handling in ShortcutsModel so action shortcuts
return a non-empty, stable group value such as action.code, preventing
SectionProxyModel from creating an empty section row; preserve existing group
values for other item types.
In
`@framework/uicomponents/qml/Muse/UiComponents/internal/ValueListSectionItem.qml`:
- Around line 31-46: Add a visible expand/collapse arrow indicator to
ValueListSectionItem, using the existing isExpanded property to switch its
direction and following the established indicator pattern from
ExpandableBlankSection.qml. Place it in the section header so the clickable
toggle affordance is clear, while preserving the existing accessibility name
behavior.
- Around line 67-71: Declare a local boolean property named drawZebra on the
ValueListSectionItem root, defaulting to true, so the SeparatorLine.visible
binding references a defined value and preserves the intended separator
behavior.
In `@framework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cpp`:
- Around line 73-137: Refactor SectionProxyModel::setSectionCollapsed() to
update m_collapsedSections and delegate row reconstruction to rebuild(),
removing the duplicated collapse/expand row-management logic and its
source-model rescan. Preserve the existing no-op behavior when the requested
state already matches, and retain the section expansion notification plus
collapsedSectionsChanged emission after rebuilding.
- Around line 366-381: Update SectionProxyModel::onSourceDataChanged so an empty
roles list does not immediately trigger rebuild(). When the section role is
configured and roles are unspecified, inspect the affected source rows’
section-role values before and after the change, and rebuild only if a section
value actually changed; preserve rebuild behavior for explicitly included
section roles and normal dataChanged forwarding otherwise.
- Around line 300-345: Update SectionProxyModel::rebuild() to group all source
rows by section value rather than comparing only with the current header,
ensuring each section creates exactly one header even when source rows are
non-contiguous. Use a two-pass build or equivalent approach that collects
section membership first, then emits headers and items in the required order
while keeping header indexes and collapse behavior consistent with sectionRow(),
isCollapsed(), and updateRowIndexes().
In `@framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 211-231: Cache the section role identifier in a new
m_sectionRoleId member initialized to INVALID_ROLE_ID, and replace the
per-comparison roleIdFromName call in lessThan() with that cached value.
Recompute the member in both setSectionRoleName() and updateRoleIds(),
preserving the existing invalid-role fallback behavior.
- Around line 270-282: Update SortFilterProxyModel::setSourceModel() to call
updateSorting() after assigning the source model and completing existing
invalidation and role-name notifications. Preserve the current filter
invalidation behavior while ensuring sectionRoleName and enabled SorterValue
sorting are reapplied once source roles are available.
In
`@framework/uicomponents/qml/Muse/UiComponents/tests/sectionproxymodel_tests.cpp`:
- Around line 114-130: Add a test fixture or test case that connects
SectionProxyModel directly to SectionedModel, bypassing SortFilterProxyModel and
its sorting configuration. Use the unsorted testItems() data to verify the
duplicate-header behavior handled by SectionProxyModel::rebuild(), while
preserving the existing sorted-model coverage.
In
`@framework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp`:
- Around line 38-98: Move the duplicated SectionedModel class and its shared
roles/data definition into a common test header, then include that header from
both sortfilterproxymodel_tests.cpp and sectionproxymodel_tests.cpp. Reconcile
the constructor differences by supporting the parameterized data used by one
test while preserving the default dataset used by the other; keep the titles
helper local to the file that uses it.
In `@framework/uicomponents/qml/Muse/UiComponents/ValueList.qml`:
- Around line 324-326: Update the shared navigation row assignment for both the
ValueListSectionItem and ValueListItem delegates so editable key rows use the
doubled row scheme. In the delegate navigation bindings around navigation.row
and the ValueListItem navigation-control section, use rowLoader.rowIndex * 2
when root.isKeyEditable is true, while preserving the existing row index for
non-editable lists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e44a93b5-80c5-4e76-b283-6b95ecc6bea1
📒 Files selected for processing (16)
framework/CMakeLists.txtframework/shortcuts_v2/api/shortcutsapi.hframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.hframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/ValueList.qmlframework/uicomponents/qml/Muse/UiComponents/internal/ValueListItem.qmlframework/uicomponents/qml/Muse/UiComponents/internal/ValueListSectionItem.qmlframework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cppframework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.hframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.hframework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/tests/sectionproxymodel_tests.cppframework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp
2793ec4 to
1a1712f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/CMakeLists.txt`:
- Around line 116-120: Update the MUSE_MODULE_SHORTCUTS_V2 link setup in
framework/CMakeLists.txt to avoid configuration failure on Windows when
symbolic-link creation is unavailable. Add a Windows-compatible fallback such as
a directory junction or copied directory, while preserving the existing symlink
behavior where supported and ensuring the shortcuts path remains available.
In `@framework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cpp`:
- Around line 422-438: Cache the resolved section role ID in a member, updating
`sectionRoleId()` to return the cached value instead of scanning
`sourceModel()->roleNames()` on every call. Recompute and store it in
`setSourceModel()`, `setSectionRoleName()`, and the handler for source role-name
changes, while preserving `INVALID_ROLE_ID` when no valid source model or role
exists.
- Around line 174-201: Update SectionProxyModel::setSourceModel to connect the
source model’s modelAboutToBeReset signal to beginResetModel(), and have the
modelReset handling call endResetModel() rather than rebuild(). Keep the
existing rebuild behavior for other source changes, and ensure each source reset
is represented by one matching proxy reset pair instead of rebuilding m_rows on
modelReset.
In `@framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 268-280: Update SortFilterProxyModel::updateSorting() to choose
the sort column using the resolved m_sectionRoleId validity, matching lessThan()
rather than checking m_sectionRoleName. Preserve the existing fallback to column
-1 when the role is invalid and column 0 when it is valid.
In
`@framework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp`:
- Around line 201-219: Update the test around
SortFilterProxyModel::setSourceModel to populate secondModel with distinct
grouped data from firstModel, then change the final titles assertion to the
expected sorted output for secondModel. Keep the firstModel assertion unchanged
so the test verifies both the initial source and replacement behavior.
In `@framework/uicomponents/qml/Muse/UiComponents/ValueList.qml`:
- Around line 88-94: Update collapseAllSections() to clear the current selection
after collapsing all sections, matching the behavior of
prv.setSectionCollapsed() when items become hidden. Keep expandAllSections()
unchanged and use the existing selection-clearing mechanism rather than
introducing new state handling.
- Around line 71-82: Update the currentSourceRow binding to depend on a proxy
model property that changes when sections collapse or expand, such as the
relevant proxy row count, so it reevaluates after layout changes. Alternatively,
reset view.currentIndex in prv.setSectionCollapsed; preserve the existing
invalid-index handling and source-row mapping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07c4f511-bf81-482e-a24e-54528bebd5b2
📒 Files selected for processing (17)
framework/CMakeLists.txtframework/shortcuts_v2/api/shortcutsapi.hframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.hframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/ValueList.qmlframework/uicomponents/qml/Muse/UiComponents/internal/ValueListItem.qmlframework/uicomponents/qml/Muse/UiComponents/internal/ValueListSectionItem.qmlframework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cppframework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.hframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.hframework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/tests/sectionedmodel.hframework/uicomponents/qml/Muse/UiComponents/tests/sectionproxymodel_tests.cppframework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp
| if (MUSE_MODULE_SHORTCUTS_V2) | ||
| file(CREATE_LINK ${CMAKE_CURRENT_LIST_DIR}/shortcuts_v2 ${PROJECT_BINARY_DIR}/shortcuts SYMBOLIC) | ||
| else() | ||
| file(REMOVE_RECURSE ${PROJECT_BINARY_DIR}/shortcuts) | ||
| endif() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify supported Windows CI jobs and the project's CMake version requirement.
rg -n -C 3 'windows|Windows|cmake_minimum_required|CMAKE_VERSION' \
-g 'CMakeLists.txt' -g '*.yml' -g '*.yaml' .Repository: musescore/muse_framework
Length of output: 25467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files mentioning MUSE_MODULE_SHORTCUTS_V2 / shortcuts:\n'
rg -n -C 3 'MUSE_MODULE_SHORTCUTS_V2|shortcuts_v2|shortcuts|CREATE_LINK|REMOVE_RECURSE' \
-g 'CMakeLists.txt' .
printf '\nTop-level CMake context:\n'
sed -n '1,40p;100,140p' CMakeLists.txt
printf '\nGitHub workflow files with Windows/job hints:\n'
if [ -d .github/workflows ]; then
rg -n -C 3 'windows|ubuntu|macos|cmake|shortcuts|module.*shortcut|MUSE_MODULE_SHORTCUTS|VSCMD|MSBuild|CMAKE_GENERATOR|MSYS|MinGW|Chocolatey|winget' .github/workflows || true
else
echo 'No .github/workflows directory'
fiRepository: musescore/muse_framework
Length of output: 26173
🌐 Web query:
CMake file CREATE_LINK SYMBOLIC directory symbolic link Windows create junction FILE_CREATE_LINK_POLICY documentation
💡 Result:
The file(CREATE_LINK) command in CMake, introduced in version 3.14, is the standard way to create filesystem links [1][2]. For your specific requirements regarding Windows and directory linking, here is the synthesis of current documentation: 1. Symbolic Links and Junctions on Windows: The file(CREATE_LINK <original> <linkname> SYMBOLIC) command creates a symbolic link on Windows [1]. However, it is important to note that file(CREATE_LINK) does not explicitly provide a flag to create a Windows "Junction" (created via mklink /J) [3][1]. Symbolic links and junctions function differently on Windows; if your build system specifically requires a directory junction for compatibility with older tools or specific path behavior, file(CREATE_LINK) may not be sufficient, and you might need to use execute_process to call cmd /c mklink /J <link> <target> directly [4][5]. 2. Policy CMP0205 (file(CREATE_LINK) with COPY_ON_ERROR): The COPY_ON_ERROR option in file(CREATE_LINK) acts as a fallback mechanism; if the system cannot create the link, it attempts to copy the source content to the destination [1]. Policy CMP0205 (introduced in CMake 4.3) governs how this option handles directories [6][7]: - OLD behavior (or not set): CMake creates the destination directory but does not copy its contents [6][7]. - NEW behavior: CMake creates the destination directory and recursively copies the contents of the source directory [6][7]. This policy ensures backward compatibility for projects that relied on the previous behavior [6]. 3. Recommendations: - For general symbolic links, use file(CREATE_LINK ... SYMBOLIC) [1]. - If you specifically require a Directory Junction on Windows, use execute_process with mklink /J [4][5]. Ensure you convert paths using file(TO_NATIVE_PATH ...) to satisfy the mklink command's requirement for backslashes [4][5]. - If using COPY_ON_ERROR in modern CMake, be aware of CMP0205 if you are managing directory links, as it changes the behavior regarding recursive copying [6][7].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/file.html
- 2: https://cmake.org/cmake/help/v4.2/command/file.html
- 3: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/mklink
- 4: https://stackoverflow.com/questions/35765106/symbolic-links-cmake
- 5: https://stackoverflow.com/questions/61243174/replacement-of-create-symlink-in-windows
- 6: https://cmake.org/cmake/help/latest/policy/CMP0205.html
- 7: https://cmake.org/cmake/help/git-stage/policy/CMP0205.html
🌐 Web query:
CMake file CREATE_LINK directories link type default directory SYMBOLIC Windows junction
💡 Result:
In CMake, the file(CREATE_LINK SYMBOLIC) command is the standard way to create symbolic links [1][2]. By default, file(CREATE_LINK) creates a hard link, but adding the SYMBOLIC option changes this behavior to create a symbolic link instead [1][3]. Regarding Windows behavior and junctions: 1. Symbolic Link Implementation: On Windows, file(CREATE_LINK... SYMBOLIC) attempts to create a standard Windows symbolic link [4][5]. This functionality is generally supported in modern versions of CMake (since 3.14) [1][4]. However, creating symbolic links on Windows often requires specific permissions, such as running with Administrator privileges or having Windows Developer Mode enabled [5][6]. 2. Junctions vs. Symbolic Links: CMake's built-in file(CREATE_LINK) command does not explicitly provide an option to create a Windows directory "junction" (/J in the mklink command) [1][7]. It aims to create cross-platform symbolic links. If you specifically require a directory junction—which behaves differently than a symbolic link regarding remote path resolution and permissions—you cannot rely on file(CREATE_LINK) [1][7]. 3. Workaround for Junctions: If a junction is required for your Windows workflow, you must use execute_process to call the native Windows mklink command [4][8]: execute_process( COMMAND cmd /C mklink /J "" "") When using this workaround, ensure that paths are converted to Windows native format using file(TO_NATIVE_PATH...) to avoid issues with path separators [4][8]. 4. Directory Handling: Note that as of CMake 4.3, file(CREATE_LINK) with the COPY_ON_ERROR option will recursively copy the contents of a source directory if the link creation fails [1][3]. In older versions, it would only create the destination directory without copying contents [9][10].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/file.html?highlight=file
- 2: https://cmake.org/cmake/help/v3.28/command/file.html
- 3: https://cmake.org/cmake/help/latest/command/file.html
- 4: https://stackoverflow.com/questions/35765106/symbolic-links-cmake
- 5: Symlinking on Windows needs privilege ament/ament_cmake#350
- 6: https://reviews.llvm.org/D99170
- 7: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/mklink
- 8: https://stackoverflow.com/questions/61243174/replacement-of-create-symlink-in-windows
- 9: https://cmake.org/cmake/help/v4.2/command/file.html
- 10: https://cmake.org/cmake/help/latest/policy/CMP0205.html
Provide a symlink-free fallback for Windows builds.
Line 117 creates a directory symbolic link with file(CREATE_LINK ... SYMBOLIC). If this filesystem or user cannot create a directory symbolic link, this command fails since there is no RESULT or COPY_ON_ERROR handling, and CMake stops the configuration. Add a compatible fallback, such as a Windows directory junction or non-symlink alias, or make the option clear why this directory link is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/CMakeLists.txt` around lines 116 - 120, Update the
MUSE_MODULE_SHORTCUTS_V2 link setup in framework/CMakeLists.txt to avoid
configuration failure on Windows when symbolic-link creation is unavailable. Add
a Windows-compatible fallback such as a directory junction or copied directory,
while preserving the existing symlink behavior where supported and ensuring the
shortcuts path remains available.
| void SectionProxyModel::setSourceModel(QAbstractItemModel* sourceModel) | ||
| { | ||
| for (const QMetaObject::Connection& connection : std::as_const(m_sourceConnections)) { | ||
| disconnect(connection); | ||
| } | ||
| m_sourceConnections.clear(); | ||
|
|
||
| QAbstractProxyModel::setSourceModel(sourceModel); | ||
|
|
||
| if (sourceModel) { | ||
| const auto rebuildOnChange = [this]() { rebuild(); }; | ||
|
|
||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::modelReset, this, rebuildOnChange); | ||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::layoutChanged, this, rebuildOnChange); | ||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::rowsInserted, this, rebuildOnChange); | ||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, rebuildOnChange); | ||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::rowsMoved, this, rebuildOnChange); | ||
| m_sourceConnections << connect(sourceModel, &QAbstractItemModel::dataChanged, this, | ||
| &SectionProxyModel::onSourceDataChanged); | ||
|
|
||
| if (auto sortFilterModel = qobject_cast<SortFilterProxyModel*>(sourceModel)) { | ||
| m_sourceConnections << connect(sortFilterModel, &SortFilterProxyModel::sourceModelRoleNamesChanged, | ||
| this, rebuildOnChange); | ||
| } | ||
| } | ||
|
|
||
| rebuild(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how SortFilterProxyModel handles source model reset, for comparison with SectionProxyModel.
set -euo pipefail
fd -t f 'sortfilterproxymodel.cpp' framework | while IFS= read -r f; do
echo "FILE: $f"
rg -n -C 8 'AboutToBeReset|modelReset|beginResetModel|endResetModel' "$f"
done
fd -t f 'sectionproxymodel.cpp' framework | while IFS= read -r f; do
echo "FILE: $f"
rg -n -C 4 'AboutToBeReset|modelReset|beginResetModel|endResetModel' "$f"
doneRepository: musescore/muse_framework
Length of output: 1570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sectionproxymodel outline and relevant sections =="
fd -t f '^sectionproxymodel\.(cpp|h)$' framework | while IFS= read -r f; do
echo "FILE: $f"
wc -l "$f"
ast-grep outline "$f" || true
done
echo "== sectionproxy relevant reset/data mapping =="
fd -t f '^sectionproxymodel\.(cpp|h)$' framework | while IFS= read -r f; do
echo "FILE: $f"
rg -n -C 7 'modelAboutToBeReset|modelReset|beginResetModel|endResetModel|rebuild\(\)|m_sourceRowToProxyRow|m_rows|mapToSource|data\(' "$f"
done
echo "== find all reset-related handlers in framework qml ui =="
rg -n 'modelAboutToBeReset|m_sourceModelAboutToBeResetConnection|beginResetModel|endResetModel' framework/uicomponents/qml/Muse/UiComponents -SRepository: musescore/muse_framework
Length of output: 18797
🌐 Web query:
Qt QAbstractProxyModel beginResetModel endResetModel modelAboutToBeReset modelReset dataInvalidated
💡 Result:
In Qt's model-view framework, the combination of beginResetModel, endResetModel, and their associated signals is used to notify views and connected components that a model's internal structure or data has undergone a significant, radical change [1]. Key components of the model reset process include: 1. beginResetModel: This protected function must be called before making any changes to the model's internal data structures [1]. It alerts connected views that the model is about to be reset, allowing them to prepare for the change [1][2]. Crucially, it emits the modelAboutToBeReset signal [1]. Calling this function before structural modifications prevents views from attempting to access invalid memory or stale model indexes during the reset, which could otherwise lead to crashes [3]. 2. endResetModel: This protected function must be called after the internal data structures have been updated [1]. It informs the views that the reset is complete and that they should re-query the model for data [1][2]. It emits the modelReset signal [1]. 3. Signals: - modelAboutToBeReset: Emitted when beginResetModel is called, this signal warns components that the current model state (including persistent indexes) is about to be invalidated [1]. - modelReset: Emitted when endResetModel is called, this signals that the model's internal state has been invalidated and that any previously cached information—such as row/column counts, flags, and data—should be considered obsolete [1]. Regarding dataInvalidated, this is not a standard signal in the QAbstractItemModel or QAbstractProxyModel API [4][1]. The reset mechanism is the preferred approach for radical changes, whereas smaller, specific updates should be handled by more targeted signals like dataChanged, rowsInserted, or rowsRemoved to maintain performance [5][2]. In the context of QAbstractProxyModel, subclasses must use this reset pattern specifically when changing the underlying source model [6][7][8]. The standard practice is to call beginResetModel, disconnect the old source model, update the source model (e.g., via setSourceModel), connect to the new source model, and then call endResetModel [6][9].
Citations:
- 1: https://doc.qt.io/qt-6/qabstractitemmodel.html
- 2: https://runebook.dev/en/docs/qt/qabstractitemmodel/endResetModel
- 3: https://www.qtcentre.org/threads/69924-QAbstractItemModel-reset()-obsolete
- 4: https://doc.qt.io/qt-6/qabstractproxymodel-members.html
- 5: https://stackoverflow.com/questions/21478746/qt-5-2-model-view-pattern-how-to-inform-model-object-about-changes-in-underlyin
- 6: https://doc.qt.io/qt-6/qabstractproxymodel.html
- 7: https://doc.qt.io/archives/qt-6.9/qabstractproxymodel.html
- 8: https://doc.qt.io/qt-6.8/qabstractproxymodel.html
- 9: https://github.com/qt/qtbase/blob/85949d70/src/corelib/itemmodels/qabstractproxymodel.cpp
🌐 Web query:
documentation QAbstractProxyModel beginResetModel endResetModel source model reset connect modelAboutToBeReset endResetModel
💡 Result:
In Qt, when implementing a custom proxy model by inheriting from QAbstractProxyModel, managing the transition between source models—or reacting to a source model reset—requires specific handling of model reset signals to ensure the view and the proxy stay synchronized [1][2]. When overriding setSourceModel(QAbstractItemModel *sourceModel), the official documentation specifies a required pattern to maintain state consistency [1][3]: 1. Call beginResetModel to notify observers (like views) that the model is about to change and invalidates existing indexes [1][3]. 2. Disconnect any signals from the old source model. 3. Call the base class implementation: QAbstractProxyModel::setSourceModel(sourceModel) [2][3]. 4. Connect the new source model's signals (e.g., dataChanged, layoutChanged) to your proxy's slots [2][4]. 5. Call endResetModel to notify observers that the reset is complete and indexes can be refreshed [1][3]. Additionally, if your proxy needs to react automatically when the source model itself triggers a reset, you should connect to the source model's modelAboutToBeReset and modelReset signals [2][5]: - Connect sourceModel->modelAboutToBeReset to a slot that calls beginResetModel [2]. - Connect sourceModel->modelReset to a slot that calls endResetModel [2]. This ensures that whenever the source model's structure is invalidated, the proxy model correctly propagates this event to its own observers [2].
Citations:
- 1: https://doc.qt.io/qt-6/qabstractproxymodel.html
- 2: https://stackoverflow.com/questions/17562181/qt-signal-forwarding-inheriting-qabstractproxymodel
- 3: https://codebrowser.dev/qt6/qtbase/src/corelib/itemmodels/qabstractproxymodel.cpp.html
- 4: https://github.com/qt/qtbase/blob/85949d70/src/corelib/itemmodels/qabstractproxymodel.cpp
- 5: https://doc.qt.io/qt-6/qabstractproxymodel-members.html
Handle the source-model reset as a single proxy reset.
Source modelAboutToBeReset can arrive before any local proxy update. Connect it and call beginResetModel(), then call endResetModel() from modelReset instead of rebuilding the whole m_rows list in response to each reset signal.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 181-181: variable 'sourceModel' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cpp` around
lines 174 - 201, Update SectionProxyModel::setSourceModel to connect the source
model’s modelAboutToBeReset signal to beginResetModel(), and have the modelReset
handling call endResetModel() rather than rebuild(). Keep the existing rebuild
behavior for other source changes, and ensure each source reset is represented
by one matching proxy reset pair instead of rebuilding m_rows on modelReset.
| int SectionProxyModel::sectionRoleId() const | ||
| { | ||
| if (m_sectionRoleName.isEmpty() || !sourceModel()) { | ||
| return INVALID_ROLE_ID; | ||
| } | ||
|
|
||
| const QByteArray roleName = m_sectionRoleName.toUtf8(); | ||
| const QHash<int, QByteArray> roles = sourceModel()->roleNames(); | ||
|
|
||
| for (auto it = roles.cbegin(); it != roles.cend(); ++it) { | ||
| if (it.value() == roleName) { | ||
| return it.key(); | ||
| } | ||
| } | ||
|
|
||
| return INVALID_ROLE_ID; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Cache the section role id instead of resolving it on every call.
sectionRoleId() copies sourceModel()->roleNames() into a QHash and linear-scans it. onSourceDataChanged() at line 385 calls it for every source data change, and rebuild() and setSectionCollapsed() call it as well. The sibling SortFilterProxyModel now caches the resolved id in m_sectionRoleId for the same reason.
Store the resolved id in a member. Recompute it in setSourceModel(), in setSectionRoleName(), and when the source role names change.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 422-422: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 422-422: method 'sectionRoleId' can be made static
(readability-convert-member-functions-to-static)
[warning] 428-428: variable 'roleName' is not initialized
(cppcoreguidelines-init-variables)
[warning] 429-429: variable 'roles' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/uicomponents/qml/Muse/UiComponents/sectionproxymodel.cpp` around
lines 422 - 438, Cache the resolved section role ID in a member, updating
`sectionRoleId()` to return the cached value instead of scanning
`sourceModel()->roleNames()` on every call. Recompute and store it in
`setSourceModel()`, `setSectionRoleName()`, and the handler for source role-name
changes, while preserving `INVALID_ROLE_ID` when no valid source model or role
exists.
|
|
||
| void SortFilterProxyModel::updateSorting() | ||
| { | ||
| Sorter* sorter = currentSorter(); | ||
| invalidate(); | ||
|
|
||
| if (!sorter) { | ||
| sort(m_sectionRoleName.isEmpty() ? -1 : 0, Qt::AscendingOrder); | ||
| return; | ||
| } | ||
|
|
||
| sort(0, sorter->sortOrder()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Guard updateSorting() on the resolved role id, not the role name.
Line 275 decides the sort column from m_sectionRoleName.isEmpty(). lessThan() at line 210 decides from m_sectionRoleId != INVALID_ROLE_ID. The two guards disagree when a name is set but does not resolve to a role of the source model. The proxy then enables column-0 sorting while lessThan() applies no section comparison. Use the same condition in both places.
♻️ Proposed refactor
if (!sorter) {
- sort(m_sectionRoleName.isEmpty() ? -1 : 0, Qt::AscendingOrder);
+ sort(m_sectionRoleId == INVALID_ROLE_ID ? -1 : 0, Qt::AscendingOrder);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void SortFilterProxyModel::updateSorting() | |
| { | |
| Sorter* sorter = currentSorter(); | |
| invalidate(); | |
| if (!sorter) { | |
| sort(m_sectionRoleName.isEmpty() ? -1 : 0, Qt::AscendingOrder); | |
| return; | |
| } | |
| sort(0, sorter->sortOrder()); | |
| } | |
| void SortFilterProxyModel::updateSorting() | |
| { | |
| Sorter* sorter = currentSorter(); | |
| invalidate(); | |
| if (!sorter) { | |
| sort(m_sectionRoleId == INVALID_ROLE_ID ? -1 : 0, Qt::AscendingOrder); | |
| return; | |
| } | |
| sort(0, sorter->sortOrder()); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 274-274: implicit conversion 'muse::uicomponents::Sorter *' -> bool
(readability-implicit-bool-conversion)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around
lines 268 - 280, Update SortFilterProxyModel::updateSorting() to choose the sort
column using the resolved m_sectionRoleId validity, matching lessThan() rather
than checking m_sectionRoleName. Preserve the existing fallback to column -1
when the role is invalid and column 0 when it is valid.
| auto firstModel = std::make_unique<SectionedModel>(); | ||
| auto secondModel = std::make_unique<SectionedModel>(); | ||
| auto proxyModel = std::make_unique<SortFilterProxyModel>(); | ||
|
|
||
| SorterValue* sorter = new SorterValue(proxyModel.get()); | ||
| sorter->setRoleName(u"title"_s); | ||
|
|
||
| QQmlListProperty<Sorter> sorters = proxyModel->sorters(); | ||
| ASSERT_TRUE(sorters.append); | ||
| sorters.append(&sorters, sorter); | ||
|
|
||
| proxyModel->setSectionRoleName(u"group"_s); | ||
| sorter->setEnabled(true); | ||
|
|
||
| proxyModel->setSourceModel(firstModel.get()); | ||
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s })); | ||
|
|
||
| proxyModel->setSourceModel(secondModel.get()); | ||
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use distinct data for secondModel.
firstModel and secondModel use the same default items. The final assertion passes even if setSourceModel() keeps the first source model. Give secondModel distinct grouped items and assert its sorted output after replacement.
Proposed test change
- auto secondModel = std::make_unique<SectionedModel>();
+ auto secondModel = std::make_unique<SectionedModel>(QList<SectionedModel::Item> {
+ { u"z2"_s, u"Z"_s },
+ { u"y1"_s, u"Y"_s },
+ { u"x"_s, QString() },
+ { u"z1"_s, u"Z"_s }
+ });
...
proxyModel->setSourceModel(secondModel.get());
- EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s }));
+ EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"y1"_s, u"z1"_s, u"z2"_s, u"x"_s }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto firstModel = std::make_unique<SectionedModel>(); | |
| auto secondModel = std::make_unique<SectionedModel>(); | |
| auto proxyModel = std::make_unique<SortFilterProxyModel>(); | |
| SorterValue* sorter = new SorterValue(proxyModel.get()); | |
| sorter->setRoleName(u"title"_s); | |
| QQmlListProperty<Sorter> sorters = proxyModel->sorters(); | |
| ASSERT_TRUE(sorters.append); | |
| sorters.append(&sorters, sorter); | |
| proxyModel->setSectionRoleName(u"group"_s); | |
| sorter->setEnabled(true); | |
| proxyModel->setSourceModel(firstModel.get()); | |
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s })); | |
| proxyModel->setSourceModel(secondModel.get()); | |
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s })); | |
| auto firstModel = std::make_unique<SectionedModel>(); | |
| auto secondModel = std::make_unique<SectionedModel>(QList<SectionedModel::Item> { | |
| { u"z2"_s, u"Z"_s }, | |
| { u"y1"_s, u"Y"_s }, | |
| { u"x"_s, QString() }, | |
| { u"z1"_s, u"Z"_s } | |
| }); | |
| auto proxyModel = std::make_unique<SortFilterProxyModel>(); | |
| SorterValue* sorter = new SorterValue(proxyModel.get()); | |
| sorter->setRoleName(u"title"_s); | |
| QQmlListProperty<Sorter> sorters = proxyModel->sorters(); | |
| ASSERT_TRUE(sorters.append); | |
| sorters.append(&sorters, sorter); | |
| proxyModel->setSectionRoleName(u"group"_s); | |
| sorter->setEnabled(true); | |
| proxyModel->setSourceModel(firstModel.get()); | |
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"a1"_s, u"a2"_s, u"b1"_s, u"b2"_s, u"x"_s })); | |
| proxyModel->setSourceModel(secondModel.get()); | |
| EXPECT_EQ(titles(proxyModel.get()), QStringList({ u"y1"_s, u"z1"_s, u"z2"_s, u"x"_s })); |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 205-205: use auto when initializing with new to avoid duplicating the type name
(modernize-use-auto)
[warning] 208-208: variable 'sorters' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp`
around lines 201 - 219, Update the test around
SortFilterProxyModel::setSourceModel to populate secondModel with distinct
grouped data from firstModel, then change the final titles assertion to the
expected sorted output for secondModel. Keep the firstModel assertion unchanged
so the test verifies both the initial source and replacement behavior.
| readonly property int currentSourceRow: { | ||
| if (view.currentIndex < 0) { | ||
| return -1 | ||
| } | ||
|
|
||
| var filteredRow = sectionProxyModel.sourceRowOf(view.currentIndex) | ||
| if (filteredRow < 0) { | ||
| return -1 | ||
| } | ||
|
|
||
| return sortFilterProxyModel.mapToSource(sortFilterProxyModel.index(filteredRow, 0)).row | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
currentSourceRow can return a stale row after a section collapses or expands.
The binding reads view.currentIndex, then calls sectionProxyModel.sourceRowOf() and sortFilterProxyModel.mapToSource(). Both are invokable methods, not properties, so they create no binding dependency. When a section collapses or expands, the proxy row layout changes while view.currentIndex keeps its value. The binding does not re-evaluate, and currentSourceRow then reports the source row of a different item.
prv.setSectionCollapsed() at line 107 clears the selection but leaves view.currentIndex unchanged, so this path is reachable from the section header toggle at line 356.
Add a dependency on the proxy row count, or reset view.currentIndex when the section state changes.
🐛 Proposed fix
readonly property int currentSourceRow: {
if (view.currentIndex < 0) {
return -1
}
+ //! NOTE: Re-evaluate when the section layout changes
+ var _ = sectionProxyModel.rowCount
+
var filteredRow = sectionProxyModel.sourceRowOf(view.currentIndex)
if (filteredRow < 0) {
return -1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readonly property int currentSourceRow: { | |
| if (view.currentIndex < 0) { | |
| return -1 | |
| } | |
| var filteredRow = sectionProxyModel.sourceRowOf(view.currentIndex) | |
| if (filteredRow < 0) { | |
| return -1 | |
| } | |
| return sortFilterProxyModel.mapToSource(sortFilterProxyModel.index(filteredRow, 0)).row | |
| } | |
| readonly property int currentSourceRow: { | |
| if (view.currentIndex < 0) { | |
| return -1 | |
| } | |
| //! NOTE: Re-evaluate when the section layout changes | |
| var _ = sectionProxyModel.rowCount | |
| var filteredRow = sectionProxyModel.sourceRowOf(view.currentIndex) | |
| if (filteredRow < 0) { | |
| return -1 | |
| } | |
| return sortFilterProxyModel.mapToSource(sortFilterProxyModel.index(filteredRow, 0)).row | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/uicomponents/qml/Muse/UiComponents/ValueList.qml` around lines 71 -
82, Update the currentSourceRow binding to depend on a proxy model property that
changes when sections collapse or expand, such as the relevant proxy row count,
so it reevaluates after layout changes. Alternatively, reset view.currentIndex
in prv.setSectionCollapsed; preserve the existing invalid-index handling and
source-row mapping behavior.
| function expandAllSections() { | ||
| sectionProxyModel.expandAll() | ||
| } | ||
|
|
||
| function collapseAllSections() { | ||
| sectionProxyModel.collapseAll() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
collapseAllSections() does not clear the selection.
prv.setSectionCollapsed() at line 107 clears the selection when a section collapses. The comment at line 109 states the reason: the selection must not stay on items that are no longer visible. collapseAllSections() hides the items of every section but calls sectionProxyModel.collapseAll() directly, so the selection survives on hidden rows.
🐛 Proposed fix
function collapseAllSections() {
+ //! NOTE: Otherwise the selection would stay on the items that are not visible anymore
+ selectionModel.clear()
sectionProxyModel.collapseAll()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function expandAllSections() { | |
| sectionProxyModel.expandAll() | |
| } | |
| function collapseAllSections() { | |
| sectionProxyModel.collapseAll() | |
| } | |
| function expandAllSections() { | |
| sectionProxyModel.expandAll() | |
| } | |
| function collapseAllSections() { | |
| //! NOTE: Otherwise the selection would stay on the items that are not visible anymore | |
| selectionModel.clear() | |
| sectionProxyModel.collapseAll() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/uicomponents/qml/Muse/UiComponents/ValueList.qml` around lines 88 -
94, Update collapseAllSections() to clear the current selection after collapsing
all sections, matching the behavior of prv.setSectionCollapsed() when items
become hidden. Keep expandAllSections() unchanged and use the existing
selection-clearing mechanism rather than introducing new state handling.
2 participants
UI isn't finished
Screen.Recording.2026-08-05.at.1.40.22.PM.mov