Add natural sorting and case-insensitive sorting for all role-types
Needs ReviewPublic

Authored by gvgeo on Dec 4 2019, 1:54 PM.

Details

Summary

Add natural sorting and case-insensitive sorting, for all role-types
that benefit from.

Bug: 406296

Test Plan

Sort by any role type specified in isRoleValueNatural()
Before: Sorting is always case sensitive
After: Sorting according to 'Sorting mode' in configuration.

Diff Detail

Repository
R318 Dolphin
Branch
sort (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21368
Build 21386: arc lint + arc unit
gvgeo created this revision.Dec 4 2019, 1:54 PM
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptDec 4 2019, 1:54 PM
gvgeo requested review of this revision.Dec 4 2019, 1:54 PM
meven added a subscriber: meven.Dec 14 2019, 9:33 AM
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.

meven requested changes to this revision.Dec 15 2019, 7:11 AM

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.

This revision now requires changes to proceed.Dec 15 2019, 7:11 AM
gvgeo updated this revision to Diff 71769.Dec 18 2019, 10:46 AM
gvgeo retitled this revision from Add case-insensitive sorting for all role-types to Add natural sorting and case-insensitive sorting for all role-types.
gvgeo edited the summary of this revision. (Show Details)

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.

gvgeo marked an inline comment as done.EditedDec 18 2019, 10:56 AM

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.

meven added inline comments.Dec 18 2019, 2:17 PM
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;
}
gvgeo updated this revision to Diff 71812.Dec 18 2019, 6:25 PM

Fixed style.
Move 'roleLessThan' part in lambda code.

gvgeo added a comment.EditedDec 18 2019, 6:32 PM

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;
}

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)

gvgeo marked 2 inline comments as done.Dec 19 2019, 6:37 AM
cfeck added a subscriber: cfeck.Dec 19 2019, 9:45 AM
cfeck added inline comments.
src/kitemviews/kfileitemmodel.h
512

The name of the function is confusing. Maybe 'isStringRole()', 'isTextualRole()', or 'roleNeedsStringSort()' ?

gvgeo updated this revision to Diff 71849.Dec 19 2019, 5:27 PM

String role was not the best option. Function's comment was also wrong.

Renamed 'stringRolesTrue' to isRoleValueNatural

gvgeo marked an inline comment as done.Dec 19 2019, 5:35 PM

Name should be more clear, marking as done for now.

elvisangelaccio requested changes to this revision.Sun, Jan 19, 9:09 PM

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 revision now requires changes to proceed.Sun, Jan 19, 9:09 PM
gvgeo updated this revision to Diff 73906.Mon, Jan 20, 7:20 AM

Made requested change.
Removed 1 empty line from header.

gvgeo marked 2 inline comments as done.Mon, Jan 20, 7:20 AM
gvgeo edited the summary of this revision. (Show Details)Mon, Jan 20, 7:23 AM
gvgeo edited the test plan for this revision. (Show Details)