Bug 375831

Summary:	Sorting by file extension should put empty extensions first
Product:	[Applications] krusader	Reporter:	Wladimir Palant <psbkde>
Component:	general	Assignee:	Krusader Bugs Distribution List <krusader-bugs-null>
Status:	RESOLVED FIXED
Severity:	normal	CC:	krusader-bugs-null
Priority:	NOR
Version First Reported In:	Git
Target Milestone:	---
Platform:	Other
OS:	Linux
Latest Commit:	https://commits.kde.org/krusader/1db4a2b8a3e6de837fea5a90988cc5576de52ea7	Version Fixed/Implemented In:
Sentry Crash Report:
Attachments:	Proposed patch

Description Wladimir Palant 2017-02-01 11:01:26 UTC

I'm using Krusader built from Git master (2.5.1-beta, revision 5056272) on Ubuntu 16.04. When files are sorted by file extension, files with empty file extension are placed last - this is unexpected and doesn't seem to be the intended behavior.

Steps to reproduce:

1. Create files with names "foo" and "bar.txt" in the same directory.
2. Click Ext column header to sort by file extension in ascending order.

Expected results:

The file "foo" is listed before "bar.txt" because "" is smaller than "txt".

Actual results:

The file "foo" is listed after "bar.txt", function compareTexts() in krsort.cpp treats empty strings as larger than non-empty strings. This is independent of sort method chosen, and also inconsistent given that "foo.tx" is correctly listed before "bar.txt" (shorter is generally smaller).

Comment 1 Wladimir Palant 2017-02-01 11:17:40 UTC

Created attachment 103745 [details]
Proposed patch

Here is a fix for this issue. Note that it is treating the case where both strings are equal specially, this is for consistency with the logic at the bottom of compareTextsAlphabetical() and compareTextsCharacterCode(). I don't see a difference if I leave out this case but if it is important then personally I would have written this differently:

> if (aS2.length() == 0) {
>     return false;
> } else if (aS1.length() == 0) {
>     return true;
> }

The checks in compareTextsAlphabetical() can be simplified similarly:

> if (lPositionS2 == aS2.length()) return false;
> else if (lPositionS1 == aS1.length()) return true;

These two comparisons produce the same result as the six comparisons currently there.

Comment 2 Wladimir Palant 2017-02-02 20:37:39 UTC

Now that I figured out how to register for Phabricator I created a proper review: https://phabricator.kde.org/D4417

Comment 3 Alex Bikadorov 2017-02-13 20:42:29 UTC

Git commit 1db4a2b8a3e6de837fea5a90988cc5576de52ea7 by Alexander Bikadorov, on behalf of Wladimir Palant.
Committed on 13/02/2017 at 20:34.
Pushed by abikadorov into branch 'master'.

#375831 - Sort empty file extensions correctly

Summary:
This change makes sure that empty strings are sorted before non-empty strings. Note that it is treating the case where both strings are equal specially, this is for consistency with the logic at the bottom of compareTextsAlphabetical() and compareTextsCharacterCode(). I don't see a difference if I leave out this case but if it is important then personally I would have written this differently:

```
if (aS2.length() == 0) {
    return false;
} else if (aS1.length() == 0) {
    return true;
}
```
The checks in compareTextsAlphabetical() can be simplified similarly:

```
if (lPositionS2 == aS2.length()) return false;
else if (lPositionS1 == aS1.length()) return true;
```

These two comparisons produce the same result as the six comparisons currently there.

Test Plan: See STR in bug.

Reviewers: #krusader, martinkostolny, abika, asensi

Reviewed By: #krusader, martinkostolny, abika, asensi

Subscribers: asensi, martinkostolny, #krusader

Tags: #krusader

Differential Revision: https://phabricator.kde.org/D4417

M  +5    -4    krusader/Panel/krsort.cpp

https://commits.kde.org/krusader/1db4a2b8a3e6de837fea5a90988cc5576de52ea7