diff --git a/lib/model/message.dart b/lib/model/message.dart index 24788bdd1e..4e080c4502 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -53,15 +53,18 @@ 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]. /// + /// The returned [Future] resolves or rejects with the edit-message request, + /// irrespective of when the edit-message event arrives (if it does). + /// /// See also: /// * [getEditMessageErrorStatus] /// * [takeFailedMessageEdit] - void editMessage({ + Future editMessage({ required int messageId, required String originalRawContent, required String newContent, @@ -104,7 +107,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 +318,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 +352,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 99eeef3b0d..b775a81b90 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'; @@ -1659,6 +1660,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(); @@ -1675,7 +1679,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]. /// @@ -1694,7 +1702,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)), @@ -1740,7 +1748,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: @@ -1766,17 +1774,17 @@ class _EditMessageBanner extends _Banner { Color getBackgroundColor(DesignVariables designVariables) => designVariables.bannerBgIntInfo; - void _handleTapSave (BuildContext context) { - final store = PerAccountStoreWidget.of(context); + void _handleTapSave (BuildContext pageContext) async { + 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; @@ -1789,16 +1797,32 @@ class _EditMessageBanner extends _Banner { return; } - store.editMessage( - messageId: controller.messageId, - originalRawContent: originalRawContent, - newContent: controller.content.textNormalized); + final messageId = controller.messageId; + final newContent = controller.content.textNormalized; composeBoxState.endEditInteraction(); + + 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 - 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), @@ -1806,7 +1830,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)), ]); } } diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 69f349c02d..bc35fb67b1 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/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index f673ebc0d8..5f111269b0 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -1780,6 +1780,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. /// @@ -1870,6 +1876,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 14d60d5700..e415b732d3 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,12 +2293,14 @@ 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)); + // (the error dialog is tested elsewhere; + // it's triggered in the "Save" tap handler, not store.editMessage) checkEditFailed(tester); connection.prepare(json: GetMessageResult(