Skip to content

Commit 71d4ad2

Browse files
PIG208PIG208
committed
msglist: Update narrow when propagateMode is changeAll or changeLater.
Navigation change is only intended for TopicNarrow. For StreamNarrow, propagateMode is unused. See also: - zulip/zulip-mobile#5251 - #787 (comment) Fixes: #150 Signed-off-by: Zixuan James Li <[email protected]>
1 parent 1bd1d45 commit 71d4ad2

File tree

7 files changed

+236
-5
lines changed

7 files changed

+236
-5
lines changed

‎lib/model/message.dart

+7
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ class MessageStoreImpl with MessageStore {
152152
final newStreamId = event.newStreamId; // null if topic-only move
153153
final origTopic = event.origTopic;
154154
final newTopic = event.newTopic;
155+
final propagateMode = event.propagateMode;
155156

156157
if (origTopic == null) {
157158
// There was no move.
@@ -178,6 +179,11 @@ class MessageStoreImpl with MessageStore {
178179
assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
179180
return;
180181
}
182+
if (propagateMode == null) {
183+
// The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
184+
assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log)
185+
return;
186+
}
181187

182188
final wasResolveOrUnresolve = (newStreamId == null
183189
&& MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!));
@@ -215,6 +221,7 @@ class MessageStoreImpl with MessageStore {
215221
origTopic: origTopic,
216222
newTopic: newTopic ?? origTopic,
217223
messageIds: event.messageIds,
224+
propagateMode: propagateMode,
218225
);
219226
}
220227
}

‎lib/model/message_list.dart

+16-2
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
359359
}
360360

361361
final PerAccountStore store;
362-
final Narrow narrow;
362+
Narrow narrow;
363363

364364
/// Whether [message] should actually appear in this message list,
365365
/// given that it does belong to the narrow.
@@ -529,6 +529,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
529529
fetchInitial();
530530
}
531531

532+
void _handlePropagateMode(PropagateMode propagateMode, Narrow newNarrow) {
533+
switch (propagateMode) {
534+
case PropagateMode.changeAll:
535+
case PropagateMode.changeLater:
536+
narrow = newNarrow;
537+
_reset();
538+
fetchInitial();
539+
case PropagateMode.changeOne:
540+
}
541+
}
542+
532543
void _messagesMovedFromNarrow(List<int> messageIds) {
533544
if (_removeMessagesById(messageIds)) {
534545
notifyListeners();
@@ -541,6 +552,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
541552
required String origTopic,
542553
required String newTopic,
543554
required List<int> messageIds,
555+
required PropagateMode propagateMode,
544556
}) {
545557
switch (narrow) {
546558
case DmNarrow():
@@ -571,7 +583,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
571583
case (false, false): return;
572584
case (true, true ): return; // TODO(log) no-op move
573585
case (false, true ): _messagesMovedIntoNarrow();
574-
case (true, false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode
586+
case (true, false):
587+
_messagesMovedFromNarrow(messageIds);
588+
_handlePropagateMode(propagateMode, TopicNarrow(newStreamId, newTopic));
575589
}
576590
}
577591
}

‎lib/widgets/message_list.dart

+14-2
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,12 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
232232
narrow = widget.initNarrow;
233233
}
234234

235+
void _narrowChanged(Narrow newNarrow) {
236+
setState(() {
237+
narrow = newNarrow;
238+
});
239+
}
240+
235241
@override
236242
Widget build(BuildContext context) {
237243
final store = PerAccountStoreWidget.of(context);
@@ -289,7 +295,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
289295
removeBottom: ComposeBox.hasComposeBox(narrow),
290296

291297
child: Expanded(
292-
child: MessageList(narrow: narrow))),
298+
child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))),
293299
ComposeBox(controllerKey: _composeBoxKey, narrow: narrow),
294300
]))));
295301
}
@@ -368,9 +374,10 @@ const _kShortMessageHeight = 80;
368374
const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight;
369375

