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).
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.
Now that I figured out how to register for Phabricator I created a proper review: https://phabricator.kde.org/D4417
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