Fix contract in Bookmark and Contact compareTo

Phillip Davis created 3 weeks ago

Null-guard getDisplayName() and use compareTo instead of equals so
the return value satisfies anti-symmetry, previously violated by
examples like this:
```
  - A: displayName "alice", jid [e-mail]
  - B: displayName "Alice", jid [e-mail]
  - C: displayName "alice", jid [e-mail]
```

Change summary

src/main/java/eu/siacs/conversations/entities/Bookmark.java             |  9 
src/main/java/eu/siacs/conversations/entities/Contact.java              |  9 
src/test/java/eu/siacs/conversations/entities/ContactCompareToTest.java | 88 
3 files changed, 100 insertions(+), 6 deletions(-)

Detailed changes

src/main/java/eu/siacs/conversations/entities/Bookmark.java

@@ -117,12 +117,15 @@ public class Bookmark extends Element implements ListItem {
 			return 1;
 		}
 
-		if (getDisplayName().equals(another.getDisplayName())) {
+		final var anotherName =
+			another.getDisplayName() == null ? "" : another.getDisplayName();
+		final var displayNameScore = getDisplayName().compareToIgnoreCase(anotherName);
+
+		if (displayNameScore == 0) {
 			return getJid().compareTo(another.getJid());
 		}
 
-		return this.getDisplayName().compareToIgnoreCase(
-				another.getDisplayName());
+		return displayNameScore;
 	}
 
     @Override

src/main/java/eu/siacs/conversations/entities/Contact.java

@@ -565,12 +565,15 @@ public class Contact implements ListItem, Blockable {
             return 1;
         }
 
-        if (getDisplayName().equals(another.getDisplayName())) {
+        final var anotherName =
+            another.getDisplayName() == null ? "" : another.getDisplayName();
+        final var displayNameScore = getDisplayName().compareToIgnoreCase(anotherName);
+
+        if (displayNameScore == 0) {
             return getJid().compareTo(another.getJid());
         }
 
-        final var anotherName = another.getDisplayName();
-        return this.getDisplayName().compareToIgnoreCase(anotherName == null ? "" : anotherName);
+        return displayNameScore;
     }
 
     public String getServer() {

src/test/java/eu/siacs/conversations/entities/ContactCompareToTest.java

@@ -0,0 +1,88 @@
+package eu.siacs.conversations.entities;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Random;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.annotation.Config;
+import org.robolectric.annotation.ConscryptMode;
+
+import android.os.Build;
+import eu.siacs.conversations.Conversations;
+import eu.siacs.conversations.xmpp.Jid;
+import junit.framework.Assert;
+
+@RunWith(RobolectricTestRunner.class)
+@Config(sdk = Build.VERSION_CODES.TIRAMISU, application = Conversations.class)
+@ConscryptMode(ConscryptMode.Mode.OFF)
+public class ContactCompareToTest {
+
+    private static final Jid ACCOUNT_JID = Jid.of("[e-mail]");
+
+    private Contact contact(String systemName, String jid) {
+        final var account = mock(Account.class);
+        when(account.getJid()).thenReturn(ACCOUNT_JID);
+        final var contact = new Contact(
+            null, systemName, null, null,
+            Jid.of(jid), 0, null, null,
+            null, null, 0, null, null, null
+        );
+        contact.setAccount(account);
+        return contact;
+    }
+
+    private Bookmark bookmark(String name, String jid) {
+        final var account = mock(Account.class);
+        when(account.getJid()).thenReturn(Jid.of("[e-mail]"));
+        final var bookmark = new Bookmark(account, Jid.of(jid));
+        bookmark.setBookmarkName(name);
+        return bookmark;
+    }
+
+    @Test
+    public void testSortContactsWithCaseVariantNames() {
+        final var items = new ArrayList<ListItem>();
+        for (int i = 0; i < 32; i++) {
+            items.add(contact("alice", String.format("[e-mail]", i)));
+            items.add(contact("Alice", String.format("[e-mail]", i)));
+        }
+
+        for (int seed = 0; seed < 100; seed++) {
+            Collections.shuffle(items, new Random(seed));
+            Collections.sort(items);
+        }
+    }
+
+    @Test
+    public void testSortMixedContactsAndBookmarksWithCaseVariantNames() {
+        final var items = new ArrayList<ListItem>();
+        for (int i = 0; i < 32; i++) {
+            items.add(contact("alice", String.format("[e-mail]", i)));
+            items.add(bookmark("Alice", String.format("[e-mail]", i)));
+        }
+
+        for (int seed = 0; seed < 100; seed++) {
+            Collections.shuffle(items, new Random(seed));
+            Collections.sort(items);
+        }
+    }
+
+    @Test
+    public void testCaseVariantNamesTieOnNameThenSortByJid() {
+        final var AliceAtB = contact("Alice", "[e-mail]");
+        final var aliceAtA = contact("alice", "[e-mail]");
+
+        Assert.assertTrue(aliceAtA.compareTo(AliceAtB) < 0);
+        Assert.assertTrue(AliceAtB.compareTo(aliceAtA) > 0);
+
+        final var AliceAtA = contact("Alice", "[e-mail]");
+        Assert.assertTrue(AliceAtA.compareTo(AliceAtB) < 0);
+        Assert.assertTrue(AliceAtB.compareTo(AliceAtA) > 0);
+    }
+}