370376
class MessageList extends StatefulWidget {
371-
const MessageList({super.key, required this.narrow});
377+
const MessageList({super.key, required this.narrow, required this.onNarrowChanged});
372378

373379
final Narrow narrow;
380+
final void Function(Narrow newNarrow) onNarrowChanged;
374381

375382
@override
376383
State<StatefulWidget> createState() => _MessageListState();
@@ -407,6 +414,11 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
407414
}
408415

409416
void _modelChanged() {
417+
if (model!.narrow != widget.narrow) {
418+
// A message move event occurred, where propagate mode is
419+
// [PropagateMode.changeAll] or [PropagateMode.changeLater].
420+
widget.onNarrowChanged(model!.narrow);
421+
}
410422
setState(() {
411423
// The actual state lives in the [MessageListView] model.
412424
// This method was called because that just changed.

‎test/example_data.dart

+6-1
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,7 @@ UpdateMessageEvent _updateMessageMoveEvent(
472472
String? origContent,
473473
String? newContent,
474474
required List<MessageFlag> flags,
475+
PropagateMode propagateMode = PropagateMode.changeOne,
475476
}) {
476477
assert(newTopic != origTopic
477478
|| (newStreamId != null && newStreamId != origStreamId));
@@ -486,7 +487,7 @@ UpdateMessageEvent _updateMessageMoveEvent(
486487
editTimestamp: 1234567890, // TODO generate timestamp
487488
origStreamId: origStreamId,
488489
newStreamId: newStreamId,
489-
propagateMode: null,
490+
propagateMode: propagateMode,
490491
origTopic: origTopic,
491492
newTopic: newTopic,
492493
origContent: origContent,
@@ -503,6 +504,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({
503504
int? newStreamId,
504505
String? newTopic,
505506
String? newContent,
507+
PropagateMode propagateMode = PropagateMode.changeOne,
506508
}) {
507509
assert(origMessages.isNotEmpty);
508510
final origMessage = origMessages.first;
@@ -516,6 +518,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({
516518
origContent: origContent,
517519
newContent: newContent,
518520
flags: origMessage.flags,
521+
propagateMode: propagateMode,
519522
);
520523
}
521524

@@ -525,6 +528,7 @@ UpdateMessageEvent updateMessageEventMoveTo({
525528
int? origStreamId,
526529
String? origTopic,
527530
String? origContent,
531+
PropagateMode propagateMode = PropagateMode.changeOne,
528532
}) {
529533
assert(newMessages.isNotEmpty);
530534
final newMessage = newMessages.first;
@@ -542,6 +546,7 @@ UpdateMessageEvent updateMessageEventMoveTo({
542546
origContent: origContent,
543547
newContent: newContent,
544548
flags: newMessage.flags,
549+
propagateMode: propagateMode,
545550
);
546551
}
547552

‎test/flutter_checks.dart

+10
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,14 @@ extension TextChecks on Subject<Text> {
6060
Subject<TextStyle?> get style => has((t) => t.style, 'style');
6161
}
6262

63+
extension TextEditingControllerChecks on Subject<TextEditingController> {
64+
Subject<String?> get text => has((t) => t.text, 'text');
65+
}
66+
6367
extension TextFieldChecks on Subject<TextField> {
6468
Subject<TextCapitalization?> get textCapitalization => has((t) => t.textCapitalization, 'textCapitalization');
69+
Subject<InputDecoration?> get decoration => has((t) => t.decoration, 'decoration');
70+
Subject<TextEditingController?> get controller => has((t) => t.controller, 'controller');
6571
}
6672

6773
extension TextStyleChecks on Subject<TextStyle> {
@@ -131,3 +137,7 @@ extension MaterialChecks on Subject<Material> {
131137
Subject<Color?> get color => has((x) => x.color, 'color');
132138
// TODO more
133139
}
140+
141+
extension InputDecorationChecks on Subject<InputDecoration> {
142+
Subject<String?> get hintText => has((x) => x.hintText, 'hintText');
143+
}

‎test/model/message_list_test.dart

+93
Original file line numberDiff line numberDiff line change
@@ -587,6 +587,37 @@ void main() {
587587
checkHasMessages(initialMessages);
588588
checkNotNotified();
589589
});
590+
591+
void testMessageMove(PropagateMode propagateMode) => awaitFakeAsync((async) async {
592+
await prepareNarrow(narrow, initialMessages + movedMessages);
593+
connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
594+
foundOldest: false,
595+
messages: movedMessages,
596+
).toJson());
597+
await store.handleEvent(eg.updateMessageEventMoveFrom(
598+
origMessages: movedMessages,
599+
newTopic: 'new',
600+
newStreamId: otherStream.streamId,
601+
propagateMode: propagateMode,
602+
));
603+
checkNotifiedOnce();
604+
async.elapse(const Duration(seconds: 1));
605+
checkHasMessages(initialMessages);
606+
check(model).narrow.equals(ChannelNarrow(stream.streamId));
607+
checkNotNotified();
608+
});
609+
610+
test('do not follow when propagateMode = changeOne', () {
611+
testMessageMove(PropagateMode.changeOne);
612+
});
613+
614+
test('do not follow when propagateMode = changeLater', () {
615+
testMessageMove(PropagateMode.changeLater);
616+
});
617+
618+
test('do not follow when propagateMode = changeAll', () {
619+
testMessageMove(PropagateMode.changeAll);
620+
});
590621
});
591622

592623
group('in topic narrow', () {
@@ -674,6 +705,68 @@ void main() {
674705
checkNotNotified();
675706
}));
676707
});
708+
709+
void handleMoveEvent(PropagateMode propagateMode) => awaitFakeAsync((async) async {
710+
await prepareNarrow(narrow, initialMessages + movedMessages);
711+
connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
712+
foundOldest: false,
713+
messages: movedMessages,
714+
).toJson());
715+
await store.handleEvent(eg.updateMessageEventMoveFrom(
716+
origMessages: movedMessages,
717+
newTopic: 'new',
718+
newStreamId: otherStream.streamId,
719+
propagateMode: propagateMode,
720+
));
721+
checkNotifiedOnce();
722+
async.elapse(const Duration(seconds: 1));
723+
});
724+
725+
test('do not follow to the new narrow when propagateMode = changeOne', () {
726+
handleMoveEvent(PropagateMode.changeOne);
727+
checkNotNotified();
728+
checkHasMessages(initialMessages);
729+
check(model).narrow.equals(TopicNarrow(stream.streamId, 'topic'));
730+
});
731+
732+
test('follow to the new narrow when propagateMode = changeLater', () {
733+
handleMoveEvent(PropagateMode.changeLater);
734+
checkNotifiedOnce();
735+
checkHasMessages(movedMessages);
736+
check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new'));
737+
});
738+
739+
test('follow to the new narrow when propagateMode = changeAll', () {
740+
handleMoveEvent(PropagateMode.changeAll);
741+
checkNotifiedOnce();
742+
checkHasMessages(movedMessages);
743+
check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new'));
744+
});
745+
746+
test('handle move event before initial fetch', () => awaitFakeAsync((async) async {
747+
await prepare(narrow: narrow);
748+
final subscription = eg.subscription(stream);
749+
await store.addStream(stream);
750+
await store.addSubscription(subscription);
751+
final followedMessage = eg.streamMessage(stream: stream, topic: 'new');
752+
753+
connection.prepare(delay: const Duration(seconds: 2), json: newestResult(
754+
foundOldest: true,
755+
messages: [followedMessage],
756+
).toJson());
757+
758+
check(model).fetched.isFalse();
759+
checkHasMessages([]);
760+
await store.handleEvent(eg.updateMessageEventMoveTo(
761+
origTopic: 'topic',
762+
newMessages: [followedMessage],
763+
propagateMode: PropagateMode.changeAll,
764+
));
765+
check(model).narrow.equals(TopicNarrow(stream.streamId, 'new'));
766+
767+
async.elapse(const Duration(seconds: 2));
768+
checkHasMessages([followedMessage]);
769+
}));
677770
});
678771

679772
group('fetch races', () {

0 commit comments

Comments
 (0)