From a29e0ff0f9f09f2eb6adfa86e673c13fa24ce58f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 3 Jul 2024 15:57:50 -0700 Subject: [PATCH 1/2] message [nfc]: Pull out orig/new values from move as local variables This lets the Dart compiler see that these are definitely constant through the function (which it can't assume when they're getters on an object). That in turn lets it narrow the types based on our null checks, so that we can leave out `!` operators later. That's small now, but will be more useful as we add more logic to this function -- if we had to scatter `!` all around that logic, we'd have to wonder whether some of them were unjustified assumptions and might throw at runtime. Also rename one local to "newStreamId", matching the property it mirrors. We'll rename both the variable and the property together, as part of a sweep coming up soon (#631). --- lib/model/message.dart | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 7ad0fe3ce2..e58049710e 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -148,11 +148,16 @@ class MessageStoreImpl with MessageStore { // The interaction between the fields of these events are a bit tricky. // For reference, see: https://zulip.com/api/get-events#update_message - if (event.origTopic == null) { + final origStreamId = event.origStreamId; + final newStreamId = event.newStreamId; // null if topic-only move + final origTopic = event.origTopic; + final newTopic = event.newTopic; + + if (origTopic == null) { // There was no move. assert(() { - if (event.newStreamId != null && event.origStreamId != null - && event.newStreamId != event.origStreamId) { + if (newStreamId != null && origStreamId != null + && newStreamId != origStreamId) { // This should be impossible; `orig_subject` (aka origTopic) is // documented to be present when either the stream or topic changed. debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log) @@ -162,22 +167,19 @@ class MessageStoreImpl with MessageStore { return; } - if (event.newTopic == null) { + if (newTopic == null) { // The `subject` field (aka newTopic) is documented to be present on moves. assert(debugLog('Malformed UpdateMessageEvent: move but no newTopic')); // TODO(log) return; } - if (event.origStreamId == null) { + if (origStreamId == null) { // The `stream_id` field (aka origStreamId) is documented to be present on moves. assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log) return; } - final newTopic = event.newTopic!; - final newChannelId = event.newStreamId; // null if topic-only move - - if (newChannelId == null - && MessageEditState.topicMoveWasResolveOrUnresolve(event.origTopic!, newTopic)) { + if (newStreamId == null + && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic)) { // The topic was only resolved/unresolved. // No change to the messages' editState. return; From d8f2f89ae3e885f86d2bc7ef168bfeed86b64226 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 3 Jul 2024 16:42:22 -0700 Subject: [PATCH 2/2] msglist [nfc]: Reorder methods to group mutators together This messageContentChanged method goes logically next to the various event-handling methods, rather than separated from them by notifyListeners and its siblings. --- lib/model/message_list.dart | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index e0f59d99c7..4f555f7124 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -474,6 +474,17 @@ class MessageListView with ChangeNotifier, _MessageSequence { notifyListeners(); } + /// Update data derived from the content of the given message. + /// + /// This does not notify listeners. + /// The caller should ensure that happens later. + void messageContentChanged(int messageId) { + final index = _findMessageWithId(messageId); + if (index != -1) { + _reparseContent(index); + } + } + // Repeal the `@protected` annotation that applies on the base implementation, // so we can call this method from [MessageStoreImpl]. @override @@ -497,17 +508,6 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } - /// Update data derived from the content of the given message. - /// - /// This does not notify listeners. - /// The caller should ensure that happens later. - void messageContentChanged(int messageId) { - final index = _findMessageWithId(messageId); - if (index != -1) { - _reparseContent(index); - } - } - /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo from scratch any computations we can, such as parsing