compose: Prototype upload-file UI #63

chrisbobbe · 2023-04-13T01:01:06Z

Fixes: #57

chrisbobbe · 2023-04-13T01:02:27Z

I found Dart's new Record feature helpful in a few places!

chrisbobbe · 2023-04-13T01:07:42Z

lib/widgets/compose_box.dart

+    final placeholder = '[Uploading $filename...]()\n\n'; // TODO(i18n)
+    _uploads[tag] = (filename: filename, placeholder: placeholder);
+    notifyListeners(); // _uploads change could affect validationErrors
+    value = value.replaced(_cursorPositionOrEnd(), placeholder);


This differs from zulip-mobile in the case where the selection is not collapsed, i.e., instead of a blinking cursor, there's a highlight on some of the text.

We still don't replace the selected text; that's unchanged here. But:

In zulip-mobile, selection-end is taken to be the cursor position, and the placeholder is inserted there.

In this branch, the placeholder is inserted at the end of the whole content-input text.

Yeah. What's the reasoning for that difference?

At first thought it seems a bit irregular that

if you put the cursor somewhere in the middle of the text and add a file, the placeholder / upload goes there;

but if you instead long-press so that a whole word or whatever is selected, then add a file, the placeholder / upload goes over at the end of the text.

I wouldn't mind following zulip-mobile here, but here's an attempt to describe why I was drawn to this new approach:

I think users are unlikely to see text selection as one of the steps to take to attach a file. The zulip-mobile design follows from that judgment too: there, instead of (e.g.) replacing the selected text, we ignore the fact that some text was selected and we insert the markdown at selection-end.

Then I thought, if some text is selected, it's probably from some previous interaction, like looking up a word in the dictionary. Or it's just an accident. Anyway, the user probably doesn't know, or has forgotten, that the UI wants a hint for a single index where the user will be happy to see the markdown inserted. So I figured nothing in selection (like the start or end) will predict that index well, and we might as well do what we do when the input is unfocused, and what "attach" normally means, which is to put it at the end.

We discussed briefly in the office just now. We'll go and do the insertion where the selection is, not because that's a spot the user was probably asking for it to go exactly (if they did have an exact spot in mind that they were trying to convey, they'd likely have placed a cursor and not a whole selection range), but because we want it to be in a place they're paying attention to, and the selection range will be that.

In particular, if the draft content is long, the end of the content may not even be visible on screen — if we insert there, the user may perceive that nothing happened. The selection area will be on screen, and it has a conspicuous pair of selection handles and conspicuous highlighting.

gnprice

Thanks! Comments below.

gnprice · 2023-04-13T18:12:08Z

lib/api/core.dart

+  @override
+  void close() {
+    _client.close();
+    _isOpen = false;


nit: this line should appear in the commit that adds close and _isOpen, rather than the next commit

gnprice · 2023-04-13T18:13:29Z

lib/model/store.dart

-      // TODO abort long-poll on [dispose]
+      // TODO abort long-poll and close LiveApiConnection on [dispose]


nit: this too

gnprice · 2023-04-13T18:20:20Z

lib/api/model/initial_snapshot.dart

@@ -17,6 +17,8 @@ class InitialSnapshot {

  final List<CustomProfileField> customProfileFields;

+  final int maxFileUploadSizeMib;


Let's put this after subscriptions, following the order in the docs.

I think for this type we'll probably want to follow the docs' order exactly, because it's such a very long list (in the docs, and eventually in our type as we fill out more of it) that it'd otherwise be unmanageable to compare the one list to the other.

gnprice · 2023-04-13T18:23:52Z

lib/api/core.dart

+  final http.Client _client = http.Client();
+
  Map<String, String> _headers() => authHeader(auth);

+  bool _isOpen = true;


nit: don't split up the fields with an unrelated method

Specifically I think putting _headers after close, before get, would be good. It's basically a shared private helper for get and post and friends.

gnprice · 2023-04-13T18:40:20Z

lib/api/core.dart

+    http.MultipartRequest request = http.MultipartRequest('POST', Uri.parse("${auth.realmUrl}/api/v1/$route"))
+      ..files.add(http.MultipartFile('file', content, length, filename: filename))
+      ..headers.addAll(_headers());


Neat. Where did you find this as the way to make this sort of request?

I noticed that zulip-mobile was using the header Content-Type: multipart/form-data for all and only the requests to send a file to the server, so I figured we wanted that header too. This MultipartRequest class seemed like a convenient way to make a request with that header—

A multipart/form-data request.

—and then I was also happy to see the convenient interface for uploading files, with ..files.add, etc.

For how to represent the file, I chose Stream<List<int>> content instead of a path, hoping that would push any file-access issues (like with permissions) out to the caller to deal with, and letting our /api/ code continue to be just a channel for data going to and from the server.

gnprice · 2023-04-13T22:25:44Z

lib/widgets/compose_box.dart

+      final readStream = file.readStream;
+      final size = file.size;
+      final name = file.name;


Can use a pattern here too:

Suggested change

final readStream = file.readStream;

final size = file.size;

final name = file.name;

final PlatformFile(:readStream, :size, :name) = file;

Oh, nice! Great to have all that on one line.

gnprice · 2023-04-13T22:30:14Z

lib/widgets/compose_box.dart

+
+      // Will fail if we didn't pass `withReadStream: true` to pickFiles
+      assert(readStream != null, 'readStream missing');


Suggested change

// Will fail if we didn't pass `withReadStream: true` to pickFiles

assert(readStream != null, 'readStream missing');

assert(readStream != null); // We passed `withReadStream: true` to pickFiles.

The description string isn't needed because it's redundant with the expression.

One of the nice things about assert being a language feature (vs. the invariant function we've used in JS in zulip-mobile) is that it can, and does, automatically include the source text of the failed expression in the failure message.

Hmm, interesting!

gnprice · 2023-04-13T22:32:14Z

lib/widgets/compose_box.dart

+        final result = await uploadFile(store.connection, content: readStream!, length: size, filename: name);
+        url = realmUrl.resolve(result.uri);


Do we need to use resolve? I think the relative URL should work fine. Indeed I think that's what web uses (which would be why that's what the upload-file API route returns.)

Oh OK, sure I think so. Will test to make sure.

gnprice · 2023-04-13T22:33:41Z

lib/widgets/compose_box.dart

+        // TODO: Specifically handle `413 Payload Too Large`
+        // TODO: On API errors, quote `msg` from server, with "The server said:"


Can help make these findable when we do the underlying work to make them possible:

Suggested change

// TODO: Specifically handle `413 Payload Too Large`

// TODO: On API errors, quote `msg` from server, with "The server said:"

// TODO(#37): Specifically handle `413 Payload Too Large`

// TODO(#37): On API errors, quote `msg` from server, with "The server said:"

Oh right; I'd forgotten we had that issue #37, thanks.

gnprice · 2023-04-13T22:36:59Z

lib/widgets/compose_box.dart

+              ],
+            ))));


nit:

Suggested change

],

))));

]))));

As we did for ContentTextEditingController's listener in the previous commit. As Greg pointed out in reviewing the previous commit: zulip#63 (comment) > Perhaps apply the same rename to the topic controller too. We > won't currently be using that extra generality, but in principle > there's no reason we wouldn't.

chrisbobbe · 2023-04-14T22:21:48Z

Thanks for the review! Revision pushed, and please see my response here: #63 (comment)

