Add natural sorting and case-insensitive sorting, for all role-types
that benefit from.
BUG: 406296
FIXED-IN: 19.12.2
nicolasfella | |
meven | |
elvisangelaccio | |
ngraham |
Dolphin |
Add natural sorting and case-insensitive sorting, for all role-types
that benefit from.
BUG: 406296
FIXED-IN: 19.12.2
Sort by any role type specified in isRoleValueNatural()
Before: Sorting is always case sensitive
After: Sorting according to 'Sorting mode' in configuration.
Lint Skipped |
Unit Tests Skipped |
And tests fail with or without this patch. see bug 413660.
Tests are currently stable in dolphin master.
Perhaps your local isn't english (the tests assume so).
You can run ctest using english local with :
ctest='LANG="en" ctest --output-on-failure
I don't reproduce the bug you mentioned.
I haven't tested your patch yet.
I have tested the patch on tags, it works.
It does not break existing tests.
src/kitemviews/kfileitemmodel.cpp | ||
---|---|---|
1190 | We need to change also ine 1207 : parallelMergeSort(newItems.begin(), newItems.end(), nameLessThan, QThread::idealThreadCount()); The nameLess function compare the name role of items, it needs to be adapted or replaced so that this the string comparison function called by parallelMergeSort takes into account the sorted Roles. |
Add funciton roleLessThan. Kept 'nameLessThan' instead of changing it to 'stringLessThan'
Add funciton stringRolesTrue. To compare multiple values to m_sortRole
Added stringRolesTrue in KFileItemModel::sort for parallelMergeSort. To speed up now that have natural sorting.
Natural sort problem was indeed locale problem. The bug was irrelevant, removed from summary.
Just for info, this was the way I managed to pass variable to ctest:
LANG=en ctest --output-on-failure
First had to generate en locale.
src/kitemviews/kfileitemmodel.cpp | ||
---|---|---|
1197 | No line return between } and else if | |
1681 | I wonder if it woulde be better to move this code directly in the lambda code. | |
1855 | Since this block is very close to the default case. Maybe we could just have (and remove this added multicase) : default: { const QByteArray role = roleForType(m_sortRole); const QString roleValueA = a->values.value(role).toString(); const QString roleValueB = b->values.value(role).toString(); if (!roleValueA.isEmpty() && roleValueB.isEmpty()) { result = -1; } else if (roleValueA.isEmpty() && !roleValueB.isEmpty()) { result = +1; } else { result = stringCompare(roleValueA, roleValueB); // <- only line changed } break; } |
As for the multicase, the are still these cases that don't benefit (if I'm not wrong) from the natural sorting and case insensitive:
AccessTimeRole PermissionsRole ImageDateTimeRole OrientationRole WordCountRole DurationRole BitrateRole AspectRatioRole FrameRateRole
The rest should not matter.
NoRole IsDirRole IsLinkRole IsHiddenRole IsExpandedRole IsExpandableRole ExpandedParentsCountRole RolesCount
It is possible to use stringRolesTrue, but already have the switch.
default: { const QByteArray role = roleForType(m_sortRole); const QString roleValueA = a->values.value(role).toString(); const QString roleValueB = b->values.value(role).toString(); if (!roleValueA.isEmpty() && roleValueB.isEmpty()) { result = -1; } else if (roleValueA.isEmpty() && !roleValueB.isEmpty()) { result = +1; } else if (stringRolesTrue(m_sortRole)) { <------------ result = stringCompare(roleValueA, roleValueB); } else { result = QString::compare(roleValueA, roleValueB); } break; }
Fair enough
I would be in favor of merging this but I would like to give @elvisangelaccio a chance to review this.
(You can mark the comments you treated as done so that next reviewer will see your progress)
src/kitemviews/kfileitemmodel.h | ||
---|---|---|
512 | The name of the function is confusing. Maybe 'isStringRole()', 'isTextualRole()', or 'roleNeedsStringSort()' ? |
String role was not the best option. Function's comment was also wrong.
Renamed 'stringRolesTrue' to isRoleValueNatural
Sorry for the delay, patch looks good but needs some polish.
src/kitemviews/kfileitemmodel.cpp | ||
---|---|---|
1855 | +1 Also, we should use the new isRoleValueNatural() function also here, instead of hardcoding again the list of roles. |
This doesn't compile for me:
/home/nate/kde/src/dolphin/src/kitemviews/kfileitemmodel.cpp: In member function ‘int KFileItemModel::sortRoleCompare(const KFileItemModel::ItemData*, const KFileItemModel::ItemData*, const QCollator&) const’: /home/nate/kde/src/dolphin/src/kitemviews/kfileitemmodel.cpp:1847:58: error: no matching function for call to ‘KFileItemModel::stringCompare(const QString&, const QString&) const’ 1847 | result = stringCompare(roleValueA, roleValueB); | ^
I'm on Qt 5.14.