Edit-message (4/n): Misc. preparation for edit-message feature #1471

chrisbobbe · 2025-04-10T01:54:14Z

Here's a small batch of miscellaneous prep work for the edit-message feature, #126.

PIG208

Thanks! This looks good to me overall. Left a small comment, and marking for Greg's review.

PIG208 · 2025-04-10T23:37:01Z

lib/widgets/dialog.dart

@@ -21,11 +21,11 @@ Widget _dialogActionText(String text) {
 ///
 /// See also:
 ///  * [showDialog], whose return value this class is intended to wrap.
-class DialogStatus {
+class DialogStatus<T> {


It might be helpful to document the meaning of the type parameter. Specially, from showDialog, this part seems relevant:

/// If the application has multiple [Navigator] objects, it may be necessary to /// call `Navigator.of(context, rootNavigator: true).pop(result)` to close the /// dialog rather than just `Navigator.pop(context, result)`. /// /// Returns a [Future] that resolves to the value (if any) that was passed to /// [Navigator.pop] when the dialog was closed.

We just need to adapt it to fit our wrapper class.

gnprice

Thanks! Generally this looks good; a couple of small comments (adding to Zixuan's above).

gnprice · 2025-04-11T23:59:47Z

lib/widgets/dialog.dart

  /// Resolves when the dialog is closed.
-  final Future<void> closed;
+  final Future<T?> closed;


What's the meaning of the value that this resolves to?

(I think this question overlaps with Zixuan's comment above.)

gnprice · 2025-04-12T00:06:41Z

test/widgets/dialog_test.dart

+        actionButtonText: 'Sure',);
+      await tester.pump();
+      await tester.tap(find.text('Sure'));
+      await check(dialog.closed).completes((it) => it.equals(SuggestedActionDialogResult.doAction));


nit: line too long: key information past 80 columns (namely the only information that distinguishes this check from its opposite in the other test case 🙂)

…sage}

To avoid wasting a lot of vertical space in the edit-message compose box (coming up) with large text-scale settings. Related: zulip#1023

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.

chrisbobbe · 2025-04-16T03:22:57Z

Thanks for the reviews! Revision pushed.

gnprice · 2025-04-16T04:03:43Z

lib/widgets/dialog.dart

          child: _dialogActionText(actionButtonText ?? zulipLocalizations.dialogContinue)),
      ]));
+  return DialogStatus(_wrapFutureExpectingNonNull(future));


Ah interesting.

What happens if the user tries to dismiss the dialog without tapping either button? E.g. by tapping outside the dialog's area (on the obscured remainder of the page that's under the dialog), or by using the Android "back" gesture/button.

I think we'd want to have those work, and have the same effect as tapping the "Cancel" button.

Indeed, thanks for catching this—addressing this in the new revision.

chrisbobbe · 2025-04-17T02:28:45Z

Thanks for the review! Revision pushed.

chrisbobbe · 2025-04-17T02:31:10Z

lib/widgets/app.dart

-                    logOutAccount(GlobalStoreWidget.of(context), accountId);
-                  });
+                  actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton);
+                if (await dialog.closed == true) {


Oh, hmm, I guess this doesn't look ideal. What it means is "did the user press the 'Log out' button", but what it looks like it means is "was the dialog closed".

Yeah. Maybe rename the field, say to result?

gnprice

Thanks for the revision! LGTM modulo the subthread above, and two nits below.

gnprice · 2025-04-17T20:08:10Z

lib/widgets/dialog.dart

+  /// If this completes with null, the dialog was dismissed.
+  /// Otherwise, completes with a [T] identifying the interaction's outcome.
+  ///
+  /// See, e.g., [showSuggestedActionDialog] and [SuggestedActionDialogResult].


nit: the latter reference no longer exists

gnprice · 2025-04-17T20:10:39Z

lib/widgets/dialog.dart

    context: context,
    builder: (BuildContext context) => AlertDialog(
      title: Text(title),
      content: SingleChildScrollView(child: Text(message)),
      actions: [
        TextButton(
-          onPressed: () => Navigator.pop(context),
+          onPressed: () => Navigator.pop<bool?>(context, null),


nit: the types allow bool here (and below):

Suggested change

onPressed: () => Navigator.pop<bool?>(context, null),

onPressed: () => Navigator.pop<bool>(context, null),

and that seems a bit more consistent with bool as the type parameter on showDialog

Discussion: zulip#1471 (comment)

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 (zulip#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).

chrisbobbe · 2025-04-18T01:19:47Z

Thanks for the review! Revision pushed.

gnprice · 2025-04-18T05:36:20Z

Thanks! Looks good; merging.

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Apr 10, 2025

chrisbobbe requested a review from PIG208 April 10, 2025 01:54

chrisbobbe assigned PIG208 Apr 10, 2025

PIG208 reviewed Apr 10, 2025

View reviewed changes

PIG208 assigned gnprice and unassigned PIG208 Apr 10, 2025

PIG208 requested a review from gnprice April 10, 2025 23:43

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 10, 2025

gnprice reviewed Apr 12, 2025

View reviewed changes

chrisbobbe added 5 commits April 16, 2025 11:00

api: Add restrict-message-editing fields to InitialSnapshot

4808904

store: Add restrict-message-editing fields to PerAccountStore

e7abf31

message [nfc]: Make helper methods _notifyMessageListViews{,ForOneMes…

13c9d7a

…sage}

compose: Clamp max scale factor of _Banner label

d5d0ea8

To avoid wasting a lot of vertical space in the edit-message compose box (coming up) with large text-scale settings. Related: zulip#1023

chrisbobbe force-pushed the pr-edit-message-4 branch from fba1233 to 1d410a7 Compare April 16, 2025 03:22

gnprice reviewed Apr 16, 2025

View reviewed changes

chrisbobbe force-pushed the pr-edit-message-4 branch from 1d410a7 to 8694baa Compare April 17, 2025 02:28

chrisbobbe commented Apr 17, 2025

View reviewed changes

gnprice reviewed Apr 17, 2025

View reviewed changes

chrisbobbe added 2 commits April 18, 2025 09:15

dialog [nfc]: s/DialogStatus.{closed,result}/

03cd9b8

Discussion: zulip#1471 (comment)

chrisbobbe force-pushed the pr-edit-message-4 branch from 8694baa to 4596245 Compare April 18, 2025 01:19

gnprice merged commit 4596245 into zulip:main Apr 18, 2025
1 check passed

chrisbobbe deleted the pr-edit-message-4 branch April 18, 2025 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit-message (4/n): Misc. preparation for edit-message feature #1471

Edit-message (4/n): Misc. preparation for edit-message feature #1471

chrisbobbe commented Apr 10, 2025

PIG208 left a comment

PIG208 Apr 10, 2025

gnprice left a comment

gnprice Apr 11, 2025

gnprice Apr 12, 2025

chrisbobbe commented Apr 16, 2025

gnprice Apr 16, 2025

chrisbobbe Apr 17, 2025

chrisbobbe commented Apr 17, 2025

chrisbobbe Apr 17, 2025 •

edited

Loading

gnprice Apr 17, 2025

gnprice left a comment

gnprice Apr 17, 2025

gnprice Apr 17, 2025

chrisbobbe commented Apr 18, 2025

gnprice commented Apr 18, 2025

	onPressed: () => Navigator.pop<bool?>(context, null),
	onPressed: () => Navigator.pop<bool>(context, null),

Edit-message (4/n): Misc. preparation for edit-message feature #1471

Edit-message (4/n): Misc. preparation for edit-message feature #1471

Conversation

chrisbobbe commented Apr 10, 2025

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 17, 2025

chrisbobbe Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 18, 2025

gnprice commented Apr 18, 2025

chrisbobbe Apr 17, 2025 •

edited

Loading