compose: Refactor some logic; implement redesigned error banner #1090

chrisbobbe · 2024-11-27T07:44:19Z

Prompted by reviewing Zixuan's #924, this is meant to make the compose-box code better prepared for that work than it is in main; details at #924 (review) and in the commits.

Please let me know as soon as anything doesn't look clear. I'm tired (it's almost bedtime 🙂) but I've spent a while on this and I wanted to get it in today if possible. Since this makes a UI change, screenshots coming soon.

Fixes: #1089

chrisbobbe · 2024-11-27T07:51:47Z

`main`	#924 (revision at `01c91ce`)	This PR

gnprice

Neat, thanks! I agree this direction of abstracting and deduplicating this logic looks useful.

Quick comments below since I'll be out for the next couple of workdays (plus the holiday).

gnprice · 2024-11-27T08:12:26Z

lib/widgets/compose_box.dart

+    topicController: controller.topic,
+    controller: controller.content,
+    focusNode: controller.contentFocusNode,


A further step that I think could be helpful is if ComposeBox grew an InheritedWidget and a static of method. Then instead of passing these controller pieces down through each layer, the descendant widgets that need them could say ComposeBox.of(context).controller and get what they need.

Or maybe the of method would even just return the controller — that's the meaningful part of the state anyway.

Alternatively, the widgets could continue taking this information as parameters — but as a single parameter that is the whole controller. That seems like it'd also clean this up a lot.

Either way, one thing I'm thinking of in particular is how #924 adds enabled and sendMessageError, and has to pass them down through layers in a bunch of places (especially enabled). So ideally I'd like to see a refactor that makes it so a change like #924 can add a field to the controller and then not have to add references to it at a bunch of widget constructors but still be able to use it at various widgets deeper inside the compose box, like the various buttons.

With any of these approaches, we still need to listen to the possible updates. Our current setup for listener has a fine granularity — _ContentInputState listens to ComposeContentController and a FocusNode, _StreamContentInputState just listens to ComposeTopicController…

Continuing this trend, we may have individual listeners for enabled and sendMessageError as well, while turning _TopicInput and probably some others into StatefulWidgets.

With InheritedWidget, maybe we can use BuildContext.dependOnInheritedWidgetOfExactType to simplify that part.

I think the existing ways we have things listen don't need to change.

The one thing worth avoiding with fine vs. coarse granularity here is that it'd be good to minimize how much needs to rebuild every time the user types another character. So things that don't need to depend on the text controllers should avoid doing so. But if things depend on enabled or sendMessageError when they don't strictly need to, that isn't a problem - those change much less frequently.

InheritedWidget

I almost made a commit like this, but then noticed the branch was getting pretty long. :) I also noticed the InheritedWidget API prompting me to think about correctly handling changes to the value (the controller), such as by not calling dependOnInheritedWidgetOfExactType in an initState method. It seemed like something to consider carefully, but I didn't have a clear idea of when the controller would change (or if/when we'd want it to), so I didn't go ahead with it.

chrisbobbe · 2024-11-27T19:04:35Z

lib/widgets/theme.dart

      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)!,
+      btnLabelAttMediumDanger: Color.lerp(btnLabelAttMediumDanger, other.btnLabelAttMediumDanger, t)!,


note to self: same comment as #924 (comment) ; this should be btnLabelAttMediumIntDanger; it's btn-label-att_medium-int_danger in the Figma

PIG208

Thanks for working on this. Looks good overall! Left some comments.

PIG208 · 2024-11-27T19:17:41Z

lib/widgets/compose_box.dart

-  final _contentFocusNode = FocusNode();
+class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> {
+  FixedDestinationComposeBoxController get controller => _controller;
+  final FixedDestinationComposeBoxController _controller = FixedDestinationComposeBoxController();


nit: it should be fine to just use final without specifying the type here

PIG208 · 2024-11-27T19:17:57Z

lib/widgets/compose_box.dart

+  final _streamComposeBoxControllerKey = GlobalKey<_StreamComposeBoxState>();
+  final _fixedDestinationComposeBoxControllerKey = GlobalKey<_FixedDestinationComposeBoxState>();
+
+


nit: extra newline

PIG208 · 2024-11-27T19:22:49Z

lib/widgets/action_sheet.dart

+    // 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.
+    assert (composeBoxController != null);


Do we still need this assertion given the crunchy-shell check that follows this?

PIG208 · 2024-11-27T23:37:09Z

lib/widgets/compose_box.dart

+        assert(_controller is FixedDestinationComposeBoxController);
+        return _FixedDestinationComposeBox(controller: (_controller as FixedDestinationComposeBoxController),


nit: how about

Suggested change

assert(_controller is FixedDestinationComposeBoxController);

return _FixedDestinationComposeBox(controller: (_controller as FixedDestinationComposeBoxController),

case TopicNarrow():

_controller as FixedDestinationComposeBoxController;

return _FixedDestinationComposeBox(controller: _controller, narrow: narrow);

for here and similarly for case DmNarrow(), so that the return's are just a bit over 70 characters. I'm not sure if the assertions are necessary.

PIG208 · 2024-11-27T23:48:07Z

test/widgets/compose_box_test.dart

-        narrow: TopicNarrow(channel.streamId, 'topic'), streams: [channel]);
-      checkComposeBoxTextFields(tester, controllerKey: key,
+      final controller = (await prepareComposeBox(tester,
+        narrow: TopicNarrow(channel.streamId, 'topic'), streams: [channel]))!;


It might be cleaner to make controller a shared late variable so that we don't have to wrap the await prepareComposeBox() call.

Ooh nice idea, thanks.

PIG208 · 2024-11-28T00:13:12Z

lib/widgets/compose_box.dart

    if (errorBanner != null) {
-      body = errorBanner;
+      body = null;


I feel that having to do this makes only calling _ComposeBoxContainer once less appealing.

Using an early return here when the errorBanner is present can make it easier to reason about the value of body and errorBanner. And we can comfortably pass errorBanner: null at the end, or body: null at the beginning, for clarity.

How about:

diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 1ca2e4ae..6514a331 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1343,12 +1343,12 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState { @override Widget build(BuildContext context) { final Widget? body; - Widget? errorBanner; - errorBanner = _errorBanner(context); + final errorBanner = _errorBanner(context); if (errorBanner != null) { - body = null; - } else { + return _ComposeBoxContainer(body: null, errorBanner: errorBanner); + } + // TODO(#720) dismissable message-send error, maybe something like: // if (controller.sendMessageError.value != null) { // errorBanner = _ErrorBanner(label: @@ -1374,8 +1374,7 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState { assert(false); body = null; } - } - return _ComposeBoxContainer(body: body, errorBanner: errorBanner); + return _ComposeBoxContainer(body: body, errorBanner: null); } }

For zulip#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: zulip#1090 (comment)

chrisbobbe · 2024-12-03T00:02:20Z

Thanks for the reviews! Revision pushed.

PIG208

Thanks for the revision! This looks good to me. The final commit sequence seems to imply that there is the 6/6 commit passing controller down. I went through the file and found no other places left to refactor, so we should be all good on this, except for updating the commit summaries.

PIG208 · 2024-12-03T17:50:52Z

lib/widgets/compose_box.dart

+      result = controller.topic.hasValidationErrors.value;
+    }
+    result |= controller.content.hasValidationErrors.value;
+    return result;


I feel that this is an indicator to further abstract away logic around validation errors, probably as a future follow-up.

I'm not yet picturing what that might look like, so I won't add a TODO in this revision, but I'm happy to if you help me understand better (or you can push that TODO yourself, wiki-style, after this is merged).

chrisbobbe · 2024-12-03T18:34:48Z

Oh, oops! Looks like I did 1, 2, 3, 3, 4, 5 😅—will fix.

chrisbobbe · 2024-12-03T19:56:29Z

(Done.)

gnprice

Thanks! This all looks good — just nits below. Also pushing a small added commit for one other nit:
ff85cef compose [nfc]: Simplify controller getters to final fields

gnprice · 2024-12-03T21:13:34Z

lib/widgets/action_sheet.dart

-    // 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!;


Convenient that we had these comments explaining why the ! had been appropriate in the first place. That particularly helps with the commit
cfe6b3a action_sheet: Fix rare null-check error on quote-and-reply

that points out it's no longer appropriate.

gnprice · 2024-12-03T21:19:01Z

lib/widgets/compose_box.dart

-    _contentController.dispose();
-    _contentFocusNode.dispose();
+    _topic.dispose();
+    _topicFocusNode.dispose();


nit:

compose [nfc]: Make state "has-a" instead of "is-a" ComposeBoxController

It looks like there's a small bug this commit fixes, in addition to its NFC changes: previously we weren't disposing the topic FocusNode.

Ooh thanks for the catch; I'll put the bugfix in a separate commit just before this one.

gnprice · 2024-12-03T21:27:18Z

test/widgets/compose_box_test.dart

+        check(controller).isA<StreamComposeBoxController>();
+        final topicTextField = tester.widgetList<TextField>(
+          find.byWidgetPredicate((widget) {
+            final topicController = (controller as StreamComposeBoxController).topic;


nit: can pull topicController's initializer outside the predicate:

Suggested change

check(controller).isA<StreamComposeBoxController>();

final topicTextField = tester.widgetList<TextField>(

find.byWidgetPredicate((widget) {

final topicController = (controller as StreamComposeBoxController).topic;

check(controller).isA<StreamComposeBoxController>();

final topicController = (controller as StreamComposeBoxController).topic;

final topicTextField = tester.widgetList<TextField>(

find.byWidgetPredicate((widget) {

gnprice · 2024-12-03T21:35:29Z

lib/widgets/compose_box.dart

+  Widget? topicInput();
+  Widget contentInput();
+  Widget sendButton();


nit: as methods, these should be named buildTopicInput etc

gnprice · 2024-12-03T21:36:00Z

lib/widgets/compose_box.dart

-            child: Column(children: [
-              if (topicInput != null) topicInput!,
-              contentInput,
+    final topicInput = this.topicInput();


so e.g.

Suggested change

final topicInput = this.topicInput();

final topicInput = buildTopicInput();

gnprice · 2024-12-03T21:43:18Z

lib/widgets/compose_box.dart

+  /// This widget should use a [SafeArea] to pad the left/right device insets.
+  /// If [body] is null it will also receive a bottom inset to pad;
+  /// it should pad that too.


How about this version?

Suggested change

/// This widget should use a [SafeArea] to pad the left/right device insets.

/// If [body] is null it will also receive a bottom inset to pad;

/// it should pad that too.

/// This widget should use a [SafeArea] to pad the left, right,

/// and bottom device insets.

/// (A bottom inset may occur if [body] is null.)

That seems like simpler advice to follow, unless there's some reason I'm missing that makes it hard in some case.

Yep, looks good!

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).

Each of these tests will get a new related test, coming up.

In 052f203 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.

…posed Thanks Greg for catching this: zulip#1090 (comment)

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

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.

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

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.

… class

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

This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#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 zulip#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 <[email protected]> Fixes: zulip#1089

For zulip#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: zulip#1090 (comment)

…Input

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.

chrisbobbe · 2024-12-03T22:40:04Z

Thanks for the review! Revision pushed.

gnprice · 2024-12-03T22:51:37Z

Thanks for the revision! Looks good — merging.

chrisbobbe requested review from gnprice and PIG208 November 27, 2024 07:44

chrisbobbe mentioned this pull request Nov 27, 2024

compose_box: Wait for send message request to complete. #924

Closed

gnprice reviewed Nov 27, 2024

View reviewed changes

chrisbobbe commented Nov 27, 2024

View reviewed changes

PIG208 reviewed Nov 28, 2024

View reviewed changes

chrisbobbe force-pushed the pr-compose-box-refactors branch from 84c812e to 6b36b35 Compare December 3, 2024 00:02

PIG208 reviewed Dec 3, 2024

View reviewed changes

PIG208 assigned gnprice Dec 3, 2024

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Dec 3, 2024

PIG208 requested a review from gnprice December 3, 2024 17:57

chrisbobbe force-pushed the pr-compose-box-refactors branch from 6b36b35 to 695624b Compare December 3, 2024 19:56

gnprice reviewed Dec 3, 2024

View reviewed changes

chrisbobbe added 12 commits December 3, 2024 14:22

msglist [nfc]: Remove no-op SizedBox.shrink as a Column child

39781c8

action_sheet test [nfc]: Add groups for 'in {topic,DM} narrow' tests

ba6424f

Each of these tests will get a new related test, coming up.

action_sheet: Fix rare null-check error on quote-and-reply

060208f

In 052f203 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.

compose test [nfc]: Make controllerKey a shared late variable

35c966e

compose: Dispose topic-input's focus node when stream compose box dis…

b5e37d6

…posed Thanks Greg for catching this: zulip#1090 (comment)

compose [nfc]: Shorten {topic,content}Controller to just {topic,content}

b233ee9

compose [nfc]: Give _ComposeBoxBody narrow and controller params

009e415

chrisbobbe and others added 12 commits December 3, 2024 14:34

compose [nfc]: Put {_Stream,FixedDestination}ComposeBoxBody near base…

6169311

… class

compose [nfc]: Add TODO for overlaid linear progress indicator

11072ed

compose [nfc]: Simplify choose-body logic by switching on controller

a883762

compose [nfc]: Pass controller down (2/6); _FixedDestinationContent…

6bc6169

…Input

compose [nfc]: Pass controller down (3/6); _ContentInput

f65dc80

compose [nfc]: Pass controller down (4/6); _TopicInput

95b77f6

compose [nfc]: Pass controller down (5/6); _SendButton

29470d3

compose [nfc]: Pass controller down (6/6); _AttachUploadsButton

e1fc763

compose [nfc]: Simplify controller getters to final fields

f46187a

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.

chrisbobbe force-pushed the pr-compose-box-refactors branch from ff85cef to f46187a Compare December 3, 2024 22:40

gnprice merged commit f46187a into zulip:main Dec 3, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Refactor some logic; implement redesigned error banner #1090

compose: Refactor some logic; implement redesigned error banner #1090

chrisbobbe commented Nov 27, 2024

chrisbobbe commented Nov 27, 2024

gnprice left a comment

gnprice Nov 27, 2024

gnprice Nov 27, 2024

PIG208 Nov 28, 2024

gnprice Nov 28, 2024

chrisbobbe Dec 2, 2024 •

edited

Loading

chrisbobbe Nov 27, 2024

PIG208 left a comment

PIG208 Nov 27, 2024

PIG208 Nov 27, 2024

PIG208 Nov 27, 2024

PIG208 Nov 27, 2024 •

edited

Loading

PIG208 Nov 27, 2024

chrisbobbe Dec 2, 2024

PIG208 Nov 28, 2024

chrisbobbe commented Dec 3, 2024

PIG208 left a comment

PIG208 Dec 3, 2024

chrisbobbe Dec 3, 2024

chrisbobbe commented Dec 3, 2024

chrisbobbe commented Dec 3, 2024

gnprice left a comment •

edited

Loading

gnprice Dec 3, 2024

gnprice Dec 3, 2024

chrisbobbe Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

chrisbobbe Dec 3, 2024

chrisbobbe commented Dec 3, 2024

gnprice commented Dec 3, 2024

		final _streamComposeBoxControllerKey = GlobalKey<_StreamComposeBoxState>();
		final _fixedDestinationComposeBoxControllerKey = GlobalKey<_FixedDestinationComposeBoxState>();

		assert(_controller is FixedDestinationComposeBoxController);
		return _FixedDestinationComposeBox(controller: (_controller as FixedDestinationComposeBoxController),

-        assert(_controller is FixedDestinationComposeBoxController);
-        return _FixedDestinationComposeBox(controller: (_controller as FixedDestinationComposeBoxController),
+      case TopicNarrow():
+        _controller as FixedDestinationComposeBoxController;
+        return _FixedDestinationComposeBox(controller: _controller, narrow: narrow);

	final topicInput = this.topicInput();
	final topicInput = buildTopicInput();

compose: Refactor some logic; implement redesigned error banner #1090

compose: Refactor some logic; implement redesigned error banner #1090

Conversation

chrisbobbe commented Nov 27, 2024

chrisbobbe commented Nov 27, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Dec 3, 2024

PIG208 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 Dec 3, 2024

chrisbobbe commented Dec 3, 2024

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Dec 3, 2024

gnprice commented Dec 3, 2024

chrisbobbe Dec 2, 2024 •

edited

Loading

PIG208 Nov 27, 2024 •

edited

Loading

gnprice left a comment •

edited

Loading