compose: Prototype compose box, using Material `TextField` widget #9

chrisbobbe · 2023-02-08T23:46:19Z

The first several commits are sent separately in #8.

Add a prototype compose box

that can compose a stream message but not a PM
that sends to stream ID 7, which on CZO is "test here"
that's missing the native iOS text-input UI features, like
scanning text with the camera (we want to write a thin wrapper
around UITextField and UITextView for that).
that doesn't have autocomplete
that doesn't have quote-and-reply, attach-a-file, etc.
that doesn't send typing notifications
(etc.; this is a prototype :-) )

chrisbobbe · 2023-02-13T23:18:54Z

(Just rebased atop my recently revised #8).

gnprice

Thanks @chrisbobbe! I think this is your first large PR in the Dart/Flutter code 🎉

Comments below. Lots of style things, and some others.

A couple of them are on the boundary of style with architecture: in this codebase I'll want to follow general Flutter practice in being somewhat more careful about performance than we have been in the React Native app.

gnprice · 2023-02-14T03:18:05Z

lib/api/route/messages.dart

+}) async {
+  final data = await connection.post('messages', {
+    'type': RawParameter('stream'), // TODO parametrize
+    'to': 7, // TODO parametrize; this is `#test here`


This is OK for the prototype, but let's do have it check the assumption that this is chat.zulip.org, and throw if not. (Because it's otherwise configurable, in lib/credential_fixture.dart as described in the README.)

Looking at connection.auth.realmUrl would be a good way to check that.

gnprice · 2023-02-14T03:24:24Z

lib/api/route/messages.dart

+
+/// https://zulip.com/api/send-message
+// TODO currently only handles stream messages; fix
+Future<SendMessageResult> sendMessage(


Let's make the lib/api/ changes as their own commit, like "api: Add send-message route".

gnprice · 2023-02-14T03:36:15Z

lib/interactions/showErrorDialog.dart

@@ -0,0 +1,32 @@
+import 'package:flutter/material.dart';
+
+void showErrorDialog(BuildContext context, String title, String? message) {


Instead of a new directory, let's put this in a file lib/widgets/dialog.dart.

I realize this function isn't itself a widget. But it is about widgets — it describes a builder that creates widgets.

Consequently it imports package:flutter/material.dart which in turn imports the framework's widgets library, package:flutter/widgets.dart. And in the future there might be some widgets of our own that we use in this or other dialog-box-creating functions.

So this depends on widgets, and in turn it is certainly depended on by widgets. Might as well put it with the widgets.

gnprice · 2023-02-14T03:46:42Z

lib/interactions/showErrorDialog.dart

@@ -0,0 +1,32 @@
+import 'package:flutter/material.dart';
+
+void showErrorDialog(BuildContext context, String title, String? message) {


In the same style as showDialog itself, and as all widgets in Flutter, let's make these arguments named:

Suggested change

void showErrorDialog(BuildContext context, String title, String? message) {

void showErrorDialog({required BuildContext context, required String title, String? message}) {

gnprice · 2023-02-14T03:47:51Z

lib/widgets/message_list.dart

        reverse: true,
+        // Dismiss compose-box keyboard when dragging up or down.
+        // TODO(?) refine with custom implementation, e.g.:
+        //   - Only dismiss when scrolling up to older messages
+        //   - Show keyboard (focus compose box) on overscroll up to new
+        //     messages (OverscrollNotification)
+        keyboardDismissBehavior: ScrollViewKeyboardDismissBehavior.onDrag,
        itemBuilder: (context, i) => MessageItem(


nit: let's put this above itemCount and reverse, because those and itemBuilder are all closely related in working with the data of the messages

gnprice · 2023-02-14T04:38:37Z

lib/widgets/compose_box.dart

+  // Mimic TextEditingController's non-default constructors, which are not
+  // inherited. (Is there a more transparent way to do this?)
+  TopicTextEditingController({ String? text }) : super(text: text);


If I'm understanding this question right, I think super parameters may be the answer.

gnprice · 2023-02-14T04:42:14Z

lib/widgets/compose_box.dart

+    return trimmed.isEmpty ? kNoTopicTopic : trimmed;
+  }
+
+  Set<TopicValidationError> get validationErrors {


Let's make this a List instead of a Set. Simpler, and therefore cheaper when small. I expect this will rarely have more than one element.

gnprice · 2023-02-14T04:46:52Z

lib/widgets/compose_box.dart

+    if (mandatory && textNormalized == kNoTopicTopic) {
+      result.add(TopicValidationError.mandatoryButEmpty);
+    }
+    if (textNormalized.length > kMaxTopicLength) {
+      result.add(TopicValidationError.tooLong);


Since we use textNormalized twice, let's save the value as a local so as not to repeat the computation.

gnprice · 2023-02-14T04:50:00Z

lib/widgets/compose_box.dart

+  Set<TopicValidationError> get validationErrors {
+    Set<TopicValidationError> result = Set.identity();
+    if (mandatory && textNormalized == kNoTopicTopic) {
+      result.add(TopicValidationError.mandatoryButEmpty);


Since this is doing some computation, not just returning a piece of data that already exists, let's make it a method and not a getter. See the Flutter style guide (though the idea is widespread, not specific to Flutter):
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#getters-feel-faster-than-methods

gnprice · 2023-02-14T05:06:06Z

lib/widgets/compose_box.dart

+    if (textNormalized.isEmpty) {
+      result.add(ContentValidationError.empty);
+    }
+    if (utf8.encode(textNormalized).length > kMaxMessageLengthBytes) {


Eep, this sounds slow to do on every build: converting the whole text from one encoding to another.

Particularly as we're rebuilding on every keystroke, given those listeners. (That should probably change too — this compose-box widget has a very large build tree, most of it doesn't care about the values, and we should avoid rebuilding the parts that don't — but we'll still need to re-ask this question on every keystroke, because it's part of how to show the send button.)

One optimization that would solve it for common cases is to use the fact that the number of UTF-8 bytes is at most 3 times the number of UTF-16 code units, i.e. at most 3 * textNormalized.length. (That's because a three-byte UTF-8 sequence holds 16 bits: 4 in the first byte, 6 in each trailing byte. So every codepoint that fits in 16 bits — i.e., every codepoint that fits into a single UTF-16 code unit — fits into three bytes of UTF-8. And no codepoint takes more than four bytes of UTF-8.)

That'd make it fast up to 3333 UTF-16 code units. But then there'd be a cliff where when the message gets long, things suddenly really get slow.

In preference to that cliff, I think I'd be happier with either of:

When you get up to kMaxMessageLengthBytes/3 code units, that's it — that's the limit. If you were writing mostly ASCII text, you could have gone on 3x longer before the server would have cared. But you probably didn't really want that, did you?

Especially when writing on a phone.

When your message gets long, we just assume that you're writing mostly ASCII text (the stuff that gets one UTF-8 byte per character) and that it's fine, up to kMaxMessageLengthBytes code units. When you actually try to send, we check then.

OK, and after writing those two options out, I think I definitely prefer the former: just set the limit conservatively.

Particularly as we're rebuilding on every keystroke, given those listeners. (That should probably change too — this compose-box widget has a very large build tree, most of it doesn't care about the values, and we should avoid rebuilding the parts that don't […])

(But this part I'm happy to let be while merging this, and I'll try refactoring it in that direction later.)

OK, turns out the server limit here is actually in Unicode codepoints:
https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/max.20message.20size/near/1508001

That's textNormalized.runes.length in Dart. But that's the length of an iterator — it works by walking through the whole string to count the codepoints. Not quite as expensive as utf8.encode, because it doesn't have to allocate a copy of the whole thing, but still not cheap.

So I think what I'd want to do is to just treat the maximum as a max number of UTF-16 code units, i.e. a bound on textNormalized.length.

In reality every codepoint occupies either one or two UTF-16 code units. So this means that if you write a giant message entirely made of emoji, we might cut you off at just 5000 emoji while the server would have accepted up to 10000; same for medieval-era Chinese characters; and more realistically if you have a smattering of those then we'll cut you off a little bit early.

chrisbobbe · 2023-02-16T23:38:42Z

Thanks for the thorough and patient reviews! 😅 Revision pushed.

In this revision, I've pulled out the send button into its own widget (stateful, to show it in disabled and non-disabled states). I'm not sure if I've written the state class's methods in the preferred order.

gnprice

Thanks for the revision! Not done reading yet, but here's a partial review.

gnprice · 2023-02-17T01:53:24Z

lib/api/route/messages.dart

+  // assert() is less verbose but would have no effect in production, I think:
+  //   https://dart.dev/guides/language/language-tour#assert
+  if (Uri.parse(connection.auth.realmUrl).origin != 'https://chat.zulip.org') {
+    throw AssertionError('This binding can currently only be used on https://chat.zulip.org.');


Yep, that's right.

Let's not make it AssertionError, since this isn't an assert statement. From the docs:
https://api.dart.dev/stable/2.19.2/dart-core/AssertionError-class.html

Error thrown by the runtime system when an assert statement fails.

Instead just Error is fine.

gnprice · 2023-02-17T01:57:55Z

lib/widgets/compose_box.dart

@@ -0,0 +1,281 @@
+import 'package:flutter/material.dart';
+import 'package:zulip/widgets/dialog.dart';


Should be a relative import:
https://dart.dev/guides/language/effective-dart/usage#prefer-relative-import-paths

It looks like a lint rule is available for that. I'll go enable it.

Done: 8071718

gnprice · 2023-02-17T02:06:26Z

lib/widgets/compose_box.dart

+    widget.topicController.addListener(topicValueChanged);
+    widget.contentController.addListener(contentValueChanged);


Cool. Yeah, in order to have the send button do its own listening to the text controllers, it needs to be stateful.

gnprice · 2023-02-17T02:07:21Z

lib/widgets/compose_box.dart

+class _StreamComposeBoxState extends State<StreamComposeBox> {
+  final _topicController = TopicTextEditingController();
+  final _contentController = ContentTextEditingController();
+
+  @override
+  void dispose() {


This still needs to listen to the topic text controller too, though — otherwise the content input's placeholder doesn't update.

(Alternatively that could be its own widget.)

("that" meaning the content input)

gnprice

OK, and here's the rest of a review.

gnprice · 2023-02-17T04:20:27Z

lib/widgets/compose_box.dart

+  late bool topicHasValidationErrors;
+  late bool contentHasValidationErrors;


Let's store the whole validationErrors() results. That way we don't have to repeat the calls in _handleSendPressed.

gnprice · 2023-02-17T04:22:35Z

lib/widgets/compose_box.dart

+  late bool topicHasValidationErrors;
+  late bool contentHasValidationErrors;
+
+  topicValueChanged() {


Let's make these private.

They would be function expressions at the addListener call sites, if not for the fact we have to pass identical values to removeListener. They'd certainly be private in that case, so they should stay that way.

gnprice · 2023-02-17T04:28:15Z

lib/widgets/compose_box.dart

+    List<TopicValidationError> result = [];
+    final normalized = textNormalized();
+    if (mandatory && normalized == kNoTopicTopic) {
+      result.add(TopicValidationError.mandatoryButEmpty);
+    }
+    if (normalized.length > kMaxTopicLength) {
+      result.add(TopicValidationError.tooLong);
+    }
+    return result;


Here's some fun Dart syntax:

Suggested change

List<TopicValidationError> result = [];

final normalized = textNormalized();

if (mandatory && normalized == kNoTopicTopic) {

result.add(TopicValidationError.mandatoryButEmpty);

}

if (normalized.length > kMaxTopicLength) {

result.add(TopicValidationError.tooLong);

}

return result;

final normalized = textNormalized();

return [

if (mandatory && normalized == kNoTopicTopic)

TopicValidationError.mandatoryButEmpty,

if (normalized.length > kMaxTopicLength)

TopicValidationError.tooLong,

];

This is called "collection if", or "if elements". See docs:
https://dart.dev/guides/language/language-tour#collection-operators
https://github.com/dart-lang/language/blob/master/accepted/2.3/control-flow-collections/feature-specification.md

gnprice · 2023-02-17T04:31:02Z

lib/widgets/compose_box.dart

+  // TODO: upload in progress
+  // TODO: quote-and-reply in progress


No need for TODO comments here, because there's nothing actionable — we'll naturally add these additional error conditions in the course of adding those features.

Could say something like this, if perhaps you feel like the structure of this code looks too elaborate for just two error cases and want to illustrate how things will get more complex later:

Suggested change

// TODO: upload in progress

// TODO: quote-and-reply in progress

// Later: upload in progress; quote-and-reply in progress

gnprice · 2023-02-17T04:34:33Z

lib/widgets/compose_box.dart

+/// The send button for StreamComposeBox.
+class _StreamSendButton extends StatefulWidget {


I feel like we'll probably want one _SendButton class that provides the send button for all compose boxes, in the end. But that can be a later refactoring after we have one or both of the other forms of compose box.

gnprice · 2023-02-17T05:49:44Z

lib/widgets/dialog.dart

+            child: const Text(
+              // TODO(i18n)
+              'OK',
+
+              // As suggested by
+              //   https://api.flutter.dev/flutter/material/AlertDialog/actions.html :
+              // > It is recommended to set the Text.textAlign to TextAlign.end
+              // > for the Text within the TextButton, so that buttons whose
+              // > labels wrap to an extra line align with the overall
+              // > OverflowBar's alignment within the dialog.
+              textAlign: TextAlign.end,
+            ),


Interesting.

Let's factor the thing this comment is concerned with into its own little helper function, private to this file. That makes a better home for the comment, so this showErrorDialog function can stay more focused on what's specific to an error dialog.

Like this:

Widget _dialogActionText(String text) { return Text( text, // As suggested by // https://api.flutter.dev/flutter/material/AlertDialog/actions.html : // > It is recommended to set the Text.textAlign to TextAlign.end // > for the Text within the TextButton, so that buttons whose // > labels wrap to an extra line align with the overall // > OverflowBar's alignment within the dialog. textAlign: TextAlign.end, ); }

gnprice · 2023-02-17T05:52:07Z

lib/widgets/dialog.dart

+  showDialog(
+      context: context,
+      builder: (BuildContext context) => AlertDialog(
+        // TODO(i18n)


Let's consolidate these TODO(i18n) comments. At the top of the function, we can say:

// TODO(i18n): title, message, and action-button text

That lets several blank lines go away too, making the whole thing much more compact.

gnprice · 2023-02-17T06:07:03Z

lib/widgets/dialog.dart

+        title: Text(title),
+
+        // TODO(i18n)
+        content: message != null ? Text(message) : null,


The docs for this property say:

Typically this is a SingleChildScrollView that contains the dialog's message. As noted in the AlertDialog documentation, it's important to use a SingleChildScrollView if there's any risk that the content will not fit.

Seems like there is such a risk here — the message could be several lines long, and the device could be short or in landscape mode or be set to a giant font size — so we should do that.

gnprice · 2023-02-17T06:12:39Z

lib/widgets/message_list.dart

+        // Dismiss compose-box keyboard when dragging up or down.
+        // TODO(?) refine with custom implementation, e.g.:
+        //   - Only dismiss when scrolling up to older messages
+        //   - Show keyboard (focus compose box) on overscroll up to new
+        //     messages (OverscrollNotification)
+        keyboardDismissBehavior: ScrollViewKeyboardDismissBehavior.onDrag,


Hmm. Playing with this behavior, I think I'm happier without it — seems like it'd be annoying when writing something where I want to refer back to things people said earlier.

I just did a quick small survey, and behavior in other messaging apps varies:

Discord leaves the keyboard up if you scroll (on Android, anyway; the below are all on Android)

Messages, i.e. the stock SMS app, dismisses it… ah, in fact, dismisses it only if you scroll up, not down

Signal leaves the keyboard up

Sure; removed. It would be good to find a way to let the user dismiss the keyboard, even if this isn't it.

gnprice · 2023-02-17T06:14:56Z

lib/widgets/dialog.dart

+        ],
+      )
+  );
+}


nit: every line should end with a newline, including the last

chrisbobbe · 2023-02-18T00:56:01Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! This looks great. Two nits below, and otherwise ready to merge.

gnprice · 2023-02-22T01:13:06Z

lib/widgets/message_list.dart

+            // This seems to offer the only way to close the keyboard on iOS.
+            // It's not ideal; see TODO above.
+            ? ScrollViewKeyboardDismissBehavior.onDrag


only built-in way — as the name of the other option, manual, suggests, we could also build such a way ourselves with a gesture recognizer, and IIRC that issue thread has several people saying they did just that (while wishing Flutter would provide a built-in way instead)

gnprice · 2023-02-22T01:19:57Z

lib/widgets/compose_box.dart

+        message: validationErrorMessages.join('\n\n'));
+  }
+
+  void _handleSendPressed (BuildContext context) {


nit:

Suggested change

void _handleSendPressed (BuildContext context) {

void _handleSendPressed(BuildContext context) {

chrisbobbe · 2023-02-22T05:53:37Z

Thanks for the review! Revision pushed.

chrisbobbe requested a review from gnprice February 8, 2023 23:46

chrisbobbe force-pushed the pr-compose-text-field branch from cb66c85 to 16e145a Compare February 13, 2023 23:18

gnprice reviewed Feb 14, 2023

View reviewed changes

chrisbobbe force-pushed the pr-compose-text-field branch from 16e145a to 4980bc8 Compare February 16, 2023 23:37

gnprice reviewed Feb 17, 2023

View reviewed changes

api: Add send-message route

f0b10c8

chrisbobbe force-pushed the pr-compose-text-field branch from 4980bc8 to 5bff7f4 Compare February 18, 2023 00:55

chrisbobbe force-pushed the pr-compose-text-field branch 2 times, most recently from ed9a965 to 7aa18c1 Compare February 18, 2023 01:22

gnprice reviewed Feb 22, 2023

View reviewed changes

compose: Prototype compose box, using Material TextField widget

cd4ff5e

chrisbobbe force-pushed the pr-compose-text-field branch from 7aa18c1 to cd4ff5e Compare February 22, 2023 05:53

gnprice merged commit cd4ff5e into zulip:main Feb 22, 2023

chrisbobbe deleted the pr-compose-text-field branch February 22, 2023 05:56

gnprice mentioned this pull request Feb 23, 2023

Add a login flow #11

Closed

gnprice mentioned this pull request Apr 4, 2023

Decide what to do about text input on iOS #47

Closed

3 tasks

gnprice mentioned this pull request Jun 1, 2023

Compose box for topic (and DM) narrows #144

Closed

		@@ -0,0 +1,32 @@
		import 'package:flutter/material.dart';

		void showErrorDialog(BuildContext context, String title, String? message) {

	void showErrorDialog(BuildContext context, String title, String? message) {
	void showErrorDialog({required BuildContext context, required String title, String? message}) {

		@@ -0,0 +1,281 @@
		import 'package:flutter/material.dart';
		import 'package:zulip/widgets/dialog.dart';

		widget.topicController.addListener(topicValueChanged);
		widget.contentController.addListener(contentValueChanged);

		late bool topicHasValidationErrors;
		late bool contentHasValidationErrors;

		// TODO: upload in progress
		// TODO: quote-and-reply in progress

	// TODO: upload in progress
	// TODO: quote-and-reply in progress
	// Later: upload in progress; quote-and-reply in progress

		/// The send button for StreamComposeBox.
		class _StreamSendButton extends StatefulWidget {

	void _handleSendPressed (BuildContext context) {
	void _handleSendPressed(BuildContext context) {

compose: Prototype compose box, using Material TextField widget #9

compose: Prototype compose box, using Material TextField widget #9

Conversation

chrisbobbe commented Feb 8, 2023 • edited Loading

chrisbobbe commented Feb 13, 2023

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

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 Feb 16, 2023

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

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

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 Feb 18, 2023

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 Feb 22, 2023

compose: Prototype compose box, using Material `TextField` widget #9

compose: Prototype compose box, using Material `TextField` widget #9

chrisbobbe commented Feb 8, 2023 •

edited

Loading