Skip to content

Add images from compose box #56

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.

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

Closed
gnprice opened this issue Apr 7, 2023 · 7 comments · Fixed by #70
Closed

Add images from compose box #56

gnprice opened this issue Apr 7, 2023 · 7 comments · Fixed by #70
Labels
a-compose Compose box, autocomplete, attaching files/images

Comments

@gnprice
Copy link
Member

gnprice commented Apr 7, 2023

I.e., the feature represented by a "picture" icon under the compose box, in the existing RN app.

This should present the user with an appropriate image-picker UI from the platform, where they're able to select from the images they have on the device.

After the user selects an image, they should have the opportunity to add text before sending the message. (We only fixed that pretty recently in the RN app: zulip/zulip-mobile#4540 . But it was a bad UX before that, which we shouldn't repeat.) Probably this means doing the same thing we do now (since zulip/zulip-mobile#5474) in the RN app, which is also what we do in the web app: we immediately insert a placeholder into the message text, then start the upload, and when the image is uploaded and we have a URL for it we fill in the placeholder.

For implementing the platform side of this feature:

  • Flutter has a first-party plugin image_picker.
    • 80 open issues, of 390 total; see ranked by upvotes.
    • Bulk of the top issues are closed. Looking at the top remaining issues, none seem worrying.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 13, 2023
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 14, 2023
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
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 17, 2023
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
@chrisbobbe
Copy link
Collaborator

Sure, good to use a first-party plugin. If we run into a problem with image_picker, I think file_picker (used in #57, attaching generic files) also offers the option to open this kind of image-specific UI, if you ask it to by passing a param (type).

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Apr 18, 2023

Ah, I may have already run into a problem: image_picker lets you call a method to pick an image OR a method to pick multiple images OR a method to pick a video. We want a method that lets you pick from a mixed list of images and videos, and that lets you pick one or more.

file_picker seems to do that job nicely, if you pass type: FileType.media; let's try using that.

@gnprice
Copy link
Member Author

gnprice commented Apr 18, 2023

We want a method that lets you pick from a mixed list of images and videos, and that lets you pick one or more.

Yeah. Looks like that's flutter/flutter#89159 and flutter/flutter#102283 .

Sure, we can try file_picker. I'm curious what kind of UI it presents in that case — whether it's a proper "image gallery" style of UI that's focused on the contents of the images, or still a "file browser" style of UI.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Apr 19, 2023

Sure, we can try file_picker. I'm curious what kind of UI it presents in that case — whether it's a proper "image gallery" style of UI that's focused on the contents of the images, or still a "file browser" style of UI.

On iOS, it looks like what we want:

I can long-press on images to see them in more detail, and note the search-bar placeholder text.

Will test on Android next. 🙂

@chrisbobbe
Copy link
Collaborator

Hmm, this doesn't look like what we'd want on Android (office device):

But before giving up on file_picker for Android, I'd like to try on Android 13 or later; the office device is running Android 9. It's possible this library uses later Android's PhotoPicker API which led to an improvement in the RN app zulip/zulip-mobile@e4d162c.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Apr 19, 2023

Hmm no, I don't think this is the fancy new PhotoPicker API (which we captured in screen recordings of the RN app here):

image

And if I search for "PhotoPicker" in the file_picker GitHub repo I get no results: https://github.com/search?q=repo%3Amiguelpruivo%2Fflutter_file_picker%20PhotoPicker&type=code

So yeah, maybe we'll want to do something different for Android. Hmm.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 19, 2023
So far we've used this code for the generic upload-file UI (zulip#57).
Soon we'll use it for media-library uploads (zulip#56) and camera uploads
(zulip#61).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 19, 2023
@gnprice
Copy link
Member Author

gnprice commented Apr 19, 2023

Hmm no, I don't think this is the fancy new PhotoPicker API

Dang, yeah.

That PhotoPicker API is available through image_picker:
https://pub.dev/packages/image_picker_android#photo-picker

So that'd be an option. But pending flutter/flutter#89159, it'd lack videos unless we had two separate buttons.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 22, 2023
So far we've used this code for the generic upload-file UI (zulip#57).
Soon we'll use it for media-library uploads (zulip#56) and camera uploads
(zulip#61).
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 22, 2023
And pull out a helper from _AttachFileButton, since it also uses
`file_picker` and it's convenient to reuse that code here.

Fixes: zulip#56
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 28, 2023
So far we've used this code for the generic upload-file UI (zulip#57).
Soon we'll use it for media-library uploads (zulip#56) and camera uploads
(zulip#61).
@gnprice gnprice added the a-compose Compose box, autocomplete, attaching files/images label May 27, 2023
@gnprice gnprice moved this to Done in Flutter app Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose Compose box, autocomplete, attaching files/images
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants