From 86a8ad9cce4dd8fa33515ddd78cfaca3690e6c70 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 5 Aug 2025 19:26:59 -0700 Subject: [PATCH 1/5] compose_box [nfc]: Reorder two calls in edit-message "Save" tap handler Making sure to capture some data that store.editMessage needs from the controller before that data is destroyed, in composeBoxState.endEditInteraction, which disposes the controller. Motivation: soon we'd like `store.editMessage` to return a Future, which we'd like to await and add some error feedback, just like we have in the send-message UX. --- lib/widgets/compose_box.dart | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 2d0160068f..257a31d994 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1815,11 +1815,13 @@ class _EditMessageBanner extends _Banner { return; } + final messageId = controller.messageId; + final newContent = controller.content.textNormalized; + composeBoxState.endEditInteraction(); store.editMessage( - messageId: controller.messageId, + messageId: messageId, originalRawContent: originalRawContent, - newContent: controller.content.textNormalized); - composeBoxState.endEditInteraction(); + newContent: newContent); } @override From 11ee6e9bd603bb42949c80f4083542e3de804b9e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 5 Aug 2025 19:38:40 -0700 Subject: [PATCH 2/5] message [nfc]: Mention outbox-like logic in in editMessage dartdoc --- lib/model/message.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index ac5e65186b..74c4454488 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -53,7 +53,7 @@ mixin MessageStore { /// and the update-message event hasn't arrived. bool? getEditMessageErrorStatus(int messageId); - /// Edit a message's content, via a request to the server. + /// Makes an edit-message request and starts an edit-outbox lifecycle. /// /// Should only be called when there is no current edit request for [messageId], /// i.e., [getEditMessageErrorStatus] returns null for [messageId]. From fffc6fcb70bdbefb94c5f9e385174a0072896977 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 5 Aug 2025 19:38:03 -0700 Subject: [PATCH 3/5] message [nfc]: Have editMessage return Future instead of void --- lib/model/message.dart | 12 +++++-- lib/widgets/compose_box.dart | 2 +- test/model/message_test.dart | 49 +++++++++++++++-------------- test/widgets/message_list_test.dart | 10 +++--- 4 files changed, 41 insertions(+), 32 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 74c4454488..48bdbf5a49 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -58,10 +58,15 @@ mixin MessageStore { /// Should only be called when there is no current edit request for [messageId], /// i.e., [getEditMessageErrorStatus] returns null for [messageId]. /// + /// The returned [Future] settles when the edit-message response is received. + /// The [Future] resolves if the request succeeded and rejects if it failed, + /// unless the event already arrived or the message was deleted, + /// in which case it resolves. + /// /// See also: /// * [getEditMessageErrorStatus] /// * [takeFailedMessageEdit] - void editMessage({ + Future editMessage({ required int messageId, required String originalRawContent, required String newContent, @@ -104,7 +109,7 @@ mixin ProxyMessageStore on MessageStore { return messageStore.getEditMessageErrorStatus(messageId); } @override - void editMessage({ + Future editMessage({ required int messageId, required String originalRawContent, required String newContent, @@ -315,7 +320,7 @@ class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageSt final Map _editMessageRequests = {}; @override - void editMessage({ + Future editMessage({ required int messageId, required String originalRawContent, required String newContent, @@ -349,6 +354,7 @@ class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageSt } status.hasError = true; _notifyMessageListViewsForOneMessage(messageId); + rethrow; } } diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 257a31d994..7997c88128 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1821,7 +1821,7 @@ class _EditMessageBanner extends _Banner { store.editMessage( messageId: messageId, originalRawContent: originalRawContent, - newContent: newContent); + newContent: newContent).onError((_, _) {}); // TODO show error feedback } @override diff --git a/test/model/message_test.dart b/test/model/message_test.dart index a7d60e8fac..79aa441771 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -7,6 +7,7 @@ import 'package:crypto/crypto.dart'; import 'package:fake_async/fake_async.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/submessage.dart'; @@ -603,8 +604,8 @@ void main() { connection.prepare( json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); checkRequest(message.id, prevContent: 'old content', content: 'new content'); @@ -634,8 +635,8 @@ void main() { connection.prepare( json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); checkRequest(message.id, prevContent: 'old content', content: 'new content'); @@ -647,8 +648,8 @@ void main() { check(store.getEditMessageErrorStatus(otherMessage.id)).isNull(); connection.prepare( json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); - store.editMessage(messageId: otherMessage.id, - originalRawContent: 'other message old content', newContent: 'other message new content'); + unawaited(store.editMessage(messageId: otherMessage.id, + originalRawContent: 'other message old content', newContent: 'other message new content')); checkRequest(otherMessage.id, prevContent: 'other message old content', content: 'other message new content'); @@ -682,8 +683,8 @@ void main() { check(store.getEditMessageErrorStatus(message.id)).isNull(); connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(check(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')).throws()); checkNotifiedOnce(); async.elapse(Duration(seconds: 1)); check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); @@ -695,8 +696,8 @@ void main() { check(store.getEditMessageErrorStatus(message.id)).isNull(); connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(check(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')).throws()); checkNotifiedOnce(); async.elapse(Duration(seconds: 1)); check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); @@ -718,8 +719,8 @@ void main() { connection.prepare( json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); async.elapse(Duration(milliseconds: 500)); check(connection.takeRequests()).length.equals(1); checkNotifiedOnce(); @@ -738,8 +739,8 @@ void main() { connection.prepare( httpException: const SocketException('failed'), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); checkNotifiedOnce(); async.elapse(Duration(milliseconds: 500)); @@ -760,8 +761,8 @@ void main() { connection.prepare( httpException: const SocketException('failed'), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(check(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')).throws()); checkNotifiedOnce(); async.elapse(Duration(seconds: 1)); @@ -781,8 +782,8 @@ void main() { connection.prepare( httpException: const SocketException('failed'), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(check(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')).throws()); checkNotifiedOnce(); async.elapse(Duration(seconds: 1)); @@ -801,8 +802,8 @@ void main() { check(store.getEditMessageErrorStatus(message.id)).isNull(); connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(check(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')).throws()); checkNotifiedOnce(); async.elapse(Duration(seconds: 1)); check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); @@ -818,8 +819,8 @@ void main() { check(store.getEditMessageErrorStatus(message.id)).isNull(); connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); checkNotifiedOnce(); async.elapse(Duration(milliseconds: 500)); @@ -843,8 +844,8 @@ void main() { connection.prepare( json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, - originalRawContent: 'old content', newContent: 'new content'); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); checkNotifiedOnce(); async.elapse(Duration(milliseconds: 500)); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 14d60d5700..e426c4dce3 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -9,6 +10,7 @@ import 'package:flutter/rendering.dart'; import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; @@ -2274,9 +2276,9 @@ void main() { messages: [message]); connection.prepare(json: UpdateMessageResult().toJson()); - store.editMessage(messageId: message.id, + unawaited(store.editMessage(messageId: message.id, originalRawContent: 'foo', - newContent: 'bar'); + newContent: 'bar')); await tester.pump(Duration.zero); checkEditInProgress(tester); await store.handleEvent(eg.updateMessageEditEvent(message)); @@ -2291,9 +2293,9 @@ void main() { messages: [message]); connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); - store.editMessage(messageId: message.id, + unawaited(check(store.editMessage(messageId: message.id, originalRawContent: 'foo', - newContent: 'bar'); + newContent: 'bar')).throws()); await tester.pump(Duration.zero); checkEditInProgress(tester); await tester.pump(Duration(seconds: 1)); From ef67a67529d37e7e0d47c88124712522d07d2f06 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 5 Aug 2025 20:10:50 -0700 Subject: [PATCH 4/5] compose [nfc]: Pass PageRoot.contextOf to _Banner.buildTrailing See added dartdoc for motivation. I don't think there's a behavior change here; we don't use the passed BuildContext after an async gap. We'd like to do that soon, though: to show an error dialog on API errors from the edit-message request; such errors will arrive after the banner has disappeared. --- lib/widgets/compose_box.dart | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 7997c88128..c1c57a9434 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -23,6 +23,7 @@ import 'color.dart'; import 'dialog.dart'; import 'icons.dart'; import 'inset_shadow.dart'; +import 'page.dart'; import 'store.dart'; import 'text.dart'; import 'theme.dart'; @@ -1685,6 +1686,9 @@ class EditMessageComposeBoxController extends ComposeBoxController { String? originalRawContent; } +/// A banner to display over or instead of interactive compose-box content. +/// +/// Must have a [PageRoot] ancestor. abstract class _Banner extends StatelessWidget { const _Banner(); @@ -1701,7 +1705,11 @@ abstract class _Banner extends StatelessWidget { /// https://github.com/zulip/zulip-flutter/pull/1432#discussion_r2023907300 /// /// To control the element's distance from the end edge, override [padEnd]. - Widget? buildTrailing(BuildContext context); + /// + /// The passed [BuildContext] will be the result of [PageRoot.contextOf], + /// so it's expected to remain mounted until the whole page disappears, + /// which may be long after the banner disappears. + Widget? buildTrailing(BuildContext pageContext); /// Whether to apply `end: 8` in [SafeArea.minimum]. /// @@ -1720,7 +1728,7 @@ abstract class _Banner extends StatelessWidget { color: getLabelColor(designVariables), ).merge(weightVariableTextStyle(context, wght: 600)); - final trailing = buildTrailing(context); + final trailing = buildTrailing(PageRoot.contextOf(context)); return DecoratedBox( decoration: BoxDecoration( color: getBackgroundColor(designVariables)), @@ -1766,7 +1774,7 @@ class _ErrorBanner extends _Banner { designVariables.bannerBgIntDanger; @override - Widget? buildTrailing(context) { + Widget? buildTrailing(pageContext) { // An "x" button can go here. // 24px square with 8px touchable padding in all directions? // and `bool get padEnd => false`; see Figma: @@ -1792,17 +1800,17 @@ class _EditMessageBanner extends _Banner { Color getBackgroundColor(DesignVariables designVariables) => designVariables.bannerBgIntInfo; - void _handleTapSave (BuildContext context) { - final store = PerAccountStoreWidget.of(context); + void _handleTapSave (BuildContext pageContext) { + final store = PerAccountStoreWidget.of(pageContext); final controller = composeBoxState.controller; if (controller is! EditMessageComposeBoxController) return; // TODO(log) - final zulipLocalizations = ZulipLocalizations.of(context); + final zulipLocalizations = ZulipLocalizations.of(pageContext); if (controller.content.hasValidationErrors.value) { final validationErrorMessages = controller.content.validationErrors.map((error) => error.message(zulipLocalizations)); - showErrorDialog(context: context, + showErrorDialog(context: pageContext, title: zulipLocalizations.errorMessageEditNotSaved, message: validationErrorMessages.join('\n\n')); return; @@ -1825,8 +1833,8 @@ class _EditMessageBanner extends _Banner { } @override - Widget buildTrailing(context) { - final zulipLocalizations = ZulipLocalizations.of(context); + Widget buildTrailing(pageContext) { + final zulipLocalizations = ZulipLocalizations.of(pageContext); return Row(mainAxisSize: MainAxisSize.min, spacing: 8, children: [ ZulipWebUiKitButton(label: zulipLocalizations.composeBoxBannerButtonCancel, onPressed: composeBoxState.endEditInteraction), @@ -1834,7 +1842,7 @@ class _EditMessageBanner extends _Banner { // or the original raw content hasn't loaded yet ZulipWebUiKitButton(label: zulipLocalizations.composeBoxBannerButtonSave, attention: ZulipWebUiKitButtonAttention.high, - onPressed: () => _handleTapSave(context)), + onPressed: () => _handleTapSave(pageContext)), ]); } } From e7cd777cf338a53ab8d2ea1236a49d90100669d7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 5 Aug 2025 20:07:59 -0700 Subject: [PATCH 5/5] compose: Show error dialog on failure of edit-message request I'm not finding the discussion where we said we wanted this (for consistency with the send-message UX), but I think we did. Error-feedback logic copied from the tap-"Send" button (_SendButtonState._send) modulo the error dialog's title, "Message not saved" vs. "Message not sent". --- lib/widgets/compose_box.dart | 24 +++++++++++++++++++----- test/widgets/action_sheet_test.dart | 7 +++++++ test/widgets/compose_box_test.dart | 9 +++++++++ test/widgets/message_list_test.dart | 2 ++ 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index c1c57a9434..1d536b88c6 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1800,7 +1800,7 @@ class _EditMessageBanner extends _Banner { Color getBackgroundColor(DesignVariables designVariables) => designVariables.bannerBgIntInfo; - void _handleTapSave (BuildContext pageContext) { + void _handleTapSave (BuildContext pageContext) async { final store = PerAccountStoreWidget.of(pageContext); final controller = composeBoxState.controller; if (controller is! EditMessageComposeBoxController) return; // TODO(log) @@ -1826,10 +1826,24 @@ class _EditMessageBanner extends _Banner { final messageId = controller.messageId; final newContent = controller.content.textNormalized; composeBoxState.endEditInteraction(); - store.editMessage( - messageId: messageId, - originalRawContent: originalRawContent, - newContent: newContent).onError((_, _) {}); // TODO show error feedback + + try { + await store.editMessage( + messageId: messageId, + originalRawContent: originalRawContent, + newContent: newContent); + } on ApiRequestException catch (e) { + if (!pageContext.mounted) return; + final zulipLocalizations = ZulipLocalizations.of(pageContext); + final message = switch (e) { + ZulipApiException() => zulipLocalizations.errorServerMessage(e.message), + _ => e.message, + }; + showErrorDialog(context: pageContext, + title: zulipLocalizations.errorMessageEditNotSaved, + message: message); + return; + } } @override diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 244535c4a1..211c513bfb 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -1952,6 +1952,12 @@ void main() { await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e } + Future takeErrorDialogAndPump(WidgetTester tester) async { + final errorDialog = checkErrorDialog(tester, expectedTitle: 'Message not saved'); + await tester.tap(find.byWidget(errorDialog)); + await tester.pump(); + } + group('present/absent appropriately', () { /// Test whether the edit-message button is visible, given params. /// @@ -2042,6 +2048,7 @@ void main() { connection.prepare(apiException: eg.apiBadRequest()); await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save')); await tester.pump(Duration.zero); + await takeErrorDialogAndPump(tester); } else if (errorStatus == false) { // We're testing the request-in-progress state. Prepare a delay, // tap Save, and wait through only part of the delay. diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index e6b3cf6760..406d582a47 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -1748,6 +1748,12 @@ void main() { await tester.pump(); // message list updates } + Future takeErrorDialogAndPump(WidgetTester tester) async { + final errorDialog = checkErrorDialog(tester, expectedTitle: 'Message not saved'); + await tester.tap(find.byWidget(errorDialog)); + await tester.pump(); + } + /// Check that the compose box is in the "Preparing…" state, /// awaiting the fetch-raw-content request. Future checkAwaitingRawMessageContent(WidgetTester tester) async { @@ -1770,6 +1776,7 @@ void main() { await tester.tap( find.widgetWithText(ZulipWebUiKitButton, 'Save'), warnIfMissed: false); await tester.pump(Duration.zero); + checkNoDialog(tester); check(connection.lastRequest).equals(lastRequest); } @@ -1788,6 +1795,7 @@ void main() { connection.prepare(apiException: eg.apiBadRequest()); await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save')); await tester.pump(Duration.zero); + await takeErrorDialogAndPump(tester); await tester.tap(find.text('EDIT NOT SAVED')); await tester.pump(); connection.takeRequests(); @@ -1966,6 +1974,7 @@ void main() { await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save')); connection.takeRequests(); await tester.pump(Duration.zero); + await takeErrorDialogAndPump(tester); checkNotInEditingMode(tester, narrow: narrow); check(find.text('EDIT NOT SAVED')).findsOne(); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index e426c4dce3..e415b732d3 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -2299,6 +2299,8 @@ void main() { await tester.pump(Duration.zero); checkEditInProgress(tester); await tester.pump(Duration(seconds: 1)); + // (the error dialog is tested elsewhere; + // it's triggered in the "Save" tap handler, not store.editMessage) checkEditFailed(tester); connection.prepare(json: GetMessageResult(