…get` Much like the `meta` package a few weeks ago in 0b89ce9, I don't understand why this causes an update, but it does.

Soon we'll give LiveApiConnection a nontrivial implementation of this method.

As Alex points out in discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20http.20.60Client.60/near/1545511 > Looks like doing so will allow connection reuse, which is a great > performance boost, particularly with HTTP/2. So that seems good. This will also help with file uploads (zulip#56, zulip#57, and zulip#61) because http.Client has a `send` method -- https://pub.dev/documentation/http/latest/http/Client/send.html -- that we can use for requests we want more control over (in particular, a file-upload request), and a counterpart toplevel convenience function like `http.send` isn't offered. See doc: https://pub.dev/documentation/http/latest/http/Client-class.html

Toward zulip#57, "Attach files from compose box, with file picker".

And, from there, to PerAccountStore.

As we did for ContentTextEditingController's listener in the previous commit. As Greg pointed out in reviewing the previous commit: zulip#63 (comment) > Perhaps apply the same rename to the topic controller too. We > won't currently be using that extra generality, but in principle > there's no reason we wouldn't.

chrisbobbe · 2023-04-17T20:43:10Z

Revision pushed, fixing #63 (comment) and some indentation things.

Also in this revision, I've made it so the content input gets focused when you select files, if it wasn't focused before.

gnprice · 2023-04-17T22:59:43Z

lib/api/core.dart

+    if (response.statusCode != 200) {
+      throw Exception("error on file-upload POST $route: status ${response.statusCode}");
+    }
+


nit:

Suggested change

(the diff at #63 (comment) had this but was pretty subtle to look at)

gnprice · 2023-04-17T23:17:28Z

lib/widgets/compose_box.dart

+  TextRange _cursorPositionOrEnd() {
+    final TextRange selection = value.selection;
+    final String text = value.text;
+    final int index = (selection.isValid && selection.end < text.length && selection.isCollapsed)


I see. Yeah, there should be an invariant that selection.end <= text.length (but I think equal to the length is fine and normal — that's what you have when the caret is at the end.) And then there's no need for us to re-check invariants that the framework should be providing.

The TextSelection API is unusual in that it has this special value it calls "invalid", namely (-1,-1), and it actually uses that "invalid" value. That should really be represented as null instead, so that an actual TextSelection value would always mean a valid selection. Everyone involved understands that the current API is unfortunate, but it's been a pain to try to migrate off of:
flutter/flutter#122480 (comment)
flutter/flutter#95978 (comment)
flutter/flutter#79615

So when dealing with that isValid getter, one should think of it as having a meaning similar to "!= null". It fortunately doesn't open up a need to start checking other "validity" conditions of that API where one otherwise wouldn't.

gnprice

Thanks for the revision! All looks good with just tiny nits (here and above.)

I also tried out the new UI (on an emulated Android 13 device) and it seems fine.

Also in this revision, I've made it so the content input gets focused when you select files, if it wasn't focused before.

Sure, seems reasonable.

gnprice · 2023-04-17T23:22:35Z

lib/widgets/compose_box.dart

+
+    for (final (tag, file) in uploadsInProgress) {
+      final PlatformFile(:readStream, :size, :name) = file;
+


nit:

Suggested change

TopicTextEditingController and ContentTextEditingController inherit from ValueNotifier, so they have a `value` property, and their listeners are notified when that property changes. They don't currently notify their listeners in any other cases, so the listeners are reasonable to have "value" in their names. But for ContentTextEditingController, soon we'll want to call notifyListeners for something other than a change in `value`: in particular, it'll have a stateful property to track in-progress file uploads, and that property will affect `validationErrors`, so we'll call `notifyListeners` when a file upload starts or ends. So, we give its listener this more general name.

As we did for ContentTextEditingController's listener in the previous commit. As Greg pointed out in reviewing the previous commit: zulip#63 (comment) > Perhaps apply the same rename to the topic controller too. We > won't currently be using that extra generality, but in principle > there's no reason we wouldn't.

This will let us query and request focus on the content input, which we'd like to do soon. (When inserting upload-file markdown, we'd like to focus the content input if it wasn't already focused.)

Fixes: zulip#57

chrisbobbe · 2023-04-18T00:11:47Z

Thanks for the review! Revision pushed.

chrisbobbe requested a review from gnprice April 13, 2023 01:01

chrisbobbe commented Apr 13, 2023

View reviewed changes

gnprice reviewed Apr 13, 2023

View reviewed changes

chrisbobbe force-pushed the pr-upload-file branch from 94d445c to fd4f94b Compare April 14, 2023 22:21

chrisbobbe added 5 commits April 17, 2023 12:29

deps: Upgrade material_color_utilities, source_span via `flutter pub …

