From b5ac5001367736dc5b8e42e57a946e8a4525f3be Mon Sep 17 00:00:00 2001 From: Sorunome Date: Thu, 10 Sep 2020 12:11:13 +0200 Subject: [PATCH] fix: Handle failed to send messages in low network significantly better --- lib/src/database/database.dart | 21 ++++++++++---- lib/src/database/database.g.dart | 4 +-- lib/src/database/database.moor | 2 +- lib/src/timeline.dart | 11 +++++++- test/timeline_test.dart | 48 ++++++++++++++++++++++++++++---- 5 files changed, 70 insertions(+), 16 deletions(-) diff --git a/lib/src/database/database.dart b/lib/src/database/database.dart index 4da4f2d..3998af9 100644 --- a/lib/src/database/database.dart +++ b/lib/src/database/database.dart @@ -399,7 +399,7 @@ class Database extends _$Database { // Is the timeline limited? Then all previous messages should be // removed from the database! if (roomUpdate.limitedTimeline) { - await removeRoomEvents(clientId, roomUpdate.id); + await removeSuccessfulRoomEvents(clientId, roomUpdate.id); await updateRoomSortOrder(0.0, 0.0, clientId, roomUpdate.id); await setRoomPrevBatch(roomUpdate.prev_batch, clientId, roomUpdate.id); } @@ -432,9 +432,10 @@ class Database extends _$Database { status = eventContent['unsigned'][MessageSendingStatusKey]; } if (eventContent['status'] is num) status = eventContent['status']; - if ((status == 1 || status == -1) && + var storeNewEvent = !((status == 1 || status == -1) && eventContent['unsigned'] is Map && - eventContent['unsigned']['transaction_id'] is String) { + eventContent['unsigned']['transaction_id'] is String); + if (!storeNewEvent) { final allOldEvents = await getEvent(clientId, eventContent['event_id'], chatId).get(); if (allOldEvents.isNotEmpty) { @@ -452,8 +453,15 @@ class Database extends _$Database { } else { // status changed and we have an old transaction id --> update event id and stuffs try { - await updateEventStatus(status, eventContent['event_id'], clientId, - eventContent['unsigned']['transaction_id'], chatId); + final updated = await updateEventStatus( + status, + eventContent['event_id'], + clientId, + eventContent['unsigned']['transaction_id'], + chatId); + if (updated == 0) { + storeNewEvent = true; + } } catch (err) { // we could not update the transaction id to the event id....so it already exists // as we just tried to fetch the event previously this is a race condition if the event comes down sync in the mean time @@ -461,7 +469,8 @@ class Database extends _$Database { // than our status. So, we just ignore this error } } - } else { + } + if (storeNewEvent) { DbEvent oldEvent; if (type == 'history') { final allOldEvents = diff --git a/lib/src/database/database.g.dart b/lib/src/database/database.g.dart index 486d2a0..103bd86 100644 --- a/lib/src/database/database.g.dart +++ b/lib/src/database/database.g.dart @@ -6479,9 +6479,9 @@ abstract class _$Database extends GeneratedDatabase { ); } - Future removeRoomEvents(int client_id, String room_id) { + Future removeSuccessfulRoomEvents(int client_id, String room_id) { return customUpdate( - 'DELETE FROM events WHERE client_id = :client_id AND room_id = :room_id', + 'DELETE FROM events WHERE client_id = :client_id AND room_id = :room_id AND status <> -1 AND status <> 0', variables: [Variable.withInt(client_id), Variable.withString(room_id)], updates: {events}, updateKind: UpdateKind.delete, diff --git a/lib/src/database/database.moor b/lib/src/database/database.moor index 9f00534..17221d4 100644 --- a/lib/src/database/database.moor +++ b/lib/src/database/database.moor @@ -230,7 +230,7 @@ getRoom: SELECT * FROM rooms WHERE client_id = :client_id AND room_id = :room_id getEvent: SELECT * FROM events WHERE client_id = :client_id AND event_id = :event_id AND room_id = :room_id; removeEvent: DELETE FROM events WHERE client_id = :client_id AND event_id = :event_id AND room_id = :room_id; removeRoom: DELETE FROM rooms WHERE client_id = :client_id AND room_id = :room_id; -removeRoomEvents: DELETE FROM events WHERE client_id = :client_id AND room_id = :room_id; +removeSuccessfulRoomEvents: DELETE FROM events WHERE client_id = :client_id AND room_id = :room_id AND status <> -1 AND status <> 0; storeFile: INSERT OR REPLACE INTO files (mxc_uri, bytes, saved_at) VALUES (:mxc_uri, :bytes, :time); dbGetFile: SELECT * FROM files WHERE mxc_uri = :mxc_uri; markPendingEventsAsError: UPDATE events SET status = -1 WHERE client_id = :client_id AND status = 0; diff --git a/lib/src/timeline.dart b/lib/src/timeline.dart index df7e091..1ec96aa 100644 --- a/lib/src/timeline.dart +++ b/lib/src/timeline.dart @@ -99,6 +99,7 @@ class Timeline { for (final e in events) { addAggregatedEvent(e); } + _sort(); } /// Don't forget to call this before you dismiss this object! @@ -274,7 +275,15 @@ class Timeline { void _sort() { if (_sortLock || events.length < 2) return; _sortLock = true; - events?.sort((a, b) => b.sortOrder - a.sortOrder > 0 ? 1 : -1); + events?.sort((a, b) { + if (b.status == -1 && a.status != -1) { + return 1; + } + if (a.status == -1 && b.status != -1) { + return -1; + } + return b.sortOrder - a.sortOrder > 0 ? 1 : -1; + }); _sortLock = false; } } diff --git a/test/timeline_test.dart b/test/timeline_test.dart index 5b308f0..3ce1a09 100644 --- a/test/timeline_test.dart +++ b/test/timeline_test.dart @@ -218,6 +218,7 @@ void main() { }); test('Resend message', () async { + timeline.events.clear(); client.onEvent.add(EventUpdate( type: 'timeline', roomID: roomID, @@ -233,6 +234,7 @@ void main() { }, sortOrder: room.newSortOrder)); await Future.delayed(Duration(milliseconds: 50)); + expect(timeline.events[0].status, -1); await timeline.events[0].sendAgain(); await Future.delayed(Duration(milliseconds: 50)); @@ -240,22 +242,23 @@ void main() { expect(updateCount, 17); expect(insertList, [0, 0, 0, 0, 0, 0, 0, 0]); - expect(timeline.events.length, 7); + expect(timeline.events.length, 1); expect(timeline.events[0].status, 1); }); test('Request history', () async { + timeline.events.clear(); await room.requestHistory(); await Future.delayed(Duration(milliseconds: 50)); expect(updateCount, 20); - expect(timeline.events.length, 10); - expect(timeline.events[7].eventId, '3143273582443PhrSn:example.org'); - expect(timeline.events[8].eventId, '2143273582443PhrSn:example.org'); - expect(timeline.events[9].eventId, '1143273582443PhrSn:example.org'); + expect(timeline.events.length, 3); + expect(timeline.events[0].eventId, '3143273582443PhrSn:example.org'); + expect(timeline.events[1].eventId, '2143273582443PhrSn:example.org'); + expect(timeline.events[2].eventId, '1143273582443PhrSn:example.org'); expect(room.prev_batch, 't47409-4357353_219380_26003_2265'); - await timeline.events[9].redact(reason: 'test', txid: '1234'); + await timeline.events[2].redact(reason: 'test', txid: '1234'); }); test('Clear cache on limited timeline', () async { @@ -271,6 +274,39 @@ void main() { expect(timeline.events.isEmpty, true); }); + test('sort errors on top', () async { + timeline.events.clear(); + client.onEvent.add(EventUpdate( + type: 'timeline', + roomID: roomID, + eventType: 'm.room.message', + content: { + 'type': 'm.room.message', + 'content': {'msgtype': 'm.text', 'body': 'Testcase'}, + 'sender': '@alice:example.com', + 'status': -1, + 'event_id': 'abc', + 'origin_server_ts': testTimeStamp + }, + sortOrder: room.newSortOrder)); + client.onEvent.add(EventUpdate( + type: 'timeline', + roomID: roomID, + eventType: 'm.room.message', + content: { + 'type': 'm.room.message', + 'content': {'msgtype': 'm.text', 'body': 'Testcase'}, + 'sender': '@alice:example.com', + 'status': 2, + 'event_id': 'def', + 'origin_server_ts': testTimeStamp + 5 + }, + sortOrder: room.newSortOrder)); + await Future.delayed(Duration(milliseconds: 50)); + expect(timeline.events[0].status, -1); + expect(timeline.events[1].status, 2); + }); + test('sending event to failed update', () async { timeline.events.clear(); client.onEvent.add(EventUpdate(