Lior Vernia has uploaded a new change for review. Change subject: core: Improve LexoNumericComparator case-insensitive behavior ......................................................................
core: Improve LexoNumericComparator case-insensitive behavior It is generally bad to return 0 when comparing strings that aren't identical - I can think of at least two good reasons. Firstly, due to dysfunctional TreeSet behavior in Java (it uses a comparator to tell whether two items are identical - this actually breaks the contract of Set, which is guaranteed to only refer to items as identical if they return the same hashCode() or true for equals()), it is best to distinguish between non-identical strings even if they're equal when compared case-insensitively. Secondly, when collections of items are sorted, it's best to have a deterministic order between them - otherwise "equal" rows may switch places (e.g. upon refresh). Change-Id: I097cdb5f4f784577586160f10c0ef151fc9c9b4f Bug-Url: https://bugzilla.redhat.com/1167641 Signed-off-by: Lior Vernia <[email protected]> --- M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparator.java M backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparatorTest.java 2 files changed, 28 insertions(+), 6 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/72/35572/1 diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparator.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparator.java index dbd9f6a..4f62821 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparator.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparator.java @@ -78,7 +78,8 @@ return -1; } - return 0; + // if the comparison is case-insensitive, differentiate between the strings unless they're truly identical + return Integer.signum(str1.compareTo(str2)); } private static int compareSequence(String seq1, String seq2, boolean digitSequence, boolean caseSensitive) { diff --git a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparatorTest.java b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparatorTest.java index 21397e6..7f1dea9 100644 --- a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparatorTest.java +++ b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/businessentities/comparators/LexoNumericComparatorTest.java @@ -52,34 +52,55 @@ { false, "12", "123", -1 }, { false, "012", "12", -1 }, { false, "2", "10", -1 }, - { false, "123Abc", "123abc", 0 }, + { false, "123abc", "123abc", 0 }, + { true, "123abc", "123abc", 0 }, + { false, "123Abc", "123abc", -1 }, { true, "123Abc", "123abc", -1 }, { false, "123abc", "456abc", -1 }, { false, "12abc", "123abc", -1 }, { false, "012abc", "12abc", -1 }, { false, "2abc", "10abc", -1 }, { false, "123", "abc", -1 }, - { false, "Abc", "abc", 0 }, + { false, "abc", "abc", 0 }, + { true, "abc", "abc", 0 }, + { false, "Abc", "abc", -1 }, { true, "Abc", "abc", -1 }, { false, "abc", "def", -1 }, { false, "ab", "abc", -1 }, { false, "123abc", "123def", -1 }, { false, "123ab", "123abc", -1 }, - { false, "Abc123", "abc123", 0 }, + { false, "abc123", "abc123", 0 }, + { true, "abc123", "abc123", 0 }, + { false, "Abc123", "abc123", -1 }, { true, "Abc123", "abc123", -1 }, + { false, "Abc234", "abc123", 1 }, + { true, "Abc234", "abc123", -1 }, + { false, "Abc1234", "abc123", 1 }, + { true, "Abc1234", "abc123", -1 }, { false, "abc123", "def123", -1 }, { false, "ab123", "abc123", -1 }, { false, "abc123", "abc456", -1 }, { false, "abc12", "abc123", -1 }, { false, "abc012", "abc12", -1 }, { false, "abc2", "abc10", -1 }, - { false, "Abc123def", "abc123def", 0 }, + { false, "abc123def", "abc123def", 0 }, + { true, "abc123def", "abc123def", 0 }, + { false, "Abc123def", "abc123def", -1 }, { true, "Abc123def", "abc123def", -1 }, + { false, "Abc234def", "abc123def", 1 }, + { true, "Abc234def", "abc123def", -1 }, + { false, "Abc1234def", "abc123def", 1 }, + { true, "Abc1234def", "abc123def", -1 }, { false, "abc123def", "abc123ghi", -1 }, { false, "abc123de", "abc123def", -1 }, { false, "abc123456789123de", "abc123456789123de", 0 }, - { false, "Abc123456789123de", "abc123456789123de", 0 }, + { true, "abc123456789123de", "abc123456789123de", 0 }, + { false, "Abc123456789123de", "abc123456789123de", -1 }, { true, "Abc123456789123de", "abc123456789123de", -1 }, + { false, "Abc123456789234de", "abc123456789123de", 1 }, + { true, "Abc123456789234de", "abc123456789123de", -1 }, + { false, "Abc1234567891234de", "abc123456789123de", 1 }, + { true, "Abc1234567891234de", "abc123456789123de", -1 }, { false, "abc123456789123de", "abc123456789123fg", -1 }, { false, "abc123456789123de", "abc123456789123def", -1 } }); -- To view, visit http://gerrit.ovirt.org/35572 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I097cdb5f4f784577586160f10c0ef151fc9c9b4f Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
