File sort algo unnatural

1

Shouldn’t the File::compareFilenames() use String::compareNatural() ?

static int compareFilenames (const String& name1, const String& name2) noexcept
{
   #if NAMES_ARE_CASE_SENSITIVE
    return name1.compareNatural (name2);
   #else
    return name1.toUpperCase().compareNatural (name2.toUpperCase());
   #endif
}

I’m looking to be able to do:

FileSearchPath mySearch (szSearchPath);

Array<File> results;

mySearch.findChildFiles (results, File::findFiles, true, szSearchFilePattern);
            
results.sort();

Which works with the above change.

Rail

2

The compareNatural() method is case-insensitive so comparing filenames where case matters wouldn’t work with your change.

Ed

3

Hmmm… I personally don’t care about LINUX so didn’t focus on that… I can certainly use my own comparator

struct NaturalFileComparator
{
    static int compareElements (const File& first, const File& second)
    {
        return first.getFullPathName().toUpperCase().compareNatural (second.getFullPathName().toUpperCase());
   }
};

and

mySearch.findChildFiles (results, File::findFiles, true, szSearchFilePattern);
            
 NaturalFileComparator sortNatural;
            
 results.sort (sortNatural);

But that still means that the default File sort on OS X and Windows doesn’t match the OS

Cheers.

Rail

4

Ok, that makes sense. I’ve added an isCaseSensitive argument to String::compareNatural() and changed File::compareFilenames() to use this method now on develop. Thanks!

Ed

1 Like
5

Thanks Ed

Cheers,

Rail

1 Like
6

This kind of behavior changes, are not always good! (Even if they are well-intentioned)
They can break existing code.

1 Like
7

Indeed it does break existing code in my case: I’m relying on consistent filename sorting for building an archive of files that should be deterministic, i.e. with equal input, the output should be identical too.

For that to happen, I obviously need to add the files to my archive always in the same order, and for that I sort the list of Files to be added. With this change the order has now changed.

@ed95, can you please revert this change so that the default behavior is restored? Then, a third boolean parameter named sortNatural defaulted to false could be added to File::compareFilenames().

Anyone wishing to sort files in natural order should then resort to using that function passing true for that argument, and for that use case it would be convienent that JUCE also provided a corresponding Comparator object, so that passing it to sort() is straightforward

8

Just reverted the change on develop so the default behaviour has been restored but there’s an optional “shouldSortNaturally” argument. Sorry for breaking your code @yfede!

Ed

1 Like
9

argh, okay i will fix my code again :wink:

10

How do you envision the 3rd param for compareFilenames() being used for a sort on an Array of File objects without a way to set the sort algorithm to use in each File object?

I can just switch back to using my own comparator.

Cheers,

Rail

11

my proposal was this:

1 Like
12

Agreed.

I looked at FileInfoComparator and ended up with:

struct NaturalFileComparator
{
    static int compareElements (const File& first, const File& second)
    {
    #if JUCE_WINDOWS
        if (first.isDirectory() != second.isDirectory())
            return first.isDirectory() ? -1 : 1;
    #endif
  
    return first.getFullPathName().compareNatural (second.getFullPathName(), false);
    }
};

Rail

1 Like
13

Good idea having the possibility to have folders first, Windows style!

For maximum flexibility, I’d think that would be better to have that behavior specified when constructing the comparator object, like this:

struct NaturalFileComparator
{ 
    NaturalFileComparator (bool alwaysPutFoldersFirst) : 
    foldersAlwaysFirst (alwaysPutFoldersFirst)
    { }

    int compareElements (const File& first, const File& second) const
    {
        if (foldersAlwaysFirst && first.isDirectory() != second.isDirectory())
            return first.isDirectory() ? -1 : 1;

        return first.getFullPathName().compareNatural (second.getFullPathName(), false);
    }

    const bool foldersAlwaysFirst; 
};
2 Likes
14

Yep, good suggestion, I’ve added this to develop now. There’s a NaturalFileComparator struct in File so you can pass that to the Array::sort() method if you want your filenames to be sorted naturally and you can specify whether to sort folders first in its constructor. I’ve also reverted the File::compareFilenames() method to its old behaviour as you were right about there being no way to specify anything other than the default argument for sorting naturally - my mistake! Thanks.

Ed

15

Whoops, I just noticed that the compareElements() method above can be declared as a const method since it does not change any of the comparator members. I have edited my post above to add it

Not sure if it is necessary for the intended usage of the NaturalFileComparator, but certainly it couldn’t hurt to add that, given the guideline “if it can be const, make it const”.

1 Like
16

Good catch, added to develop.

Ed