fix(service): Fix only the first roster item being added to the database

lib/service/moxxmpp/roster.dart

@@ -1,21 +1,91 @@
 import 'dart:async';
 import 'package:get_it/get_it.dart';
 import 'package:moxxmpp/moxxmpp.dart';
+import 'package:moxxyv2/service/roster.dart';
+import 'package:moxxyv2/service/service.dart';
 import 'package:moxxyv2/service/xmpp.dart';
+import 'package:moxxyv2/shared/events.dart';
+import 'package:moxxyv2/shared/models/roster.dart';
 
-class MoxxyRosterManager extends RosterManager {
+class MoxxyRosterStateManager extends BaseRosterStateManager {
   @override
-  Future<void> commitLastRosterVersion(String version) async {
-    await GetIt.I.get<XmppService>().modifyXmppState((state) => state.copyWith(
-        lastRosterVersion: version,
-    ),);
+  Future<RosterCacheLoadResult> loadRosterCache() async {
+    final rs = GetIt.I.get<RosterService>();
+    return RosterCacheLoadResult(
+      (await GetIt.I.get<XmppService>().getXmppState()).lastRosterVersion,
+      (await rs.getRoster()).map((item) => XmppRosterItem(
+        jid: item.jid,
+        name: item.title,
+        subscription: item.subscription,
+        ask: item.ask.isEmpty ? null : item.ask,
+        groups: item.groups,
+      ),).toList(),
+    );
   }
 
   @override
-  Future<void> loadLastRosterVersion() async {
-    final ver = (await GetIt.I.get<XmppService>().getXmppState()).lastRosterVersion;
-    if (ver != null) {
-      setRosterVersion(ver);
+  Future<void> commitRoster(String? version, List<String> removed, List<XmppRosterItem> modified, List<XmppRosterItem> added) async {
+    final rs = GetIt.I.get<RosterService>();
+    final xs = GetIt.I.get<XmppService>();
+    await xs.modifyXmppState((state) => state.copyWith(
+      lastRosterVersion: version,
+    ),);
+
+    // Remove stale items
+    for (final jid in removed) {
+      await rs.removeRosterItemByJid(jid);
     }
+    
+    // Create new roster items
+    final rosterAdded = List<RosterItem>.empty(growable: true);
+    for (final item in added) {
+      rosterAdded.add(
+        await rs.addRosterItemFromData(
+          '',
+          '',
+          item.jid,
+          item.name ?? item.jid.split('@').first,
+          item.subscription,
+          item.ask ?? '',
+          false,
+          null,
+          null,
+          null,
+          groups: item.groups,
+        ),
+      );
+
+      // TODO(PapaTutuWawa): Fetch the avatar
+    }
+
+    // Update modified items
+    final rosterModified = List<RosterItem>.empty(growable: true);
+    for (final item in modified) {
+      final ritem = await rs.getRosterItemByJid(item.jid);
+      if (ritem == null) {
+        //_log.warning('Could not find roster item with JID $jid during update');
+        continue;
+      }
+
+      rosterModified.add(
+        await rs.updateRosterItem(
+          ritem.id,
+          title: item.name,
+          subscription: item.subscription,
+          ask: item.ask,
+          groups: item.groups,
+        ),
+      );
+    }
+
+    // Tell the UI
+    // TODO(Unknown): This may not be the cleanest place to put it
+    sendEvent(
+      RosterDiffEvent(
+        added: rosterAdded,
+        modified: rosterModified,
+        removed: removed,
+      ),
+    );
   }
 }

lib/service/omemo/omemo.dart

@@ -67,7 +67,7 @@ class OmemoService {
     );
 
     if (device == null) {
-      await commitDevice(device!);
+      await commitDevice(await omemoManager.getDevice());
       await commitDeviceMap(<String, List<int>>{});
       await commitTrustManager(await omemoManager.trustManager.toJson());
     }
@@ -370,7 +370,6 @@ class OmemoService {
 
   Future<void> removeAllSessions(String jid) async {
     await ensureInitialized();
-    // TODO(PapaTutuWawa): Reset trust decisions in the TrustManager
     await omemoManager.removeAllRatchets(jid);
   }
 

lib/service/roster.dart

@@ -1,222 +1,28 @@
 import 'dart:async';
-import 'dart:collection';
-import 'package:flutter/foundation.dart';
 import 'package:get_it/get_it.dart';
 import 'package:logging/logging.dart';
-import 'package:moxlib/moxlib.dart';
-import 'package:moxplatform/moxplatform.dart';
 import 'package:moxxmpp/moxxmpp.dart';
 import 'package:moxxyv2/service/contacts.dart';
-import 'package:moxxyv2/service/conversation.dart';
 import 'package:moxxyv2/service/database/database.dart';
 import 'package:moxxyv2/service/not_specified.dart';
 import 'package:moxxyv2/service/service.dart';
 import 'package:moxxyv2/shared/events.dart';
-import 'package:moxxyv2/shared/models/conversation.dart';
 import 'package:moxxyv2/shared/models/roster.dart';
 
-/// Closure which returns true if the jid of a [RosterItem] is equal to [jid].
-bool Function(RosterItem) _jidEqualsWrapper(String jid) {
-  return (i) => i.jid == jid;
-}
-
-typedef AddRosterItemFunction = Future<RosterItem> Function(
-  String avatarUrl,
-  String avatarHash,
-  String jid,
-  String title,
-  String subscription,
-  String ask,
-  bool pseudoRosterItem,
-  String? contactId,
-  String? contactAvatarPath,
-  String? contactDisplayName,
-  {
-    List<String> groups,
-  }
-);
-typedef UpdateRosterItemFunction = Future<RosterItem> Function(
-  int id, {
-    String? avatarUrl,
-    String? avatarHash,
-    String? title,
-    String? subscription,
-    String? ask,
-    Object pseudoRosterItem,
-    List<String>? groups,
-  }
-);
-typedef RemoveRosterItemFunction = Future<void> Function(String jid);
-typedef GetConversationFunction = Future<Conversation?> Function(String jid);
-typedef SendEventFunction = void Function(BackgroundEvent event, { String? id });
-
-/// Compare the local roster with the roster we received either by request or by push.
-/// Returns a diff between the roster before and after the request or the push.
-/// NOTE: This abuses the [RosterDiffEvent] type a bit.
-Future<RosterDiffEvent> processRosterDiff(
-  List<RosterItem> currentRoster,
-  List<XmppRosterItem> remoteRoster,
-  bool isRosterPush,
-  AddRosterItemFunction addRosterItemFromData,
-  UpdateRosterItemFunction updateRosterItem,
-  RemoveRosterItemFunction removeRosterItemByJid,
-  GetConversationFunction getConversationByJid,
-  SendEventFunction _sendEvent,
-) async {
-  final css = GetIt.I.get<ContactsService>();
-  final removed = List<String>.empty(growable: true);
-  final modified = List<RosterItem>.empty(growable: true);
-  final added = List<RosterItem>.empty(growable: true);
-
-  for (final item in remoteRoster) {
-    if (isRosterPush) {
-      final litem = firstWhereOrNull(currentRoster, _jidEqualsWrapper(item.jid));
-      if (litem != null) {
-        if (item.subscription == 'remove') {
-          // We have the item locally but it has been removed
-
-          if (litem.contactId != null) {
-            // We have the contact associated with a contact
-            final newItem = await updateRosterItem(
-              litem.id,
-              ask: 'none',
-              subscription: 'none',
-              pseudoRosterItem: true,
-            );
-            modified.add(newItem);
-          } else {
-            await removeRosterItemByJid(item.jid);
-            removed.add(item.jid);
-          }
-
-          continue;
-        }
-
-        // Item has been modified
-        final newItem = await updateRosterItem(
-          litem.id,
-          subscription: item.subscription,
-          title: item.name,
-          ask: item.ask,
-          pseudoRosterItem: false,
-          groups: item.groups,
-        );
-
-        modified.add(newItem);
-
-        // Check if we have a conversation that we need to modify
-        final conv = await getConversationByJid(item.jid);
-        if (conv != null) {
-          _sendEvent(
-            ConversationUpdatedEvent(
-              conversation: conv.copyWith(subscription: item.subscription),
-            ),
-          );
-        }
-      } else {
-        // Item does not exist locally
-        if (item.subscription == 'remove') {
-          // Item has been removed but we don't have it locally
-          removed.add(item.jid);
-        } else {
-          // Item has been added and we don't have it locally
-          final contactId = await css.getContactIdForJid(item.jid);
-          final newItem = await addRosterItemFromData(
-            '',
-            '',
-            item.jid,
-            item.name ?? item.jid.split('@')[0],
-            item.subscription,
-            item.ask ?? '',
-            false,
-            contactId,
-            await css.getProfilePicturePathForJid(item.jid),
-            await css.getContactDisplayName(contactId),
-            groups: item.groups,
-          );
-
-          added.add(newItem);
-        }
-      }
-    } else {
-      final litem = firstWhereOrNull(currentRoster, _jidEqualsWrapper(item.jid));
-      if (litem != null) {
-        // Item is modified
-        if (litem.title != item.name || litem.subscription != item.subscription || !listEquals(litem.groups, item.groups)) {
-          final modifiedItem = await updateRosterItem(
-            litem.id,
-            title: item.name,
-            subscription: item.subscription,
-            pseudoRosterItem: false,
-            groups: item.groups,
-          );
-          modified.add(modifiedItem);
-
-          // Check if we have a conversation that we need to modify
-          final conv = await getConversationByJid(litem.jid);
-          if (conv != null) {
-            _sendEvent(
-              ConversationUpdatedEvent(
-                conversation: conv.copyWith(subscription: item.subscription),
-              ),
-            );
-          }
-        }
-      } else {
-        // Item is new
-        final contactId = await css.getContactIdForJid(item.jid);
-        added.add(await addRosterItemFromData(
-            '',
-            '',
-            item.jid,
-            item.jid.split('@')[0],
-            item.subscription,
-            item.ask ?? '',
-            false,
-            contactId,
-            await css.getProfilePicturePathForJid(item.jid),
-            await css.getContactDisplayName(contactId),
-            groups: item.groups,
-        ),);
-      }
-    }
-  }
-
-  if (!isRosterPush) {
-    for (final item in currentRoster) {
-      final ritem = firstWhereOrNull(remoteRoster, (XmppRosterItem i) => i.jid == item.jid);
-      if (ritem == null) {
-        await removeRosterItemByJid(item.jid);
-        removed.add(item.jid);
-      }
-      // We don't handle the modification case here as that is covered by the huge
-      // loop above
-    }
-  }
-  
-  return RosterDiffEvent(
-    added: added,
-    modified: modified,
-    removed: removed,
-  );
-}
-
 class RosterService {
-  
-  RosterService()
-    : _rosterCache = HashMap(),
-      _rosterLoaded = false,
-      _log = Logger('RosterService');
-  final HashMap<String, RosterItem> _rosterCache;
-  bool _rosterLoaded;
+  RosterService() : _log = Logger('RosterService');
+  Map<String, RosterItem>? _rosterCache;
   final Logger _log;
-  
-  Future<bool> isInRoster(String jid) async {
-    if (!_rosterLoaded) {
+
+  Future<void> _loadRosterIfNeeded() async {
+    if (_rosterCache == null) {
       await loadRosterFromDatabase();
     }
+  }
 
-    return _rosterCache.containsKey(jid);
+  Future<bool> isInRoster(String jid) async {
+    await _loadRosterIfNeeded();
+    return _rosterCache!.containsKey(jid);
   }
 
   /// Wrapper around [DatabaseService]'s addRosterItemFromData that updates the cache.
@@ -250,7 +56,7 @@ class RosterService {
     );
 
     // Update the cache
-    _rosterCache[item.jid] = item;
+    _rosterCache![item.jid] = item;
 
     return item;
   }
@@ -285,26 +91,26 @@ class RosterService {
     );
 
     // Update cache
-    _rosterCache[newItem.jid] = newItem;
+    _rosterCache![newItem.jid] = newItem;
     
     return newItem;
   }
 
   /// Wrapper around [DatabaseService]'s removeRosterItem.
   Future<void> removeRosterItem(int id) async {
+    // NOTE: This call ensures that _rosterCache != null
     await GetIt.I.get<DatabaseService>().removeRosterItem(id);
-
+    assert(_rosterCache != null, '_rosterCache must be non-null');
+    
     /// Update cache
-    _rosterCache.removeWhere((_, value) => value.id == id);
+    _rosterCache!.removeWhere((_, value) => value.id == id);
   }
 
   /// Removes a roster item from the database based on its JID.
   Future<void> removeRosterItemByJid(String jid) async {
-    if (!_rosterLoaded) {
-      await loadRosterFromDatabase();
-    }
+    await _loadRosterIfNeeded();
 
-    for (final item in _rosterCache.values) {
+    for (final item in _rosterCache!.values) {
       if (item.jid == jid) {
         await removeRosterItem(item.id);
         return;
@@ -314,17 +120,14 @@ class RosterService {
   
   /// Returns the entire roster
   Future<List<RosterItem>> getRoster() async {
-    if (!_rosterLoaded) {
-      await loadRosterFromDatabase();
-    }
-
-    return _rosterCache.values.toList();
+    await _loadRosterIfNeeded();
+    return _rosterCache!.values.toList();
   }
 
   /// Returns the roster item with jid [jid] if it exists. Null otherwise.
   Future<RosterItem?> getRosterItemByJid(String jid) async {
     if (await isInRoster(jid)) {
-      return _rosterCache[jid];
+      return _rosterCache![jid];
     }
 
     return null;
@@ -335,9 +138,9 @@ class RosterService {
   Future<List<RosterItem>> loadRosterFromDatabase() async {
     final items = await GetIt.I.get<DatabaseService>().loadRosterItems();
 
-    _rosterLoaded = true;
+    _rosterCache = <String, RosterItem>{};
     for (final item in items) {
-      _rosterCache[item.jid] = item;
+      _rosterCache![item.jid] = item;
     }
     
     return items;
@@ -392,59 +195,6 @@ class RosterService {
     return false;
   }
 
-  Future<void> requestRoster() async {
-    final roster = GetIt.I.get<XmppConnection>().getManagerById<RosterManager>(rosterManager)!;
-    Result<RosterRequestResult?, RosterError> result;
-    if (roster.rosterVersioningAvailable()) {
-      _log.fine('Stream supports roster versioning');
-      result = await roster.requestRosterPushes();
-      _log.fine('Requesting roster pushes done');
-    } else {
-      _log.fine('Stream does not support roster versioning');
-      result = await roster.requestRoster();
-    }
-
-    if (result.isType<RosterError>()) {
-      _log.warning('Failed to request roster');
-      return;
-    }
-
-    final value = result.get<RosterRequestResult?>();
-    if (value != null) {
-      final currentRoster = await getRoster();
-      sendEvent(
-        await processRosterDiff(
-          currentRoster,
-          value.items,
-          false,
-          addRosterItemFromData,
-          updateRosterItem,
-          removeRosterItemByJid,
-          GetIt.I.get<ConversationService>().getConversationByJid,
-          sendEvent,
-        ),
-      );
-    }
-  }
-
-  /// Handles a roster push.
-  Future<void> handleRosterPushEvent(RosterPushEvent event) async {
-    final item = event.item;
-    final currentRoster = await getRoster();
-    sendEvent(
-      await processRosterDiff(
-        currentRoster,
-        [ item ],
-        true,
-        addRosterItemFromData,
-        updateRosterItem,
-        removeRosterItemByJid,
-        GetIt.I.get<ConversationService>().getConversationByJid,
-        sendEvent,
-      ),
-    );
-  }
-
   Future<void> acceptSubscriptionRequest(String jid) async {
     GetIt.I.get<XmppConnection>().getPresenceManager().sendSubscriptionRequestApproval(jid);
   }

lib/service/service.dart

@@ -178,7 +178,7 @@ Future<void> entrypoint() async {
   )..registerManagers([
       MoxxyStreamManagementManager(),
       MoxxyDiscoManager(),
-      MoxxyRosterManager(),
+      RosterManager(MoxxyRosterStateManager()),
       MoxxyOmemoManager(),
       PingManager(),
       MessageManager(),

lib/service/xmpp.dart

@@ -55,7 +55,6 @@ class XmppService {
         EventTypeMatcher<SubscriptionRequestReceivedEvent>(_onSubscriptionRequestReceived),
         EventTypeMatcher<DeliveryReceiptReceivedEvent>(_onDeliveryReceiptReceived),
         EventTypeMatcher<ChatMarkerEvent>(_onChatMarker),
-        EventTypeMatcher<RosterPushEvent>(_onRosterPush),
         EventTypeMatcher<AvatarUpdatedEvent>(_onAvatarUpdated),
         EventTypeMatcher<StanzaAckedEvent>(_onStanzaAcked),
         EventTypeMatcher<MessageEvent>(_onMessage),
@@ -663,7 +662,7 @@ class XmppService {
 
       _log.finest('Connection connected. Is resumed? ${event.resumed}');
       unawaited(_initializeOmemoService(settings.jid.toString()));
-
+      
       if (!event.resumed) {
         // Reset the blocking service's cache
         GetIt.I.get<BlocklistService>().onNewConnection();
@@ -681,8 +680,10 @@ class XmppService {
 
         // In section 5 of XEP-0198 it says that a client should not request the roster
         // in case of a stream resumption.
-        await GetIt.I.get<RosterService>().requestRoster();
-
+        await connection
+          .getManagerById<RosterManager>(rosterManager)!
+          .requestRoster();
+        
         // TODO(Unknown): Once groupchats come into the equation, this gets trickier
         final roster = await GetIt.I.get<RosterService>().getRoster();
         for (final item in roster) {
@@ -1402,11 +1403,6 @@ class XmppService {
     }
   }
   
-  Future<void> _onRosterPush(RosterPushEvent event, { dynamic extra }) async {
-    _log.fine("Roster push version: ${event.ver ?? "(null)"}");
-    await GetIt.I.get<RosterService>().handleRosterPushEvent(event);
-  }
-
   Future<void> _onAvatarUpdated(AvatarUpdatedEvent event, { dynamic extra }) async {
     await GetIt.I.get<AvatarService>().handleAvatarUpdate(event);
   }

pubspec.lock

@@ -836,7 +836,7 @@ packages:
     description:
       path: "packages/moxxmpp"
       ref: HEAD
-      resolved-ref: "55d2ef9c25d383806bc780597a96d3c2887a4aa1"
+      resolved-ref: d8c2ef6f3b3c7aa7147f0d0a4713793d1bc51743
       url: "https://git.polynom.me/Moxxy/moxxmpp.git"
     source: git
     version: "0.1.6+1"
@@ -888,7 +888,7 @@ packages:
       name: omemo_dart
       url: "https://git.polynom.me/api/packages/PapaTutuWawa/pub/"
     source: hosted
-    version: "0.4.1"
+    version: "0.4.2"
   package_config:
     dependency: transitive
     description:

pubspec.yaml

@@ -75,7 +75,7 @@ dependencies:
   native_imaging: 0.1.0
   omemo_dart:
     hosted: https://git.polynom.me/api/packages/PapaTutuWawa/pub
-    version: 0.4.1
+    version: 0.4.2
   page_transition: 2.0.9
   path: 1.8.2
   path_provider: 2.0.11
@@ -140,7 +140,7 @@ dependency_overrides:
   moxxmpp:
     git:
       url: https://git.polynom.me/Moxxy/moxxmpp.git
-      rev: 55d2ef9c25d383806bc780597a96d3c2887a4aa1
+      rev: d8c2ef6f3b3c7aa7147f0d0a4713793d1bc51743
       path: packages/moxxmpp
  
 extra_licenses: