Skip to content

Commit b57ca78

Browse files
committed
msglist: Update narrow when propageMode is changeAll or changeLater.
We only implement navigation change for the TopicNarrow. For StreamNarrow, propagateMode is practically unused. See also: zulip/zulip-mobile#5251 Fixes: zulip#150. Signed-off-by: Zixuan James Li <[email protected]>
1 parent b4a3497 commit b57ca78

File tree

7 files changed

+195
-7
lines changed

7 files changed

+195
-7
lines changed

lib/model/message.dart

Lines changed: 7 additions & 0 deletions
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

Lines changed: 24 additions & 4 deletions
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.
@@ -527,8 +527,26 @@ class MessageListView with ChangeNotifier, _MessageSequence {
527527
fetchInitial();
528528
}
529529

530-
void _messagesMovedFromNarrow(List<int> messageIds) {
530+
void _handlePropagateMode(PropagateMode propagateMode, Narrow newNarrow) {
531+
switch (propagateMode) {
532+
case PropagateMode.changeAll:
533+
case PropagateMode.changeLater:
534+
narrow = newNarrow;
535+
_reset();
536+
fetchInitial();
537+
case PropagateMode.changeOne:
538+
}
539+
}
540+
541+
void _messagesMovedFromNarrow(
542+
List<int> messageIds,
543+
PropagateMode propagateMode,
544+
{TopicNarrow? newNarrow}
545+
) {
531546
if (_removeMessagesById(messageIds)) {
547+
if (newNarrow != null) {
548+
_handlePropagateMode(propagateMode, newNarrow);
549+
}
532550
notifyListeners();
533551
}
534552
}
@@ -539,6 +557,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
539557
required String origTopic,
540558
required String newTopic,
541559
required List<int> messageIds,
560+
required PropagateMode propagateMode,
542561
}) {
543562
switch (narrow) {
544563
case DmNarrow():
@@ -558,7 +577,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
558577
case (false, false): return;
559578
case (true, true ): _messagesMovedInternally(messageIds);
560579
case (false, true ): _messagesMovedIntoNarrow();
561-
case (true, false): _messagesMovedFromNarrow(messageIds);
580+
case (true, false): _messagesMovedFromNarrow(messageIds, propagateMode);
562581
}
563582

564583
case TopicNarrow(:final streamId, :final topic):
@@ -568,7 +587,8 @@ class MessageListView with ChangeNotifier, _MessageSequence {
568587
case (false, false): return;
569588
case (true, true ): return; // TODO(log) no-op move
570589
case (false, true ): _messagesMovedIntoNarrow();
571-
case (true, false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode
590+
case (true, false): _messagesMovedFromNarrow(messageIds, propagateMode,
591+
newNarrow: TopicNarrow(newStreamId, newTopic));
572592
}
573593
}
574594
}

lib/widgets/message_list.dart

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,12 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
232232
super.initState();
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);
@@ -290,7 +296,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
290296
removeBottom: narrow is! CombinedFeedNarrow,
291297

292298
child: Expanded(
293-
child: MessageList(narrow: narrow))),
299+
child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))),
294300
ComposeBox(controllerKey: _composeBoxKey, narrow: narrow),
295301
]))));
296302
}
@@ -366,9 +372,10 @@ const _kShortMessageHeight = 80;
366372
const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight;
367373

368374
class MessageList extends StatefulWidget {
369-
const MessageList({super.key, required this.narrow});
375+
const MessageList({super.key, required this.narrow, required this.onNarrowChanged});
370376

371377
final Narrow narrow;
378+
final void Function(Narrow newNarrow) onNarrowChanged;
372379

373380
@override
374381
State<StatefulWidget> createState() => _MessageListState();
@@ -405,6 +412,11 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
405412
}
406413

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

test/example_data.dart

Lines changed: 4 additions & 1 deletion
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

test/flutter_checks.dart

Lines changed: 10 additions & 0 deletions
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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,33 @@ void main() {
563563
checkHasMessages(initialMessages);
564564
checkNotifiedOnce();
565565
});
566+
567+
void testMessageMove(PropagateMode propagateMode) => awaitFakeAsync((async) async {
568+
await prepareNarrow(narrow, initialMessages + movedMessages);
569+
connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
570+
foundOldest: false,
571+
messages: movedMessages,
572+
).toJson());
573+
await store.handleEvent(eg.updateMessageEventMoveFrom(
574+
origMessages: movedMessages,
575+
newTopic: 'new',
576+
newStreamId: otherStream.streamId,
577+
propagateMode: propagateMode,
578+
));
579+
checkNotifiedOnce();
580+
async.elapse(const Duration(seconds: 1));
581+
checkHasMessages(initialMessages);
582+
check(model).narrow.equals(ChannelNarrow(stream.streamId));
583+
checkNotNotified();
584+
});
585+
586+
test('do not follow when propagateMode = changeLater', () {
587+
testMessageMove(PropagateMode.changeLater);
588+
});
589+
590+
test('do not follow when propagateMode = changeAll', () {
591+
testMessageMove(PropagateMode.changeAll);
592+
});
566593
});
567594

