Fix position updates for conversations

The previous logic was partially broken, moving conversations down did
not work.

It was also more complicated than necessary.

src/services/webclient.ts

@@ -2056,29 +2056,22 @@ export class WebClientService {
                 // To find out, we'll look at the unread count. If it has been
                 // incremented, it must be a new message.
                 if (data.unreadCount > 0) {
-                    // Find the correct conversation in the conversation list
-                    const conversation = this.conversations.find(data);
-                    if (data === null) {
-                        // Conversation not found, add it!
-                        this.conversations.add(data);
-                        this.onNewMessage(data.latestMessage, conversation);
+                    const oldConversation = this.conversations.updateOrAdd(data);
+                    if (oldConversation === null) {
+                        this.onNewMessage(data.latestMessage, data);
                     } else {
                         // Check for unread count changes
-                        const unreadCountIncreased = data.unreadCount > conversation.unreadCount;
-                        const unreadCountDecreased = data.unreadCount < conversation.unreadCount;
-
-                        // Update the conversation
-                        this.conversations.updateOrAdd(data);
+                        const unreadCountIncreased = data.unreadCount > oldConversation.unreadCount;
+                        const unreadCountDecreased = data.unreadCount < oldConversation.unreadCount;
 
                         // If the unreadcount has increased, we received a new message.
                         // Otherwise, if it has decreased, hide the notification.
                         if (unreadCountIncreased) {
-                            this.onNewMessage(data.latestMessage, conversation);
+                            this.onNewMessage(data.latestMessage, data);
                         } else if (unreadCountDecreased) {
                             this.notificationService.hideNotification(data.type + '-' + data.id);
                         }
                     }
-
                 } else {
                     // Update the conversation and hide any notifications
                     this.conversations.updateOrAdd(data);

src/threema.d.ts

@@ -700,7 +700,7 @@ declare namespace threema {
             set(data: Conversation[]): void;
             find(pattern: Conversation | Receiver): Conversation | null;
             add(conversation: Conversation): void;
-            updateOrAdd(conversation: Conversation): void;
+            updateOrAdd(conversation: Conversation): Conversation | null;
             remove(conversation: Conversation): void;
             setFilter(filter: (data: Conversation[]) => Conversation[]): void;
             setConverter(converter: (data: Conversation) => Conversation): void;

src/threema/container.ts

@@ -250,6 +250,11 @@ export class Conversations implements threema.Container.Conversations {
      * one provided.
      */
     public set(data: threema.Conversation[]): void {
+        for (const conversation of data) {
+            if (conversation.position !== undefined) {
+                delete conversation.position;
+            }
+        }
         this.conversations = data;
     }
 
@@ -269,54 +274,36 @@ export class Conversations implements threema.Container.Conversations {
         return null;
     }
 
+    /**
+     * Add a conversation at the correct position.
+     * Don't check whether the conversation already exists.
+     */
     public add(conversation: threema.ConversationWithPosition): void {
         this.conversations.splice(conversation.position, 0, conversation);
     }
 
-    public updateOrAdd(conversation: threema.ConversationWithPosition): void {
-        let moveDirection = 0;
-        let updated = false;
-        for (const p of this.conversations.keys()) {
-            if (this.receiverService.compare(this.conversations[p], conversation)) {
-                // ok, replace me and break
-                const old = this.conversations[p];
-                if (old.position !== conversation.position) {
-                    // position also changed...
-                    moveDirection = old.position > conversation.position ? -1 : 1;
-                }
-                this.conversations[p] = conversation;
-                updated = true;
-            }
-        }
-
-        // reset the position field to correct the sorting
-        if (moveDirection !== 0) {
-            // reindex
-            let before = true;
-            for (const p in this.conversations) {
-                if (this.receiverService.compare(this.conversations[p], conversation)) {
-                    before = false;
-                } else if (before && moveDirection < 0) {
-                    this.conversations[p].position++;
-                } else if (!before && moveDirection > 0) {
-                    this.conversations[p].position++;
-                }
+    /**
+     * Add a conversation at the correct position.
+     * If a conversation already exists, replace it and return the old conversation.
+     */
+    public updateOrAdd(conversation: threema.ConversationWithPosition): threema.Conversation | null {
+        let replaced = null;
+        for (const i of this.conversations.keys()) {
+            if (this.receiverService.compare(this.conversations[i], conversation)) {
+                replaced = this.conversations.splice(i, 1)[0];
             }
-
-            // sort by position field
-            this.conversations.sort(function(convA, convB) {
-                return convA.position - convB.position;
-            });
-        } else if (!updated) {
-            this.add(conversation);
         }
+        this.add(conversation);
+        return replaced;
     }
 
+    /**
+     * Remove a conversation.
+     */
     public remove(conversation: threema.Conversation): void {
-        for (const p of this.conversations.keys()) {
-            if (this.receiverService.compare(this.conversations[p], conversation)) {
-                // remove conversation from array
-                this.conversations.splice(p, 1);
+        for (const i of this.conversations.keys()) {
+            if (this.receiverService.compare(this.conversations[i], conversation)) {
+                this.conversations.splice(i, 1);
                 return;
             }
         }

tests/ts/containers.ts

@@ -28,7 +28,7 @@ function getConversations(): Conversations {
     return new Conversations(receiverService);
 }
 
-function makeContactConversation(id: string, position: number): threema.ConversationWithPosition {
+function makeContactConversation(id: string, position?: number): threema.ConversationWithPosition {
     return {
         type: 'contact',
         id: id,
@@ -39,8 +39,8 @@ function makeContactConversation(id: string, position: number): threema.Conversa
     };
 }
 
-function simplifyConversation(c: threema.Conversation): Array<string | number> {
-    return [c.id, c.position];
+function getId(c: threema.Conversation): string {
+    return c.id;
 }
 
 describe('Container', () => {
@@ -49,17 +49,17 @@ describe('Container', () => {
         it('find', function() {
             const conversations = getConversations();
             conversations.set([
-                makeContactConversation('1', 0),
-                makeContactConversation('2', 1),
-                makeContactConversation('3', 2),
-                makeContactConversation('4', 3),
+                makeContactConversation('1'),
+                makeContactConversation('2'),
+                makeContactConversation('3'),
+                makeContactConversation('4'),
             ]);
 
             const receiver1: threema.BaseReceiver = { id: '2', type: 'contact' };
             const receiver2: threema.BaseReceiver = { id: '5', type: 'contact' };
             const receiver3: threema.BaseReceiver = { id: '2', type: 'me' };
 
-            expect(conversations.find(receiver1)).toEqual(makeContactConversation('2', 1));
+            expect(conversations.find(receiver1)).toEqual(makeContactConversation('2'));
             expect(conversations.find(receiver2)).toEqual(null);
             expect(conversations.find(receiver3)).toEqual(null);
         });
@@ -69,32 +69,73 @@ describe('Container', () => {
                 const conversations = getConversations();
                 expect(conversations.get()).toEqual([]);
 
-                conversations.add(makeContactConversation('0', 0));
-                expect(conversations.get().map(simplifyConversation)).toEqual([['0', 0]]);
+                conversations.add(makeContactConversation('0'));
+                expect(conversations.get().map(getId)).toEqual(['0']);
+
+                conversations.set([makeContactConversation('1')]);
+                expect(conversations.get().map(getId)).toEqual(['1']);
+            });
+
+            it('clears position field', function() {
+                const conversations = getConversations();
+                conversations.set([makeContactConversation('1', 7)]);
 
-                conversations.set([makeContactConversation('1', 1)]);
-                expect(conversations.get().map(simplifyConversation)).toEqual([['1', 1]]);
+                const expected = makeContactConversation('1');
+                delete expected.position;
+                expect((conversations as any).conversations).toEqual([expected]);
             });
         });
 
         describe('add', function() {
-            it('adds a conversation at the correct location', function() {
+            it('adds a new conversation at the correct location', function() {
                 const conversations = getConversations();
                 expect(conversations.get()).toEqual([]);
 
                 conversations.add(makeContactConversation('0', 0));
                 conversations.add(makeContactConversation('1', 1));
-                expect(conversations.get().map(simplifyConversation)).toEqual([
-                    ['0', 0],
-                    ['1', 1],
-                ]);
+                expect(conversations.get().map(getId)).toEqual(['0', '1']);
 
                 conversations.add(makeContactConversation('2', 1));
-                expect(conversations.get().map(simplifyConversation)).toEqual([
-                    ['0', 0],
-                    ['2', 1],
-                    ['1', 1],
+                expect(conversations.get().map(getId)).toEqual(['0', '2', '1']);
+            });
+        });
+
+        describe('updateOrAdd', function() {
+            it('adds a new conversation at the correct location', function() {
+                const conversations = getConversations();
+                conversations.set([
+                    makeContactConversation('0'),
+                    makeContactConversation('1'),
                 ]);
+                expect(conversations.get().map(getId)).toEqual(['0', '1']);
+
+                conversations.updateOrAdd(makeContactConversation('2', 2));
+                expect(conversations.get().map(getId)).toEqual(['0', '1', '2']);
+
+                conversations.updateOrAdd(makeContactConversation('3', 2));
+                expect(conversations.get().map(getId)).toEqual(['0', '1', '3', '2']);
+            });
+
+            it('moves an existing conversation to the correct location', function() {
+                const conversations = getConversations();
+                conversations.set([
+                    makeContactConversation('0'),
+                    makeContactConversation('1'),
+                    makeContactConversation('2'),
+                ]);
+                expect(conversations.get().map(getId)).toEqual(['0', '1', '2']);
+
+                conversations.updateOrAdd(makeContactConversation('2', 1));
+                expect(conversations.get().map(getId)).toEqual(['0', '2', '1']);
+
+                conversations.updateOrAdd(makeContactConversation('1', 0));
+                expect(conversations.get().map(getId)).toEqual(['1', '0', '2']);
+
+                conversations.updateOrAdd(makeContactConversation('0', 2));
+                expect(conversations.get().map(getId)).toEqual(['1', '2', '0']);
+
+                conversations.updateOrAdd(makeContactConversation('1', 7));
+                expect(conversations.get().map(getId)).toEqual(['2', '0', '1']);
             });
         });
     });