2b3e781

…get` Much like the `meta` package a few weeks ago in 0b89ce9, I don't understand why this causes an update, but it does.

api: Add close method to ApiConnection

ef3db68

Soon we'll give LiveApiConnection a nontrivial implementation of this method.

deps: Add file_picker at latest

ca20d0e

Toward zulip#57, "Attach files from compose box, with file picker".

model: Add maxFileUploadSizeMib to InitialSnapshot

723b399

And, from there, to PerAccountStore.

chrisbobbe force-pushed the pr-upload-file branch from fd4f94b to 72186a3 Compare April 17, 2023 20:40

gnprice reviewed Apr 17, 2023

View reviewed changes

gnprice approved these changes Apr 17, 2023

View reviewed changes

chrisbobbe added 6 commits April 17, 2023 16:57

api: Add ApiConnection.postFileFromStream

1650f0d

api: Add upload-file route

f9139e4

compose [nfc]: Add _contentFocusNode and pass to TextField

0c029b5

This will let us query and request focus on the content input, which we'd like to do soon. (When inserting upload-file markdown, we'd like to focus the content input if it wasn't already focused.)

compose: Prototype upload-file UI

cb25643

Fixes: zulip#57

chrisbobbe force-pushed the pr-upload-file branch from 72186a3 to cb25643 Compare April 18, 2023 00:11

gnprice merged commit cb25643 into zulip:main Apr 18, 2023

chrisbobbe mentioned this pull request Apr 19, 2023

urls [nfc]: Represent realm URL as Uri object, not String #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Prototype upload-file UI #63

compose: Prototype upload-file UI #63

chrisbobbe commented Apr 13, 2023

chrisbobbe commented Apr 13, 2023

chrisbobbe Apr 13, 2023 •

edited

Loading

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023 •

edited

Loading

gnprice Apr 14, 2023

gnprice left a comment

gnprice Apr 13, 2023

gnprice Apr 13, 2023

gnprice Apr 13, 2023

gnprice Apr 13, 2023

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023

gnprice Apr 13, 2023

chrisbobbe Apr 14, 2023

gnprice Apr 13, 2023

chrisbobbe commented Apr 14, 2023

chrisbobbe commented Apr 17, 2023

gnprice Apr 17, 2023

gnprice Apr 17, 2023

gnprice left a comment

gnprice Apr 17, 2023

chrisbobbe commented Apr 18, 2023

		// TODO abort long-poll on [dispose]
		// TODO abort long-poll and close LiveApiConnection on [dispose]

		@@ -17,6 +17,8 @@ class InitialSnapshot {

		final List<CustomProfileField> customProfileFields;

		final int maxFileUploadSizeMib;


		// Will fail if we didn't pass `withReadStream: true` to pickFiles
		assert(readStream != null, 'readStream missing');

		final result = await uploadFile(store.connection, content: readStream!, length: size, filename: name);
		url = realmUrl.resolve(result.uri);

		// TODO: Specifically handle `413 Payload Too Large`
		// TODO: On API errors, quote `msg` from server, with "The server said:"


		for (final (tag, file) in uploadsInProgress) {
		final PlatformFile(:readStream, :size, :name) = file;

compose: Prototype upload-file UI #63

compose: Prototype upload-file UI #63

Conversation

chrisbobbe commented Apr 13, 2023

chrisbobbe commented Apr 13, 2023

chrisbobbe Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 14, 2023 • 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

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 Apr 14, 2023

chrisbobbe commented Apr 17, 2023

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

chrisbobbe commented Apr 18, 2023

chrisbobbe Apr 13, 2023 •

edited

Loading

chrisbobbe Apr 14, 2023 •

edited

Loading