568595
group('in topic narrow', () {
@@ -600,6 +627,34 @@ void main() {
600627
checkHasMessages(initialMessages);
601628
checkNotifiedOnce();
602629
});
630+
631+
void testMessageMove(PropagateMode propagateMode) => awaitFakeAsync((async) async {
632+
await prepareNarrow(narrow, initialMessages + movedMessages);
633+
connection.prepare(delay: const Duration(seconds: 1), json: newestResult(
634+
foundOldest: false,
635+
messages: movedMessages,
636+
).toJson());
637+
await store.handleEvent(eg.updateMessageEventMoveFrom(
638+
origMessages: movedMessages,
639+
newTopic: 'new',
640+
newStreamId: otherStream.streamId,
641+
propagateMode: propagateMode,
642+
));
643+
checkNotifiedOnce();
644+
async.elapse(const Duration(seconds: 1));
645+
checkHasMessages(movedMessages);
646+
check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new'));
647+
checkNotifiedOnce();
648+
});
649+
650+
test('follow to the new narrow when propagateMode = changeLater', () {
651+
testMessageMove(PropagateMode.changeLater);
652+
});
653+
654+
655+
test('follow to the new narrow when propagateMode = changeAll', () {
656+
testMessageMove(PropagateMode.changeAll);
657+
});
603658
});
604659

605660
group('fetch races', () {

test/widgets/message_list_test.dart

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import 'package:zulip/api/route/messages.dart';
1414
import 'package:zulip/model/localizations.dart';
1515
import 'package:zulip/model/narrow.dart';
1616
import 'package:zulip/model/store.dart';
17+
import 'package:zulip/widgets/autocomplete.dart';
1718
import 'package:zulip/widgets/content.dart';
1819
import 'package:zulip/widgets/emoji_reaction.dart';
1920
import 'package:zulip/widgets/icons.dart';
@@ -605,6 +606,86 @@ void main() {
605606
expectedMessage: 'NetworkException: Oops (ClientException: Oops)');
606607
});
607608
});
609+
610+
group('Update Narrow on message move', () {
611+
final channel = eg.stream(name: 'move test stream');
612+
final message = eg.streamMessage(stream: channel, content: 'Message to move');
613+
final narrow = TopicNarrow.ofMessage(message);
614+
615+
void prepareGetMessageResponse(List<Message> messages) {
616+
connection.prepare(json: newestResult(
617+
foundOldest: false, messages: messages).toJson());
618+
}
619+
620+
void handleMessageMoveEvent(List<StreamMessage> messages, String newTopic) {
621+
store.handleEvent(eg.updateMessageEventMoveFrom(
622+
origMessages: messages,
623+
newTopic: newTopic,
624+
propagateMode: PropagateMode.changeAll));
625+
}
626+
627+
testWidgets('compose box send message after move', (WidgetTester tester) async {
628+
await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]);
629+
630+
final channelContentInputFinder = find.descendant(
631+
of: find.byType(ComposeAutocomplete),
632+
matching: find.byType(TextField));
633+
634+
await tester.enterText(channelContentInputFinder, 'Some text');
635+
prepareGetMessageResponse([message]);
636+
handleMessageMoveEvent([message], 'new topic');
637+
await tester.pump(const Duration(seconds: 1));
638+
check(tester.widget<TextField>(channelContentInputFinder))
639+
..decoration.isNotNull().hintText.equals('Message #${channel.name} > new topic')
640+
..controller.isNotNull().text.equals('Some text');
641+
642+
connection.prepare(json: SendMessageResult(id: 1).toJson());
643+
await tester.tap(find.byIcon(Icons.send));
644+
await tester.pump();
645+
check(connection.lastRequest).isA<http.Request>()
646+
..method.equals('POST')
647+
..url.path.equals('/api/v1/messages')
648+
..bodyFields.deepEquals({
649+
'type': 'stream',
650+
'to': '${message.streamId}',
651+
'topic': 'new topic',
652+
'content': 'Some text',
653+
'read_by_sender': 'true'});
654+
await tester.pumpAndSettle();
655+
});
656+
657+
testWidgets('Move to narrow with existing messages', (WidgetTester tester) async {
658+
await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]);
659+
check(find.textContaining('Existing message').evaluate()).length.equals(0);
660+
check(find.textContaining('Message to move').evaluate()).length.equals(1);
661+
662+
final existingMessage = eg.streamMessage(
663+
stream: eg.stream(), topic: 'new topic', content: 'Existing message');
664+
prepareGetMessageResponse([existingMessage, message]);
665+
handleMessageMoveEvent([message], 'new topic');
666+
await tester.pump(const Duration(seconds: 1));
667+
668+
check(find.textContaining('Existing message').evaluate()).length.equals(1);
669+
check(find.textContaining('Message to move').evaluate()).length.equals(1);
670+
});
671+
672+
testWidgets('show new topic in TopicNarrow after move', (tester) async {
673+
await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]);
674+
675+
prepareGetMessageResponse([message]);
676+
handleMessageMoveEvent([message], 'new topic');
677+
await tester.pump(const Duration(seconds: 1));
678+
679+
check(find.descendant(
680+
of: find.byType(RecipientHeader),
681+
matching: find.text('new topic')).evaluate()
682+
).length.equals(1);
683+
check(find.descendant(
684+
of: find.byType(MessageListAppBarTitle),
685+
matching: find.text('${channel.name} > new topic')).evaluate()
686+
).length.equals(1);
687+
});
688+
});
608689
});
609690

610691
group('recipient headers', () {

0 commit comments

Comments
 (0)