Fuzzy text search, part 0: Allow subclassing filters and sorters of SortFilterProxyModel#33052
Open
juli27 wants to merge 7 commits intomusescore:masterfrom
Open
Fuzzy text search, part 0: Allow subclassing filters and sorters of SortFilterProxyModel#33052juli27 wants to merge 7 commits intomusescore:masterfrom
SortFilterProxyModel#33052juli27 wants to merge 7 commits intomusescore:masterfrom
Conversation
đ WalkthroughWalkthroughAdds new QML-exposed base types Filter and Sorter (headers and implementations) and registers their sources in the Muse.UiComponents QML module. Refactors FilterValue and SorterValue to derive from these bases and implement row-acceptance ( đĨ Pre-merge checks | â 4 | â 1â Failed checks (1 warning)
â Passed checks (4 passed)
âī¸ Tip: You can configure your own custom pre-merge checks in the settings. ⨠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 |
There was a problem hiding this comment.
Actionable comments posted: 4
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/filter.h`:
- Around line 34-47: Add a focused unit test that exercises FilterValue
instances when accessed through the base-typed SortFilterProxyModel::filters
API: create a SortFilterProxyModel, append multiple FilterValue-derived filters
targeting the same role via the model's filters property, and assert the
combined accept/reject behavior by calling acceptsRow (or using the proxy
filtering) to ensure multiple filters on the same role behave correctly; also
verify inherited property behavior by toggling the base Filter properties
enabled and async (Filter::enabled, Filter::async) on parent/base-typed filters
and asserting those settings propagate/affect the FilterValue instances as
expected. Ensure the test constructs filters only through the public
SortFilterProxyModel::filters interface (not concrete types), covers both
enabling/disabling and async on/off permutations, and asserts the final filtered
row set matches the expected outcome.
In `@src/framework/uicomponents/qml/Muse/UiComponents/sorter.h`:
- Around line 34-46: Add a small unit/regression test that exercises the new
abstract QML API by inserting a concrete SorterValue into the
SortFilterProxyModel::sorters list and verifying sorting behavior and properties
propagate through the base type; specifically, instantiate a SorterValue (or
equivalent concrete subclass), append it to SortFilterProxyModel::sorters,
toggle the Sorter::enabled and Sorter::sortOrder properties and assert that the
proxy model's ordering changes accordingly and that inherited enabled/sortOrder
values are observed when sorting (use the Sorter class/interface and
SortFilterProxyModel APIs to locate where to add the test).
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cpp`:
- Around line 36-49: In SorterValue::lessThan, avoid calling Val::fromQVariant
for unsupported QVariant types on every comparison: first call
proxyModel.roleFromRoleName(m_roleName) and compare to a named invalid constant
(use SortFilterProxyModel::INVALID_KEY if made public, or define a local const
int INVALID_ROLE = -1) instead of a bare -1; then check the
QVariant::typeId()/type() from sourceModel->data(...) and if the type is
unsupported return a stable fallback (e.g., compare sourceLeft < sourceRight or
compare row/column) without invoking Val::fromQVariant; additionally, to prevent
log flooding, implement a one-time warning mechanism keyed by (role,
qMetaTypeId) (e.g., a static thread-safe set of logged pairs) and only call the
error log once per pair when encountering an unsupported type.
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 38-58: The filter invalidation sequence is duplicated in the
onFilterChanged lambda; extract the common beginFilterChange() + Qt-versioned
endFilterChange(...)/invalidateFilter() block into a single helper (e.g., a
private method named performFilterInvalidation or scheduleFilterInvalidation)
and call that helper from both branches of onFilterChanged (the async branch
should QTimer::singleShot(0, this, [this]{ helper(); }) and the sync branch
should call helper() directly); also replace other occurrences in
setAlwaysIncludeIndices and setAlwaysExcludeIndices to call the same helper so
the invalidation logic is centralized and cannot drift.
đĒ Autofix (Beta)
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
Run ID: aad8c111-2e17-4a93-a2d4-54b6f60d377d
đ Files selected for processing (13)
src/framework/ui/internal/uiconfiguration.cppsrc/framework/ui/internal/uicontextconfiguration.cppsrc/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/filter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filter.hsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
đ¤ Files with no reviewable changes (1)
- src/framework/ui/internal/uiconfiguration.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sorter.h`:
- Around line 40-41: Replace the empty-message and stray semicolon: remove the
trailing semicolon after the QML_ELEMENT macro and supply a meaningful reason
string to QML_UNCREATABLE (e.g., a short explanatory message describing why
instances are not creatable from QML and how to obtain one) so QML error
messages are actionable; update the line containing QML_ELEMENT and the
QML_UNCREATABLE("") usage in sorter.h (referenced symbols: QML_ELEMENT,
QML_UNCREATABLE) accordingly.
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cpp`:
- Around line 34-48: The less-than logic in SorterValue::lessThan is wrong:
instead of falling back to comparing sourceLeft < sourceRight for all non-Less
results, only use the source-index tiebreaker when the QVariant comparison is
Equivalent or Unordered. Change SorterValue::lessThan so it returns true if
ordering == QPartialOrdering::Less, false if ordering ==
QPartialOrdering::Greater, and only when ordering is Equivalent or Unordered
evaluate and return sourceLeft < sourceRight; this preserves the required
antisymmetry for QSortFilterProxyModel.
đĒ Autofix (Beta)
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
Run ID: 9315a1f5-4177-4526-b02b-5a9dc353eeb2
đ Files selected for processing (7)
src/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
There was a problem hiding this comment.
Actionable comments posted: 2
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 78-82: The sorter-append lambda currently calls onSortersChanged()
unconditionally causing unnecessary invalidate()/sort() for newly appended
disabled sorters; change the lambda to fetch the appended Sorter via
m_sorters.at(index), check its enabled state (e.g., call sorter->enabled() or
equivalent), and only call onSortersChanged() if enabled, while still connecting
the Sorter::dataChanged signal to onSortersChanged so future enablement or data
changes trigger updates; reference the m_sorters.notifier()->appended handler,
m_sorters.at(index), onSortersChanged, and the
Sorter::dataChanged/Sorter::enabled symbols.
- Around line 143-146: roleFromRoleName currently does an O(n) reverse lookup
via roleNames().key() and is called per-row/per-comparison
(FilterValue::acceptsRow and SorterValue::lessThan), causing quadratic-like
costs; fix by avoiding repeated scans: either cache the resolved role id inside
FilterValue and SorterValue (store an int member, compute once on first use and
reset when sourceModelRoleNamesChanged fires) or add a lazy reverse map
(QHash<QByteArray,int>) inside SortFilterProxyModel that roleFromRoleName
consults and that is rebuilt/cleared when sourceModelRoleNamesChanged is
emitted; update the existing sourceModelRoleNamesChanged handler to invalidate
the cached id(s) or reverse map accordingly.
đĒ Autofix (Beta)
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
Run ID: bc537158-d0d5-47d1-aa95-5c484967aa59
đ Files selected for processing (11)
src/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/filter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filter.hsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
â ī¸ Outside diff range comments (1)
src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp (1)
38-90: 𧚠Nitpick | đĩ TrivialLGTM — wiring is correct and previous review feedback is addressed.
invalidateRowscentralizes the Qt-versioned filter invalidation, and the append handlers now gate the initialonFilterChanged/onSortersChangeddispatch onenabled(), which avoids the spurious invalidations flagged previously. DelegatingfilterAcceptsRow/lessThantoFilter/Sorterkeeps the proxy free of role-type logic, exactly as intended by the refactor.One optional note: the same
beginFilterChange()/ versionedendFilterChange(Direction::Rows)/invalidateFilter()block is still open-coded insetAlwaysIncludeIndices(Lines 113-119) andsetAlwaysExcludeIndices(Lines 135-141). PromotinginvalidateRowsto a private member function and reusing it there would eliminate the remaining duplication.đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 38 - 90, The duplicate Qt-versioned filter invalidation logic (beginFilterChange()/endFilterChange(Direction::Rows)/invalidateFilter()) is implemented in the local invalidateRows lambda and also repeated in setAlwaysIncludeIndices and setAlwaysExcludeIndices; refactor by promoting invalidateRows into a private member function (e.g., void invalidateRows()) that encapsulates the versioned logic using beginFilterChange(), endFilterChange(Direction::Rows) and invalidateFilter(), then replace the lambda with a call to this new member and update setAlwaysIncludeIndices and setAlwaysExcludeIndices to call the same member to remove the duplication.
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 38-90: The duplicate Qt-versioned filter invalidation logic
(beginFilterChange()/endFilterChange(Direction::Rows)/invalidateFilter()) is
implemented in the local invalidateRows lambda and also repeated in
setAlwaysIncludeIndices and setAlwaysExcludeIndices; refactor by promoting
invalidateRows into a private member function (e.g., void invalidateRows()) that
encapsulates the versioned logic using beginFilterChange(),
endFilterChange(Direction::Rows) and invalidateFilter(), then replace the lambda
with a call to this new member and update setAlwaysIncludeIndices and
setAlwaysExcludeIndices to call the same member to remove the duplication.
âšī¸ Review info
âī¸ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c467f75-4df1-4267-b537-afd23a532aeb
đ Files selected for processing (7)
src/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
â ī¸ Outside diff range comments (2)
src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp (2)
64-76: 𧚠Nitpick | đĩ TrivialMinor:
invalidate()is called unconditionally on every sorter change, even no-op transitions.
onSortersChangedalways callsinvalidate()before dispatchingsort(...). For the common case of togglingsortOrderon the already-active sorter,sort(0, newOrder)is sufficient on its own — the extrainvalidate()causes an unnecessary full filter re-evaluation. Low impact in practice (filters are usually cheap), but worth considering if you ever hit large models with expensiveFilter::acceptsRowimplementations.đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 64 - 76, onSortersChanged currently always calls invalidate() which forces a full re-filter even for no-op transitions (e.g., toggling the sortOrder on the same Sorter); change the logic so invalidate() is only called when the active Sorter itself changes (not when only its sortOrder toggles). Implement this by tracking the previous active sorter pointer and its last sortOrder in member variables (e.g., m_activeSorter, m_activeSortOrder), compare them inside onSortersChanged (use currentSorter() and sorter->sortOrder()), and only call invalidate() when the sorter pointer differs; always call sort(0, newOrder) when sourceModel() exists to update ordering without invalidating when only the order changed.
54-85:â ī¸ Potential issue | đ MajorHandle
cleared()signal to disconnect stale filter/sorter listeners.
QmlListPropertyNotifierexposes acleared()signal (lines 40–42 of qmllistproperty.h) that is emitted when the list is cleared viaQmlListProperty<T>::clear(). This method is wired into the QQmlListProperty callbacks (line 62) and can be invoked from QML or C++.Currently, only
appendedis connected in lines 54–85. Whencleared()fires, the existingFilter::dataChangedandSorter::dataChangedsignal connections remain active. Removed-but-alive filter and sorter objects continue to triggeronFilterChangedandonSortersChanged, causing the proxy model to invalidate or re-sort based on stale items no longer in the active list.Connect to the
cleared()signal and disconnect all per-item listeners, or re-initialize the filter/sorter connections after a clear to prevent orphaned listeners from corrupting model state.đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 54 - 85, The appended-only handlers leave per-item listeners attached when the list is cleared; connect m_filters.notifier()->cleared and m_sorters.notifier()->cleared and in those lambdas iterate the removed items (or iterate current m_filters/m_sorters before they are cleared) and call QObject::disconnect(filter, &Filter::dataChanged, this, nullptr) and QObject::disconnect(sorter, &Sorter::dataChanged, this, nullptr) respectively (or otherwise remove stored QMetaObject::Connection handles) to ensure Filter::dataChanged and Sorter::dataChanged handlers (the connections created in the appended lambdas that call onFilterChanged/onSortersChanged) are disconnected when the lists are cleared.
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cpp`:
- Around line 34-51: The call site in SorterValue::lessThan currently compares
role == -1 which re-encodes the file-static sentinel from
SortFilterProxyModel::roleFromRoleName; replace the magic -1 with the public
symbol (export INVALID_KEY from sortfilterproxymodel.h and use that constant
here) or update roleFromRoleName to return std::optional<int> and check
has_value() instead; also add a short assertion or comment before using
proxyModel.sourceModel() in SorterValue::lessThan to ensure the returned
QAbstractItemModel* is non-null (use the same function names:
SorterValue::lessThan, SortFilterProxyModel::roleFromRoleName, and the
INVALID_KEY symbol) to make the contract explicit and avoid silent breakage.
---
Outside diff comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 64-76: onSortersChanged currently always calls invalidate() which
forces a full re-filter even for no-op transitions (e.g., toggling the sortOrder
on the same Sorter); change the logic so invalidate() is only called when the
active Sorter itself changes (not when only its sortOrder toggles). Implement
this by tracking the previous active sorter pointer and its last sortOrder in
member variables (e.g., m_activeSorter, m_activeSortOrder), compare them inside
onSortersChanged (use currentSorter() and sorter->sortOrder()), and only call
invalidate() when the sorter pointer differs; always call sort(0, newOrder) when
sourceModel() exists to update ordering without invalidating when only the order
changed.
- Around line 54-85: The appended-only handlers leave per-item listeners
attached when the list is cleared; connect m_filters.notifier()->cleared and
m_sorters.notifier()->cleared and in those lambdas iterate the removed items (or
iterate current m_filters/m_sorters before they are cleared) and call
QObject::disconnect(filter, &Filter::dataChanged, this, nullptr) and
QObject::disconnect(sorter, &Sorter::dataChanged, this, nullptr) respectively
(or otherwise remove stored QMetaObject::Connection handles) to ensure
Filter::dataChanged and Sorter::dataChanged handlers (the connections created in
the appended lambdas that call onFilterChanged/onSortersChanged) are
disconnected when the lists are cleared.
đĒ Autofix (Beta)
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
Run ID: 39a0806a-15a5-417f-bae8-9fc075a019f8
đ Files selected for processing (8)
src/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/ValueList.qmlsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
- Remove unused private method `reset()` - Remove unused invokable method `refresh()` - Remove unused signal `filtersChanged` - it is never emitted - Simplify collection iteration
This looks unnecessarily complicated to me. It also prevents multiple filters to operate on the same role.
Contributor
Author
|
@coderabbitai review |
â Actions performedReviews resumed. |
â Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
â ī¸ Outside diff range comments (2)
src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp (2)
169-179:â ī¸ Potential issue | đ MajorRefresh the role cache when a chained proxy’s roles change.
m_rolesis rebuilt only duringsetSourceModel(). When the source is anotherSortFilterProxyModel, lines 177-179 forwardsourceModelRoleNamesChangedwithout rebuilding this cache, soroleFromRoleName()can return stale role IDs after the upstream proxy changes source roles.Suggested fix
- m_roles.clear(); - for (const auto& [role, roleName] : roleNames().asKeyValueRange()) { - const auto[it, didInsert] = m_roles.try_emplace(roleName, role); - DO_ASSERT(didInsert); - } + const auto updateRoleCache = [this]() { + m_roles.clear(); + for (const auto& [role, roleName] : roleNames().asKeyValueRange()) { + const auto[it, didInsert] = m_roles.try_emplace(roleName, role); + DO_ASSERT(didInsert); + } + }; + + updateRoleCache(); emit sourceModelRoleNamesChanged(); if (auto sourceSortFilterModel = qobject_cast<SortFilterProxyModel*>(sourceModel)) { m_subSourceModelConnection = connect(sourceSortFilterModel, &SortFilterProxyModel::sourceModelRoleNamesChanged, - this, &SortFilterProxyModel::sourceModelRoleNamesChanged); + this, [this, updateRoleCache]() { + updateRoleCache(); + emit sourceModelRoleNamesChanged(); + }); }đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 169 - 179, When chaining another SortFilterProxyModel, the local role cache m_roles is not refreshed when the upstream proxy emits sourceModelRoleNamesChanged, leading roleFromRoleName() to return stale IDs; update the handler connected in setSourceModel (the lambda or the slot bound to SortFilterProxyModel::sourceModelRoleNamesChanged) to rebuild m_roles the same way setSourceModel does: clear m_roles, repopulate it from roleNames().asKeyValueRange() using try_emplace (preserving DO_ASSERT on didInsert), then emit sourceModelRoleNamesChanged so downstream users see the updated roles.
55-85:â ī¸ Potential issue | đĄ MinorHandle cleared filter/sorter lists.
The append handlers update the proxy when items are added, but
QmlListProperty::clear()also fires a clear notification. If QML clearsfiltersorsorters, the proxy can keep stale filtering/sorting until some later change triggers invalidation.Suggested fix
connect(m_filters.notifier(), &QmlListPropertyNotifier::appended, this, [this, onFilterChanged](int index) { Filter* filter = m_filters.at(index); if (filter->enabled()) { onFilterChanged(filter); } connect(filter, &Filter::dataChanged, this, [onFilterChanged, filter] { onFilterChanged(filter); }); }); + connect(m_filters.notifier(), &QmlListPropertyNotifier::cleared, this, invalidateRows); auto onSortersChanged = [this] { Sorter* sorter = currentSorter(); invalidate(); @@ connect(sorter, &Sorter::dataChanged, this, onSortersChanged); }); + connect(m_sorters.notifier(), &QmlListPropertyNotifier::cleared, this, onSortersChanged);đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 55 - 85, The append-only handlers miss the QmlListPropertyNotifier::cleared case, leaving stale state when QML calls clear(); add connections to m_filters.notifier() and m_sorters.notifier() for the cleared signal and in those cleared handlers call the same update logic as on append: for filters simply invalidate() (so Filter-related filtering is recomputed) and for sorters call the onSortersChanged logic (or replicate it: invalidate(), then if no currentSorter() call sort(-1, Qt::AscendingOrder) else if sourceModel() call sort(0, sorter->sortOrder())). Use the existing symbols (m_filters.notifier(), m_sorters.notifier(), onFilterChanged, onSortersChanged, invalidate(), currentSorter(), sort(), sourceModel()) so the cleared handlers match the append behavior.
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/ui/internal/uicontextconfiguration.cpp`:
- Around line 25-30: The added/ reordered includes (QApplication, QScreen) and
the new Windows-guarded include QOperatingSystemVersion (with Q_OS_WIN) in
uicontextconfiguration.cpp are unrelated to the SortFilterProxyModel refactor;
either remove/revert these include edits from this branch and restore the
original includes, or extract them into a separate cleanup PR; if the
QOperatingSystemVersion addition is required for the fuzzy-search change, add a
short justification to this PR description explaining why that transitive
include is necessary.
---
Outside diff comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 169-179: When chaining another SortFilterProxyModel, the local
role cache m_roles is not refreshed when the upstream proxy emits
sourceModelRoleNamesChanged, leading roleFromRoleName() to return stale IDs;
update the handler connected in setSourceModel (the lambda or the slot bound to
SortFilterProxyModel::sourceModelRoleNamesChanged) to rebuild m_roles the same
way setSourceModel does: clear m_roles, repopulate it from
roleNames().asKeyValueRange() using try_emplace (preserving DO_ASSERT on
didInsert), then emit sourceModelRoleNamesChanged so downstream users see the
updated roles.
- Around line 55-85: The append-only handlers miss the
QmlListPropertyNotifier::cleared case, leaving stale state when QML calls
clear(); add connections to m_filters.notifier() and m_sorters.notifier() for
the cleared signal and in those cleared handlers call the same update logic as
on append: for filters simply invalidate() (so Filter-related filtering is
recomputed) and for sorters call the onSortersChanged logic (or replicate it:
invalidate(), then if no currentSorter() call sort(-1, Qt::AscendingOrder) else
if sourceModel() call sort(0, sorter->sortOrder())). Use the existing symbols
(m_filters.notifier(), m_sorters.notifier(), onFilterChanged, onSortersChanged,
invalidate(), currentSorter(), sort(), sourceModel()) so the cleared handlers
match the append behavior.
đĒ Autofix (Beta)
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
Run ID: 9ddcbba5-dfea-4ec8-8176-f6b402bb890c
đ Files selected for processing (16)
src/framework/ui/internal/uiconfiguration.cppsrc/framework/ui/internal/uicontextconfiguration.cppsrc/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/ValueList.qmlsrc/framework/uicomponents/qml/Muse/UiComponents/filter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filter.hsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/filtervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sorter.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortervalue.hsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.hsrc/framework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp
đ¤ Files with no reviewable changes (1)
- src/framework/ui/internal/uiconfiguration.cpp
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
â ī¸ Outside diff range comments (1)
src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp (1)
65-76:â ī¸ Potential issue | đ MajorCall sort(0, ...) even before the source model is attached to preserve sorter state.
When an enabled sorter is appended before
setSourceModel()is called, line 74's guard skips thesort(0, sorter->sortOrder())call. Later, when the source model is assigned,setSourceModel()only updates role mappings and invalidates—it does not restore sorting. SinceQSortFilterProxyModel::sort()records the sort column/order internally even with a null sourceModel, removing the guard allows that state to be captured and reapplied wheninvalidate()is triggered bysourceModelRoleNamesChanged.Proposed fix
if (!sorter) { sort(-1, Qt::AscendingOrder); return; } - if (sourceModel()) { - sort(0, sorter->sortOrder()); - } + sort(0, sorter->sortOrder()); };Add a test case that enables a
SorterValuebeforesetSourceModel()and verifies the model is sorted correctly once the source is assigned.Also applies to: 161–171
đ¤ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp` around lines 65 - 76, The onSortersChanged lambda currently skips calling sort(0, sorter->sortOrder()) when sourceModel() is null, so enabled sorter state set before setSourceModel() is lost; remove the guard in onSortersChanged (the lambda using currentSorter(), invalidate(), and sort()) so that sort(0, sorter->sortOrder()) is called unconditionally when a sorter exists, ensuring QSortFilterProxyModel records the sort column/order even with a null source model; after this change add a unit test that enables a SorterValue before calling setSourceModel() and then assigns the source model to verify sorting is applied once sourceModelRoleNamesChanged triggers invalidate().
đ¤ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 65-76: The onSortersChanged lambda currently skips calling sort(0,
sorter->sortOrder()) when sourceModel() is null, so enabled sorter state set
before setSourceModel() is lost; remove the guard in onSortersChanged (the
lambda using currentSorter(), invalidate(), and sort()) so that sort(0,
sorter->sortOrder()) is called unconditionally when a sorter exists, ensuring
QSortFilterProxyModel records the sort column/order even with a null source
model; after this change add a unit test that enables a SorterValue before
calling setSourceModel() and then assigns the source model to verify sorting is
applied once sourceModelRoleNamesChanged triggers invalidate().
âšī¸ Review info
âī¸ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: de1e9c3e-247a-4b63-ad1d-51c8f508ec1b
đ Files selected for processing (4)
src/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.hsrc/framework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/tests/sortfilterproxymodel_tests.cpp
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
3 participants
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.
I realized that using another model (
FuzzySortFilterProxyModel)in part 1 (#32634) makes it unnecessarily hard to use in a lot of places where aSortFilterProxyModelis used right now. So instead of that approach, the fuzzy search will be implemented via aFuzzyFilterandFuzzyScoreSorter. This PR provides the foundation for such an implementation, by allowing subclassing the filters and sorters.