From 4808904e2d71ef59dca540d3f355278ca21055e4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Apr 2025 09:17:07 +0800 Subject: [PATCH 1/7] api: Add restrict-message-editing fields to InitialSnapshot --- lib/api/model/initial_snapshot.dart | 5 +++++ lib/api/model/initial_snapshot.g.dart | 6 ++++++ test/example_data.dart | 4 ++++ 3 files changed, 15 insertions(+) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 5882122baa..054230a256 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -79,6 +79,9 @@ class InitialSnapshot { /// https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member final int realmWaitingPeriodThreshold; + final bool realmAllowMessageEditing; + final int? realmMessageContentEditLimitSeconds; + final Map realmDefaultExternalAccounts; final int maxFileUploadSizeMib; @@ -137,6 +140,8 @@ class InitialSnapshot { required this.realmWildcardMentionPolicy, required this.realmMandatoryTopics, required this.realmWaitingPeriodThreshold, + required this.realmAllowMessageEditing, + required this.realmMessageContentEditLimitSeconds, required this.realmDefaultExternalAccounts, required this.maxFileUploadSizeMib, required this.serverEmojiDataUrl, diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 79cfbe5557..570d7c2bba 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -73,6 +73,9 @@ InitialSnapshot _$InitialSnapshotFromJson( realmMandatoryTopics: json['realm_mandatory_topics'] as bool, realmWaitingPeriodThreshold: (json['realm_waiting_period_threshold'] as num).toInt(), + realmAllowMessageEditing: json['realm_allow_message_editing'] as bool, + realmMessageContentEditLimitSeconds: + (json['realm_message_content_edit_limit_seconds'] as num?)?.toInt(), realmDefaultExternalAccounts: (json['realm_default_external_accounts'] as Map).map( (k, e) => MapEntry( @@ -133,6 +136,9 @@ Map _$InitialSnapshotToJson(InitialSnapshot instance) => 'realm_wildcard_mention_policy': instance.realmWildcardMentionPolicy, 'realm_mandatory_topics': instance.realmMandatoryTopics, 'realm_waiting_period_threshold': instance.realmWaitingPeriodThreshold, + 'realm_allow_message_editing': instance.realmAllowMessageEditing, + 'realm_message_content_edit_limit_seconds': + instance.realmMessageContentEditLimitSeconds, 'realm_default_external_accounts': instance.realmDefaultExternalAccounts, 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, 'server_emoji_data_url': instance.serverEmojiDataUrl?.toString(), diff --git a/test/example_data.dart b/test/example_data.dart index afa378aa7e..f31337d303 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -916,6 +916,8 @@ InitialSnapshot initialSnapshot({ RealmWildcardMentionPolicy? realmWildcardMentionPolicy, bool? realmMandatoryTopics, int? realmWaitingPeriodThreshold, + bool? realmAllowMessageEditing, + int? realmMessageContentEditLimitSeconds, Map? realmDefaultExternalAccounts, int? maxFileUploadSizeMib, Uri? serverEmojiDataUrl, @@ -953,6 +955,8 @@ InitialSnapshot initialSnapshot({ realmWildcardMentionPolicy: realmWildcardMentionPolicy ?? RealmWildcardMentionPolicy.everyone, realmMandatoryTopics: realmMandatoryTopics ?? true, realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0, + realmAllowMessageEditing: realmAllowMessageEditing ?? true, + realmMessageContentEditLimitSeconds: realmMessageContentEditLimitSeconds ?? 600, realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {}, maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, serverEmojiDataUrl: serverEmojiDataUrl From e7abf315858932343f1bcfa4a2a347e92c0ed189 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Apr 2025 09:23:54 +0800 Subject: [PATCH 2/7] store: Add restrict-message-editing fields to PerAccountStore --- lib/model/store.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index 049ee73922..939120113e 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -472,6 +472,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold, maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib, realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName, + realmAllowMessageEditing: initialSnapshot.realmAllowMessageEditing, + realmMessageContentEditLimitSeconds: initialSnapshot.realmMessageContentEditLimitSeconds, realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts, customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields), emailAddressVisibility: initialSnapshot.emailAddressVisibility, @@ -509,6 +511,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.realmWaitingPeriodThreshold, required this.maxFileUploadSizeMib, required String? realmEmptyTopicDisplayName, + required this.realmAllowMessageEditing, + required this.realmMessageContentEditLimitSeconds, required this.realmDefaultExternalAccounts, required this.customProfileFields, required this.emailAddressVisibility, @@ -563,6 +567,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor final bool realmMandatoryTopics; // TODO(#668): update this realm setting /// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold]. final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting + final bool realmAllowMessageEditing; // TODO(#668): update this realm setting + final int? realmMessageContentEditLimitSeconds; // TODO(#668): update this realm setting final int maxFileUploadSizeMib; // No event for this. /// The display name to use for empty topics. From 13c9d7afeed9554e8bb69e884031cc8621a84043 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 21 Mar 2025 16:44:03 -0700 Subject: [PATCH 3/7] message [nfc]: Make helper methods _notifyMessageListViews{,ForOneMessage} --- lib/model/message.dart | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 962d79eeab..fd5de1adbd 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -63,6 +63,18 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { assert(removed); } + void _notifyMessageListViewsForOneMessage(int messageId) { + for (final view in _messageListViews) { + view.notifyListenersIfMessagePresent(messageId); + } + } + + void _notifyMessageListViews(Iterable messageIds) { + for (final view in _messageListViews) { + view.notifyListenersIfAnyMessagePresent(messageIds); + } + } + void reassemble() { for (final view in _messageListViews) { view.reassemble(); @@ -142,9 +154,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { _handleUpdateMessageEventTimestamp(event); _handleUpdateMessageEventContent(event); _handleUpdateMessageEventMove(event); - for (final view in _messageListViews) { - view.notifyListenersIfAnyMessagePresent(event.messageIds); - } + _notifyMessageListViews(event.messageIds); } void _handleUpdateMessageEventTimestamp(UpdateMessageEvent event) { @@ -268,17 +278,15 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { : message.flags.remove(event.flag); } if (anyMessageFound) { - for (final view in _messageListViews) { - view.notifyListenersIfAnyMessagePresent(event.messages); - // TODO(#818): Support MentionsNarrow live-updates when handling - // @-mention flags. - - // To make it easier to re-star a message, we opt-out from supporting - // live-updates when starred flag is removed. - // - // TODO: Support StarredMessagesNarrow live-updates when starred flag - // is added. - } + // TODO(#818): Support MentionsNarrow live-updates when handling + // @-mention flags. + + // To make it easier to re-star a message, we opt-out from supporting + // live-updates when starred flag is removed. + // + // TODO: Support StarredMessagesNarrow live-updates when starred flag + // is added. + _notifyMessageListViews(event.messages); } } } @@ -305,10 +313,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { userId: event.userId, ); } - - for (final view in _messageListViews) { - view.notifyListenersIfMessagePresent(event.messageId); - } + _notifyMessageListViewsForOneMessage(event.messageId); } void handleSubmessageEvent(SubmessageEvent event) { From d5d0ea86e5e20f0b0348a8f33ae97e7e02039764 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 9 Apr 2025 11:56:15 +0800 Subject: [PATCH 4/7] compose: Clamp max scale factor of _Banner label To avoid wasting a lot of vertical space in the edit-message compose box (coming up) with large text-scale settings. Related: #1023 --- lib/widgets/compose_box.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index aabf592dae..52c9bbeab1 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1437,7 +1437,9 @@ abstract class _Banner extends StatelessWidget { Expanded( child: Padding( padding: const EdgeInsets.symmetric(vertical: 4), - child: Text(style: labelTextStyle, + child: Text( + style: labelTextStyle, + textScaler: MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 1.5), getLabel(zulipLocalizations)))), if (trailing != null) ...[ const SizedBox(width: 8), From 07d60a708e79b398d9e61f34c55c5bcb258463db Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 9 Apr 2025 11:43:10 +0800 Subject: [PATCH 5/7] compose [nfc]: Refactor outer vertical padding on label in _Banner Move vertical padding so it won't wrap the trailing element, when present (as a `buildTrailing` that returns non-null). When we add the "Cancel" and "Save" buttons for the edit-message banner, those will want to control their own outer vertical padding to make it respond to taps, because the painted button is supposed to be just 28px tall, which is too small for a touch target; see discussion linked in dartdoc. NFC because `buildTrailing` on _Banner's only subclass, _ErrorBanner, returns null. --- lib/widgets/compose_box.dart | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 52c9bbeab1..8fc92e7d26 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1400,7 +1400,13 @@ abstract class _Banner extends StatelessWidget { Color getLabelColor(DesignVariables designVariables); Color getBackgroundColor(DesignVariables designVariables); - /// A trailing element, with no outer padding for spacing/positioning. + /// A trailing element, with vertical but not horizontal outer padding + /// for spacing/positioning. + /// + /// An interactive element's touchable area should have height at least 44px, + /// with some of that as "slop" vertical outer padding above and below + /// what gets painted: + /// 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); @@ -1431,12 +1437,12 @@ abstract class _Banner extends StatelessWidget { // (SafeArea.minimum doesn't take an EdgeInsetsDirectional) .resolve(Directionality.of(context)), child: Padding( - padding: const EdgeInsetsDirectional.fromSTEB(8, 5, 0, 5), + padding: const EdgeInsetsDirectional.only(start: 8), child: Row( children: [ Expanded( child: Padding( - padding: const EdgeInsets.symmetric(vertical: 4), + padding: const EdgeInsets.symmetric(vertical: 9), child: Text( style: labelTextStyle, textScaler: MediaQuery.textScalerOf(context).clamp(maxScaleFactor: 1.5), From 03cd9b8e4b0d0c669f97c3b68dea6d31e7a16953 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 18 Apr 2025 09:15:04 +0800 Subject: [PATCH 6/7] dialog [nfc]: s/DialogStatus.{closed,result}/ Discussion: https://github.com/zulip/zulip-flutter/pull/1471#discussion_r2049569126 --- lib/widgets/dialog.dart | 6 +++--- lib/widgets/lightbox.dart | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/widgets/dialog.dart b/lib/widgets/dialog.dart index 7ff373db6d..60e430a8e2 100644 --- a/lib/widgets/dialog.dart +++ b/lib/widgets/dialog.dart @@ -22,16 +22,16 @@ Widget _dialogActionText(String text) { /// See also: /// * [showDialog], whose return value this class is intended to wrap. class DialogStatus { - const DialogStatus(this.closed); + const DialogStatus(this.result); /// Resolves when the dialog is closed. - final Future closed; + final Future result; } /// Displays an [AlertDialog] with a dismiss button /// and optional "Learn more" button. /// -/// The [DialogStatus.closed] field of the return value can be used +/// The [DialogStatus.result] field of the return value can be used /// for waiting for the dialog to be closed. // This API is inspired by [ScaffoldManager.showSnackBar]. We wrap // [showDialog]'s return value, a [Future], inside [DialogStatus] diff --git a/lib/widgets/lightbox.dart b/lib/widgets/lightbox.dart index 0ddad1ef38..a66e4137dd 100644 --- a/lib/widgets/lightbox.dart +++ b/lib/widgets/lightbox.dart @@ -484,7 +484,7 @@ class _VideoLightboxPageState extends State with PerAccountSt context: context, title: zulipLocalizations.errorDialogTitle, message: zulipLocalizations.errorVideoPlayerFailed); - await dialog.closed; + await dialog.result; if (!mounted) return; Navigator.pop(context); // Pops the lightbox } From 459624571254ed25101e45fdf06ba04321c21a36 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 9 Apr 2025 15:40:09 +0800 Subject: [PATCH 7/7] dialog [nfc]: showSuggestedActionDialog returns DialogStatus with result With this API, we can use showSuggestedActionDialog in an "early return" style -- await the user's choice, and early return if they chose to dismiss the dialog. That's particularly helpful when the confirmation dialog is opened in an `if` block. We'll use this for the upcoming "edit message" feature (#126), where we plan to offer a confirmation dialog before entering an edit-message session, but only if the compose box has text for a new message in it (which would be discarded if the user wants to go ahead). --- lib/widgets/app.dart | 15 +++++------ lib/widgets/compose_box.dart | 21 ++++++++-------- lib/widgets/dialog.dart | 41 ++++++++++++++++++++++-------- test/widgets/dialog_test.dart | 47 +++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 28 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index d2f5b36e1d..54ba92588b 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -308,16 +308,17 @@ class ChooseAccountPage extends StatelessWidget { trailing: MenuAnchor( menuChildren: [ MenuItemButton( - onPressed: () { - showSuggestedActionDialog(context: context, + onPressed: () async { + final dialog = showSuggestedActionDialog(context: context, title: zulipLocalizations.logOutConfirmationDialogTitle, message: zulipLocalizations.logOutConfirmationDialogMessage, // TODO(#1032) "destructive" style for action button - actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton, - onActionButtonPress: () { - // TODO error handling if db write fails? - logOutAccount(GlobalStoreWidget.of(context), accountId); - }); + actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton); + if (await dialog.result == true) { + if (!context.mounted) return; + // TODO error handling if db write fails? + unawaited(logOutAccount(GlobalStoreWidget.of(context), accountId)); + } }, child: Text(zulipLocalizations.chooseAccountPageLogOutButton)), ], diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 8fc92e7d26..005321719d 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:math'; import 'package:app_settings/app_settings.dart'; @@ -907,13 +908,13 @@ Future> _getFilePickerFiles(BuildContext context, FileType type) // If the user hasn't checked "Don't ask again", they can always dismiss // our prompt and retry, and the permissions request will reappear, // letting them grant permissions and complete the upload. - showSuggestedActionDialog(context: context, + final dialog = showSuggestedActionDialog(context: context, title: zulipLocalizations.permissionsNeededTitle, message: zulipLocalizations.permissionsDeniedReadExternalStorage, - actionButtonText: zulipLocalizations.permissionsNeededOpenSettings, - onActionButtonPress: () { - AppSettings.openAppSettings(); - }); + actionButtonText: zulipLocalizations.permissionsNeededOpenSettings); + if (await dialog.result == true) { + unawaited(AppSettings.openAppSettings()); + } } else { showErrorDialog(context: context, title: zulipLocalizations.errorDialogTitle, @@ -1008,13 +1009,13 @@ class _AttachFromCameraButton extends _AttachUploadsButton { // permission-request alert once, the first time the app wants to // use a protected resource. After that, the only way the user can // grant it is in Settings. - showSuggestedActionDialog(context: context, + final dialog = showSuggestedActionDialog(context: context, title: zulipLocalizations.permissionsNeededTitle, message: zulipLocalizations.permissionsDeniedCameraAccess, - actionButtonText: zulipLocalizations.permissionsNeededOpenSettings, - onActionButtonPress: () { - AppSettings.openAppSettings(); - }); + actionButtonText: zulipLocalizations.permissionsNeededOpenSettings); + if (await dialog.result == true) { + unawaited(AppSettings.openAppSettings()); + } } else { showErrorDialog(context: context, title: zulipLocalizations.errorDialogTitle, diff --git a/lib/widgets/dialog.dart b/lib/widgets/dialog.dart index 60e430a8e2..0d2b4c5a7d 100644 --- a/lib/widgets/dialog.dart +++ b/lib/widgets/dialog.dart @@ -19,13 +19,29 @@ Widget _dialogActionText(String text) { /// Tracks the status of a dialog, in being still open or already closed. /// +/// Use [T] to identify the outcome of the interaction: +/// - Pass `void` for an informational dialog with just the option to dismiss. +/// - For confirmation dialogs with an option to dismiss +/// plus an option to proceed with an action, pass `bool`. +/// The action button should pass true for Navigator.pop's `result` argument. +/// - For dialogs with an option to dismiss plus multiple other options, +/// pass a custom enum. +/// For the latter two cases, a cancel button should call Navigator.pop +/// with null for the `result` argument, to match what Flutter does +/// when you dismiss the dialog by tapping outside its area. +/// /// See also: /// * [showDialog], whose return value this class is intended to wrap. -class DialogStatus { +class DialogStatus { const DialogStatus(this.result); /// Resolves when the dialog is closed. - final Future result; + /// + /// If this completes with null, the dialog was dismissed. + /// Otherwise, completes with a [T] identifying the interaction's outcome. + /// + /// See, e.g., [showSuggestedActionDialog]. + final Future result; } /// Displays an [AlertDialog] with a dismiss button @@ -37,7 +53,7 @@ class DialogStatus { // [showDialog]'s return value, a [Future], inside [DialogStatus] // whose documentation can be accessed. This helps avoid confusion when // intepreting the meaning of the [Future]. -DialogStatus showErrorDialog({ +DialogStatus showErrorDialog({ required BuildContext context, required String title, String? message, @@ -61,28 +77,31 @@ DialogStatus showErrorDialog({ return DialogStatus(future); } -void showSuggestedActionDialog({ +/// Displays an alert dialog with a cancel button and an action button. +/// +/// The [DialogStatus.result] Future gives true if the action button was tapped. +/// If the dialog was canceled, +/// either with the cancel button or by tapping outside the dialog's area, +/// it completes with null. +DialogStatus showSuggestedActionDialog({ required BuildContext context, required String title, required String message, required String? actionButtonText, - required VoidCallback onActionButtonPress, }) { final zulipLocalizations = ZulipLocalizations.of(context); - showDialog( + final future = showDialog( context: context, builder: (BuildContext context) => AlertDialog( title: Text(title), content: SingleChildScrollView(child: Text(message)), actions: [ TextButton( - onPressed: () => Navigator.pop(context), + onPressed: () => Navigator.pop(context, null), child: _dialogActionText(zulipLocalizations.dialogCancel)), TextButton( - onPressed: () { - onActionButtonPress(); - Navigator.pop(context); - }, + onPressed: () => Navigator.pop(context, true), child: _dialogActionText(actionButtonText ?? zulipLocalizations.dialogContinue)), ])); + return DialogStatus(future); } diff --git a/test/widgets/dialog_test.dart b/test/widgets/dialog_test.dart index 4082e45424..c86aae478e 100644 --- a/test/widgets/dialog_test.dart +++ b/test/widgets/dialog_test.dart @@ -26,4 +26,51 @@ void main() { mode: LaunchMode.inAppBrowserView)); }); }); + + group('showSuggestedActionDialog', () { + testWidgets('tap action button', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(TestZulipApp()); + await tester.pump(); + final element = tester.element(find.byType(Placeholder)); + + final dialog = showSuggestedActionDialog(context: element, + title: 'Continue?', + message: 'Do the thing?', + actionButtonText: 'Sure'); + await tester.pump(); + await tester.tap(find.text('Sure')); + await check(dialog.result).completes((it) => it.equals(true)); + }); + + testWidgets('tap cancel', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(TestZulipApp()); + await tester.pump(); + final element = tester.element(find.byType(Placeholder)); + + final dialog = showSuggestedActionDialog(context: element, + title: 'Continue?', + message: 'Do the thing?', + actionButtonText: 'Sure'); + await tester.pump(); + await tester.tap(find.text('Cancel')); + await check(dialog.result).completes((it) => it.equals(null)); + }); + + testWidgets('tap outside dialog area', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(TestZulipApp()); + await tester.pump(); + final element = tester.element(find.byType(Placeholder)); + + final dialog = showSuggestedActionDialog(context: element, + title: 'Continue?', + message: 'Do the thing?', + actionButtonText: 'Sure'); + await tester.pump(); + await tester.tapAt(tester.getTopLeft(find.byType(TestZulipApp))); + await check(dialog.result).completes((it) => it.equals(null)); + }); + }); }