diff --git a/src/App/MappedElement.cpp b/src/App/MappedElement.cpp index 7b69e35c70..82d8daf4ee 100644 --- a/src/App/MappedElement.cpp +++ b/src/App/MappedElement.cpp @@ -31,137 +31,236 @@ #include "DocumentObject.h" #include "MappedElement.h" +#include + using namespace Data; +// hextoa converts a single hex character to its integer value. +inline +int hextoa(char c) +{ + constexpr int HEX_OFFSET = 10; + if (c >= '0' && c <= '9') { + return c - '0'; + } + + if (c >= 'a' && c <= 'f') { + return c - 'a' + HEX_OFFSET; + } + + if (c >= 'A' && c <= 'F') { + return c - 'A' + HEX_OFFSET; + } + + return -1; // invalid +} + + +// getHashLength returns the length of the #hexvalue without the leading '#'. +// If the string does not start with '#', it returns -1. +inline +int getHashLength(const MappedName& name) +{ + if (name[0] != '#') { + return 0; + } + + int len = name.size(); + int i = 1; // start after the '#' + for (; i < len; ++i) { + if (!std::isxdigit(name[i])) { + break; + } + } + + return i - 1; +} + +// getIntLength returns the length of the integer starting at the given start +// index. If the string does not start with a digit or the start is past the +// end of the string, it returns 0. +inline +int getIntLength(const MappedName& name, int start) +{ + int len = name.size(); + + int i = start; + for (; i < len; ++i) { + if (std::isdigit(name[i]) == 0) { + break; // not a digit, stop counting + } + } + + return i - start; // return the length of the integer +} + +// compareText compares the text part of two MappedNames starting at the given +// index. It returns -1 if left is less than right, 1 if left is greater than +// right, and 0 if they are equal. +inline +int compareText(const MappedName& left, const MappedName& right, int start) +{ + int leftLength = left.size(); + int rightLength = right.size(); + int minLength = std::min(leftLength, rightLength); + + for (int i = start; i < minLength; ++i) { + char ac = left[i]; + char bc = right[i]; + + if (ac != bc) { + return ac < bc ? -1 : 1; // left is less than right + } + } + + if (leftLength == rightLength) { + return 0; // they are equal + } + + // If we reach here, it means one is a prefix of the other. + return leftLength < rightLength ? -1 : 1; // left is less than right +} + +// compareHashed compares two MappedNames that could be hex values, or a +// combination of types. It returns: +// - -1 if left is less than right, +// - 1 if left is greater than right, +// - 0 if they are equal. +// This function handles identifiers being compared against hex values. +inline +int compareHashed(const MappedName& left, const MappedName& right) +{ + int leftLength = getHashLength(left); + int rightLength = getHashLength(right); + + if (leftLength < 1 || rightLength < 1 || // one of them is not a hex value + leftLength != rightLength) { // they are not the same length + + // Choose the shorter one as less favors identifiers first and numbers + // to be smallest to largest. + return leftLength < rightLength ? -1 : 1; + } + + // They are the same length and both are hex values, compare the hex digits. + int i = 1; + for (; i <= leftLength; ++i) { + int leftValue = hextoa(left[i]); + int rightValue = hextoa(right[i]); + + if (leftValue != rightValue) { + return leftValue < rightValue ? -1 : 1; + } + } + + // If we reach here, the hex values are equal, compare the text part. + return compareText(left, right, i); +} + +// compareNumbers compares parts of two MappedNames that are identifiers +// containing numbers. It returns: +// - -1 if left is less than right, +// - 1 if left is greater than right, +// - 0 if they are equal. +inline +int compareNumbers(const MappedName& left, const MappedName& right, int start) +{ + int leftLength = getIntLength(left, start); + int rightLength = getIntLength(right, start); + + if (leftLength != rightLength) { + // The shorter number is less than the longer one. + return leftLength < rightLength ? -1 : 1; + } + + int i = start; + for (; i < leftLength; ++i) { + char lc = left[i]; + char rc = right[i]; + + if (lc != rc) { + return lc < rc ? -1 : 1; + } + } + + // If we reach here, the numbers are equal, compare the text part. + return compareText(left, right, i); +} + +// compareIdentifiers compares two MappedNames that are identifiers. +// It returns: +// - -1 if left is less than right, +// - 1 if left is greater than right, +// - 0 if they are equal. +inline +int compareIdentifiers(const MappedName& left, const MappedName& right) +{ + int leftLength = left.size(); + int rightLength = right.size(); + int minLength = std::min(leftLength, rightLength); + + int i = 0; + for (i = 0; i < minLength; ++i) { + char lc = left[i]; + char rc = right[i]; + bool ldigit = std::isdigit(lc); + bool rdigit = std::isdigit(rc); + + if (!ldigit && !rdigit) { + if (lc != rc) { + return lc < rc ? -1 : 1; // left is less than right + } + + continue; // equal, continue to next character + } + + if (ldigit && rdigit) { + // both identifiers are the same up to this point, + return compareNumbers(left, right, i); + } + + return ldigit ? -1 : 1; // left is less than right + } + + // The identifier is the entire string, so we compare the lengths. + if (leftLength == rightLength) { + return 0; // they are equal + } + + return leftLength < rightLength ? -1 : 1; // left is less than right +} + +// Compare and sort the names. +// +// There are two forms of names that we compare: +// - Hex value: # like: #123:8;whatever or #aff-otherstuff; +// - Identifier: like: Edge123:8;whatever or Edge1234:8;whatever +// +// 1. Empty names are less than any other name. +// 2. Identifiers are ordered before hex values. +// 3. If both names are hex values, compare the hex values, smaller first. +// 3. If both names are identifiers, compare the identifiers, lexicographically +// for the name and numerically for the number, smaller first. +// 6. If the identifiers or hex values are equal, compare the text part lexicographically. +// 7. If the text parts are equal but one is shorter, the shorter name is first. bool ElementNameComparator::operator()(const MappedName& leftName, const MappedName& rightName) const { - int size = static_cast(std::min(leftName.size(), rightName.size())); - if (size == 0U) { - return leftName.size() < rightName.size(); - } - int currentIndex = 0; - if (rightName[0] == '#') { - if (leftName[0] != '#') { - return true; - } - // If both string starts with '#', compare the following hex digits by - // its integer value. - int res = 0; - for (currentIndex = 1; currentIndex < size; ++currentIndex) { - auto ac = (unsigned char)leftName[currentIndex]; - auto bc = (unsigned char)rightName[currentIndex]; - if (std::isxdigit(bc) != 0) { - if (std::isxdigit(ac) == 0) { - return true; - } - if (res == 0) { - if (ac < bc) { - res = -1; - } - else if (ac > bc) { - res = 1; - } - } - } - else if (std::isxdigit(ac) != 0) { - res = 1; - } - else { - break; - } - } - if (res < 0) { - return true; - } - if (res > 0) { - return false; - } + int leftLength = leftName.size(); + int rightLength = rightName.size(); - for (; currentIndex < size; ++currentIndex) { - char ac = leftName[currentIndex]; - char bc = rightName[currentIndex]; - if (ac < bc) { - return true; - } - if (ac > bc) { - return false; - } - } - return leftName.size() < rightName.size(); - } - if (leftName[0] == '#') { - return false; + if (std::min(leftLength, rightLength) == 0) { + return leftLength < rightLength; // empty names are less than any other name } - // If the string does not start with '#', compare the non-digits prefix - // using lexical order. - for (currentIndex = 0; currentIndex < size; ++currentIndex) { - auto ac = (unsigned char)leftName[currentIndex]; - auto bc = (unsigned char)rightName[currentIndex]; - if (std::isdigit(bc) == 0) { - if (std::isdigit(ac) != 0) { - return true; - } - if (ac < bc) { - return true; - } - if (ac > bc) { - return false; - } - } - else if (std::isdigit(ac) == 0) { - return false; - } - else { - break; - } + // If at least one of the names is a hex value, we compare them as such. + if (leftName[0] == '#' || rightName[0] == '#') { + int got = compareHashed(leftName, rightName); + return got < 0; // if leftName is less than rightName, return true } - // Then compare the following digits part by integer value - int res = 0; - for (; currentIndex < size; ++currentIndex) { - auto ac = (unsigned char)leftName[currentIndex]; - auto bc = (unsigned char)rightName[currentIndex]; - if (std::isdigit(bc) != 0) { - if (std::isdigit(ac) == 0) { - return true; - } - if (res == 0) { - if (ac < bc) { - res = -1; - } - else if (ac > bc) { - res = 1; - } - } - } - else if (std::isdigit(ac) != 0) { - return false; - } - else { - break; - } - } - if (res < 0) { - return true; - } - if (res > 0) { - return false; - } + int got = compareIdentifiers(leftName, rightName); - // Finally, compare the remaining tail using lexical order - for (; currentIndex < size; ++currentIndex) { - char ac = leftName[currentIndex]; - char bc = rightName[currentIndex]; - if (ac < bc) { - return true; - } - if (ac > bc) { - return false; - } - } - return leftName.size() < rightName.size(); + return got < 0; // if leftName is less than rightName or 0, return true } HistoryItem::HistoryItem(App::DocumentObject* obj, const Data::MappedName& name) diff --git a/tests/src/App/MappedElement.cpp b/tests/src/App/MappedElement.cpp index 3f1ecb622f..6421a6aca3 100644 --- a/tests/src/App/MappedElement.cpp +++ b/tests/src/App/MappedElement.cpp @@ -1,6 +1,11 @@ // SPDX-License-Identifier: LGPL-2.1-or-later #include +#include +#include +#include +#include + #include "App/IndexedName.h" #include "App/MappedElement.h" @@ -172,13 +177,24 @@ TEST_F(MappedElementTest, comparatorBothStartWithTheSameHexDigits) EXPECT_FALSE(comp(mappedName1, mappedName2)); } -TEST_F(MappedElementTest, DISABLED_comparatorHexWithoutTerminatorIsBroken) +TEST_F(MappedElementTest, comparatorHexWithoutTerminator) { // Arrange Data::MappedName mappedName1 {"#fed"}; Data::MappedName mappedName2 {"#abcdef"}; auto comp = Data::ElementNameComparator(); + // Act & Assert + EXPECT_TRUE(comp(mappedName1, mappedName2)); +} + +TEST_F(MappedElementTest, comparatorHexWithoutTerminatorSameLength) +{ + // Arrange + Data::MappedName mappedName1 {"#fed"}; + Data::MappedName mappedName2 {"#abc"}; + auto comp = Data::ElementNameComparator(); + // Act & Assert EXPECT_FALSE(comp(mappedName1, mappedName2)); } @@ -205,7 +221,7 @@ TEST_F(MappedElementTest, comparatorNoHexDigitsSameStringNumericCompare) EXPECT_FALSE(comp(mappedName1, mappedName2)); } -TEST_F(MappedElementTest, DISABLED_comparatorIntegerWithoutTerminatorIsBroken) +TEST_F(MappedElementTest, comparatorIntegerWithoutTerminatorIsBroken) { // Arrange Data::MappedName mappedName1 {"Edge123456"}; @@ -215,3 +231,87 @@ TEST_F(MappedElementTest, DISABLED_comparatorIntegerWithoutTerminatorIsBroken) // Act & Assert EXPECT_FALSE(comp(mappedName1, mappedName2)); } + +TEST_F(MappedElementTest, comparatorThreeComplexHexNamesInMap) +{ + // Arrange + Data::MappedName name1("#19c9:e;:U;FUS;:Hce4:7,E"); + Data::MappedName name2("#1dadb:11;:L#1061a;FUS;:H:d,E"); + Data::MappedName name3("#1dae6:8;:L#1dae4;FUS;:H:d,E"); + + std::map testMap; + testMap[name1] = 1; + testMap[name2] = 2; + testMap[name3] = 3; + + // Assert: map should have 3 unique keys + EXPECT_EQ(testMap.size(), 3); + + // Collect keys in order + std::vector keys; + for (const auto& kv : testMap) { + keys.push_back(kv.first.toString()); + } + + // Print for debug (optional) + // for (const auto& k : keys) std::cout << k << std::endl; + + // The expected order depends on your comparator logic. + // If you want to check the exact order, set it here: + // (Replace with the correct expected order if needed) + std::vector expectedOrder = {"#19c9:e;:U;FUS;:Hce4:7,E", + "#1dadb:11;:L#1061a;FUS;:H:d,E", + "#1dae6:8;:L#1dae4;FUS;:H:d,E"}; + + EXPECT_EQ(keys, expectedOrder); +} + +TEST_F(MappedElementTest, comparatorLargerWorkedExampleWithMap) +{ + // Arrange + Data::MappedName name0("Edge123;:U;FUS;:Hce4:7,E"); + Data::MappedName name1("#1dad:e;:U;FUS;:Hce4:7,E"); + Data::MappedName name2("#1dadb:11;:L#1061a;FUS;:H:d,E"); + Data::MappedName name3("#1dae6:8;:L#1dae4;FUS;:H:d,E"); + Data::MappedName name4("Edge999;;:L#1dae4;FUS;:H:d,E"); + Data::MappedName name5("g4v2;SKT;:H1234,F;:H5678:2,E;:G0(g1;SKT;:H9012,E);XTR;:H3456:2,F"); + + + std::map testMap; + testMap[name0] = 1; + testMap[name1] = 2; + testMap[name2] = 3; + testMap[name3] = 4; + testMap[name0] = 5; // Duplicate, should not affect size + testMap[name1] = 6; // Duplicate, should not affect size + testMap[name4] = 7; // New entry + testMap[name4] = 8; // Duplicate, should not affect size + testMap[name2] = 9; // Duplicate, should not affect size + testMap[name3] = 10; // Duplicate, should not affect size + testMap[name5] = 11; + + // Assert: map should have 5 unique keys + EXPECT_EQ(testMap.size(), 6); + + // Collect keys in order + std::vector keys; + for (const auto& kv : testMap) { + keys.push_back(kv.first.toString()); + } + + // Print for debug (optional) + // for (const auto& k : keys) std::cout << k << std::endl; + + // The expected order depends on your comparator logic. + // If you want to check the exact order, set it here: + // (Replace with the correct expected order if needed) + std::vector expectedOrder = { + "Edge123;:U;FUS;:Hce4:7,E", + "Edge999;;:L#1dae4;FUS;:H:d,E", + "g4v2;SKT;:H1234,F;:H5678:2,E;:G0(g1;SKT;:H9012,E);XTR;:H3456:2,F", + "#1dad:e;:U;FUS;:Hce4:7,E", + "#1dadb:11;:L#1061a;FUS;:H:d,E", + "#1dae6:8;:L#1dae4;FUS;:H:d,E"}; + + EXPECT_EQ(keys, expectedOrder); +}