Exiv2::XmpData::sortByKey does not sort numerically correct

[phako]

Describe the bug
sortByKey() does not sort numerically correct, i.e. 10, 11, 1, 2, instead of the expected 1, 2, 10, 11:

Xmp.xmpMM.History
Xmp.xmpMM.History[10]
Xmp.xmpMM.History[10]/stEvt:action
Xmp.xmpMM.History[10]/stEvt:parameters
Xmp.xmpMM.History[11]
Xmp.xmpMM.History[11]/stEvt:action
Xmp.xmpMM.History[11]/stEvt:changed
Xmp.xmpMM.History[11]/stEvt:instanceID
Xmp.xmpMM.History[11]/stEvt:softwareAgent
Xmp.xmpMM.History[11]/stEvt:when
Xmp.xmpMM.History[1]
Xmp.xmpMM.History[1]/stEvt:action
Xmp.xmpMM.History[1]/stEvt:parameters
Xmp.xmpMM.History[2]
Xmp.xmpMM.History[2]/stEvt:action
Xmp.xmpMM.History[2]/stEvt:changed
Xmp.xmpMM.History[2]/stEvt:instanceID
Xmp.xmpMM.History[2]/stEvt:softwareAgent
Xmp.xmpMM.History[2]/stEvt:when
Xmp.xmpMM.History[3]
Xmp.xmpMM.History[3]/stEvt:action
Xmp.xmpMM.History[3]/stEvt:parameters
Xmp.xmpMM.History[4]
Xmp.xmpMM.History[4]/stEvt:action
Xmp.xmpMM.History[4]/stEvt:changed
Xmp.xmpMM.History[4]/stEvt:instanceID
Xmp.xmpMM.History[4]/stEvt:softwareAgent
Xmp.xmpMM.History[4]/stEvt:when
Xmp.xmpMM.History[5]
Xmp.xmpMM.History[5]/stEvt:action
Xmp.xmpMM.History[5]/stEvt:changed
Xmp.xmpMM.History[5]/stEvt:instanceID
Xmp.xmpMM.History[5]/stEvt:softwareAgent
Xmp.xmpMM.History[5]/stEvt:when
Xmp.xmpMM.History[6]
Xmp.xmpMM.History[6]/stEvt:action
Xmp.xmpMM.History[6]/stEvt:parameters
Xmp.xmpMM.History[7]
Xmp.xmpMM.History[7]/stEvt:action
Xmp.xmpMM.History[7]/stEvt:parameters
Xmp.xmpMM.History[8]
Xmp.xmpMM.History[8]/stEvt:action
Xmp.xmpMM.History[8]/stEvt:changed
Xmp.xmpMM.History[8]/stEvt:instanceID
Xmp.xmpMM.History[8]/stEvt:softwareAgent
Xmp.xmpMM.History[8]/stEvt:when
Xmp.xmpMM.History[9]
Xmp.xmpMM.History[9]/stEvt:action
Xmp.xmpMM.History[9]/stEvt:parameters

To Reproduce

    auto xmp_data = image->xmpData();
    xmp_data.sortByKey();

    for(auto k: xmp_data) {
        std::cout << k.key() << std::endl;
    }

Steps to reproduce the behaviour:

1. Provide the image with which you observed the issue (commonly named PoC)
2. Provide exact command to reproduce the issue
3. Mention the branch/commit in which you observed the issue (i.e: master or 0.27-maintenance)

Expected behavior
10 and 11 are sorted after 9:

Xmp.xmpMM.History
Xmp.xmpMM.History[1]
Xmp.xmpMM.History[1]/stEvt:action
Xmp.xmpMM.History[1]/stEvt:parameters
Xmp.xmpMM.History[2]
Xmp.xmpMM.History[2]/stEvt:action
Xmp.xmpMM.History[2]/stEvt:changed
Xmp.xmpMM.History[2]/stEvt:instanceID
Xmp.xmpMM.History[2]/stEvt:softwareAgent
Xmp.xmpMM.History[2]/stEvt:when
Xmp.xmpMM.History[3]
Xmp.xmpMM.History[3]/stEvt:action
Xmp.xmpMM.History[3]/stEvt:parameters
Xmp.xmpMM.History[4]
Xmp.xmpMM.History[4]/stEvt:action
Xmp.xmpMM.History[4]/stEvt:changed
Xmp.xmpMM.History[4]/stEvt:instanceID
Xmp.xmpMM.History[4]/stEvt:softwareAgent
Xmp.xmpMM.History[4]/stEvt:when
Xmp.xmpMM.History[5]
Xmp.xmpMM.History[5]/stEvt:action
Xmp.xmpMM.History[5]/stEvt:changed
Xmp.xmpMM.History[5]/stEvt:instanceID
Xmp.xmpMM.History[5]/stEvt:softwareAgent
Xmp.xmpMM.History[5]/stEvt:when
Xmp.xmpMM.History[6]
Xmp.xmpMM.History[6]/stEvt:action
Xmp.xmpMM.History[6]/stEvt:parameters
Xmp.xmpMM.History[7]
Xmp.xmpMM.History[7]/stEvt:action
Xmp.xmpMM.History[7]/stEvt:parameters
Xmp.xmpMM.History[8]
Xmp.xmpMM.History[8]/stEvt:action
Xmp.xmpMM.History[8]/stEvt:changed
Xmp.xmpMM.History[8]/stEvt:instanceID
Xmp.xmpMM.History[8]/stEvt:softwareAgent
Xmp.xmpMM.History[8]/stEvt:when
Xmp.xmpMM.History[9]
Xmp.xmpMM.History[9]/stEvt:action
Xmp.xmpMM.History[9]/stEvt:parameters
Xmp.xmpMM.History[10]
Xmp.xmpMM.History[10]/stEvt:action
Xmp.xmpMM.History[10]/stEvt:parameters
Xmp.xmpMM.History[11]
Xmp.xmpMM.History[11]/stEvt:action
Xmp.xmpMM.History[11]/stEvt:changed
Xmp.xmpMM.History[11]/stEvt:instanceID
Xmp.xmpMM.History[11]/stEvt:softwareAgent
Xmp.xmpMM.History[11]/stEvt:when

I've written a port of Shotwell's collate function (https://gitlab.gnome.org/GNOME/shotwell/-/blob/master/src/NaturalCollate.vala) to C++. The port completely ignores the existence of UTF8 but I think this is not an issue for keys of meta-data.

I am using this this in gexiv2 here:

https://gitlab.gnome.org/GNOME/gexiv2/-/blob/wip/58/gexiv2/gexiv2-metadata.cpp#L244

Desktop (please complete the following information):

• OS: Fedora Linux 33
• Compiler & Version: GCC 10.2.1

[phako]

Sample image and sample code

#include <exiv2/exiv2.hpp>
#include <iostream>

int main(int argc, char *argv[])
{
    auto image = Exiv2::ImageFactory::open(argv[1]);
    image->readMetadata();

    auto xmp_data = image->xmpData();
    xmp_data.sortByKey();

    for(auto k: xmp_data) {
        std::cout << k.key() << std::endl;
    }
    return 0;
}

[clanmills]

Nice photo. South Island?

Clearly sort is "alphabet" and not "numeric".

1255 rmills@rmillsmm-local:~/temp $ echo 1 > foo; echo 11 >> foo ; echo 2 >> foo ; sort foo ; echo ------------------- ; sort --numeric foo
1
11
2
-------------------
1
2
11
1256 rmills@rmillsmm-local:~/temp $ 

I believe you can pass a comparator to std::sort as an optional additional parameter to provide the result you seek.

[clanmills]

I've looked in the code and it works as I suggested earlier. Here's code from src/iptc.cpp

    bool cmpMetadataByKey(const Metadatum& lhs, const Metadatum& rhs)
    {
        return lhs.key() < rhs.key();
    }
    ...
    void IptcData::sortByKey()
    {
        std::sort(iptcMetadata_.begin(), iptcMetadata_.end(), cmpMetadataByKey);
    }

So, you can sort the keys in your code using your own comparator. I believe cmpMetadataByKey() has been in Exiv2 for 15+ years, so changing the behaviour could upset another user. As you can use std::sort in your code, I would vote to leave the code as is.

I haven't assigned this to the 0.27-maintenance branch and leave it to a future maintainer to consider the issue.

[phako]

Thank you for checking, I know that std::sort works and that's what I am currently using.

Also that's why I filed #1423 because I was trying to use it consistently for all three metadata classes.

[clanmills]

You know, even after 12+ years of working on this code, many mysteries and puzzles remain in the plumbing. However, the code is working, robust and well documented.

It's up to the community to maintain Exiv2. Good work by Andreas, Brad, Luis, Dan and others (including me) does deserve to live into the future.

[clanmills]

@phako I'm going to close this and #1423. When we discussed this, I left this open to be considered for 'master'. However, as we've decided to hibernate 'master', I have to consider this for 0.27-maintenance and/or main. I'm not making a change such as this in a "dot" release. When I work on 'main', the primary objective is to port the unique_ptr from 'old-master' and little else.

You are welcome to take up this matter with a future maintainer.