compose: Disable compose box widgets when sending message

PIG208 · PIG208 · commit 010212f4574a · 2024-12-09T16:47:01.000-05:00
Also clear the content input only after the message is successfully
sent.

For the send button, unlike when there are validation errors, having
an error dialog when its disabled for sending a message feels a bit
redundant and doesn't help much.  This can be annoying when the user
misclicks and hit the send button multiple times.

Signed-off-by: Zixuan James Li &lt;zixuan@zulip.com&gt;
diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart
@@ -295,6 +295,7 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve
     super.initState();
     widget.controller.content.addListener(_contentChanged);
     widget.controller.contentFocusNode.addListener(_focusChanged);
+    widget.controller._enabled.addListener(_enabledChanged);
     WidgetsBinding.instance.addObserver(this);
   }
 
@@ -306,13 +307,16 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve
       widget.controller.content.addListener(_contentChanged);
       oldWidget.controller.contentFocusNode.removeListener(_focusChanged);
       widget.controller.contentFocusNode.addListener(_focusChanged);
+      oldWidget.controller._enabled.removeListener(_enabledChanged);
+      widget.controller._enabled.addListener(_enabledChanged);
     }
   }
 
   @override
   void dispose() {
     widget.controller.content.removeListener(_contentChanged);
     widget.controller.contentFocusNode.removeListener(_focusChanged);
+    widget.controller._enabled.removeListener(_enabledChanged);
     WidgetsBinding.instance.removeObserver(this);
     super.dispose();
   }
@@ -334,6 +338,12 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve
     store.typingNotifier.stoppedComposing();
   }
 
+  void _enabledChanged() {
+    setState(() {
+      // The actual state lives in `widget.controller._enabled`.
+    });
+  }
+
   @override
   void didChangeAppLifecycleState(AppLifecycleState state) {
     switch (state) {
@@ -395,44 +405,47 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve
       narrow: widget.narrow,
       controller: widget.controller.content,
       focusNode: widget.controller.contentFocusNode,
-      fieldViewBuilder: (context) => ConstrainedBox(
-        constraints: BoxConstraints(maxHeight: maxHeight(context)),
-        // This [ClipRect] replaces the [TextField] clipping we disable below.
-        child: ClipRect(
-          child: InsetShadowBox(
-            top: _verticalPadding, bottom: _verticalPadding,
-            color: designVariables.composeBoxBg,
-            child: TextField(
-              controller: widget.controller.content,
-              focusNode: widget.controller.contentFocusNode,
-              // Let the content show through the `contentPadding` so that
-              // our [InsetShadowBox] can fade it smoothly there.
-              clipBehavior: Clip.none,
-              style: TextStyle(
-                fontSize: _fontSize,
-                height: _lineHeightRatio,
-                color: designVariables.textInput),
-              // From the spec at
-              //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3960-5147&node-type=text&m=dev
-              // > Compose box has the height to fit 2 lines. This is [done] to
-              // > have a bigger hit area for the user to start the input. […]
-              minLines: 2,
-              maxLines: null,
-              textCapitalization: TextCapitalization.sentences,
-              decoration: InputDecoration(
-                // This padding ensures that the user can always scroll long
-                // content entirely out of the top or bottom shadow if desired.
-                // With this and the `minLines: 2` above, an empty content input
-                // gets 60px vertical distance (with no text-size scaling)
-                // between the top of the top shadow and the bottom of the
-                // bottom shadow. That's a bit more than the 54px given in the
-                // Figma, and we can revisit if needed, but it's tricky to get
-                // that 54px distance while also making the scrolling work like
-                // this and offering two lines of touchable area.
-                contentPadding: const EdgeInsets.symmetric(vertical: _verticalPadding),
-                hintText: widget.hintText,
-                hintStyle: TextStyle(
-                  color: designVariables.textInput.withFadedAlpha(0.5))))))));
+      fieldViewBuilder: (context) => Opacity(
+        opacity: widget.controller.enabled ? 1 : 0.5,
+        child: ConstrainedBox(
+          constraints: BoxConstraints(maxHeight: maxHeight(context)),
+          // This [ClipRect] replaces the [TextField] clipping we disable below.
+          child: ClipRect(
+            child: InsetShadowBox(
+              top: _verticalPadding, bottom: _verticalPadding,
+              color: designVariables.composeBoxBg,
+              child: TextField(
+                enabled: widget.controller.enabled,
+                controller: widget.controller.content,
+                focusNode: widget.controller.contentFocusNode,
+                // Let the content show through the `contentPadding` so that
+                // our [InsetShadowBox] can fade it smoothly there.
+                clipBehavior: Clip.none,
+                style: TextStyle(
+                  fontSize: _fontSize,
+                  height: _lineHeightRatio,
+                  color: designVariables.textInput),
+                // From the spec at
+                //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3960-5147&node-type=text&m=dev
+                // > Compose box has the height to fit 2 lines. This is [done] to
+                // > have a bigger hit area for the user to start the input. […]
+                minLines: 2,
+                maxLines: null,
+                textCapitalization: TextCapitalization.sentences,
+                decoration: InputDecoration(
+                  // This padding ensures that the user can always scroll long
+                  // content entirely out of the top or bottom shadow if desired.
+                  // With this and the `minLines: 2` above, an empty content input
+                  // gets 60px vertical distance (with no text-size scaling)
+                  // between the top of the top shadow and the bottom of the
+                  // bottom shadow. That's a bit more than the 54px given in the
+                  // Figma, and we can revisit if needed, but it's tricky to get
+                  // that 54px distance while also making the scrolling work like
+                  // this and offering two lines of touchable area.
+                  contentPadding: const EdgeInsets.symmetric(vertical: _verticalPadding),
+                  hintText: widget.hintText,
+                  hintStyle: TextStyle(
+                    color: designVariables.textInput.withFadedAlpha(0.5)))))))));
   }
 }
 
@@ -513,20 +526,28 @@ class _TopicInput extends StatelessWidget {
       controller: controller.topic,
       focusNode: controller.topicFocusNode,
       contentFocusNode: controller.contentFocusNode,
-      fieldViewBuilder: (context) => Container(
-        padding: const EdgeInsets.only(top: 10, bottom: 9),
-        decoration: BoxDecoration(border: Border(bottom: BorderSide(
-          width: 1,
-          color: designVariables.foreground.withFadedAlpha(0.2)))),
-        child: TextField(
-          controller: controller.topic,
-          focusNode: controller.topicFocusNode,
-          textInputAction: TextInputAction.next,
-          style: topicTextStyle,
-          decoration: InputDecoration(
-            hintText: zulipLocalizations.composeBoxTopicHintText,
-            hintStyle: topicTextStyle.copyWith(
-              color: designVariables.textInput.withFadedAlpha(0.5))))));
+      fieldViewBuilder: (context) => ValueListenableBuilder(
+        valueListenable: controller._enabled,
+        builder: (context, enabled, child) {
+          return Opacity(
+            opacity: enabled ? 1 : 0.5,
+            child: Container(
+              padding: const EdgeInsets.only(top: 10, bottom: 9),
+              decoration: BoxDecoration(border: Border(bottom: BorderSide(
+                width: 1,
+                color: designVariables.foreground.withFadedAlpha(0.2)))),
+              child: TextField(
+                enabled: enabled,
+                controller: controller.topic,
+                focusNode: controller.topicFocusNode,
+                textInputAction: TextInputAction.next,
+                style: topicTextStyle,
+                decoration: InputDecoration(
+                  hintText: zulipLocalizations.composeBoxTopicHintText,
+                  hintStyle: topicTextStyle.copyWith(
+                    color: designVariables.textInput.withFadedAlpha(0.5))))));
+        }
+      ));
   }
 }
 
@@ -699,10 +720,14 @@ abstract class _AttachUploadsButton extends StatelessWidget {
     final zulipLocalizations = ZulipLocalizations.of(context);
     return SizedBox(
       width: _composeButtonSize,
-      child: IconButton(
-        icon: Icon(icon, color: designVariables.foreground.withFadedAlpha(0.5)),
-        tooltip: tooltip(zulipLocalizations),
-        onPressed: () => _handlePress(context)));
+      child: ValueListenableBuilder(
+        valueListenable: controller._enabled,
+        builder: (context, enabled, child) {
+          return IconButton(
+            icon: Icon(icon, color: designVariables.foreground.withFadedAlpha(0.5)),
+            tooltip: tooltip(zulipLocalizations),
+            onPressed: enabled ? () => _handlePress(context) : null);
+        }));
   }
 }
 
@@ -882,6 +907,12 @@ class _SendButtonState extends State<_SendButton> {
     });
   }
 
+  void _hasEnabledChanged() {
+    setState(() {
+      // The actual state lives in `widget.controller._enabled`.
+    });
+  }
+
   @override
   void initState() {
     super.initState();
@@ -890,6 +921,7 @@ class _SendButtonState extends State<_SendButton> {
       controller.topic.hasValidationErrors.addListener(_hasErrorsChanged);
     }
     controller.content.hasValidationErrors.addListener(_hasErrorsChanged);
+    controller._enabled.addListener(_hasEnabledChanged);
   }
 
   @override
@@ -908,6 +940,8 @@ class _SendButtonState extends State<_SendButton> {
     }
     oldController.content.hasValidationErrors.removeListener(_hasErrorsChanged);
     controller.content.hasValidationErrors.addListener(_hasErrorsChanged);
+    oldController._enabled.removeListener(_hasEnabledChanged);
+    controller._enabled.addListener(_hasEnabledChanged);
   }
 
   @override
@@ -917,6 +951,7 @@ class _SendButtonState extends State<_SendButton> {
       controller.topic.hasValidationErrors.removeListener(_hasErrorsChanged);
     }
     controller.content.hasValidationErrors.removeListener(_hasErrorsChanged);
+    controller._enabled.removeListener(_hasEnabledChanged);
     super.dispose();
   }
 
@@ -950,20 +985,20 @@ class _SendButtonState extends State<_SendButton> {
       return;
     }
 
+    if (!widget.controller.enabled) {
+      // TODO update comment
+      return;
+    }
+
     final store = PerAccountStoreWidget.of(context);
     final content = controller.content.textNormalized;
 
-    controller.content.clear();
-    // The following `stoppedComposing` call is currently redundant,
-    // because clearing input sends a "typing stopped" notice.
-    // It will be necessary once we resolve #720.
     store.typingNotifier.stoppedComposing();
+    widget.controller._enabled.value = false;
 
     try {
-      // TODO(#720) clear content input only on success response;
-      //   while waiting, put input(s) and send button into a disabled
-      //   "working on it" state (letting input text be selected for copying).
       await store.sendMessage(destination: widget.getDestination(), content: content);
+      widget.controller.content.clear();
     } on ApiRequestException catch (e) {
       if (!mounted) return;
       final zulipLocalizations = ZulipLocalizations.of(context);
@@ -975,6 +1010,8 @@ class _SendButtonState extends State<_SendButton> {
         title: zulipLocalizations.errorMessageNotSent,
         message: message);
       return;
+    } finally {
+      widget.controller._enabled.value = true;
     }
   }
 
@@ -983,7 +1020,7 @@ class _SendButtonState extends State<_SendButton> {
     final designVariables = DesignVariables.of(context);
     final zulipLocalizations = ZulipLocalizations.of(context);
 
-    final iconColor = _hasValidationErrors
+    final iconColor = _hasValidationErrors || !widget.controller.enabled
       ? designVariables.icon.withFadedAlpha(0.5)
       : designVariables.icon;
 
@@ -997,7 +1034,7 @@ class _SendButtonState extends State<_SendButton> {
           // ambient [ButtonStyle.overlayColor], where we set the color for
           // the highlight state to match the Figma design.
           color: iconColor),
-        onPressed: _send));
+        onPressed: (widget.controller.enabled) ? _send : null));
   }
 }
 
@@ -1117,7 +1154,12 @@ abstract class _ComposeBoxBody extends StatelessWidget {
         child: Row(
           mainAxisAlignment: MainAxisAlignment.spaceBetween,
           children: [
-            Row(children: composeButtons),
+            ValueListenableBuilder(
+              valueListenable: controller._enabled,
+              builder: (context, enabled, child) {
+                return Opacity(opacity: enabled ? 1 : 0.5, child: child);
+              },
+              child: Row(children: composeButtons)),
             buildSendButton(),
           ])),
     ]);
@@ -1180,10 +1222,14 @@ sealed class ComposeBoxController {
   final content = ComposeContentController();
   final contentFocusNode = FocusNode();
 
+  bool get enabled => _enabled.value;
+  final ValueNotifier<bool> _enabled = ValueNotifier<bool>(true);
+
   @mustCallSuper
   void dispose() {
     content.dispose();
     contentFocusNode.dispose();
+    _enabled.dispose();
   }
 }
 
diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart
@@ -410,7 +410,7 @@ void main() {
       await tester.tap(find.byTooltip(zulipLocalizations.composeBoxSendTooltip));
       await tester.pump(Duration.zero);
 
-      check(connection.lastRequest).isA<http.Request>()
+      check(connection.takeRequests()).single.isA<http.Request>()
         ..method.equals('POST')
         ..url.path.equals('/api/v1/messages')
         ..bodyFields.deepEquals({
@@ -430,6 +430,43 @@ void main() {
       check(errorDialogs).isEmpty();
     });
 
+    testWidgets('disable compose box while pending; clear text when finished', (tester) async {
+      await setupAndTapSend(tester, prepareResponse: (int messageId) {
+        connection.prepare(json: SendMessageResult(
+          id: messageId).toJson(), delay: const Duration(seconds: 2));
+      });
+      check(controller!.enabled).isFalse();
+      check(controller!.content.text).isNotEmpty();
+
+      await tester.tap(find.byIcon(ZulipIcons.send));
+      await tester.pump(Duration.zero);
+      check(connection.takeRequests()).isEmpty();
+
+      await tester.tap(find.byIcon(ZulipIcons.attach_file));
+      await tester.pump(Duration.zero);
+      check(testBinding.takePickFilesCalls()).isEmpty();
+
+      await tester.pump(const Duration(seconds: 2));
+      check(controller!.enabled).isTrue();
+      check(controller!.content.text).isEmpty();
+    });
+
+    testWidgets('re-enable compose box even on failure; do not clear text', (tester) async {
+      await setupAndTapSend(tester, prepareResponse: (_) {
+        connection.prepare(
+          httpStatus: 400,
+          json: {'result': 'error', 'code': 'BAD_REQUEST'},
+          delay: const Duration(seconds: 2));
+      });
+      check(controller!.enabled).isFalse();
+      final oldText = controller!.content.text;
+      check(oldText).isNotEmpty();
+
+      await tester.pump(const Duration(seconds: 2));
+      check(controller!.enabled).isTrue();
+      check(controller!.content.text).equals(oldText);
+    });
+
     testWidgets('ZulipApiException', (tester) async {
       await setupAndTapSend(tester, prepareResponse: (message) {
         connection.prepare(
