From 5e011a8848df0fbc83b591c18ceb03bfec61cf55 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 25 Nov 2024 14:06:54 -0800 Subject: [PATCH 01/24] compose [nfc]: Move "there is none" logic out of ComposeBox widget It's a little confusing to see a widget named ComposeBox returning something that's neither a compose box nor a substitute/placeholder for one, but something (SizedBox.shrink) that's meant to show nothing at all. So, pull that logic out to the caller, which nicely puts it near some related logic (the `hasComposeBox` condition for a MediaQuery.removePadding). --- lib/widgets/compose_box.dart | 4 +++- lib/widgets/message_list.dart | 4 +++- test/widgets/action_sheet_test.dart | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index a22e4f7c78..16daac4224 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1232,7 +1232,8 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox } class ComposeBox extends StatelessWidget { - const ComposeBox({super.key, this.controllerKey, required this.narrow}); + ComposeBox({super.key, this.controllerKey, required this.narrow}) + : assert(ComposeBox.hasComposeBox(narrow)); final GlobalKey? controllerKey; final Narrow narrow; @@ -1296,6 +1297,7 @@ class ComposeBox extends StatelessWidget { case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): + assert(false); return const SizedBox.shrink(); } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b8fcf3adbe..0ac1681172 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -301,7 +301,9 @@ class _MessageListPageState extends State implements MessageLis child: Expanded( child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), - ComposeBox(controllerKey: _composeBoxKey, narrow: narrow), + if (ComposeBox.hasComposeBox(narrow)) + ComposeBox(controllerKey: _composeBoxKey, narrow: narrow) + else const SizedBox.shrink(), ])))); } } diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 3dade81b48..c15c5ac1b5 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -224,8 +224,8 @@ void main() { group('QuoteAndReplyButton', () { ComposeBoxController? findComposeBoxController(WidgetTester tester) { - return tester.widget(find.byType(ComposeBox)) - .controllerKey?.currentState; + return tester.widgetList(find.byType(ComposeBox)) + .singleOrNull?.controllerKey?.currentState; } Widget? findQuoteAndReplyButton(WidgetTester tester) { From 39781c8137b52250ce533c37b488bcd78d659008 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 25 Nov 2024 14:12:31 -0800 Subject: [PATCH 02/24] msglist [nfc]: Remove no-op SizedBox.shrink as a Column child --- lib/widgets/message_list.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 0ac1681172..c6803f2484 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -303,7 +303,6 @@ class _MessageListPageState extends State implements MessageLis child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), if (ComposeBox.hasComposeBox(narrow)) ComposeBox(controllerKey: _composeBoxKey, narrow: narrow) - else const SizedBox.shrink(), ])))); } } From ba6424f5ddd0b7a87e81c6cc294f28f62d28b0a2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 15:51:41 -0800 Subject: [PATCH 03/24] action_sheet test [nfc]: Add groups for 'in {topic,DM} narrow' tests Each of these tests will get a new related test, coming up. --- test/widgets/action_sheet_test.dart | 64 +++++++++++++++-------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index c15c5ac1b5..d6582ed52f 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -296,39 +296,43 @@ void main() { valueBefore: valueBefore, message: message, rawContent: 'Hello world'); }); - testWidgets('in topic narrow', (tester) async { - final message = eg.streamMessage(); - await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); - - final composeBoxController = findComposeBoxController(tester)!; - final contentController = composeBoxController.contentController; + group('in topic narrow', () { + testWidgets('smoke', (tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); - final valueBefore = contentController.value; - prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); - await tapQuoteAndReplyButton(tester); - checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); - await tester.pump(Duration.zero); // message is fetched; compose box updates - check(composeBoxController.contentFocusNode.hasFocus).isTrue(); - checkSuccessState(store, contentController, - valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value; + prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // message is fetched; compose box updates + check(composeBoxController.contentFocusNode.hasFocus).isTrue(); + checkSuccessState(store, contentController, + valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + }); }); - testWidgets('in DM narrow', (tester) async { - final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); - await setupToMessageActionSheet(tester, - message: message, narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); - - final composeBoxController = findComposeBoxController(tester)!; - final contentController = composeBoxController.contentController; - - final valueBefore = contentController.value; - prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); - await tapQuoteAndReplyButton(tester); - checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); - await tester.pump(Duration.zero); // message is fetched; compose box updates - check(composeBoxController.contentFocusNode.hasFocus).isTrue(); - checkSuccessState(store, contentController, - valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + group('in DM narrow', () { + testWidgets('smoke', (tester) async { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + await setupToMessageActionSheet(tester, + message: message, narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); + + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value; + prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // message is fetched; compose box updates + check(composeBoxController.contentFocusNode.hasFocus).isTrue(); + checkSuccessState(store, contentController, + valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + }); }); testWidgets('request has an error', (tester) async { From 060208f889f3ecf81b41460947575e73e1224454 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 12:11:29 -0800 Subject: [PATCH 04/24] action_sheet: Fix rare null-check error on quote-and-reply In 052f2036e it became possible for the compose box to disappear, which would null out MessageListPageState._composeBoxKey.currentState. Anyway, a small and rare bug, but easy to fix. --- lib/widgets/action_sheet.dart | 13 +++++---- test/widgets/action_sheet_test.dart | 45 ++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index c271df6bfe..be35bd0a31 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -327,8 +327,10 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { // This will be null only if the compose box disappeared after the // message action sheet opened, and before "Quote and reply" was pressed. - // Currently a compose box can't ever disappear, so this is impossible. - var composeBoxController = findMessageListPage().composeBoxController!; + // This is rare: it happens when the self-user loses posting permission + // or when a DM recipient becomes deactivated. + var composeBoxController = findMessageListPage().composeBoxController; + if (composeBoxController == null) return; final topicController = composeBoxController.topicController; if ( topicController != null @@ -354,9 +356,10 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { if (!pageContext.mounted) return; // This will be null only if the compose box disappeared during the - // quotation request. Currently a compose box can't ever disappear, - // so this is impossible. - composeBoxController = findMessageListPage().composeBoxController!; + // quotation request. This is rare: it happens when the self-user loses + // posting permission or when a DM recipient becomes deactivated. + composeBoxController = findMessageListPage().composeBoxController; + if (composeBoxController == null) return; composeBoxController.contentController .registerQuoteAndReplyEnd(PerAccountStoreWidget.of(pageContext), tag, message: message, diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index d6582ed52f..719a1f18d6 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -43,10 +43,16 @@ Future setupToMessageActionSheet(WidgetTester tester, { required Narrow narrow, }) async { addTearDown(testBinding.reset); + assert(narrow.containsMessage(message)); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); store = await testBinding.globalStore.perAccount(eg.selfAccount.id); - await store.addUsers([eg.selfUser, eg.user(userId: message.senderId)]); + await store.addUsers([ + eg.selfUser, + eg.user(userId: message.senderId), + if (narrow is DmNarrow) + ...narrow.otherRecipientIds.map((id) => eg.user(userId: id)), + ]); if (message is StreamMessage) { final stream = eg.stream(streamId: message.streamId); await store.addStream(stream); @@ -313,6 +319,22 @@ void main() { checkSuccessState(store, contentController, valueBefore: valueBefore, message: message, rawContent: 'Hello world'); }); + + testWidgets('no error if user lost posting permission after action sheet opened', (tester) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: eg.selfUser.userId, + role: UserRole.guest)); + await store.handleEvent(eg.channelUpdateEvent(stream, + property: ChannelPropertyName.channelPostPolicy, + value: ChannelPostPolicy.administrators)); + await tester.pump(); + + await tapQuoteAndReplyButton(tester); + // no error + }); }); group('in DM narrow', () { @@ -333,6 +355,27 @@ void main() { checkSuccessState(store, contentController, valueBefore: valueBefore, message: message, rawContent: 'Hello world'); }); + + testWidgets('no error if recipient was deactivated while raw-content request in progress', (tester) async { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + await setupToMessageActionSheet(tester, + message: message, + narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); + + prepareRawContentResponseSuccess( + message: message, + rawContent: 'Hello world', + delay: const Duration(seconds: 5), + ); + await tapQuoteAndReplyButton(tester); + await tester.pump(const Duration(seconds: 1)); // message not yet fetched + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: eg.otherUser.userId, + isActive: false)); + await tester.pump(); + // no error + await tester.pump(const Duration(seconds: 4)); + }); }); testWidgets('request has an error', (tester) async { From 35c966ec7801290e5e258a27cbe206705b71b3d1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 13:59:01 -0800 Subject: [PATCH 05/24] compose test [nfc]: Make controllerKey a shared `late` variable --- test/widgets/compose_box_test.dart | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index a19356ef40..f611357cb3 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -38,11 +38,12 @@ void main() { late PerAccountStore store; late FakeApiConnection connection; + late GlobalKey controllerKey; final contentInputFinder = find.byWidgetPredicate( (widget) => widget is TextField && widget.controller is ComposeContentController); - Future> prepareComposeBox(WidgetTester tester, { + Future prepareComposeBox(WidgetTester tester, { required Narrow narrow, User? selfUser, int? realmWaitingPeriodThreshold, @@ -65,7 +66,7 @@ void main() { await store.addStreams(streams); connection = store.connection as FakeApiConnection; - final controllerKey = GlobalKey(); + controllerKey = GlobalKey(); await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, child: Column( // This positions the compose box at the bottom of the screen, @@ -75,8 +76,6 @@ void main() { ComposeBox(controllerKey: controllerKey, narrow: narrow), ]))); await tester.pumpAndSettle(); - - return controllerKey; } Future enterTopic(WidgetTester tester, { @@ -198,7 +197,6 @@ void main() { group('ComposeBox textCapitalization', () { void checkComposeBoxTextFields(WidgetTester tester, { - required GlobalKey controllerKey, required bool expectTopicTextField, }) { final composeBoxController = controllerKey.currentState!; @@ -222,18 +220,16 @@ void main() { testWidgets('_StreamComposeBox', (tester) async { final channel = eg.stream(); - final key = await prepareComposeBox(tester, + await prepareComposeBox(tester, narrow: ChannelNarrow(channel.streamId), streams: [channel]); - checkComposeBoxTextFields(tester, controllerKey: key, - expectTopicTextField: true); + checkComposeBoxTextFields(tester, expectTopicTextField: true); }); testWidgets('_FixedDestinationComposeBox', (tester) async { final channel = eg.stream(); - final key = await prepareComposeBox(tester, + await prepareComposeBox(tester, narrow: TopicNarrow(channel.streamId, 'topic'), streams: [channel]); - checkComposeBoxTextFields(tester, controllerKey: key, - expectTopicTextField: false); + checkComposeBoxTextFields(tester, expectTopicTextField: false); }); }); @@ -354,7 +350,7 @@ void main() { }); testWidgets('selection change sends a "typing started" notice', (tester) async { - final controllerKey = await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, narrow: narrow, streams: [channel]); final composeBoxController = controllerKey.currentState!; await checkStartTyping(tester, narrow); @@ -474,8 +470,7 @@ void main() { final channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); - final controllerKey = await prepareComposeBox(tester, - narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, narrow: narrow, streams: [channel]); final composeBoxController = controllerKey.currentState!; // (When we check that the send button looks disabled, it should be because @@ -536,8 +531,7 @@ void main() { final channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); - final controllerKey = await prepareComposeBox(tester, - narrow: narrow, streams: [channel]); + await prepareComposeBox(tester, narrow: narrow, streams: [channel]); final composeBoxController = controllerKey.currentState!; // (When we check that the send button looks disabled, it should be because From b5e37d6ed6e7f65ed42952aedce98a57864ad10a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 3 Dec 2024 14:24:22 -0800 Subject: [PATCH 06/24] compose: Dispose topic-input's focus node when stream compose box disposed Thanks Greg for catching this: https://github.com/zulip/zulip-flutter/pull/1090#discussion_r1868380295 --- lib/widgets/compose_box.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 16daac4224..5d3a379dab 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1137,6 +1137,7 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> implements Compose _topicController.dispose(); _contentController.dispose(); _contentFocusNode.dispose(); + _topicFocusNode.dispose(); super.dispose(); } From 1e45ca276f64a2b622af04c8381463ddc52088ab Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 25 Nov 2024 19:05:13 -0800 Subject: [PATCH 07/24] compose [nfc]: Make state "has-a" instead of "is-a" ComposeBoxController We've been using ComposeBoxController as an interface, implemented by _StreamComposeBoxState and _FixedDestinationComposeBoxState. This commit puts the relevant implementation in ComposeBoxController itself -- really in subclasses StreamComposeBoxController and FixedDestinationComposeBoxController -- and makes those _FooState classes instantiate it and store the instance in a field. Next, we'll move the field up from those _FooState classes onto ComposeBox's state to make the controller straightforwardly available there (i.e. available without going through GlobalKey logic). That will be helpful for #720 when the controller grows a send-in-progress flag; ComposeBox will be a conveniently central place to build a progress indicator for that state. --- lib/widgets/action_sheet.dart | 8 +- lib/widgets/compose_box.dart | 142 +++++++++++++++++----------- lib/widgets/message_list.dart | 6 +- test/widgets/action_sheet_test.dart | 8 +- test/widgets/compose_box_test.dart | 38 ++++---- 5 files changed, 118 insertions(+), 84 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index be35bd0a31..ba3ad34d2b 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -14,6 +14,7 @@ import '../model/narrow.dart'; import 'actions.dart'; import 'clipboard.dart'; import 'color.dart'; +import 'compose_box.dart'; import 'dialog.dart'; import 'icons.dart'; import 'inset_shadow.dart'; @@ -331,13 +332,12 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { // or when a DM recipient becomes deactivated. var composeBoxController = findMessageListPage().composeBoxController; if (composeBoxController == null) return; - final topicController = composeBoxController.topicController; if ( - topicController != null - && topicController.textNormalized == kNoTopicTopic + composeBoxController is StreamComposeBoxController + && composeBoxController.topicController.textNormalized == kNoTopicTopic && message is StreamMessage ) { - topicController.value = TextEditingValue(text: message.topic); + composeBoxController.topicController.value = TextEditingValue(text: message.topic); } // This inserts a "[Quoting…]" placeholder into the content input, diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 5d3a379dab..15a3c7448e 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1099,12 +1099,37 @@ class _ComposeBoxLayout extends StatelessWidget { } } -abstract class ComposeBoxController extends State { - ComposeTopicController? get topicController; - ComposeContentController get contentController; - FocusNode get contentFocusNode; +sealed class ComposeBoxController { + ComposeContentController get contentController => _contentController; + final _contentController = ComposeContentController(); + + FocusNode get contentFocusNode => _contentFocusNode; + final _contentFocusNode = FocusNode(); + + @mustCallSuper + void dispose() { + _contentController.dispose(); + _contentFocusNode.dispose(); + } } +class StreamComposeBoxController extends ComposeBoxController { + ComposeTopicController get topicController => _topicController; + final _topicController = ComposeTopicController(); + + FocusNode get topicFocusNode => _topicFocusNode; + final _topicFocusNode = FocusNode(); + + @override + void dispose() { + _topicController.dispose(); + _topicFocusNode.dispose(); + super.dispose(); + } +} + +class FixedDestinationComposeBoxController extends ComposeBoxController {} + /// A compose box for use in a channel narrow. /// /// This offers a text input for the topic to send to, @@ -1119,50 +1144,42 @@ class _StreamComposeBox extends StatefulWidget { State<_StreamComposeBox> createState() => _StreamComposeBoxState(); } -class _StreamComposeBoxState extends State<_StreamComposeBox> implements ComposeBoxController<_StreamComposeBox> { - @override ComposeTopicController get topicController => _topicController; - final _topicController = ComposeTopicController(); - - @override ComposeContentController get contentController => _contentController; - final _contentController = ComposeContentController(); - - @override FocusNode get contentFocusNode => _contentFocusNode; - final _contentFocusNode = FocusNode(); - - FocusNode get topicFocusNode => _topicFocusNode; - final _topicFocusNode = FocusNode(); +class _StreamComposeBoxState extends State<_StreamComposeBox> { + StreamComposeBoxController get controller => _controller; + final _controller = StreamComposeBoxController(); @override void dispose() { - _topicController.dispose(); - _contentController.dispose(); - _contentFocusNode.dispose(); - _topicFocusNode.dispose(); + _controller.dispose(); super.dispose(); } @override Widget build(BuildContext context) { + final StreamComposeBoxController( + :topicController, :contentController, :topicFocusNode, :contentFocusNode, + ) = controller; + return _ComposeBoxLayout( - contentController: _contentController, - contentFocusNode: _contentFocusNode, + contentController: contentController, + contentFocusNode: contentFocusNode, topicInput: _TopicInput( streamId: widget.narrow.streamId, - controller: _topicController, + controller: topicController, focusNode: topicFocusNode, - contentFocusNode: _contentFocusNode, + contentFocusNode: contentFocusNode, ), contentInput: _StreamContentInput( narrow: widget.narrow, - topicController: _topicController, - controller: _contentController, - focusNode: _contentFocusNode, + topicController: topicController, + controller: contentController, + focusNode: contentFocusNode, ), sendButton: _SendButton( - topicController: _topicController, - contentController: _contentController, + topicController: topicController, + contentController: contentController, getDestination: () => StreamDestination( - widget.narrow.streamId, _topicController.textNormalized), + widget.narrow.streamId, topicController.textNormalized), )); } } @@ -1197,46 +1214,43 @@ class _FixedDestinationComposeBox extends StatefulWidget { State<_FixedDestinationComposeBox> createState() => _FixedDestinationComposeBoxState(); } -class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> implements ComposeBoxController<_FixedDestinationComposeBox> { - @override ComposeTopicController? get topicController => null; - - @override ComposeContentController get contentController => _contentController; - final _contentController = ComposeContentController(); - - @override FocusNode get contentFocusNode => _contentFocusNode; - final _contentFocusNode = FocusNode(); +class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> { + FixedDestinationComposeBoxController get controller => _controller; + final _controller = FixedDestinationComposeBoxController(); @override void dispose() { - _contentController.dispose(); - _contentFocusNode.dispose(); + _controller.dispose(); super.dispose(); } @override Widget build(BuildContext context) { + final FixedDestinationComposeBoxController( + :contentController, :contentFocusNode, + ) = controller; + return _ComposeBoxLayout( - contentController: _contentController, - contentFocusNode: _contentFocusNode, + contentController: contentController, + contentFocusNode: contentFocusNode, topicInput: null, contentInput: _FixedDestinationContentInput( narrow: widget.narrow, - controller: _contentController, - focusNode: _contentFocusNode, + controller: contentController, + focusNode: contentFocusNode, ), sendButton: _SendButton( topicController: null, - contentController: _contentController, + contentController: contentController, getDestination: () => widget.narrow.destination, )); } } -class ComposeBox extends StatelessWidget { - ComposeBox({super.key, this.controllerKey, required this.narrow}) +class ComposeBox extends StatefulWidget { + ComposeBox({super.key, required this.narrow}) : assert(ComposeBox.hasComposeBox(narrow)); - final GlobalKey? controllerKey; final Narrow narrow; static bool hasComposeBox(Narrow narrow) { @@ -1253,10 +1267,30 @@ class ComposeBox extends StatelessWidget { } } + @override + State createState() => _ComposeBoxState(); +} + +/// The interface for the state of a [ComposeBox]. +abstract class ComposeBoxState extends State { + ComposeBoxController? get controller; +} + +class _ComposeBoxState extends State implements ComposeBoxState { + @override ComposeBoxController? get controller { + final forStream = _streamComposeBoxControllerKey.currentState?.controller; + final forFixedDestination = _fixedDestinationComposeBoxControllerKey.currentState?.controller; + assert(forStream == null || forFixedDestination == null); + // Both can be null (error-banner case). + return forStream ?? forFixedDestination; + } + final _streamComposeBoxControllerKey = GlobalKey<_StreamComposeBoxState>(); + final _fixedDestinationComposeBoxControllerKey = GlobalKey<_FixedDestinationComposeBoxState>(); + Widget? _errorBanner(BuildContext context) { final store = PerAccountStoreWidget.of(context); final selfUser = store.users[store.selfUserId]!; - switch (narrow) { + switch (widget.narrow) { case ChannelNarrow(:final streamId): case TopicNarrow(:final streamId): final channel = store.streams[streamId]; @@ -1287,14 +1321,16 @@ class ComposeBox extends StatelessWidget { return _ComposeBoxContainer(child: errorBanner); } - final narrow = this.narrow; + final narrow = widget.narrow; switch (narrow) { case ChannelNarrow(): - return _StreamComposeBox(key: controllerKey, narrow: narrow); + return _StreamComposeBox(key: _streamComposeBoxControllerKey, narrow: narrow); case TopicNarrow(): - return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow); + return _FixedDestinationComposeBox(key: _fixedDestinationComposeBoxControllerKey, + narrow: narrow); case DmNarrow(): - return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow); + return _FixedDestinationComposeBox(key: _fixedDestinationComposeBoxControllerKey, + narrow: narrow); case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c6803f2484..f9e62f01ce 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -213,9 +213,9 @@ class _MessageListPageState extends State implements MessageLis late Narrow narrow; @override - ComposeBoxController? get composeBoxController => _composeBoxKey.currentState; + ComposeBoxController? get composeBoxController => _composeBoxKey.currentState?.controller; - final GlobalKey _composeBoxKey = GlobalKey(); + final GlobalKey _composeBoxKey = GlobalKey(); @override void initState() { @@ -302,7 +302,7 @@ class _MessageListPageState extends State implements MessageLis child: Expanded( child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), if (ComposeBox.hasComposeBox(narrow)) - ComposeBox(controllerKey: _composeBoxKey, narrow: narrow) + ComposeBox(key: _composeBoxKey, narrow: narrow) ])))); } } diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 719a1f18d6..c581c505e8 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -230,8 +230,8 @@ void main() { group('QuoteAndReplyButton', () { ComposeBoxController? findComposeBoxController(WidgetTester tester) { - return tester.widgetList(find.byType(ComposeBox)) - .singleOrNull?.controllerKey?.currentState; + return tester.stateList(find.byType(ComposeBox)) + .singleOrNull?.controller; } Widget? findQuoteAndReplyButton(WidgetTester tester) { @@ -283,14 +283,14 @@ void main() { final message = eg.streamMessage(); await setupToMessageActionSheet(tester, message: message, narrow: ChannelNarrow(message.streamId)); - final composeBoxController = findComposeBoxController(tester)!; + final composeBoxController = findComposeBoxController(tester) as StreamComposeBoxController; final contentController = composeBoxController.contentController; // Ensure channel-topics are loaded before testing quote & reply behavior connection.prepare(body: jsonEncode(GetStreamTopicsResult(topics: [eg.getStreamTopicsEntry()]).toJson())); final topicController = composeBoxController.topicController; - topicController?.value = const TextEditingValue(text: kNoTopicTopic); + topicController.value = const TextEditingValue(text: kNoTopicTopic); final valueBefore = contentController.value; prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index f611357cb3..8f297350c6 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -38,7 +38,7 @@ void main() { late PerAccountStore store; late FakeApiConnection connection; - late GlobalKey controllerKey; + late ComposeBoxController? controller; final contentInputFinder = find.byWidgetPredicate( (widget) => widget is TextField && widget.controller is ComposeContentController); @@ -66,16 +66,17 @@ void main() { await store.addStreams(streams); connection = store.connection as FakeApiConnection; - controllerKey = GlobalKey(); await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, child: Column( // This positions the compose box at the bottom of the screen, // simulating the layout of the message list page. children: [ const Expanded(child: SizedBox.expand()), - ComposeBox(controllerKey: controllerKey, narrow: narrow), + ComposeBox(narrow: narrow), ]))); await tester.pumpAndSettle(); + + controller = tester.state(find.byType(ComposeBox)).controller; } Future enterTopic(WidgetTester tester, { @@ -199,21 +200,21 @@ void main() { void checkComposeBoxTextFields(WidgetTester tester, { required bool expectTopicTextField, }) { - final composeBoxController = controllerKey.currentState!; - - final topicTextField = tester.widgetList(find.byWidgetPredicate( - (widget) => widget is TextField - && widget.controller == composeBoxController.topicController)).singleOrNull; if (expectTopicTextField) { + final topicController = (controller as StreamComposeBoxController).topicController; + final topicTextField = tester.widgetList(find.byWidgetPredicate( + (widget) => widget is TextField && widget.controller == topicController + )).singleOrNull; check(topicTextField).isNotNull() .textCapitalization.equals(TextCapitalization.none); } else { - check(topicTextField).isNull(); + check(controller).isA(); + check(find.byType(TextField)).findsOne(); // just content input, no topic } final contentTextField = tester.widget(find.byWidgetPredicate( (widget) => widget is TextField - && widget.controller == composeBoxController.contentController)); + && widget.controller == controller!.contentController)); check(contentTextField) .textCapitalization.equals(TextCapitalization.sentences); } @@ -351,7 +352,6 @@ void main() { testWidgets('selection change sends a "typing started" notice', (tester) async { await prepareComposeBox(tester, narrow: narrow, streams: [channel]); - final composeBoxController = controllerKey.currentState!; await checkStartTyping(tester, narrow); @@ -360,7 +360,7 @@ void main() { checkTypingRequest(TypingOp.stop, narrow); connection.prepare(json: {}); - composeBoxController.contentController.selection = + controller!.contentController.selection = const TextSelection(baseOffset: 0, extentOffset: 2); checkTypingRequest(TypingOp.start, narrow); @@ -471,12 +471,11 @@ void main() { final channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); - final composeBoxController = controllerKey.currentState!; // (When we check that the send button looks disabled, it should be because // the file is uploading, not a pre-existing reason.) await enterTopic(tester, narrow: narrow, topic: 'some topic'); - composeBoxController.contentController.value = const TextEditingValue(text: 'see image: '); + controller!.contentController.value = const TextEditingValue(text: 'see image: '); await tester.pump(); checkAppearsLoading(tester, false); @@ -500,7 +499,7 @@ void main() { final errorDialogs = tester.widgetList(find.byType(AlertDialog)); check(errorDialogs).isEmpty(); - check(composeBoxController.contentController.text) + check(controller!.contentController.text) .equals('see image: [Uploading image.jpg…]()\n\n'); // (the request is checked more thoroughly in API tests) check(connection.lastRequest!).isA() @@ -516,7 +515,7 @@ void main() { checkAppearsLoading(tester, true); await tester.pump(const Duration(seconds: 1)); - check(composeBoxController.contentController.text) + check(controller!.contentController.text) .equals('see image: [image.jpg](/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/image.jpg)\n\n'); checkAppearsLoading(tester, false); }); @@ -532,12 +531,11 @@ void main() { final channel = eg.stream(); final narrow = ChannelNarrow(channel.streamId); await prepareComposeBox(tester, narrow: narrow, streams: [channel]); - final composeBoxController = controllerKey.currentState!; // (When we check that the send button looks disabled, it should be because // the file is uploading, not a pre-existing reason.) await enterTopic(tester, narrow: narrow, topic: 'some topic'); - composeBoxController.contentController.value = const TextEditingValue(text: 'see image: '); + controller!.contentController.value = const TextEditingValue(text: 'see image: '); await tester.pump(); checkAppearsLoading(tester, false); @@ -561,7 +559,7 @@ void main() { final errorDialogs = tester.widgetList(find.byType(AlertDialog)); check(errorDialogs).isEmpty(); - check(composeBoxController.contentController.text) + check(controller!.contentController.text) .equals('see image: [Uploading image.jpg…]()\n\n'); // (the request is checked more thoroughly in API tests) check(connection.lastRequest!).isA() @@ -577,7 +575,7 @@ void main() { checkAppearsLoading(tester, true); await tester.pump(const Duration(seconds: 1)); - check(composeBoxController.contentController.text) + check(controller!.contentController.text) .equals('see image: [image.jpg](/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/image.jpg)\n\n'); checkAppearsLoading(tester, false); }); From b233ee920471add154f9245529669b474866bf2c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 13:13:39 -0800 Subject: [PATCH 08/24] compose [nfc]: Shorten {topic,content}Controller to just {topic,content} --- lib/widgets/action_sheet.dart | 8 +++---- lib/widgets/compose_box.dart | 36 ++++++++++++++--------------- test/widgets/action_sheet_test.dart | 10 ++++---- test/widgets/compose_box_test.dart | 18 +++++++-------- 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index ba3ad34d2b..c9b88f4996 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -334,15 +334,15 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { if (composeBoxController == null) return; if ( composeBoxController is StreamComposeBoxController - && composeBoxController.topicController.textNormalized == kNoTopicTopic + && composeBoxController.topic.textNormalized == kNoTopicTopic && message is StreamMessage ) { - composeBoxController.topicController.value = TextEditingValue(text: message.topic); + composeBoxController.topic.value = TextEditingValue(text: message.topic); } // This inserts a "[Quoting…]" placeholder into the content input, // giving the user a form of progress feedback. - final tag = composeBoxController.contentController + final tag = composeBoxController.content .registerQuoteAndReplyStart(PerAccountStoreWidget.of(pageContext), message: message, ); @@ -360,7 +360,7 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { // posting permission or when a DM recipient becomes deactivated. composeBoxController = findMessageListPage().composeBoxController; if (composeBoxController == null) return; - composeBoxController.contentController + composeBoxController.content .registerQuoteAndReplyEnd(PerAccountStoreWidget.of(pageContext), tag, message: message, rawContent: rawContent, diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 15a3c7448e..f95e507cac 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1100,29 +1100,29 @@ class _ComposeBoxLayout extends StatelessWidget { } sealed class ComposeBoxController { - ComposeContentController get contentController => _contentController; - final _contentController = ComposeContentController(); + ComposeContentController get content => _content; + final _content = ComposeContentController(); FocusNode get contentFocusNode => _contentFocusNode; final _contentFocusNode = FocusNode(); @mustCallSuper void dispose() { - _contentController.dispose(); + _content.dispose(); _contentFocusNode.dispose(); } } class StreamComposeBoxController extends ComposeBoxController { - ComposeTopicController get topicController => _topicController; - final _topicController = ComposeTopicController(); + ComposeTopicController get topic => _topic; + final _topic = ComposeTopicController(); FocusNode get topicFocusNode => _topicFocusNode; final _topicFocusNode = FocusNode(); @override void dispose() { - _topicController.dispose(); + _topic.dispose(); _topicFocusNode.dispose(); super.dispose(); } @@ -1157,29 +1157,29 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> { @override Widget build(BuildContext context) { final StreamComposeBoxController( - :topicController, :contentController, :topicFocusNode, :contentFocusNode, + :topic, :content, :topicFocusNode, :contentFocusNode, ) = controller; return _ComposeBoxLayout( - contentController: contentController, + contentController: content, contentFocusNode: contentFocusNode, topicInput: _TopicInput( streamId: widget.narrow.streamId, - controller: topicController, + controller: topic, focusNode: topicFocusNode, contentFocusNode: contentFocusNode, ), contentInput: _StreamContentInput( narrow: widget.narrow, - topicController: topicController, - controller: contentController, + topicController: topic, + controller: content, focusNode: contentFocusNode, ), sendButton: _SendButton( - topicController: topicController, - contentController: contentController, + topicController: topic, + contentController: content, getDestination: () => StreamDestination( - widget.narrow.streamId, topicController.textNormalized), + widget.narrow.streamId, topic.textNormalized), )); } } @@ -1227,21 +1227,21 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox @override Widget build(BuildContext context) { final FixedDestinationComposeBoxController( - :contentController, :contentFocusNode, + :content, :contentFocusNode, ) = controller; return _ComposeBoxLayout( - contentController: contentController, + contentController: content, contentFocusNode: contentFocusNode, topicInput: null, contentInput: _FixedDestinationContentInput( narrow: widget.narrow, - controller: contentController, + controller: content, focusNode: contentFocusNode, ), sendButton: _SendButton( topicController: null, - contentController: contentController, + contentController: content, getDestination: () => widget.narrow.destination, )); } diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index c581c505e8..b47a49dba9 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -284,12 +284,12 @@ void main() { await setupToMessageActionSheet(tester, message: message, narrow: ChannelNarrow(message.streamId)); final composeBoxController = findComposeBoxController(tester) as StreamComposeBoxController; - final contentController = composeBoxController.contentController; + final contentController = composeBoxController.content; // Ensure channel-topics are loaded before testing quote & reply behavior connection.prepare(body: jsonEncode(GetStreamTopicsResult(topics: [eg.getStreamTopicsEntry()]).toJson())); - final topicController = composeBoxController.topicController; + final topicController = composeBoxController.topic; topicController.value = const TextEditingValue(text: kNoTopicTopic); final valueBefore = contentController.value; @@ -308,7 +308,7 @@ void main() { await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); final composeBoxController = findComposeBoxController(tester)!; - final contentController = composeBoxController.contentController; + final contentController = composeBoxController.content; final valueBefore = contentController.value; prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); @@ -344,7 +344,7 @@ void main() { message: message, narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); final composeBoxController = findComposeBoxController(tester)!; - final contentController = composeBoxController.contentController; + final contentController = composeBoxController.content; final valueBefore = contentController.value; prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); @@ -383,7 +383,7 @@ void main() { await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); final composeBoxController = findComposeBoxController(tester)!; - final contentController = composeBoxController.contentController; + final contentController = composeBoxController.content; final valueBefore = contentController.value = TextEditingValue.empty; prepareRawContentResponseError(); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 8f297350c6..11f3afdec1 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -201,7 +201,7 @@ void main() { required bool expectTopicTextField, }) { if (expectTopicTextField) { - final topicController = (controller as StreamComposeBoxController).topicController; + final topicController = (controller as StreamComposeBoxController).topic; final topicTextField = tester.widgetList(find.byWidgetPredicate( (widget) => widget is TextField && widget.controller == topicController )).singleOrNull; @@ -214,7 +214,7 @@ void main() { final contentTextField = tester.widget(find.byWidgetPredicate( (widget) => widget is TextField - && widget.controller == controller!.contentController)); + && widget.controller == controller!.content)); check(contentTextField) .textCapitalization.equals(TextCapitalization.sentences); } @@ -360,7 +360,7 @@ void main() { checkTypingRequest(TypingOp.stop, narrow); connection.prepare(json: {}); - controller!.contentController.selection = + controller!.content.selection = const TextSelection(baseOffset: 0, extentOffset: 2); checkTypingRequest(TypingOp.start, narrow); @@ -475,7 +475,7 @@ void main() { // (When we check that the send button looks disabled, it should be because // the file is uploading, not a pre-existing reason.) await enterTopic(tester, narrow: narrow, topic: 'some topic'); - controller!.contentController.value = const TextEditingValue(text: 'see image: '); + controller!.content.value = const TextEditingValue(text: 'see image: '); await tester.pump(); checkAppearsLoading(tester, false); @@ -499,7 +499,7 @@ void main() { final errorDialogs = tester.widgetList(find.byType(AlertDialog)); check(errorDialogs).isEmpty(); - check(controller!.contentController.text) + check(controller!.content.text) .equals('see image: [Uploading image.jpg…]()\n\n'); // (the request is checked more thoroughly in API tests) check(connection.lastRequest!).isA() @@ -515,7 +515,7 @@ void main() { checkAppearsLoading(tester, true); await tester.pump(const Duration(seconds: 1)); - check(controller!.contentController.text) + check(controller!.content.text) .equals('see image: [image.jpg](/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/image.jpg)\n\n'); checkAppearsLoading(tester, false); }); @@ -535,7 +535,7 @@ void main() { // (When we check that the send button looks disabled, it should be because // the file is uploading, not a pre-existing reason.) await enterTopic(tester, narrow: narrow, topic: 'some topic'); - controller!.contentController.value = const TextEditingValue(text: 'see image: '); + controller!.content.value = const TextEditingValue(text: 'see image: '); await tester.pump(); checkAppearsLoading(tester, false); @@ -559,7 +559,7 @@ void main() { final errorDialogs = tester.widgetList(find.byType(AlertDialog)); check(errorDialogs).isEmpty(); - check(controller!.contentController.text) + check(controller!.content.text) .equals('see image: [Uploading image.jpg…]()\n\n'); // (the request is checked more thoroughly in API tests) check(connection.lastRequest!).isA() @@ -575,7 +575,7 @@ void main() { checkAppearsLoading(tester, true); await tester.pump(const Duration(seconds: 1)); - check(controller!.contentController.text) + check(controller!.content.text) .equals('see image: [image.jpg](/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/image.jpg)\n\n'); checkAppearsLoading(tester, false); }); From f0b0b5bbf8fdf21b95b4365a159e54a23ae67898 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 12:05:57 -0800 Subject: [PATCH 09/24] compose: Store controller on ComposeBox widget, not descendant Now there's just one place in the code where the controller is stored, instead of in both _StreamComposeBoxState and _FixedDestinationComposeBoxState. Also, those two widgets are simplified to be stateless. One small functional change: if the compose box disappears and then reappears -- because the self-user lost and gained posting permission, or a DM recipient was deactivated then reactivated -- the compose box returns to its state before it disappeared, instead of being re-initialized. This should be rare, but seems fine and helpful when it happens; it's one fewer reason your compose progress might get lost. --- lib/widgets/action_sheet.dart | 18 ++++--- lib/widgets/compose_box.dart | 90 ++++++++++++++++------------------- 2 files changed, 49 insertions(+), 59 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index c9b88f4996..c1e2c0e1f2 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -326,12 +326,11 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { @override void onPressed() async { final zulipLocalizations = ZulipLocalizations.of(pageContext); - // This will be null only if the compose box disappeared after the - // message action sheet opened, and before "Quote and reply" was pressed. - // This is rare: it happens when the self-user loses posting permission - // or when a DM recipient becomes deactivated. var composeBoxController = findMessageListPage().composeBoxController; - if (composeBoxController == null) return; + // The compose box doesn't null out its controller; it's either always null + // (e.g. in Combined Feed) or always non-null; it can't have been nulled out + // after the action sheet opened. + composeBoxController!; if ( composeBoxController is StreamComposeBoxController && composeBoxController.topic.textNormalized == kNoTopicTopic @@ -355,12 +354,11 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { if (!pageContext.mounted) return; - // This will be null only if the compose box disappeared during the - // quotation request. This is rare: it happens when the self-user loses - // posting permission or when a DM recipient becomes deactivated. composeBoxController = findMessageListPage().composeBoxController; - if (composeBoxController == null) return; - composeBoxController.content + // The compose box doesn't null out its controller; it's either always null + // (e.g. in Combined Feed) or always non-null; it can't have been nulled out + // during the raw-content request. + composeBoxController!.content .registerQuoteAndReplyEnd(PerAccountStoreWidget.of(pageContext), tag, message: message, rawContent: rawContent, diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index f95e507cac..08eb14885f 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1134,25 +1134,13 @@ class FixedDestinationComposeBoxController extends ComposeBoxController {} /// /// This offers a text input for the topic to send to, /// in addition to a text input for the message content. -class _StreamComposeBox extends StatefulWidget { - const _StreamComposeBox({super.key, required this.narrow}); +class _StreamComposeBox extends StatelessWidget { + const _StreamComposeBox({required this.narrow, required this.controller}); /// The narrow on view in the message list. final ChannelNarrow narrow; - @override - State<_StreamComposeBox> createState() => _StreamComposeBoxState(); -} - -class _StreamComposeBoxState extends State<_StreamComposeBox> { - StreamComposeBoxController get controller => _controller; - final _controller = StreamComposeBoxController(); - - @override - void dispose() { - _controller.dispose(); - super.dispose(); - } + final StreamComposeBoxController controller; @override Widget build(BuildContext context) { @@ -1164,13 +1152,13 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> { contentController: content, contentFocusNode: contentFocusNode, topicInput: _TopicInput( - streamId: widget.narrow.streamId, + streamId: narrow.streamId, controller: topic, focusNode: topicFocusNode, contentFocusNode: contentFocusNode, ), contentInput: _StreamContentInput( - narrow: widget.narrow, + narrow: narrow, topicController: topic, controller: content, focusNode: contentFocusNode, @@ -1179,7 +1167,7 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> { topicController: topic, contentController: content, getDestination: () => StreamDestination( - widget.narrow.streamId, topic.textNormalized), + narrow.streamId, topic.textNormalized), )); } } @@ -1205,24 +1193,12 @@ class _ErrorBanner extends StatelessWidget { } } -class _FixedDestinationComposeBox extends StatefulWidget { - const _FixedDestinationComposeBox({super.key, required this.narrow}); +class _FixedDestinationComposeBox extends StatelessWidget { + const _FixedDestinationComposeBox({required this.narrow, required this.controller}); final SendableNarrow narrow; - @override - State<_FixedDestinationComposeBox> createState() => _FixedDestinationComposeBoxState(); -} - -class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> { - FixedDestinationComposeBoxController get controller => _controller; - final _controller = FixedDestinationComposeBoxController(); - - @override - void dispose() { - _controller.dispose(); - super.dispose(); - } + final FixedDestinationComposeBoxController controller; @override Widget build(BuildContext context) { @@ -1235,14 +1211,14 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox contentFocusNode: contentFocusNode, topicInput: null, contentInput: _FixedDestinationContentInput( - narrow: widget.narrow, + narrow: narrow, controller: content, focusNode: contentFocusNode, ), sendButton: _SendButton( topicController: null, contentController: content, - getDestination: () => widget.narrow.destination, + getDestination: () => narrow.destination, )); } } @@ -1273,19 +1249,34 @@ class ComposeBox extends StatefulWidget { /// The interface for the state of a [ComposeBox]. abstract class ComposeBoxState extends State { - ComposeBoxController? get controller; + ComposeBoxController get controller; } class _ComposeBoxState extends State implements ComposeBoxState { - @override ComposeBoxController? get controller { - final forStream = _streamComposeBoxControllerKey.currentState?.controller; - final forFixedDestination = _fixedDestinationComposeBoxControllerKey.currentState?.controller; - assert(forStream == null || forFixedDestination == null); - // Both can be null (error-banner case). - return forStream ?? forFixedDestination; + @override ComposeBoxController get controller => _controller; + late final ComposeBoxController _controller; + + @override + void initState() { + super.initState(); + switch (widget.narrow) { + case ChannelNarrow(): + _controller = StreamComposeBoxController(); + case TopicNarrow(): + case DmNarrow(): + _controller = FixedDestinationComposeBoxController(); + case CombinedFeedNarrow(): + case MentionsNarrow(): + case StarredMessagesNarrow(): + assert(false); + } + } + + @override + void dispose() { + _controller.dispose(); + super.dispose(); } - final _streamComposeBoxControllerKey = GlobalKey<_StreamComposeBoxState>(); - final _fixedDestinationComposeBoxControllerKey = GlobalKey<_FixedDestinationComposeBoxState>(); Widget? _errorBanner(BuildContext context) { final store = PerAccountStoreWidget.of(context); @@ -1324,13 +1315,14 @@ class _ComposeBoxState extends State implements ComposeBoxState { final narrow = widget.narrow; switch (narrow) { case ChannelNarrow(): - return _StreamComposeBox(key: _streamComposeBoxControllerKey, narrow: narrow); + _controller as StreamComposeBoxController; + return _StreamComposeBox(controller: _controller, narrow: narrow); case TopicNarrow(): - return _FixedDestinationComposeBox(key: _fixedDestinationComposeBoxControllerKey, - narrow: narrow); + _controller as FixedDestinationComposeBoxController; + return _FixedDestinationComposeBox(controller: _controller, narrow: narrow); case DmNarrow(): - return _FixedDestinationComposeBox(key: _fixedDestinationComposeBoxControllerKey, - narrow: narrow); + _controller as FixedDestinationComposeBoxController; + return _FixedDestinationComposeBox(controller: _controller, narrow: narrow); case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): From 69d5c6169386f2a90e8a19f7c9693276fd462a3e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 13:23:21 -0800 Subject: [PATCH 10/24] compose [nfc]: s/_ComposeBoxLayout/_ComposeBoxBody/ This widget is already doing more than layout (sizing and positioning its inputs on the screen): it creates the attachment buttons and styles their touch feedback, and it defeats an unwanted border that its `topicInput` and `contentInput` params might otherwise create. I chose the name "body" to distinguish this from some other elements that will need to share the compose-box surface, for the UI part of issue #720: an error banner and a linear progress indicator. Then also add "body" to the names _StreamComposeBox and _FixedDestinationComposeBox. Since the recent commits that simplified those, they're really just thin, stateless wrappers that configure a _ComposeBoxBody. --- lib/widgets/compose_box.dart | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 08eb14885f..c38df5bc50 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1030,8 +1030,8 @@ class _ComposeBoxContainer extends StatelessWidget { } } -class _ComposeBoxLayout extends StatelessWidget { - const _ComposeBoxLayout({ +class _ComposeBoxBody extends StatelessWidget { + const _ComposeBoxBody({ required this.topicInput, required this.contentInput, required this.sendButton, @@ -1134,8 +1134,8 @@ class FixedDestinationComposeBoxController extends ComposeBoxController {} /// /// This offers a text input for the topic to send to, /// in addition to a text input for the message content. -class _StreamComposeBox extends StatelessWidget { - const _StreamComposeBox({required this.narrow, required this.controller}); +class _StreamComposeBoxBody extends StatelessWidget { + const _StreamComposeBoxBody({required this.narrow, required this.controller}); /// The narrow on view in the message list. final ChannelNarrow narrow; @@ -1148,7 +1148,7 @@ class _StreamComposeBox extends StatelessWidget { :topic, :content, :topicFocusNode, :contentFocusNode, ) = controller; - return _ComposeBoxLayout( + return _ComposeBoxBody( contentController: content, contentFocusNode: contentFocusNode, topicInput: _TopicInput( @@ -1193,8 +1193,8 @@ class _ErrorBanner extends StatelessWidget { } } -class _FixedDestinationComposeBox extends StatelessWidget { - const _FixedDestinationComposeBox({required this.narrow, required this.controller}); +class _FixedDestinationComposeBoxBody extends StatelessWidget { + const _FixedDestinationComposeBoxBody({required this.narrow, required this.controller}); final SendableNarrow narrow; @@ -1206,7 +1206,7 @@ class _FixedDestinationComposeBox extends StatelessWidget { :content, :contentFocusNode, ) = controller; - return _ComposeBoxLayout( + return _ComposeBoxBody( contentController: content, contentFocusNode: contentFocusNode, topicInput: null, @@ -1316,13 +1316,13 @@ class _ComposeBoxState extends State implements ComposeBoxState { switch (narrow) { case ChannelNarrow(): _controller as StreamComposeBoxController; - return _StreamComposeBox(controller: _controller, narrow: narrow); + return _StreamComposeBoxBody(controller: _controller, narrow: narrow); case TopicNarrow(): _controller as FixedDestinationComposeBoxController; - return _FixedDestinationComposeBox(controller: _controller, narrow: narrow); + return _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); case DmNarrow(): _controller as FixedDestinationComposeBoxController; - return _FixedDestinationComposeBox(controller: _controller, narrow: narrow); + return _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): From 009e4152397b5d63860cfe5c41875bf06ca03bfb Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 13:49:38 -0800 Subject: [PATCH 11/24] compose [nfc]: Give _ComposeBoxBody `narrow` and `controller` params --- lib/widgets/compose_box.dart | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index c38df5bc50..511f6194fb 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1032,18 +1032,21 @@ class _ComposeBoxContainer extends StatelessWidget { class _ComposeBoxBody extends StatelessWidget { const _ComposeBoxBody({ + required this.narrow, + required this.controller, required this.topicInput, required this.contentInput, required this.sendButton, - required this.contentController, - required this.contentFocusNode, }); + /// The narrow on view in the message list. + final Narrow narrow; + + final ComposeBoxController controller; + final Widget? topicInput; final Widget contentInput; final Widget sendButton; - final ComposeContentController contentController; - final FocusNode contentFocusNode; @override Widget build(BuildContext context) { @@ -1069,10 +1072,11 @@ class _ComposeBoxBody extends StatelessWidget { shape: const RoundedRectangleBorder( borderRadius: BorderRadius.all(Radius.circular(4))))); + final ComposeBoxController(:content, :contentFocusNode) = controller; final composeButtons = [ - _AttachFileButton(contentController: contentController, contentFocusNode: contentFocusNode), - _AttachMediaButton(contentController: contentController, contentFocusNode: contentFocusNode), - _AttachFromCameraButton(contentController: contentController, contentFocusNode: contentFocusNode), + _AttachFileButton(contentController: content, contentFocusNode: contentFocusNode), + _AttachMediaButton(contentController: content, contentFocusNode: contentFocusNode), + _AttachFromCameraButton(contentController: content, contentFocusNode: contentFocusNode), ]; return _ComposeBoxContainer( @@ -1149,8 +1153,8 @@ class _StreamComposeBoxBody extends StatelessWidget { ) = controller; return _ComposeBoxBody( - contentController: content, - contentFocusNode: contentFocusNode, + narrow: narrow, + controller: controller, topicInput: _TopicInput( streamId: narrow.streamId, controller: topic, @@ -1207,8 +1211,8 @@ class _FixedDestinationComposeBoxBody extends StatelessWidget { ) = controller; return _ComposeBoxBody( - contentController: content, - contentFocusNode: contentFocusNode, + narrow: narrow, + controller: controller, topicInput: null, contentInput: _FixedDestinationContentInput( narrow: narrow, From d124755faf073483bf09a9e3f70d51893b87b129 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 13:47:16 -0800 Subject: [PATCH 12/24] compose [nfc]: Refactor _{Stream,FixedDestination}ComposeBoxBody As mentioned in a previous commit, these have become thin, stateless wrappers that configure _ComposeBoxBody. Factor them as subclasses, which feels neater and should help keep their implementations from diverging unintentionally. --- lib/widgets/compose_box.dart | 121 +++++++++++++++-------------------- 1 file changed, 51 insertions(+), 70 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 511f6194fb..3663e6b64e 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1030,23 +1030,16 @@ class _ComposeBoxContainer extends StatelessWidget { } } -class _ComposeBoxBody extends StatelessWidget { - const _ComposeBoxBody({ - required this.narrow, - required this.controller, - required this.topicInput, - required this.contentInput, - required this.sendButton, - }); - +/// The text inputs, compose-button row, and send button for the compose box. +abstract class _ComposeBoxBody extends StatelessWidget { /// The narrow on view in the message list. - final Narrow narrow; + Narrow get narrow; - final ComposeBoxController controller; + ComposeBoxController get controller; - final Widget? topicInput; - final Widget contentInput; - final Widget sendButton; + Widget? buildTopicInput(); + Widget buildContentInput(); + Widget buildSendButton(); @override Widget build(BuildContext context) { @@ -1079,6 +1072,7 @@ class _ComposeBoxBody extends StatelessWidget { _AttachFromCameraButton(contentController: content, contentFocusNode: contentFocusNode), ]; + final topicInput = buildTopicInput(); return _ComposeBoxContainer( child: Column(children: [ Padding( @@ -1086,8 +1080,8 @@ class _ComposeBoxBody extends StatelessWidget { child: Theme( data: inputThemeData, child: Column(children: [ - if (topicInput != null) topicInput!, - contentInput, + if (topicInput != null) topicInput, + buildContentInput(), ]))), SizedBox( height: _composeButtonSize, @@ -1097,7 +1091,7 @@ class _ComposeBoxBody extends StatelessWidget { mainAxisAlignment: MainAxisAlignment.spaceBetween, children: [ Row(children: composeButtons), - sendButton, + buildSendButton(), ]))), ])); } @@ -1138,42 +1132,35 @@ class FixedDestinationComposeBoxController extends ComposeBoxController {} /// /// This offers a text input for the topic to send to, /// in addition to a text input for the message content. -class _StreamComposeBoxBody extends StatelessWidget { - const _StreamComposeBoxBody({required this.narrow, required this.controller}); +class _StreamComposeBoxBody extends _ComposeBoxBody { + _StreamComposeBoxBody({required this.narrow, required this.controller}); - /// The narrow on view in the message list. + @override final ChannelNarrow narrow; - final StreamComposeBoxController controller; - @override - Widget build(BuildContext context) { - final StreamComposeBoxController( - :topic, :content, :topicFocusNode, :contentFocusNode, - ) = controller; + final StreamComposeBoxController controller; - return _ComposeBoxBody( - narrow: narrow, - controller: controller, - topicInput: _TopicInput( - streamId: narrow.streamId, - controller: topic, - focusNode: topicFocusNode, - contentFocusNode: contentFocusNode, - ), - contentInput: _StreamContentInput( - narrow: narrow, - topicController: topic, - controller: content, - focusNode: contentFocusNode, - ), - sendButton: _SendButton( - topicController: topic, - contentController: content, - getDestination: () => StreamDestination( - narrow.streamId, topic.textNormalized), - )); - } + @override Widget buildTopicInput() => _TopicInput( + streamId: narrow.streamId, + controller: controller.topic, + focusNode: controller.topicFocusNode, + contentFocusNode: controller.contentFocusNode, + ); + + @override Widget buildContentInput() => _StreamContentInput( + narrow: narrow, + topicController: controller.topic, + controller: controller.content, + focusNode: controller.contentFocusNode, + ); + + @override Widget buildSendButton() => _SendButton( + topicController: controller.topic, + contentController: controller.content, + getDestination: () => StreamDestination( + narrow.streamId, controller.topic.textNormalized), + ); } class _ErrorBanner extends StatelessWidget { @@ -1197,34 +1184,28 @@ class _ErrorBanner extends StatelessWidget { } } -class _FixedDestinationComposeBoxBody extends StatelessWidget { - const _FixedDestinationComposeBoxBody({required this.narrow, required this.controller}); +class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { + _FixedDestinationComposeBoxBody({required this.narrow, required this.controller}); + @override final SendableNarrow narrow; + @override final FixedDestinationComposeBoxController controller; - @override - Widget build(BuildContext context) { - final FixedDestinationComposeBoxController( - :content, :contentFocusNode, - ) = controller; + @override Widget? buildTopicInput() => null; - return _ComposeBoxBody( - narrow: narrow, - controller: controller, - topicInput: null, - contentInput: _FixedDestinationContentInput( - narrow: narrow, - controller: content, - focusNode: contentFocusNode, - ), - sendButton: _SendButton( - topicController: null, - contentController: content, - getDestination: () => narrow.destination, - )); - } + @override Widget buildContentInput() => _FixedDestinationContentInput( + narrow: narrow, + controller: controller.content, + focusNode: controller.contentFocusNode, + ); + + @override Widget buildSendButton() => _SendButton( + topicController: null, + contentController: controller.content, + getDestination: () => narrow.destination, + ); } class ComposeBox extends StatefulWidget { From 61693116e92eed6a22b2447b9249b941d8d08ac4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 15:04:29 -0800 Subject: [PATCH 13/24] compose [nfc]: Put {_Stream,FixedDestination}ComposeBoxBody near base class --- lib/widgets/compose_box.dart | 104 +++++++++++++++++------------------ 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 3663e6b64e..df35a76435 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1097,37 +1097,6 @@ abstract class _ComposeBoxBody extends StatelessWidget { } } -sealed class ComposeBoxController { - ComposeContentController get content => _content; - final _content = ComposeContentController(); - - FocusNode get contentFocusNode => _contentFocusNode; - final _contentFocusNode = FocusNode(); - - @mustCallSuper - void dispose() { - _content.dispose(); - _contentFocusNode.dispose(); - } -} - -class StreamComposeBoxController extends ComposeBoxController { - ComposeTopicController get topic => _topic; - final _topic = ComposeTopicController(); - - FocusNode get topicFocusNode => _topicFocusNode; - final _topicFocusNode = FocusNode(); - - @override - void dispose() { - _topic.dispose(); - _topicFocusNode.dispose(); - super.dispose(); - } -} - -class FixedDestinationComposeBoxController extends ComposeBoxController {} - /// A compose box for use in a channel narrow. /// /// This offers a text input for the topic to send to, @@ -1163,27 +1132,6 @@ class _StreamComposeBoxBody extends _ComposeBoxBody { ); } -class _ErrorBanner extends StatelessWidget { - const _ErrorBanner({required this.label}); - - final String label; - - @override - Widget build(BuildContext context) { - final designVariables = DesignVariables.of(context); - return Container( - padding: const EdgeInsets.all(8), - decoration: BoxDecoration( - color: designVariables.errorBannerBackground, - border: Border.all(color: designVariables.errorBannerBorder), - borderRadius: BorderRadius.circular(5)), - child: Text(label, - style: TextStyle(fontSize: 18, color: designVariables.errorBannerLabel), - ), - ); - } -} - class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { _FixedDestinationComposeBoxBody({required this.narrow, required this.controller}); @@ -1208,6 +1156,58 @@ class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { ); } +sealed class ComposeBoxController { + ComposeContentController get content => _content; + final _content = ComposeContentController(); + + FocusNode get contentFocusNode => _contentFocusNode; + final _contentFocusNode = FocusNode(); + + @mustCallSuper + void dispose() { + _content.dispose(); + _contentFocusNode.dispose(); + } +} + +class StreamComposeBoxController extends ComposeBoxController { + ComposeTopicController get topic => _topic; + final _topic = ComposeTopicController(); + + FocusNode get topicFocusNode => _topicFocusNode; + final _topicFocusNode = FocusNode(); + + @override + void dispose() { + _topic.dispose(); + _topicFocusNode.dispose(); + super.dispose(); + } +} + +class FixedDestinationComposeBoxController extends ComposeBoxController {} + +class _ErrorBanner extends StatelessWidget { + const _ErrorBanner({required this.label}); + + final String label; + + @override + Widget build(BuildContext context) { + final designVariables = DesignVariables.of(context); + return Container( + padding: const EdgeInsets.all(8), + decoration: BoxDecoration( + color: designVariables.errorBannerBackground, + border: Border.all(color: designVariables.errorBannerBorder), + borderRadius: BorderRadius.circular(5)), + child: Text(label, + style: TextStyle(fontSize: 18, color: designVariables.errorBannerLabel), + ), + ); + } +} + class ComposeBox extends StatefulWidget { ComposeBox({super.key, required this.narrow}) : assert(ComposeBox.hasComposeBox(narrow)); From 6d1f026b68497e3c19e8247c1d276b0ee80155b1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 14:12:10 -0800 Subject: [PATCH 14/24] compose [nfc]: Bring _ComposeBoxContainer out of _ComposeBoxBody Now, _ComposeBoxContainer is available as a nice place to put things other than the "body" that (unlike the body) will want to consume the left and right insets. Specifically, for #720, an error banner and an overlaid progress indicator. Next, we'll rename `_ComposeBoxContainer.child` to `body`. We'll also go ahead and add a param for the error banner, redesign the error banner that we currently have and are passing as `child` / `body`, and pass the banner as that new param instead. --- lib/widgets/compose_box.dart | 50 +++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index df35a76435..42b76e3c06 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1073,27 +1073,26 @@ abstract class _ComposeBoxBody extends StatelessWidget { ]; final topicInput = buildTopicInput(); - return _ComposeBoxContainer( - child: Column(children: [ - Padding( - padding: const EdgeInsets.symmetric(horizontal: 8), - child: Theme( - data: inputThemeData, - child: Column(children: [ - if (topicInput != null) topicInput, - buildContentInput(), + return Column(children: [ + Padding( + padding: const EdgeInsets.symmetric(horizontal: 8), + child: Theme( + data: inputThemeData, + child: Column(children: [ + if (topicInput != null) topicInput, + buildContentInput(), + ]))), + SizedBox( + height: _composeButtonSize, + child: IconButtonTheme( + data: iconButtonThemeData, + child: Row( + mainAxisAlignment: MainAxisAlignment.spaceBetween, + children: [ + Row(children: composeButtons), + buildSendButton(), ]))), - SizedBox( - height: _composeButtonSize, - child: IconButtonTheme( - data: iconButtonThemeData, - child: Row( - mainAxisAlignment: MainAxisAlignment.spaceBetween, - children: [ - Row(children: composeButtons), - buildSendButton(), - ]))), - ])); + ]); } } @@ -1298,21 +1297,24 @@ class _ComposeBoxState extends State implements ComposeBoxState { } final narrow = widget.narrow; + final Widget body; switch (narrow) { case ChannelNarrow(): _controller as StreamComposeBoxController; - return _StreamComposeBoxBody(controller: _controller, narrow: narrow); + body = _StreamComposeBoxBody(controller: _controller, narrow: narrow); case TopicNarrow(): _controller as FixedDestinationComposeBoxController; - return _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); + body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); case DmNarrow(): _controller as FixedDestinationComposeBoxController; - return _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); + body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): assert(false); - return const SizedBox.shrink(); + body = const SizedBox.shrink(); } + + return _ComposeBoxContainer(child: body); } } From 5b3ca514118486c7f77ef348ce4b86e9324143a6 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 14:37:48 -0800 Subject: [PATCH 15/24] compose: Redesign error banner that replaces the compose box This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR #924 (for issue #720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to put the banner at the very top of the compose-box surface, with no margin. So #1089, where the old banner's top margin disappeared, looking buggy, is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li Fixes: #1089 --- lib/widgets/compose_box.dart | 91 +++++++++++++++++++++++++++++------- lib/widgets/theme.dart | 42 ++++++++--------- 2 files changed, 95 insertions(+), 38 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 42b76e3c06..0a73cb927d 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1010,14 +1010,51 @@ class _SendButtonState extends State<_SendButton> { } class _ComposeBoxContainer extends StatelessWidget { - const _ComposeBoxContainer({required this.child}); + const _ComposeBoxContainer({ + required this.body, + this.errorBanner, + }) : assert(body != null || errorBanner != null); - final Widget child; + /// The text inputs, compose-button row, and send button. + /// + /// This widget does not need a [SafeArea] to consume any device insets. + /// + /// Can be null, but only if [errorBanner] is non-null. + final Widget? body; + + /// An error bar that goes at the top. + /// + /// This may be present on its own or with a [body]. + /// If [body] is null this must be present. + /// + /// This widget should use a [SafeArea] to pad the left, right, + /// and bottom device insets. + /// (A bottom inset may occur if [body] is null.) + final Widget? errorBanner; + + Widget _paddedBody() { + assert(body != null); + return SafeArea(minimum: const EdgeInsets.symmetric(horizontal: 8), + child: body!); + } @override Widget build(BuildContext context) { final designVariables = DesignVariables.of(context); + final List children = switch ((errorBanner, body)) { + (Widget(), Widget()) => [ + // _paddedBody() already pads the bottom inset, + // so make sure the error banner doesn't double-pad it. + MediaQuery.removePadding(context: context, removeBottom: true, + child: errorBanner!), + _paddedBody(), + ], + (Widget(), null) => [errorBanner!], + (null, Widget()) => [_paddedBody()], + (null, null) => throw UnimplementedError(), // not allowed, see dartdoc + }; + // TODO(design): Maybe put a max width on the compose box, like we do on // the message list itself return Container(width: double.infinity, @@ -1025,8 +1062,8 @@ class _ComposeBoxContainer extends StatelessWidget { border: Border(top: BorderSide(color: designVariables.borderBar))), child: Material( color: designVariables.composeBoxBg, - child: SafeArea(minimum: const EdgeInsets.symmetric(horizontal: 8), - child: child))); + child: Column( + children: children))); } } @@ -1194,16 +1231,30 @@ class _ErrorBanner extends StatelessWidget { @override Widget build(BuildContext context) { final designVariables = DesignVariables.of(context); - return Container( - padding: const EdgeInsets.all(8), + final labelTextStyle = TextStyle( + fontSize: 17, + height: 22 / 17, + color: designVariables.btnLabelAttMediumIntDanger, + ).merge(weightVariableTextStyle(context, wght: 600)); + + return DecoratedBox( decoration: BoxDecoration( - color: designVariables.errorBannerBackground, - border: Border.all(color: designVariables.errorBannerBorder), - borderRadius: BorderRadius.circular(5)), - child: Text(label, - style: TextStyle(fontSize: 18, color: designVariables.errorBannerLabel), - ), - ); + color: designVariables.bannerBgIntDanger), + child: SafeArea( + minimum: const EdgeInsetsDirectional.only(start: 8) + // (SafeArea.minimum doesn't take an EdgeInsetsDirectional) + .resolve(Directionality.of(context)), + child: Row( + children: [ + Expanded( + child: Padding( + padding: const EdgeInsetsDirectional.fromSTEB(8, 9, 0, 9), + child: Text(style: labelTextStyle, + label))), + const SizedBox(width: 8), + // TODO(#720) "x" button goes here. + // 24px square with 8px touchable padding in all directions? + ]))); } } @@ -1291,13 +1342,14 @@ class _ComposeBoxState extends State implements ComposeBoxState { @override Widget build(BuildContext context) { + final Widget? body; + final errorBanner = _errorBanner(context); if (errorBanner != null) { - return _ComposeBoxContainer(child: errorBanner); + return _ComposeBoxContainer(body: null, errorBanner: errorBanner); } final narrow = widget.narrow; - final Widget body; switch (narrow) { case ChannelNarrow(): _controller as StreamComposeBoxController; @@ -1312,9 +1364,14 @@ class _ComposeBoxState extends State implements ComposeBoxState { case MentionsNarrow(): case StarredMessagesNarrow(): assert(false); - body = const SizedBox.shrink(); + body = null; } - return _ComposeBoxContainer(child: body); + // TODO(#720) dismissable message-send error, maybe something like: + // if (controller.sendMessageError.value != null) { + // errorBanner = _ErrorBanner(label: + // ZulipLocalizations.of(context).errorSendMessageTimeout); + // } + return _ComposeBoxContainer(body: body, errorBanner: null); } } diff --git a/lib/widgets/theme.dart b/lib/widgets/theme.dart index be0ae32ae8..3ba5221245 100644 --- a/lib/widgets/theme.dart +++ b/lib/widgets/theme.dart @@ -114,10 +114,13 @@ class DesignVariables extends ThemeExtension { DesignVariables.light() : this._( background: const Color(0xffffffff), + bannerBgIntDanger: const Color(0xfff2e4e4), bgContextMenu: const Color(0xfff2f2f2), bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.15), bgTopBar: const Color(0xfff5f5f5), borderBar: Colors.black.withValues(alpha: 0.2), + btnLabelAttLowIntDanger: const Color(0xffc0070a), + btnLabelAttMediumIntDanger: const Color(0xffac0508), composeBoxBg: const Color(0xffffffff), contextMenuCancelText: const Color(0xff222222), contextMenuItemBg: const Color(0xff6159e1), @@ -135,9 +138,6 @@ class DesignVariables extends ThemeExtension { atMentionMarker: const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor(), contextMenuCancelBg: const Color(0xff797986), dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.35, 0.93).toColor(), - errorBannerBackground: const HSLColor.fromAHSL(1, 4, 0.33, 0.90).toColor(), - errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.57, 0.33).toColor(), - errorBannerLabel: const HSLColor.fromAHSL(1, 4, 0.58, 0.33).toColor(), groupDmConversationIcon: Colors.black.withValues(alpha: 0.5), groupDmConversationIconBg: const Color(0x33808080), loginOrDivider: const Color(0xffdedede), @@ -154,10 +154,13 @@ class DesignVariables extends ThemeExtension { DesignVariables.dark() : this._( background: const Color(0xff000000), + bannerBgIntDanger: const Color(0xff461616), bgContextMenu: const Color(0xff262626), bgCounterUnread: const Color(0xff666699).withValues(alpha: 0.37), bgTopBar: const Color(0xff242424), borderBar: Colors.black.withValues(alpha: 0.5), + btnLabelAttLowIntDanger: const Color(0xffff8b7c), + btnLabelAttMediumIntDanger: const Color(0xffff8b7c), composeBoxBg: const Color(0xff0f0f0f), contextMenuCancelText: const Color(0xffffffff).withValues(alpha: 0.75), contextMenuItemBg: const Color(0xff7977fe), @@ -176,9 +179,6 @@ class DesignVariables extends ThemeExtension { // TODO(design-dark) need proper dark-theme color (this is ad hoc) atMentionMarker: const HSLColor.fromAHSL(0.4, 0, 0, 1).toColor(), dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.15, 0.2).toColor(), - errorBannerBackground: const HSLColor.fromAHSL(1, 0, 0.61, 0.19).toColor(), - errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.73, 0.74).toColor(), - errorBannerLabel: const HSLColor.fromAHSL(1, 2, 0.73, 0.80).toColor(), // TODO(design-dark) need proper dark-theme color (this is ad hoc) groupDmConversationIcon: Colors.white.withValues(alpha: 0.5), // TODO(design-dark) need proper dark-theme color (this is ad hoc) @@ -201,10 +201,13 @@ class DesignVariables extends ThemeExtension { DesignVariables._({ required this.background, + required this.bannerBgIntDanger, required this.bgContextMenu, required this.bgCounterUnread, required this.bgTopBar, required this.borderBar, + required this.btnLabelAttLowIntDanger, + required this.btnLabelAttMediumIntDanger, required this.composeBoxBg, required this.contextMenuCancelText, required this.contextMenuItemBg, @@ -222,9 +225,6 @@ class DesignVariables extends ThemeExtension { required this.atMentionMarker, required this.contextMenuCancelBg, required this.dmHeaderBg, - required this.errorBannerBackground, - required this.errorBannerBorder, - required this.errorBannerLabel, required this.groupDmConversationIcon, required this.groupDmConversationIconBg, required this.loginOrDivider, @@ -249,10 +249,13 @@ class DesignVariables extends ThemeExtension { } final Color background; + final Color bannerBgIntDanger; final Color bgContextMenu; final Color bgCounterUnread; final Color bgTopBar; final Color borderBar; + final Color btnLabelAttLowIntDanger; + final Color btnLabelAttMediumIntDanger; final Color composeBoxBg; final Color contextMenuCancelText; final Color contextMenuItemBg; @@ -274,9 +277,6 @@ class DesignVariables extends ThemeExtension { final Color atMentionMarker; final Color contextMenuCancelBg; // In Figma, but unnamed. final Color dmHeaderBg; - final Color errorBannerBackground; - final Color errorBannerBorder; - final Color errorBannerLabel; final Color groupDmConversationIcon; final Color groupDmConversationIconBg; final Color loginOrDivider; // TODO(design-dark) need proper dark-theme color (this is ad hoc) @@ -292,10 +292,13 @@ class DesignVariables extends ThemeExtension { @override DesignVariables copyWith({ Color? background, + Color? bannerBgIntDanger, Color? bgContextMenu, Color? bgCounterUnread, Color? bgTopBar, Color? borderBar, + Color? btnLabelAttLowIntDanger, + Color? btnLabelAttMediumIntDanger, Color? composeBoxBg, Color? contextMenuCancelText, Color? contextMenuItemBg, @@ -313,9 +316,6 @@ class DesignVariables extends ThemeExtension { Color? atMentionMarker, Color? contextMenuCancelBg, Color? dmHeaderBg, - Color? errorBannerBackground, - Color? errorBannerBorder, - Color? errorBannerLabel, Color? groupDmConversationIcon, Color? groupDmConversationIconBg, Color? loginOrDivider, @@ -330,10 +330,13 @@ class DesignVariables extends ThemeExtension { }) { return DesignVariables._( background: background ?? this.background, + bannerBgIntDanger: bannerBgIntDanger ?? this.bannerBgIntDanger, bgContextMenu: bgContextMenu ?? this.bgContextMenu, bgCounterUnread: bgCounterUnread ?? this.bgCounterUnread, bgTopBar: bgTopBar ?? this.bgTopBar, borderBar: borderBar ?? this.borderBar, + btnLabelAttLowIntDanger: btnLabelAttLowIntDanger ?? this.btnLabelAttLowIntDanger, + btnLabelAttMediumIntDanger: btnLabelAttMediumIntDanger ?? this.btnLabelAttMediumIntDanger, composeBoxBg: composeBoxBg ?? this.composeBoxBg, contextMenuCancelText: contextMenuCancelText ?? this.contextMenuCancelText, contextMenuItemBg: contextMenuItemBg ?? this.contextMenuItemBg, @@ -351,9 +354,6 @@ class DesignVariables extends ThemeExtension { atMentionMarker: atMentionMarker ?? this.atMentionMarker, contextMenuCancelBg: contextMenuCancelBg ?? this.contextMenuCancelBg, dmHeaderBg: dmHeaderBg ?? this.dmHeaderBg, - errorBannerBackground: errorBannerBackground ?? this.errorBannerBackground, - errorBannerBorder: errorBannerBorder ?? this.errorBannerBorder, - errorBannerLabel: errorBannerLabel ?? this.errorBannerLabel, groupDmConversationIcon: groupDmConversationIcon ?? this.groupDmConversationIcon, groupDmConversationIconBg: groupDmConversationIconBg ?? this.groupDmConversationIconBg, loginOrDivider: loginOrDivider ?? this.loginOrDivider, @@ -375,10 +375,13 @@ class DesignVariables extends ThemeExtension { } return DesignVariables._( background: Color.lerp(background, other.background, t)!, + bannerBgIntDanger: Color.lerp(bannerBgIntDanger, other.bannerBgIntDanger, t)!, bgContextMenu: Color.lerp(bgContextMenu, other.bgContextMenu, t)!, bgCounterUnread: Color.lerp(bgCounterUnread, other.bgCounterUnread, t)!, bgTopBar: Color.lerp(bgTopBar, other.bgTopBar, t)!, borderBar: Color.lerp(borderBar, other.borderBar, t)!, + btnLabelAttLowIntDanger: Color.lerp(btnLabelAttLowIntDanger, other.btnLabelAttLowIntDanger, t)!, + btnLabelAttMediumIntDanger: Color.lerp(btnLabelAttMediumIntDanger, other.btnLabelAttMediumIntDanger, t)!, composeBoxBg: Color.lerp(composeBoxBg, other.composeBoxBg, t)!, contextMenuCancelText: Color.lerp(contextMenuCancelText, other.contextMenuCancelText, t)!, contextMenuItemBg: Color.lerp(contextMenuItemBg, other.contextMenuItemBg, t)!, @@ -396,9 +399,6 @@ class DesignVariables extends ThemeExtension { atMentionMarker: Color.lerp(atMentionMarker, other.atMentionMarker, t)!, contextMenuCancelBg: Color.lerp(contextMenuCancelBg, other.contextMenuCancelBg, t)!, dmHeaderBg: Color.lerp(dmHeaderBg, other.dmHeaderBg, t)!, - errorBannerBackground: Color.lerp(errorBannerBackground, other.errorBannerBackground, t)!, - errorBannerBorder: Color.lerp(errorBannerBorder, other.errorBannerBorder, t)!, - errorBannerLabel: Color.lerp(errorBannerLabel, other.errorBannerLabel, t)!, groupDmConversationIcon: Color.lerp(groupDmConversationIcon, other.groupDmConversationIcon, t)!, groupDmConversationIconBg: Color.lerp(groupDmConversationIconBg, other.groupDmConversationIconBg, t)!, loginOrDivider: Color.lerp(loginOrDivider, other.loginOrDivider, t)!, From 11072edac21afff5c95d870eeb920f9ced99672e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 14:46:46 -0800 Subject: [PATCH 16/24] compose [nfc]: Add TODO for overlaid linear progress indicator --- lib/widgets/compose_box.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 0a73cb927d..60f6ff2db3 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1060,6 +1060,7 @@ class _ComposeBoxContainer extends StatelessWidget { return Container(width: double.infinity, decoration: BoxDecoration( border: Border(top: BorderSide(color: designVariables.borderBar))), + // TODO(#720) try a Stack for the overlaid linear progress indicator child: Material( color: designVariables.composeBoxBg, child: Column( From a8837620e11f5d27c72b21bc6ec4e8517f117093 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 26 Nov 2024 14:56:20 -0800 Subject: [PATCH 17/24] compose [nfc]: Simplify choose-body logic by switching on controller --- lib/widgets/compose_box.dart | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 60f6ff2db3..36801809ac 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1351,21 +1351,15 @@ class _ComposeBoxState extends State implements ComposeBoxState { } final narrow = widget.narrow; - switch (narrow) { - case ChannelNarrow(): - _controller as StreamComposeBoxController; + switch (_controller) { + case StreamComposeBoxController(): { + narrow as ChannelNarrow; body = _StreamComposeBoxBody(controller: _controller, narrow: narrow); - case TopicNarrow(): - _controller as FixedDestinationComposeBoxController; - body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); - case DmNarrow(): - _controller as FixedDestinationComposeBoxController; + } + case FixedDestinationComposeBoxController(): { + narrow as SendableNarrow; body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); - case CombinedFeedNarrow(): - case MentionsNarrow(): - case StarredMessagesNarrow(): - assert(false); - body = null; + } } // TODO(#720) dismissable message-send error, maybe something like: From 0c5af34beb80d0db441cc7d3f67034cd83fb1f62 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 12:25:45 -0800 Subject: [PATCH 18/24] compose [nfc]: Pass whole `controller` down (1/6); _StreamContentInput For #720, we'd like to add fields to ComposeBoxController and use them in a bunch of widgets deeper in the compose box, like the various buttons, without having to add references to them in those widgets' constructors. With that in mind, pass the whole controller down, and the widgets can access the fields directly from it. As suggested by Greg: https://github.com/zulip/zulip-flutter/pull/1090#discussion_r1860183031 --- lib/widgets/compose_box.dart | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 36801809ac..274b2b4a31 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -442,17 +442,10 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve /// The content input for _StreamComposeBox. class _StreamContentInput extends StatefulWidget { - const _StreamContentInput({ - required this.narrow, - required this.controller, - required this.topicController, - required this.focusNode, - }); + const _StreamContentInput({required this.narrow, required this.controller}); final ChannelNarrow narrow; - final ComposeContentController controller; - final ComposeTopicController topicController; - final FocusNode focusNode; + final StreamComposeBoxController controller; @override State<_StreamContentInput> createState() => _StreamContentInputState(); @@ -463,29 +456,29 @@ class _StreamContentInputState extends State<_StreamContentInput> { void _topicChanged() { setState(() { - _topicTextNormalized = widget.topicController.textNormalized; + _topicTextNormalized = widget.controller.topic.textNormalized; }); } @override void initState() { super.initState(); - _topicTextNormalized = widget.topicController.textNormalized; - widget.topicController.addListener(_topicChanged); + _topicTextNormalized = widget.controller.topic.textNormalized; + widget.controller.topic.addListener(_topicChanged); } @override void didUpdateWidget(covariant _StreamContentInput oldWidget) { super.didUpdateWidget(oldWidget); - if (widget.topicController != oldWidget.topicController) { - oldWidget.topicController.removeListener(_topicChanged); - widget.topicController.addListener(_topicChanged); + if (widget.controller.topic != oldWidget.controller.topic) { + oldWidget.controller.topic.removeListener(_topicChanged); + widget.controller.topic.addListener(_topicChanged); } } @override void dispose() { - widget.topicController.removeListener(_topicChanged); + widget.controller.topic.removeListener(_topicChanged); super.dispose(); } @@ -498,8 +491,8 @@ class _StreamContentInputState extends State<_StreamContentInput> { return _ContentInput( narrow: widget.narrow, destination: TopicNarrow(widget.narrow.streamId, _topicTextNormalized), - controller: widget.controller, - focusNode: widget.focusNode, + controller: widget.controller.content, + focusNode: widget.controller.contentFocusNode, hintText: zulipLocalizations.composeBoxChannelContentHint(streamName, _topicTextNormalized)); } } @@ -1156,9 +1149,7 @@ class _StreamComposeBoxBody extends _ComposeBoxBody { @override Widget buildContentInput() => _StreamContentInput( narrow: narrow, - topicController: controller.topic, - controller: controller.content, - focusNode: controller.contentFocusNode, + controller: controller, ); @override Widget buildSendButton() => _SendButton( From 6bc616963d4123afd5a6748bfad59a6257da0787 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 12:52:07 -0800 Subject: [PATCH 19/24] compose [nfc]: Pass `controller` down (2/6); _FixedDestinationContentInput --- lib/widgets/compose_box.dart | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 274b2b4a31..07f69c62f2 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -545,12 +545,10 @@ class _FixedDestinationContentInput extends StatelessWidget { const _FixedDestinationContentInput({ required this.narrow, required this.controller, - required this.focusNode, }); final SendableNarrow narrow; - final ComposeContentController controller; - final FocusNode focusNode; + final FixedDestinationComposeBoxController controller; String _hintText(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); @@ -580,8 +578,8 @@ class _FixedDestinationContentInput extends StatelessWidget { return _ContentInput( narrow: narrow, destination: narrow, - controller: controller, - focusNode: focusNode, + controller: controller.content, + focusNode: controller.contentFocusNode, hintText: _hintText(context)); } } @@ -1173,8 +1171,7 @@ class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { @override Widget buildContentInput() => _FixedDestinationContentInput( narrow: narrow, - controller: controller.content, - focusNode: controller.contentFocusNode, + controller: controller, ); @override Widget buildSendButton() => _SendButton( From f65dc800d523bc303e632cb9a0fdf902dec30cb9 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 12:55:26 -0800 Subject: [PATCH 20/24] compose [nfc]: Pass `controller` down (3/6); _ContentInput --- lib/widgets/compose_box.dart | 40 +++++++++++++++--------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 07f69c62f2..d80c4a7534 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -277,14 +277,12 @@ class _ContentInput extends StatefulWidget { required this.narrow, required this.destination, required this.controller, - required this.focusNode, required this.hintText, }); final Narrow narrow; final SendableNarrow destination; - final ComposeContentController controller; - final FocusNode focusNode; + final ComposeBoxController controller; final String hintText; @override @@ -295,8 +293,8 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve @override void initState() { super.initState(); - widget.controller.addListener(_contentChanged); - widget.focusNode.addListener(_focusChanged); + widget.controller.content.addListener(_contentChanged); + widget.controller.contentFocusNode.addListener(_focusChanged); WidgetsBinding.instance.addObserver(this); } @@ -304,32 +302,30 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve void didUpdateWidget(covariant _ContentInput oldWidget) { super.didUpdateWidget(oldWidget); if (widget.controller != oldWidget.controller) { - oldWidget.controller.removeListener(_contentChanged); - widget.controller.addListener(_contentChanged); - } - if (widget.focusNode != oldWidget.focusNode) { - oldWidget.focusNode.removeListener(_focusChanged); - widget.focusNode.addListener(_focusChanged); + oldWidget.controller.content.removeListener(_contentChanged); + widget.controller.content.addListener(_contentChanged); + oldWidget.controller.contentFocusNode.removeListener(_focusChanged); + widget.controller.contentFocusNode.addListener(_focusChanged); } } @override void dispose() { - widget.controller.removeListener(_contentChanged); - widget.focusNode.removeListener(_focusChanged); + widget.controller.content.removeListener(_contentChanged); + widget.controller.contentFocusNode.removeListener(_focusChanged); WidgetsBinding.instance.removeObserver(this); super.dispose(); } void _contentChanged() { final store = PerAccountStoreWidget.of(context); - (widget.controller.text.isEmpty) + (widget.controller.content.text.isEmpty) ? store.typingNotifier.stoppedComposing() : store.typingNotifier.keystroke(widget.destination); } void _focusChanged() { - if (widget.focusNode.hasFocus) { + if (widget.controller.contentFocusNode.hasFocus) { // Content input getting focus doesn't necessarily mean that // the user started typing, so do nothing. return; @@ -397,8 +393,8 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve return ComposeAutocomplete( narrow: widget.narrow, - controller: widget.controller, - focusNode: widget.focusNode, + controller: widget.controller.content, + focusNode: widget.controller.contentFocusNode, fieldViewBuilder: (context) => ConstrainedBox( constraints: BoxConstraints(maxHeight: maxHeight(context)), // This [ClipRect] replaces the [TextField] clipping we disable below. @@ -407,8 +403,8 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve top: _verticalPadding, bottom: _verticalPadding, color: designVariables.composeBoxBg, child: TextField( - controller: widget.controller, - focusNode: widget.focusNode, + controller: widget.controller.content, + focusNode: widget.controller.contentFocusNode, // Let the content show through the `contentPadding` so that // our [InsetShadowBox] can fade it smoothly there. clipBehavior: Clip.none, @@ -491,8 +487,7 @@ class _StreamContentInputState extends State<_StreamContentInput> { return _ContentInput( narrow: widget.narrow, destination: TopicNarrow(widget.narrow.streamId, _topicTextNormalized), - controller: widget.controller.content, - focusNode: widget.controller.contentFocusNode, + controller: widget.controller, hintText: zulipLocalizations.composeBoxChannelContentHint(streamName, _topicTextNormalized)); } } @@ -578,8 +573,7 @@ class _FixedDestinationContentInput extends StatelessWidget { return _ContentInput( narrow: narrow, destination: narrow, - controller: controller.content, - focusNode: controller.contentFocusNode, + controller: controller, hintText: _hintText(context)); } } From 95b77f63ede554c03b7cd1b6cf158e08147ba1f7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 12:31:15 -0800 Subject: [PATCH 21/24] compose [nfc]: Pass `controller` down (4/6); _TopicInput --- lib/widgets/compose_box.dart | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index d80c4a7534..72c7e992ea 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -493,16 +493,10 @@ class _StreamContentInputState extends State<_StreamContentInput> { } class _TopicInput extends StatelessWidget { - const _TopicInput({ - required this.streamId, - required this.controller, - required this.focusNode, - required this.contentFocusNode}); + const _TopicInput({required this.streamId, required this.controller}); final int streamId; - final ComposeTopicController controller; - final FocusNode focusNode; - final FocusNode contentFocusNode; + final StreamComposeBoxController controller; @override Widget build(BuildContext context) { @@ -516,17 +510,17 @@ class _TopicInput extends StatelessWidget { return TopicAutocomplete( streamId: streamId, - controller: controller, - focusNode: focusNode, - contentFocusNode: contentFocusNode, + controller: controller.topic, + focusNode: controller.topicFocusNode, + contentFocusNode: controller.contentFocusNode, fieldViewBuilder: (context) => Container( padding: const EdgeInsets.only(top: 10, bottom: 9), decoration: BoxDecoration(border: Border(bottom: BorderSide( width: 1, color: designVariables.foreground.withFadedAlpha(0.2)))), child: TextField( - controller: controller, - focusNode: focusNode, + controller: controller.topic, + focusNode: controller.topicFocusNode, textInputAction: TextInputAction.next, style: topicTextStyle, decoration: InputDecoration( @@ -1134,9 +1128,7 @@ class _StreamComposeBoxBody extends _ComposeBoxBody { @override Widget buildTopicInput() => _TopicInput( streamId: narrow.streamId, - controller: controller.topic, - focusNode: controller.topicFocusNode, - contentFocusNode: controller.contentFocusNode, + controller: controller, ); @override Widget buildContentInput() => _StreamContentInput( From 29470d30145901ebfddf8c772057980d1961e88e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 12:48:47 -0800 Subject: [PATCH 22/24] compose [nfc]: Pass `controller` down (5/6); _SendButton --- lib/widgets/compose_box.dart | 68 +++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 72c7e992ea..eda817a3ef 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -867,14 +867,9 @@ class _AttachFromCameraButton extends _AttachUploadsButton { } class _SendButton extends StatefulWidget { - const _SendButton({ - required this.topicController, - required this.contentController, - required this.getDestination, - }); + const _SendButton({required this.controller, required this.getDestination}); - final ComposeTopicController? topicController; - final ComposeContentController contentController; + final ComposeBoxController controller; final MessageDestination Function() getDestination; @override @@ -891,43 +886,62 @@ class _SendButtonState extends State<_SendButton> { @override void initState() { super.initState(); - widget.topicController?.hasValidationErrors.addListener(_hasErrorsChanged); - widget.contentController.hasValidationErrors.addListener(_hasErrorsChanged); + final controller = widget.controller; + if (controller is StreamComposeBoxController) { + controller.topic.hasValidationErrors.addListener(_hasErrorsChanged); + } + controller.content.hasValidationErrors.addListener(_hasErrorsChanged); } @override void didUpdateWidget(covariant _SendButton oldWidget) { super.didUpdateWidget(oldWidget); - if (widget.topicController != oldWidget.topicController) { - oldWidget.topicController?.hasValidationErrors.removeListener(_hasErrorsChanged); - widget.topicController?.hasValidationErrors.addListener(_hasErrorsChanged); + + final controller = widget.controller; + final oldController = oldWidget.controller; + if (controller == oldController) return; + + if (oldController is StreamComposeBoxController) { + oldController.topic.hasValidationErrors.removeListener(_hasErrorsChanged); } - if (widget.contentController != oldWidget.contentController) { - oldWidget.contentController.hasValidationErrors.removeListener(_hasErrorsChanged); - widget.contentController.hasValidationErrors.addListener(_hasErrorsChanged); + if (controller is StreamComposeBoxController) { + controller.topic.hasValidationErrors.addListener(_hasErrorsChanged); } + oldController.content.hasValidationErrors.removeListener(_hasErrorsChanged); + controller.content.hasValidationErrors.addListener(_hasErrorsChanged); } @override void dispose() { - widget.topicController?.hasValidationErrors.removeListener(_hasErrorsChanged); - widget.contentController.hasValidationErrors.removeListener(_hasErrorsChanged); + final controller = widget.controller; + if (controller is StreamComposeBoxController) { + controller.topic.hasValidationErrors.removeListener(_hasErrorsChanged); + } + controller.content.hasValidationErrors.removeListener(_hasErrorsChanged); super.dispose(); } bool get _hasValidationErrors { - return (widget.topicController?.hasValidationErrors.value ?? false) - || widget.contentController.hasValidationErrors.value; + bool result = false; + final controller = widget.controller; + if (controller is StreamComposeBoxController) { + result = controller.topic.hasValidationErrors.value; + } + result |= controller.content.hasValidationErrors.value; + return result; } void _send() async { + final controller = widget.controller; + if (_hasValidationErrors) { final zulipLocalizations = ZulipLocalizations.of(context); List validationErrorMessages = [ - for (final error in widget.topicController?.validationErrors - ?? const []) + for (final error in (controller is StreamComposeBoxController + ? controller.topic.validationErrors + : const [])) error.message(zulipLocalizations), - for (final error in widget.contentController.validationErrors) + for (final error in controller.content.validationErrors) error.message(zulipLocalizations), ]; showErrorDialog( @@ -938,9 +952,9 @@ class _SendButtonState extends State<_SendButton> { } final store = PerAccountStoreWidget.of(context); - final content = widget.contentController.textNormalized; + final content = controller.content.textNormalized; - widget.contentController.clear(); + controller.content.clear(); // The following `stoppedComposing` call is currently redundant, // because clearing input sends a "typing stopped" notice. // It will be necessary once we resolve #720. @@ -1137,8 +1151,7 @@ class _StreamComposeBoxBody extends _ComposeBoxBody { ); @override Widget buildSendButton() => _SendButton( - topicController: controller.topic, - contentController: controller.content, + controller: controller, getDestination: () => StreamDestination( narrow.streamId, controller.topic.textNormalized), ); @@ -1161,8 +1174,7 @@ class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { ); @override Widget buildSendButton() => _SendButton( - topicController: null, - contentController: controller.content, + controller: controller, getDestination: () => narrow.destination, ); } From e1fc763820bd3e7b4ea655a38039d65a285230fe Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 2 Dec 2024 13:00:22 -0800 Subject: [PATCH 23/24] compose [nfc]: Pass `controller` down (6/6); _AttachUploadsButton --- lib/widgets/compose_box.dart | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index eda817a3ef..87d670c217 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -658,10 +658,9 @@ Future _uploadFiles({ } abstract class _AttachUploadsButton extends StatelessWidget { - const _AttachUploadsButton({required this.contentController, required this.contentFocusNode}); + const _AttachUploadsButton({required this.controller}); - final ComposeContentController contentController; - final FocusNode contentFocusNode; + final ComposeBoxController controller; IconData get icon; String tooltip(ZulipLocalizations zulipLocalizations); @@ -689,8 +688,8 @@ abstract class _AttachUploadsButton extends StatelessWidget { await _uploadFiles( context: context, - contentController: contentController, - contentFocusNode: contentFocusNode, + contentController: controller.content, + contentFocusNode: controller.contentFocusNode, files: files); } @@ -762,7 +761,7 @@ Future> _getFilePickerFiles(BuildContext context, FileType type) } class _AttachFileButton extends _AttachUploadsButton { - const _AttachFileButton({required super.contentController, required super.contentFocusNode}); + const _AttachFileButton({required super.controller}); @override IconData get icon => ZulipIcons.attach_file; @@ -778,7 +777,7 @@ class _AttachFileButton extends _AttachUploadsButton { } class _AttachMediaButton extends _AttachUploadsButton { - const _AttachMediaButton({required super.contentController, required super.contentFocusNode}); + const _AttachMediaButton({required super.controller}); @override IconData get icon => ZulipIcons.image; @@ -795,7 +794,7 @@ class _AttachMediaButton extends _AttachUploadsButton { } class _AttachFromCameraButton extends _AttachUploadsButton { - const _AttachFromCameraButton({required super.contentController, required super.contentFocusNode}); + const _AttachFromCameraButton({required super.controller}); @override IconData get icon => ZulipIcons.camera; @@ -1096,11 +1095,10 @@ abstract class _ComposeBoxBody extends StatelessWidget { shape: const RoundedRectangleBorder( borderRadius: BorderRadius.all(Radius.circular(4))))); - final ComposeBoxController(:content, :contentFocusNode) = controller; final composeButtons = [ - _AttachFileButton(contentController: content, contentFocusNode: contentFocusNode), - _AttachMediaButton(contentController: content, contentFocusNode: contentFocusNode), - _AttachFromCameraButton(contentController: content, contentFocusNode: contentFocusNode), + _AttachFileButton(controller: controller), + _AttachMediaButton(controller: controller), + _AttachFromCameraButton(controller: controller), ]; final topicInput = buildTopicInput(); From f46187af1f366e58fa45c5a4b2678a966b24dbdc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 3 Dec 2024 14:00:31 -0800 Subject: [PATCH 24/24] compose [nfc]: Simplify controller getters to final fields A private final field exposed by a public getter is equivalent to a public final field: either means that any library can get, and no library can set. --- lib/widgets/compose_box.dart | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 87d670c217..114e392bc7 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1178,30 +1178,24 @@ class _FixedDestinationComposeBoxBody extends _ComposeBoxBody { } sealed class ComposeBoxController { - ComposeContentController get content => _content; - final _content = ComposeContentController(); - - FocusNode get contentFocusNode => _contentFocusNode; - final _contentFocusNode = FocusNode(); + final content = ComposeContentController(); + final contentFocusNode = FocusNode(); @mustCallSuper void dispose() { - _content.dispose(); - _contentFocusNode.dispose(); + content.dispose(); + contentFocusNode.dispose(); } } class StreamComposeBoxController extends ComposeBoxController { - ComposeTopicController get topic => _topic; - final _topic = ComposeTopicController(); - - FocusNode get topicFocusNode => _topicFocusNode; - final _topicFocusNode = FocusNode(); + final topic = ComposeTopicController(); + final topicFocusNode = FocusNode(); @override void dispose() { - _topic.dispose(); - _topicFocusNode.dispose(); + topic.dispose(); + topicFocusNode.dispose(); super.dispose(); } }