TableSortFilterProxyModel#33036
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 🚥 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 3
🤖 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/tablesortfilterproxymodel.cpp`:
- Around line 30-37: The TableSortFilterProxyModel constructor redundantly calls
m_selectionModel->setModel(this) after constructing m_selectionModel with new
muse::uicomponents::ItemMultiSelectionModel(this) because
ItemMultiSelectionModel already sets the model in its constructor; remove the
explicit setModel(this) call (the reference is in
TableSortFilterProxyModel::TableSortFilterProxyModel and m_selectionModel) or
alternatively change the construction to new ItemMultiSelectionModel(nullptr) if
you prefer to keep an explicit setModel call—either approach removes the
redundant model binding.
- Around line 145-158: The code currently assumes that when leftData/rightData
canConvert<muse::uicomponents::TableViewCell*>() is true, both pointers are
non-null; if either l or r is null leftVal/rightVal remain default and
compareCells returns equal, corrupting ordering. Change the branch so you handle
each side independently: if leftData canConvert<TableViewCell*> then set leftVal
= l ? l->value() : muse::Val::fromQVariant(leftData) (or a deterministic null
sentinel if you prefer nulls-last), and do the same for rightVal using r;
alternatively explicitly define a deterministic ordering for null TableViewCell*
(e.g., treat null as greater) and use that when l or r is null, ensuring
compareCells never sees both default-constructed vals.
In
`@src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.h`:
- Line 45: The Q_PROPERTY declaration for selectionModel places the asterisk
after the type which violates Uncrustify rules; update the Q_PROPERTY for
selectionModel so the pointer asterisk binds to the identifier (move the '*'
from after the type to directly before the identifier) and then run the
project's uncrustify formatter to apply codestyle; also scan other Q_PROPERTY
pointer declarations (selectionModel) and adjust them to the same
pointer-binding style.
🪄 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: c590cbee-29a8-47e2-a10a-49a56b755b99
📒 Files selected for processing (6)
src/appshell/qml/MuseScore/AppShell/DevTools/Gallery/GeneralComponentsGallery.qmlsrc/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.hsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.h
saintmatthieu
force-pushed
the
allow-empty-table-views
branch
2 times, most recently
from
467c14b to
193af2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tablesortfilterproxymodel.cpp`:
- Around line 101-105: The call in TableSortFilterProxyModel::invalidateFilters
uses endFilterChange(QSortFilterProxyModel::Direction::Rows) which requires Qt
6.10; add a Qt version guard so builds with Qt 6.8/6.9 fall back to the older
API (call invalidateFilter()) — i.e., in invalidateFilters(), check QT_VERSION
or QT_VERSION_CHECK for 6.10.0 and call
endFilterChange(QSortFilterProxyModel::Direction::Rows) when available,
otherwise call invalidateFilter(); follow the same pattern used in
sortfilterproxymodel.cpp and palettemodel.h and keep the beginFilterChange()
call as-is.
🪄 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: def72643-8eaf-4cff-852d-2dc53d7c8839
📒 Files selected for processing (3)
src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tablesortfilterproxymodel.cpp`:
- Around line 73-90: Currently toggleColumnSort erases an existing SortKey and
re-adds it (promoting priority) and only supports two states; to preserve
priority when flipping and optionally implement a 3-state cycle, change
TableSortFilterProxyModel::toggleColumnSort to: when the column is not found
push_back new {column,true} and trim m_sortPipeline as before; when found, if
you want a 2-state behavior simply flip it in-place by setting it->ascending =
!it->ascending (do not erase/push_back) and call reapplySort(); if you want a
3-state cycle (asc → desc → unsorted) then implement: if it->ascending then set
it->ascending = false; else (it is descending) erase it from m_sortPipeline;
finally call reapplySort(); reference m_sortPipeline, m_maxSortKeys, and
toggleColumnSort to locate changes.
🪄 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: 5cf749c0-ab45-49d5-99de-f32fd60dcc58
📒 Files selected for processing (6)
src/appshell/qml/MuseScore/AppShell/DevTools/Gallery/GeneralComponentsGallery.qmlsrc/framework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtsrc/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.hsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.cppsrc/framework/uicomponents/qml/Muse/UiComponents/testtableviewmodel.h
| return; | ||
| } | ||
| invalidate(); | ||
| sort(0, Qt::AscendingOrder); |
There was a problem hiding this comment.
I believe that sort(0, Qt::AscendingOrder) doesn't do anything? Otherwise sorting would be incorrect?
and the real sorting is applied in invalidate: https://doc.qt.io/qt-6/qsortfilterproxymodel.html#invalidate
There was a problem hiding this comment.
We're overriding the way Qt does it: we call sort just to trigger the sorting, but what columns we compare actually gets decided in ::lessThan.
I will at least comment on this.
| return; | ||
| } | ||
| if (m_sortPipeline.empty()) { | ||
| sort(-1); |
There was a problem hiding this comment.
I believe it uses the order as the source model returns it.
| * MuseScore | ||
| * Music Composition & Notation | ||
| * | ||
| * Copyright (C) 2021 MuseScore Limited and others |
There was a problem hiding this comment.
Oops, I thought I had. Is committed, will push in due time.
saintmatthieu
force-pushed
the
allow-empty-table-views
branch
from
193af2f to
a86bca3
Compare
saintmatthieu
force-pushed
the
allow-empty-table-views
branch
2 times, most recently
from
d4cbca7 to
39fdfd3
Compare
saintmatthieu
force-pushed
the
allow-empty-table-views
branch
from
39fdfd3 to
21e4db3
Compare
| if (column != m_sortIconColumn) { | ||
| return ColumnSortOrder::Type::Unsorted; | ||
| } | ||
|
|
||
| const auto it = std::find_if(m_sortPipeline.begin(), m_sortPipeline.end(), | ||
| [column](const SortKey& k) { return k.column == column; }); | ||
| if (it != m_sortPipeline.end() - 1) { | ||
| return ColumnSortOrder::Type::Unsorted; | ||
| } | ||
| return it->ascending ? ColumnSortOrder::Type::Ascending : ColumnSortOrder::Type::Descending; |
There was a problem hiding this comment.
| if (column != m_sortIconColumn) { | |
| return ColumnSortOrder::Type::Unsorted; | |
| } | |
| const auto it = std::find_if(m_sortPipeline.begin(), m_sortPipeline.end(), | |
| [column](const SortKey& k) { return k.column == column; }); | |
| if (it != m_sortPipeline.end() - 1) { | |
| return ColumnSortOrder::Type::Unsorted; | |
| } | |
| return it->ascending ? ColumnSortOrder::Type::Ascending : ColumnSortOrder::Type::Descending; | |
| if (column != m_sortIconColumn || m_sortPipeline.empty()) { | |
| return ColumnSortOrder::Type::Unsorted; | |
| } | |
| const auto& last = m_sortPipeline.back(); | |
| return last.ascending ? ColumnSortOrder::Type::Ascending | |
| : ColumnSortOrder::Type::Descending; |
There was a problem hiding this comment.
This assumes that if column is the last toggled column, the back of the pipeline is necessarily that matching column. But in fact, when toggling from descending to unordered, we remove the entry from the pipeline.
- click column A -> A has ascending order, m_sortIconColumn = 0
- click column B thrice -> column B has unordered order, m_sortIconColumn = 1, but B isn't in the pipeline anymore
- call this method with your suggestion and
column == 1as input:column != m_sortIconColumn || m_sortPipeline.empty()is false, and the back of the pipeline is column A, so returnsascending.
There was a problem hiding this comment.
yes this suggestion works only if we assume that m_sortingColumn == -1 if there is no sorting column: https://github.com/musescore/MuseScore/pull/33036/changes#r3148500702
There was a problem hiding this comment.
How should that other method then look like?
| m_sortPipeline.push_back({ column, ascending }); | ||
| } | ||
|
|
||
| m_sortIconColumn = column; |
There was a problem hiding this comment.
it the column was erased on line 87, we should set m_sortIconColumn to -1
There was a problem hiding this comment.
No, that's the trick: an unordered column isn't represented in the sort pipeline - which makes sense. Nevertheless, m_sortIconColumn must still represent the index of the column that should display an icon - even if this icon is the unordered icon and it turns out to be no icon at all.
With the current implementation, if we decide to have an icon for unordered state, I believe it'd still work.
There was a problem hiding this comment.
I don't understand it, what is the meaning of unordered state of the sort column?
There was a problem hiding this comment.
I mean the column is erased from the sort pipeline, so it is not used in any way for the sorting or any table presentation, why would we want to keep track of it?
There was a problem hiding this comment.
We need to keep track of this because we don't want a column other than the last one clicked to display a sort icon - like you pointed out on Friday. Whether it has an entry in the sort-pipeline implementation or not is another matter.
There was a problem hiding this comment.
With the current implementation, if we decide to have an icon for unordered state, I believe it'd still work.
I was wrong here:
if (column != m_sortIconColumn) {
return ColumnSortOrder::Type::Unsorted;
}assumes that ColumnSortOrder::Type::Unsorted will result in the UI not displaying any icon.
That doesn't mean that the current implementation is wrong, just that it isn't perfectly modeled.
saintmatthieu
force-pushed
the
allow-empty-table-views
branch
from
c0a05b8 to
f7773fd
Compare
4 participants
Needed for Audacity PR audacity/audacity#10649
displayTruncatedTextOnHoverproperty ofStyledTableView: in lieu of tooltipsTableSortFilterProxyModelin the framework and integrates it for demo in the component gallery's existing table demo. Video here: 10592 plugin manager UI audacity/audacity#10649 (comment)