Fuzzy text search, part 1: algorithm & searching shortcuts#160
Conversation
- Remove unused private method `reset()` - Remove unused invokable method `refresh()` - Remove unused signal `filtersChanged` - it is never emitted - expose roleIdFromName (formerly roleKey) - this is needed by later commits
This prevented multiple filters from using the same role. Also, filter subclasses won't necessarily operate on a single role.
📝 WalkthroughWalkthroughThe change adds a UTF-32 fuzzy matcher with edit-distance matching, transposition support, iterators, and tests. String splitting now uses templated string-view-based overloads. QML filtering and sorting are reorganized around shared 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/global/stringsearch.cpp`:
- Around line 92-96: Guard FuzzyMatcher::findMatch against an uninitialized or
cleared matcher before accessing m_suffixTable, returning no match when the
suffix table is empty. Ensure empty() and begin()-based iteration remain safe
both before the first match() and after clear(), and add regression coverage for
those states.
In `@framework/uicomponents/qml/Muse/UiComponents/fuzzyfilter.cpp`:
- Around line 88-97: Update FuzzyFilter::setCaseSensitivity to assign
m_caseSensitivity before calling compilePattern(), while preserving the existing
no-op check and dataChanged emission. Add a regression test that sets a
mixed-case pattern, switches to Qt::CaseInsensitive, and verifies matching uses
the new sensitivity.
In `@framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 166-176: Update the source-model replacement flow in
SortFilterProxyModel so invalidateFilters() runs before
QSortFilterProxyModel::setSourceModel(sourceModel), then explicitly invalidate
the proxy after clearing the FuzzyFilter caches. Preserve the existing signal
disconnection and reconnection behavior while ensuring old fuzzy scores cannot
carry across source replacement.
🪄 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: d397ba58-6e07-43ff-8dc7-95afd7f93343
📒 Files selected for processing (30)
framework/global/CMakeLists.txtframework/global/stringsearch.cppframework/global/stringsearch.hframework/global/stringutils.cppframework/global/stringutils.hframework/global/tests/CMakeLists.txtframework/global/tests/stringsearch_tests.cppframework/global/tests/stringutils_tests.cppframework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/ValueList.qmlframework/uicomponents/qml/Muse/UiComponents/filter.cppframework/uicomponents/qml/Muse/UiComponents/filter.hframework/uicomponents/qml/Muse/UiComponents/filtervalue.cppframework/uicomponents/qml/Muse/UiComponents/filtervalue.hframework/uicomponents/qml/Muse/UiComponents/fuzzyfilter.cppframework/uicomponents/qml/Muse/UiComponents/fuzzyfilter.hframework/uicomponents/qml/Muse/UiComponents/fuzzyscoresorter.cppframework/uicomponents/qml/Muse/UiComponents/fuzzyscoresorter.hframework/uicomponents/qml/Muse/UiComponents/sorter.cppframework/uicomponents/qml/Muse/UiComponents/sorter.hframework/uicomponents/qml/Muse/UiComponents/sortervalue.cppframework/uicomponents/qml/Muse/UiComponents/sortervalue.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/sortfilterproxymodel_tests.cpp
The current implementation is "Seller's variant for string search" of the Wagner-Fischer edit distance algorithm[^1], extended to also allow transposition errors (Damerau-Levenshtein distance (optimal string alignment) [^1]: https://en.wikipedia.org/w/index.php?title=Wagner%E2%80%93Fischer_algorithm&oldid=1344122699#Seller's_variant_for_string_search
The function can now be used with any std string type. The output can now be written into any output iterator.
fee0076 to
72c0036
Compare
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 (1)
framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp (1)
53-59: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCoalesce async invalidations to prevent redundant re-evaluations.
Using
QTimer::singleShotfor asynchronous invalidation will queue a separateinvalidateRowsexecution for everydataChangedemission. If multiple filters are modified synchronously, or if a filter rapidly emitsdataChanged,invalidateRows()will execute multiple times in the next event loop iteration, causing redundantO(N)re-filtering overheads.To prevent this, coalesce the updates by using a dedicated single-shot
QTimermember variable.🛠️ Proposed solution
Add a timer member to
SortFilterProxyModel.h:QTimer m_asyncUpdateTimer;In the constructor, configure it and use
start()to coalesce overlapping calls:m_asyncUpdateTimer.setSingleShot(true); m_asyncUpdateTimer.setInterval(0); connect(&m_asyncUpdateTimer, &QTimer::timeout, this, invalidateRows); auto onFilterChanged = [this, invalidateRows](Filter* changedFilter) { if (changedFilter->async()) { m_asyncUpdateTimer.start(); // Resets and coalesces timer } else { invalidateRows(); } };🤖 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 53 - 59, Replace the per-emission QTimer::singleShot scheduling in the onFilterChanged callback with a dedicated single-shot QTimer member on SortFilterProxyModel. Configure the timer with a zero interval, connect its timeout to invalidateRows, and call start() for asynchronous filters so repeated changes coalesce into one invalidation; keep synchronous filters calling invalidateRows() immediately.
🤖 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/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 173-178: Remove the sourceModel dataChanged connection to
SortFilterProxyModel::invalidateFilters so normal QSortFilterProxyModel
row-level optimization remains intact. Update FuzzyFilter to evict only affected
cached QPersistentModelIndex entries when the source model emits dataChanged,
using either a targeted cache-invalidation contract on Filter or FuzzyFilter’s
sourceModel connection; retain full invalidation only for model reset handling.
---
Outside diff comments:
In `@framework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cpp`:
- Around line 53-59: Replace the per-emission QTimer::singleShot scheduling in
the onFilterChanged callback with a dedicated single-shot QTimer member on
SortFilterProxyModel. Configure the timer with a zero interval, connect its
timeout to invalidateRows, and call start() for asynchronous filters so repeated
changes coalesce into one invalidation; keep synchronous filters calling
invalidateRows() immediately.
🪄 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: 4c945ce8-6ec0-41e4-a46d-1bfd99a19c23
📒 Files selected for processing (21)
framework/global/CMakeLists.txtframework/global/stringsearch.cppframework/global/stringsearch.hframework/global/stringutils.cppframework/global/stringutils.hframework/global/tests/CMakeLists.txtframework/global/tests/stringsearch_tests.cppframework/global/tests/stringutils_tests.cppframework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/filter.cppframework/uicomponents/qml/Muse/UiComponents/filter.hframework/uicomponents/qml/Muse/UiComponents/fuzzyfilter.cppframework/uicomponents/qml/Muse/UiComponents/fuzzyfilter.hframework/uicomponents/qml/Muse/UiComponents/fuzzyscoresorter.cppframework/uicomponents/qml/Muse/UiComponents/fuzzyscoresorter.hframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.cppframework/uicomponents/qml/Muse/UiComponents/sortfilterproxymodel.h
Part of: musescore/MuseScore#15983
Rebased after framework split (previous PR: musescore/MuseScore#32634)
Requires: #14
This PR implements a fuzzy filter and score sorter and adds fuzzy search support to the shortcuts list in preferences.
Overview of the Algorithm
An error can be a missing character, an excess character, different characters, and swapped characters.
The implemented fuzzy search algorithm is an extended version of Seller's variant for string search of the Wagner-Fischer algorithm (this is the algorithm that is used in our implementation of
muse::string::levenshteinDistance). It is extended to also allow transposition errors (Damerau-Levenshtein distance).