Differential D25741

Add natural sorting and case-insensitive sorting for all role-types
ClosedPublic
Actions

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

Details

Reviewers
nicolasfella
meven
elvisangelaccio
ngraham
Group Reviewers
Dolphin
Commits
R318:9e3418bd5582: Add natural sorting and case-insensitive sorting for all role-types
Summary

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

BUG: 406296
FIXED-IN: 19.12.2

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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;
}
meven added a reviewer: ngraham.Dec 18 2019, 9:23 PM
In D25741#579933, @gvgeo wrote:

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.Jan 19 2020, 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.Jan 19 2020, 9:09 PM
gvgeo updated this revision to Diff 73906.Jan 20 2020, 7:20 AM

Made requested change.
Removed 1 empty line from header.

gvgeo marked 2 inline comments as done.Jan 20 2020, 7:20 AM
gvgeo edited the summary of this revision. (Show Details)Jan 20 2020, 7:23 AM
gvgeo edited the test plan for this revision. (Show Details)
meven accepted this revision.Jan 26 2020, 4:36 PM

LGTM :)

This revision is now accepted and ready to land.Jan 26 2020, 4:36 PM
ngraham added a comment.Jan 27 2020, 8:29 PM

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.

gvgeo updated this revision to Diff 74477.Jan 28 2020, 7:30 AM

Fix build error.

ngraham accepted this revision.Jan 28 2020, 3:51 PM
ngraham edited the summary of this revision. (Show Details)
Closed by commit R318:9e3418bd5582: Add natural sorting and case-insensitive sorting for all role-types (authored by gvgeo, committed by ngraham). · Explain WhyJan 28 2020, 3:53 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 74505

View Options

src/kitemviews/kfileitemmodel.h

Loading...
View Options

src/kitemviews/kfileitemmodel.cpp

Loading...