From e13b00d127bc68c0659188b1e8aa25f510e9398a Mon Sep 17 00:00:00 2001 From: Christian Pauly Date: Wed, 8 Sep 2021 09:54:39 +0200 Subject: [PATCH] refactor: Make RoomUpdate class null safe by removing it RoomUpdate came from a time where we had no data model for SyncUpdates but now we have and therefore this class is just code duplication. This removes the class and uses the SyncRoomUpdate class from the package matrix_api_lite instead. It needed a lot of refactoring at some places where I also have removed some unnecessary null or type checks. --- lib/matrix.dart | 1 - lib/src/client.dart | 78 ++++++++++++------------ lib/src/database/database_api.dart | 3 +- lib/src/database/hive_database.dart | 69 +++++++++++++-------- lib/src/timeline.dart | 17 +++--- lib/src/utils/room_update.dart | 93 ----------------------------- test/client_test.dart | 26 -------- test/database_api_test.dart | 16 +++-- test/timeline_test.dart | 21 ++++--- 9 files changed, 112 insertions(+), 212 deletions(-) delete mode 100644 lib/src/utils/room_update.dart diff --git a/lib/matrix.dart b/lib/matrix.dart index 90dd0234..140b8e5c 100644 --- a/lib/matrix.dart +++ b/lib/matrix.dart @@ -21,7 +21,6 @@ library matrix; export 'package:matrix_api_lite/matrix_api_lite.dart'; -export 'src/utils/room_update.dart'; export 'src/utils/event_update.dart'; export 'src/utils/image_pack_extension.dart'; export 'src/utils/device_keys_list.dart'; diff --git a/lib/src/client.dart b/lib/src/client.dart index 3a2e4831..ad52f4f4 100644 --- a/lib/src/client.dart +++ b/lib/src/client.dart @@ -37,7 +37,6 @@ import 'utils/device_keys_list.dart'; import 'utils/event_update.dart'; import 'utils/http_timeout.dart'; import 'utils/matrix_file.dart'; -import 'utils/room_update.dart'; import 'utils/to_device_event.dart'; import 'utils/uia_request.dart'; import 'utils/multilock.dart'; @@ -758,11 +757,6 @@ class Client extends MatrixApi { /// onRoomEvent( "m.room.message", "!chat_id:server.com", "timeline", {sender: "@bob:server.com", body: "Hello world"} ) final StreamController onEvent = StreamController.broadcast(); - /// Outside of the events there are updates for the global chat states which - /// are handled by this signal: - final StreamController onRoomUpdate = - StreamController.broadcast(); - /// The onToDeviceEvent is called when there comes a new to device event. It is /// already decrypted if necessary. final StreamController onToDeviceEvent = @@ -1255,14 +1249,12 @@ class Client extends MatrixApi { final id = entry.key; final room = entry.value; - final update = RoomUpdate.fromSyncRoomUpdate(room, id); if (database != null) { // TODO: This method seems to be rather slow for some updates // Perhaps don't dynamically build that one query? - await database.storeRoomUpdate(this.id, update, getRoomById(id)); + await database.storeRoomUpdate(this.id, id, room, getRoomById(id)); } - _updateRoomsByRoomUpdate(update); - onRoomUpdate.add(update); + _updateRoomsByRoomUpdate(id, room); /// Handle now all room events and save them in the database if (room is JoinedRoomUpdate) { @@ -1432,49 +1424,58 @@ class Client extends MatrixApi { } } - void _updateRoomsByRoomUpdate(RoomUpdate chatUpdate) { + void _updateRoomsByRoomUpdate(String roomId, SyncRoomUpdate chatUpdate) { // Update the chat list item. // Search the room in the rooms num j = 0; for (j = 0; j < rooms.length; j++) { - if (rooms[j].id == chatUpdate.id) break; + if (rooms[j].id == roomId) break; } - final found = (j < rooms.length && rooms[j].id == chatUpdate.id); - final isLeftRoom = chatUpdate.membership == Membership.leave; + final found = (j < rooms.length && rooms[j].id == roomId); + final membership = chatUpdate is LeftRoomUpdate + ? Membership.leave + : chatUpdate is InvitedRoomUpdate + ? Membership.invite + : Membership.join; // Does the chat already exist in the list rooms? - if (!found && !isLeftRoom) { - final position = chatUpdate.membership == Membership.invite ? 0 : j; + if (!found && membership != Membership.leave) { + final position = membership == Membership.invite ? 0 : j; // Add the new chat to the list - final newRoom = Room( - id: chatUpdate.id, - membership: chatUpdate.membership, - prev_batch: chatUpdate.prev_batch, - highlightCount: chatUpdate.highlight_count, - notificationCount: chatUpdate.notification_count, - summary: chatUpdate.summary, - roomAccountData: {}, - client: this, - ); + final newRoom = chatUpdate is JoinedRoomUpdate + ? Room( + id: roomId, + membership: membership, + prev_batch: chatUpdate.timeline?.prevBatch, + highlightCount: chatUpdate.unreadNotifications?.highlightCount, + notificationCount: + chatUpdate.unreadNotifications?.notificationCount, + summary: chatUpdate.summary, + client: this, + ) + : Room(id: roomId, membership: membership, client: this); rooms.insert(position, newRoom); } // If the membership is "leave" then remove the item and stop here - else if (found && isLeftRoom) { + else if (found && membership == Membership.leave) { rooms.removeAt(j); } // Update notification, highlight count and/or additional informations else if (found && - chatUpdate.membership != Membership.leave && - (rooms[j].membership != chatUpdate.membership || - rooms[j].notificationCount != chatUpdate.notification_count || - rooms[j].highlightCount != chatUpdate.highlight_count || + chatUpdate is JoinedRoomUpdate && + (rooms[j].membership != membership || + rooms[j].notificationCount != + (chatUpdate.unreadNotifications?.notificationCount ?? 0) || + rooms[j].highlightCount != + (chatUpdate.unreadNotifications?.highlightCount ?? 0) || chatUpdate.summary != null || - chatUpdate.prev_batch != null)) { - rooms[j].membership = chatUpdate.membership; - rooms[j].notificationCount = chatUpdate.notification_count; - rooms[j].highlightCount = chatUpdate.highlight_count; - if (chatUpdate.prev_batch != null) { - rooms[j].prev_batch = chatUpdate.prev_batch; + chatUpdate.timeline?.prevBatch != null)) { + rooms[j].membership = membership; + rooms[j].notificationCount = + chatUpdate.unreadNotifications?.notificationCount; + rooms[j].highlightCount = chatUpdate.unreadNotifications?.highlightCount; + if (chatUpdate.timeline?.prevBatch != null) { + rooms[j].prev_batch = chatUpdate.timeline?.prevBatch; } if (chatUpdate.summary != null) { final roomSummaryJson = rooms[j].summary.toJson() @@ -1482,7 +1483,8 @@ class Client extends MatrixApi { rooms[j].summary = RoomSummary.fromJson(roomSummaryJson); } if (rooms[j].onUpdate != null) rooms[j].onUpdate.add(rooms[j].id); - if (chatUpdate.limitedTimeline && requestHistoryOnLimitedTimeline) { + if ((chatUpdate?.timeline?.limited ?? false) && + requestHistoryOnLimitedTimeline) { Logs().v('Limited timeline for ${rooms[j].id} request history now'); runInRoot(rooms[j].requestHistory); } diff --git a/lib/src/database/database_api.dart b/lib/src/database/database_api.dart index 32fc8c01..554ede22 100644 --- a/lib/src/database/database_api.dart +++ b/lib/src/database/database_api.dart @@ -59,7 +59,8 @@ abstract class DatabaseApi { /// Stores a RoomUpdate object in the database. Must be called inside of /// [transaction]. - Future storeRoomUpdate(int clientId, RoomUpdate roomUpdate, + Future storeRoomUpdate( + int clientId, String roomId, SyncRoomUpdate roomUpdate, [Room oldRoom]); /// Stores an EventUpdate object in the database. Must be called inside of diff --git a/lib/src/database/hive_database.dart b/lib/src/database/hive_database.dart index 0ba951c9..b7b255fd 100644 --- a/lib/src/database/hive_database.dart +++ b/lib/src/database/hive_database.dart @@ -990,38 +990,55 @@ class FamedlySdkHiveDatabase extends DatabaseApi { } @override - Future storeRoomUpdate(int clientId, RoomUpdate roomUpdate, + Future storeRoomUpdate( + int clientId, String roomId, SyncRoomUpdate roomUpdate, [dynamic _]) async { // Leave room if membership is leave - if ({Membership.leave, Membership.ban}.contains(roomUpdate.membership)) { - await forgetRoom(clientId, roomUpdate.id); + if (roomUpdate is LeftRoomUpdate) { + await forgetRoom(clientId, roomId); return; } + final membership = roomUpdate is LeftRoomUpdate + ? Membership.leave + : roomUpdate is InvitedRoomUpdate + ? Membership.invite + : Membership.join; // Make sure room exists - if (!_roomsBox.containsKey(roomUpdate.id.toHiveKey)) { + if (!_roomsBox.containsKey(roomId.toHiveKey)) { await _roomsBox.put( - roomUpdate.id.toHiveKey, - Room( - id: roomUpdate.id, - membership: roomUpdate.membership, - highlightCount: roomUpdate.highlight_count?.toInt(), - notificationCount: roomUpdate.notification_count?.toInt(), - prev_batch: roomUpdate.prev_batch, - summary: roomUpdate.summary, - ).toJson()); - } else { - final currentRawRoom = await _roomsBox.get(roomUpdate.id.toHiveKey); + roomId.toHiveKey, + roomUpdate is JoinedRoomUpdate + ? Room( + id: roomId, + membership: membership, + highlightCount: + roomUpdate.unreadNotifications?.highlightCount?.toInt(), + notificationCount: roomUpdate + .unreadNotifications?.notificationCount + ?.toInt(), + prev_batch: roomUpdate.timeline?.prevBatch, + summary: roomUpdate.summary, + ).toJson() + : Room( + id: roomId, + membership: membership, + ).toJson()); + } else if (roomUpdate is JoinedRoomUpdate) { + final currentRawRoom = await _roomsBox.get(roomId.toHiveKey); final currentRoom = Room.fromJson(convertToJson(currentRawRoom)); await _roomsBox.put( - roomUpdate.id.toHiveKey, + roomId.toHiveKey, Room( - id: roomUpdate.id, - membership: roomUpdate.membership ?? currentRoom.membership, - highlightCount: roomUpdate.highlight_count?.toInt() ?? - currentRoom.highlightCount, - notificationCount: roomUpdate.notification_count?.toInt() ?? - currentRoom.notificationCount, - prev_batch: roomUpdate.prev_batch ?? currentRoom.prev_batch, + id: roomId, + membership: membership, + highlightCount: + roomUpdate.unreadNotifications?.highlightCount?.toInt() ?? + currentRoom.highlightCount, + notificationCount: + roomUpdate.unreadNotifications?.notificationCount?.toInt() ?? + currentRoom.notificationCount, + prev_batch: + roomUpdate.timeline?.prevBatch ?? currentRoom.prev_batch, summary: RoomSummary.fromJson(currentRoom.summary.toJson() ..addAll(roomUpdate.summary?.toJson() ?? {})), ).toJson()); @@ -1029,9 +1046,9 @@ class FamedlySdkHiveDatabase extends DatabaseApi { // Is the timeline limited? Then all previous messages should be // removed from the database! - if (roomUpdate.limitedTimeline) { - await _timelineFragmentsBox - .delete(MultiKey(roomUpdate.id, '').toString()); + if (roomUpdate is JoinedRoomUpdate && + roomUpdate.timeline?.limited == true) { + await _timelineFragmentsBox.delete(MultiKey(roomId, '').toString()); } } diff --git a/lib/src/timeline.dart b/lib/src/timeline.dart index 7dde6927..66817dc7 100644 --- a/lib/src/timeline.dart +++ b/lib/src/timeline.dart @@ -23,7 +23,6 @@ import '../matrix.dart'; import 'event.dart'; import 'room.dart'; import 'utils/event_update.dart'; -import 'utils/room_update.dart'; /// Represents the timeline of a room. The callback [onUpdate] will be triggered /// automatically. The initial @@ -39,7 +38,7 @@ class Timeline { final void Function(int insertID) onInsert; StreamSubscription sub; - StreamSubscription roomSub; + StreamSubscription roomSub; StreamSubscription sessionIdReceivedSub; bool isRequestingHistory = false; @@ -107,13 +106,13 @@ class Timeline { Timeline({this.room, List events, this.onUpdate, this.onInsert}) : events = events ?? [] { sub ??= room.client.onEvent.stream.listen(_handleEventUpdate); - // if the timeline is limited we want to clear our events cache - // as r.limitedTimeline can be "null" sometimes, we need to check for == true - // as after receiving a limited timeline room update new events are expected - // to be received via the onEvent stream, it is unneeded to call sortAndUpdate - roomSub ??= room.client.onRoomUpdate.stream - .where((r) => r.id == room.id && r.limitedTimeline == true) - .listen((r) { + // If the timeline is limited we want to clear our events cache + roomSub ??= room.client.onSync.stream + .where((sync) => + sync.rooms?.join != null && + sync.rooms.join.containsKey(room.id) && + sync.rooms.join[room.id]?.timeline?.limited == true) + .listen((_) { events.clear(); aggregatedEvents.clear(); }); diff --git a/lib/src/utils/room_update.dart b/lib/src/utils/room_update.dart deleted file mode 100644 index 9ea86de0..00000000 --- a/lib/src/utils/room_update.dart +++ /dev/null @@ -1,93 +0,0 @@ -// @dart=2.9 -/* - * Famedly Matrix SDK - * Copyright (C) 2019, 2020, 2021 Famedly GmbH - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import '../../matrix.dart'; - -/// Represents a new room or an update for an -/// already known room. -class RoomUpdate { - /// All rooms have an idea in the format: !uniqueid:server.abc - final String id; - - /// The current membership state of the user in this room. - final Membership membership; - - /// Represents the number of unead notifications. This probably doesn't fit the number - /// of unread messages. - final num notification_count; - - // The number of unread highlighted notifications. - final num highlight_count; - - /// If there are too much new messages, the `homeserver` will only send the - /// last X (default is 10) messages and set the `limitedTimeline` flag to true. - final bool limitedTimeline; - - /// Represents the current position of the client in the room history. - final String prev_batch; - - final RoomSummary summary; - - RoomUpdate({ - this.id, - this.membership, - this.notification_count, - this.highlight_count, - this.limitedTimeline, - this.prev_batch, - this.summary, - }); - - factory RoomUpdate.fromSyncRoomUpdate( - SyncRoomUpdate update, - String roomId, - ) => - update is JoinedRoomUpdate - ? RoomUpdate( - id: roomId, - membership: Membership.join, - notification_count: - update.unreadNotifications?.notificationCount ?? 0, - highlight_count: update.unreadNotifications?.highlightCount ?? 0, - limitedTimeline: update.timeline?.limited ?? false, - prev_batch: update.timeline?.prevBatch, - summary: update.summary, - ) - : update is InvitedRoomUpdate - ? RoomUpdate( - id: roomId, - membership: Membership.invite, - notification_count: 0, - highlight_count: 0, - limitedTimeline: false, - prev_batch: null, - summary: null, - ) - : update is LeftRoomUpdate - ? RoomUpdate( - id: roomId, - membership: Membership.leave, - notification_count: 0, - highlight_count: 0, - limitedTimeline: update.timeline?.limited ?? false, - prev_batch: update.timeline?.prevBatch, - summary: null, - ) - : null; -} diff --git a/test/client_test.dart b/test/client_test.dart index 84c70cdd..858f5d21 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -26,7 +26,6 @@ import 'package:matrix/matrix.dart'; import 'package:matrix/src/client.dart'; import 'package:matrix/src/utils/event_update.dart'; import 'package:matrix/src/utils/matrix_file.dart'; -import 'package:matrix/src/utils/room_update.dart'; import 'package:olm/olm.dart' as olm; import 'package:test/test.dart'; import 'package:canonical_json/canonical_json.dart'; @@ -38,7 +37,6 @@ import 'fake_matrix_api.dart'; void main() { Client matrix; - Future> roomUpdateListFuture; Future> eventUpdateListFuture; Future> toDeviceUpdateListFuture; @@ -56,7 +54,6 @@ void main() { matrix = Client('testclient', httpClient: FakeMatrixApi()); - roomUpdateListFuture = matrix.onRoomUpdate.stream.toList(); eventUpdateListFuture = matrix.onEvent.stream.toList(); toDeviceUpdateListFuture = matrix.onToDeviceEvent.stream.toList(); @@ -217,28 +214,6 @@ void main() { expect(loginState, LoginState.loggedOut); }); - test('Room Update Test', () async { - await matrix.onRoomUpdate.close(); - - final roomUpdateList = await roomUpdateListFuture; - - expect(roomUpdateList.length, 4); - - expect(roomUpdateList[0].id == '!726s6s6q:example.com', true); - expect(roomUpdateList[0].membership == Membership.join, true); - expect(roomUpdateList[0].prev_batch == 't34-23535_0_0', true); - expect(roomUpdateList[0].limitedTimeline == true, true); - expect(roomUpdateList[0].notification_count == 2, true); - expect(roomUpdateList[0].highlight_count == 2, true); - - expect(roomUpdateList[1].id == '!696r7674:example.com', true); - expect(roomUpdateList[1].membership == Membership.invite, true); - expect(roomUpdateList[1].prev_batch == null, true); - expect(roomUpdateList[1].limitedTimeline == false, true); - expect(roomUpdateList[1].notification_count == 0, true); - expect(roomUpdateList[1].highlight_count == 0, true); - }); - test('Event Update Test', () async { await matrix.onEvent.close(); @@ -318,7 +293,6 @@ void main() { test('Login', () async { matrix = Client('testclient', httpClient: FakeMatrixApi()); - roomUpdateListFuture = matrix.onRoomUpdate.stream.toList(); eventUpdateListFuture = matrix.onEvent.stream.toList(); await matrix.checkHomeserver('https://fakeserver.notexisting', diff --git a/test/database_api_test.dart b/test/database_api_test.dart index abfb84a8..a06faa44 100644 --- a/test/database_api_test.dart +++ b/test/database_api_test.dart @@ -106,15 +106,13 @@ void testDatabase(Future futureDatabase, int clientId) { expect(file == null, true); }); test('storeRoomUpdate', () async { - await database.storeRoomUpdate( - clientId, - RoomUpdate( - id: '!testroom', - highlight_count: 0, - notification_count: 0, - limitedTimeline: false, - membership: Membership.join, - )); + final roomUpdate = JoinedRoomUpdate.fromJson({ + 'highlight_count': 0, + 'notification_count': 0, + 'limited_timeline': false, + 'membership': Membership.join, + }); + await database.storeRoomUpdate(clientId, '!testroom', roomUpdate); final rooms = await database.getRoomList(Client('testclient')); expect(rooms.single.id, '!testroom'); }); diff --git a/test/timeline_test.dart b/test/timeline_test.dart index 68f042b2..2df20a58 100644 --- a/test/timeline_test.dart +++ b/test/timeline_test.dart @@ -24,7 +24,6 @@ import 'package:matrix/src/client.dart'; import 'package:matrix/src/room.dart'; import 'package:matrix/src/timeline.dart'; import 'package:matrix/src/utils/event_update.dart'; -import 'package:matrix/src/utils/room_update.dart'; import 'package:olm/olm.dart' as olm; import 'fake_client.dart'; @@ -293,14 +292,18 @@ void main() { }); test('Clear cache on limited timeline', () async { - client.onRoomUpdate.add(RoomUpdate( - id: roomID, - membership: Membership.join, - notification_count: 0, - highlight_count: 0, - limitedTimeline: true, - prev_batch: 'blah', - )); + client.onSync.add(SyncUpdate(nextBatch: '1234') + ..rooms = (RoomsUpdate() + ..join = { + roomID: (JoinedRoomUpdate() + ..timeline = (TimelineUpdate() + ..limited = true + ..prevBatch = 'blah') + ..unreadNotifications = UnreadNotificationCounts.fromJson({ + 'highlight_count': 0, + 'notification_count': 0, + })) + })); await Future.delayed(Duration(milliseconds: 50)); expect(timeline.events.isEmpty, true); });