Skip to content

compose: Prototype upload-file UI #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 18, 2023
Merged

compose: Prototype upload-file UI #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2b3e781
deps: Upgrade material_color_utilities, source_span via `flutter pub …
chrisbobbe Apr 12, 2023
ef3db68
api: Add `close` method to ApiConnection
chrisbobbe Apr 11, 2023
ab60489
api: Have LiveApiConnection instance use a persistent http.Client
chrisbobbe Apr 11, 2023
ca20d0e
deps: Add file_picker at latest
chrisbobbe Apr 12, 2023
723b399
model: Add maxFileUploadSizeMib to InitialSnapshot
chrisbobbe Apr 12, 2023
1650f0d
api: Add ApiConnection.postFileFromStream
chrisbobbe Apr 12, 2023
f9139e4
api: Add upload-file route
chrisbobbe Apr 12, 2023
5dca335
compose [nfc]: Rename ContentTextEditingController's listener
chrisbobbe Apr 12, 2023
e221d46
compose [nfc]: Rename TopicTextEditingController's listeners
chrisbobbe Apr 14, 2023
0c029b5
compose [nfc]: Add _contentFocusNode and pass to TextField
chrisbobbe Apr 17, 2023
cb25643
compose: Prototype upload-file UI
chrisbobbe Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions ios/Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,70 @@
PODS:
- device_info_plus (0.0.1):
- Flutter
- DKImagePickerController/Core (4.3.4):
- DKImagePickerController/ImageDataManager
- DKImagePickerController/Resource
- DKImagePickerController/ImageDataManager (4.3.4)
- DKImagePickerController/PhotoGallery (4.3.4):
- DKImagePickerController/Core
- DKPhotoGallery
- DKImagePickerController/Resource (4.3.4)
- DKPhotoGallery (0.0.17):
- DKPhotoGallery/Core (= 0.0.17)
- DKPhotoGallery/Model (= 0.0.17)
- DKPhotoGallery/Preview (= 0.0.17)
- DKPhotoGallery/Resource (= 0.0.17)
- SDWebImage
- SwiftyGif
- DKPhotoGallery/Core (0.0.17):
- DKPhotoGallery/Model
- DKPhotoGallery/Preview
- SDWebImage
- SwiftyGif
- DKPhotoGallery/Model (0.0.17):
- SDWebImage
- SwiftyGif
- DKPhotoGallery/Preview (0.0.17):
- DKPhotoGallery/Model
- DKPhotoGallery/Resource
- SDWebImage
- SwiftyGif
- DKPhotoGallery/Resource (0.0.17):
- SDWebImage
- SwiftyGif
- file_picker (0.0.1):
- DKImagePickerController/PhotoGallery
- Flutter
- Flutter (1.0.0)
- path_provider_foundation (0.0.1):
- Flutter
- FlutterMacOS
- SDWebImage (5.15.5):
- SDWebImage/Core (= 5.15.5)
- SDWebImage/Core (5.15.5)
- share_plus (0.0.1):
- Flutter
- SwiftyGif (5.4.4)

DEPENDENCIES:
- device_info_plus (from `.symlinks/plugins/device_info_plus/ios`)
- file_picker (from `.symlinks/plugins/file_picker/ios`)
- Flutter (from `Flutter`)
- path_provider_foundation (from `.symlinks/plugins/path_provider_foundation/darwin`)
- share_plus (from `.symlinks/plugins/share_plus/ios`)

SPEC REPOS:
trunk:
- DKImagePickerController
- DKPhotoGallery
- SDWebImage
- SwiftyGif

EXTERNAL SOURCES:
device_info_plus:
:path: ".symlinks/plugins/device_info_plus/ios"
file_picker:
:path: ".symlinks/plugins/file_picker/ios"
Flutter:
:path: Flutter
path_provider_foundation:
Expand All @@ -26,9 +74,14 @@ EXTERNAL SOURCES:

SPEC CHECKSUMS:
device_info_plus: e5c5da33f982a436e103237c0c85f9031142abed
DKImagePickerController: b512c28220a2b8ac7419f21c491fc8534b7601ac
DKPhotoGallery: fdfad5125a9fdda9cc57df834d49df790dbb4179
file_picker: ce3938a0df3cc1ef404671531facef740d03f920
Flutter: f04841e97a9d0b0a8025694d0796dd46242b2854
path_provider_foundation: c68054786f1b4f3343858c1e1d0caaded73f0be9
SDWebImage: fd7e1a22f00303e058058278639bf6196ee431fe
share_plus: 056a1e8ac890df3e33cb503afffaf1e9b4fbae68
SwiftyGif: 93a1cc87bf3a51916001cf8f3d63835fb64c819f

PODFILE CHECKSUM: 985e5b058f26709dc81f9ae74ea2b2775bdbcefe

Expand Down
4 changes: 4 additions & 0 deletions ios/Runner/Info.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,9 @@
<true/>
<key>UIApplicationSupportsIndirectInputEvents</key>
<true/>
<key>UIBackgroundModes</key>
<array>
<string>fetch</string>
</array>
</dict>
</plist>
34 changes: 32 additions & 2 deletions lib/api/core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ abstract class ApiConnection {
// that ensures nothing assumes base class has a real API key
final Auth auth;

void close();

Future<String> get(String route, Map<String, dynamic>? params);

Future<String> post(String route, Map<String, dynamic>? params);

Future<String> postFileFromStream(String route, Stream<List<int>> content, int length, { String? filename });
}

// TODO memoize
Expand All @@ -49,10 +53,22 @@ Map<String, String> authHeader(Auth auth) {
class LiveApiConnection extends ApiConnection {
LiveApiConnection({required super.auth});

final http.Client _client = http.Client();

bool _isOpen = true;

@override
void close() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void close() {
void close() {
assert(_isOpen);

Attempting to close twice seems like that'd be a sign of a bug and we'd want to catch it with an assert.

assert(_isOpen);
_client.close();
_isOpen = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Map<String, String> _headers() => authHeader(auth);

@override
Future<String> get(String route, Map<String, dynamic>? params) async {
assert(_isOpen);
final baseUrl = Uri.parse(auth.realmUrl);
final url = Uri(
scheme: baseUrl.scheme,
Expand All @@ -62,7 +78,7 @@ class LiveApiConnection extends ApiConnection {
path: "/api/v1/$route",
queryParameters: encodeParameters(params));
if (kDebugMode) print("GET $url");
final response = await http.get(url, headers: _headers());
final response = await _client.get(url, headers: _headers());
if (response.statusCode != 200) {
throw Exception("error on GET $route: status ${response.statusCode}");
}
Expand All @@ -71,7 +87,8 @@ class LiveApiConnection extends ApiConnection {

@override
Future<String> post(String route, Map<String, dynamic>? params) async {
final response = await http.post(
assert(_isOpen);
final response = await _client.post(
Uri.parse("${auth.realmUrl}/api/v1/$route"),
headers: _headers(),
body: encodeParameters(params));
Expand All @@ -80,6 +97,19 @@ class LiveApiConnection extends ApiConnection {
}
return utf8.decode(response.bodyBytes);
}

@override
Future<String> postFileFromStream(String route, Stream<List<int>> content, int length, { String? filename }) async {
assert(_isOpen);
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());
Comment on lines +104 to +106
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final response = await http.Response.fromStream(await _client.send(request));
if (response.statusCode != 200) {
throw Exception("error on file-upload POST $route: status ${response.statusCode}");
}
return utf8.decode(response.bodyBytes);
}
}

Map<String, dynamic>? encodeParameters(Map<String, dynamic>? params) {
Expand Down
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class InitialSnapshot {

final List<Subscription> subscriptions;

final int maxFileUploadSizeMib;

// TODO etc., etc.

InitialSnapshot({
Expand All @@ -32,6 +34,7 @@ class InitialSnapshot {
required this.alertWords,
required this.customProfileFields,
required this.subscriptions,
required this.maxFileUploadSizeMib,
});

factory InitialSnapshot.fromJson(Map<String, dynamic> json) =>
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/initial_snapshot.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,28 @@ class SendMessageResult {

Map<String, dynamic> toJson() => _$SendMessageResultToJson(this);
}

/// https://zulip.com/api/upload-file
Future<UploadFileResult> uploadFile(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in commit message:

api: Add uploadFile

The form in previous commits would be:

api: Add upload-file route

In particular the explicitness that the name it's mentioning is the name of a route seems helpful.

ApiConnection connection, {
required Stream<List<int>> content,
required int length,
required String filename,
}) async {
final data = await connection.postFileFromStream('user_uploads', content, length, filename: filename);
return UploadFileResult.fromJson(jsonDecode(data));
}

@JsonSerializable(fieldRename: FieldRename.snake)
class UploadFileResult {
final String uri;

UploadFileResult({
required this.uri,
});

factory UploadFileResult.fromJson(Map<String, dynamic> json) =>
_$UploadFileResultFromJson(json);

Map<String, dynamic> toJson() => _$UploadFileResultToJson(this);
}
10 changes: 10 additions & 0 deletions lib/api/route/messages.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,15 @@ class PerAccountStore extends ChangeNotifier {
required InitialSnapshot initialSnapshot,
}) : zulipVersion = initialSnapshot.zulipVersion,
subscriptions = Map.fromEntries(initialSnapshot.subscriptions.map(
(subscription) => MapEntry(subscription.streamId, subscription)));
(subscription) => MapEntry(subscription.streamId, subscription))),
maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib;

final Account account;
final ApiConnection connection;

final String zulipVersion;
final Map<int, Subscription> subscriptions;
final int maxFileUploadSizeMib; // No event for this.

// TODO lots more data. When adding, be sure to update handleEvent too.

Expand Down Expand Up @@ -294,7 +296,7 @@ class LivePerAccountStore extends PerAccountStore {
final result = await getEvents(connection,
queueId: queueId, lastEventId: lastEventId);
// TODO handle errors on get-events; retry with backoff
// TODO abort long-poll on [dispose]
// TODO abort long-poll and close LiveApiConnection on [dispose]
Comment on lines -297 to +299
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this too

final events = result.events;
for (final event in events) {
handleEvent(event);
Expand Down
Loading