compose: Don't send immediately on adding image #5474

chrisbobbe · 2022-08-23T21:54:32Z

Thanks @AkashDhiman for your PR #5125 and your prep work for this in #4590; those were quite helpful and informative. 🙂

Marked as a draft/blocked for a reason I'll explain, but that reason doesn't affect the first three commits—

ComposeMenu [nfc]: Use our own new type definition for attachments
ComposeMenu [nfc]: Fix name of insertAttachment prop to be plural
ComposeMenu [nfc]: Convert to a function component

—and indeed those three would be good to land as prep for removing the blocker.

The blocker is that we have basically the behavior of #5291, except with the insertion of image links instead of Jitsi links. On iOS, you take or select a photo to upload, and sometimes the message text input is unchanged when you expect it to get an inserted [Uploading …]() link.

Fixes: #2366
Fixes: #4540
Replaces: #5125

gnprice · 2022-08-25T17:53:56Z

Chat thread about the resulting UI on Android:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Android.20select.20multiple.20photos/near/1423109

gnprice

Thanks @chrisbobbe! This code all looks good except the small points below.

That leaves as the interesting questions:

The expansion of iOS: Add-video-call button sometimes doesn't insert link #5291. I think the plan of merging the ComposeMenu refactors from the start of this branch, then going for Make "+" open an action sheet instead of row of buttons #5141, makes sense.
The UI that results on Android. One hopes to continue getting a nice photo-gallery-style UI, rather than a file-oriented UI.

ISTR we found that newer Android versions, including what one can get in the emulator, do better here and the file-oriented UI might be limited to older versions like in the physical test device you have.

gnprice · 2022-08-31T20:26:07Z

src/compose/ComposeMenu.js

-class ComposeMenuInner extends PureComponent<Props> {
-  static contextType = TranslationContext;
-  context: GetText;
+export default function ComposeMenuInner(props: Props): Node {


nit: There's no longer a distinct "inner" vs "outer" component here, right? So this function should be called just ComposeMenu.

(Another angle on the same thing: it's the default export, so should have the same name as the file.)

gnprice · 2022-08-31T20:37:06Z

src/compose/ComposeMenu.js

+          msg = _('Failed to attach your file.');
+          return;


This sets a local and then returns -- so the assignment has no effect, as nothing else can see it.

Perhaps just put the showErrorAlert call here? It's pretty concise, not much more complex than the variable assignment.

chrisbobbe · 2022-08-31T21:30:54Z

Thanks for the review! Revision pushed.

chrisbobbe · 2023-01-11T19:58:32Z

@gnprice, I've just rebased this atop #5635, so it's ready for another code review. I'm working on spinning up the simulator to make a screen recording now. 🙂

chrisbobbe · 2023-01-11T21:24:28Z

Just pushed a revision fixing a rebase error in the new type Attachment's definition (r-n-document-picker changed an optional string property to required with string | null).

Here's a video of the feature working on my iPhone:

RPReplay_Final1673470049.mp4

gnprice

Thanks for the revision!

Code all looks good; small comment below.

On the UI this gets us on Android: I'm still not a fan of that file-picker-style UI when the user asks to upload a photo. The first part of this branch, the "don't send immediately", is a major functionality improvement. But "select multiple images at once" doesn't seem as necessary:

Lots of messaging apps don't even let you send multiple images in one message.
If you do want to send multiple images, then given the "don't send immediately" part of this branch, all you have to do is go through the selection multiple times.

So I'm not sure that that part is a good trade for the added inconvenience of the file-picker flow each time you want to send one photo.

Therefore I think what I want to do here is:

Let's merge the first part of this branch: the "don't send immediately", plus the prep commits for "select multiple images", so in total all but the last commit.
- With that one code adjustment mentioned below.
Then for [Android] Multiple file selection when uploading pictures. #2366… hmm, looking at it, both of the actual user requests there (the [Android] Multiple file selection when uploading pictures. #2366 OP, plus the report at https://chat.zulip.org/#narrow/stream/48-mobile/topic/Bug.20reports/near/1292530 ) are about sending multiple images at once, not selecting from the picker all at once. Moreover, both of them are really mostly about "don't send immediately, let me add some text", i.e. Don't immediately send attachment on upload #4540.
So I think it would actually be appropriate to have the "Don't immediately send" commit close [Android] Multiple file selection when uploading pictures. #2366 as well as Don't immediately send attachment on upload #4540. Then we can have a fresh issue thread for anything further we want to do here.
And if the code you've already written in that last commit gives a UX improvement on iOS, then we can follow up with a version of that commit that's tweaked to be conditional on the platform. No need to spend further effort on the Android side — I think we've already determined there's no easy solution there, and the "select multiple images at a time" functionality doesn't seem important enough to justify a bigger effort.

gnprice · 2023-01-12T19:28:49Z

src/compose/ComposeMenu.js

        });
        return;
      }

-      dispatch(uploadFile(destinationNarrow, uri, chooseUploadImageFilename(uri, fileName)));


Looks like this was the only call site of that uploadFile function — so let's cut that out too.

This function is designed to insert multiple attachments.

Soon, we'd like to also use the `insertAttachments` prop not just for react-native-document-picker payloads, but also for react-native-image-picker payloads. Better to define our own type that's meant to accommodate both kinds, rather than, e.g., using react-native-document-picker's type and fudging image-picker payloads to flow through that type. While we're at it, trim down the type to just what `insertAttachments` currently uses: the url [1] and a nullable name for the file. The `type` property in particular smells like something that might not be easy to make uniform across document-picker and image-picker payloads; there's no comment linking to a spec that says what the possible values of `type` mean, for example. [1] renamed from "uri"; see style guide

For why this closes zulip#2366, see Greg's explanation: zulip#5474 (review) Fixes: zulip#4540 Fixes: zulip#2366 Co-authored-by: Akash Dhiman <[email protected]>

…ehavior A skim through Sentry events matching the old error message suggests that we haven't yet had reports of firstAsset being present but with nullish `url` or `fileName`. But the types say they can be undefined, and we'll want to know if that's ever the case.

chrisbobbe · 2023-01-12T21:24:58Z

Thanks for the review! Revision pushed.

gnprice · 2023-01-12T22:09:57Z

Thanks! Looks good; merging.

We recently completed the feature of not immediately sending messages with image uploads, in zulip#5474. With that, it became possible to send a message with multiple uploaded images in it. But you had to go through the image-selection flow once for each of your chosen images; see discussion: zulip#5474 (review) This gives a smoother experience by letting you choose multiple images in one image-selection session. Related: zulip#2366 Co-authored-by: Akash Dhiman <[email protected]>

chrisbobbe added P1 high-priority blocked on other work To come back to after another related PR, or some other task. labels Aug 23, 2022

chrisbobbe requested a review from gnprice August 23, 2022 21:54

chrisbobbe assigned chrisbobbe and unassigned chrisbobbe Aug 23, 2022

chrisbobbe marked this pull request as draft August 25, 2022 17:49

chrisbobbe mentioned this pull request Aug 25, 2022

iOS: Add-video-call button sometimes doesn't insert link #5291

Closed

gnprice reviewed Aug 31, 2022

View reviewed changes

chrisbobbe force-pushed the pr-send-multiple-images branch from 4990e13 to a69b623 Compare August 31, 2022 21:30

chrisbobbe mentioned this pull request Jan 6, 2023

ComposeBox: Move compose buttons into a row below message-text input #5635

Merged

chrisbobbe force-pushed the pr-send-multiple-images branch from a69b623 to d07845f Compare January 11, 2023 19:57

chrisbobbe removed the blocked on other work To come back to after another related PR, or some other task. label Jan 11, 2023

chrisbobbe marked this pull request as ready for review January 11, 2023 19:57

chrisbobbe force-pushed the pr-send-multiple-images branch from d07845f to 9f61c33 Compare January 11, 2023 21:22

gnprice reviewed Jan 12, 2023

View reviewed changes

chrisbobbe and others added 5 commits January 12, 2023 12:03

ComposeMenu [nfc]: Fix name of insertAttachment prop to be plural

3b1109d

This function is designed to insert multiple attachments.

compose: Don't immediately send selected image

d74f46a

For why this closes zulip#2366, see Greg's explanation: zulip#5474 (review) Fixes: zulip#4540 Fixes: zulip#2366 Co-authored-by: Akash Dhiman <[email protected]>

ComposeMenu [nfc]: Bring out assets const

d815680

chrisbobbe force-pushed the pr-send-multiple-images branch from 9f61c33 to d815680 Compare January 12, 2023 21:24

gnprice changed the title ~~compose: Allow multiple image selection; don't send immediately~~ compose: Don't send immediately on adding image Jan 12, 2023

gnprice merged commit d815680 into zulip:main Jan 12, 2023

chrisbobbe deleted the pr-send-multiple-images branch January 12, 2023 22:11

chrisbobbe mentioned this pull request Jan 12, 2023

compose: Allow image multi-selection, where supported #5638

Merged

This was referenced Jan 12, 2023

Don't immediately send & send multiple images. #5125

Closed

Attached picture is posted before caption is typed #3299

Closed

Improve Image Upload Workflow #2887

Closed

gnprice mentioned this pull request Apr 7, 2023

Add images from compose box zulip/zulip-flutter#56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Don't send immediately on adding image #5474

compose: Don't send immediately on adding image #5474

chrisbobbe commented Aug 23, 2022 •

edited

Loading

gnprice commented Aug 25, 2022

gnprice left a comment

gnprice Aug 31, 2022

gnprice Aug 31, 2022

chrisbobbe commented Aug 31, 2022

chrisbobbe commented Jan 11, 2023

chrisbobbe commented Jan 11, 2023

gnprice left a comment

gnprice Jan 12, 2023

chrisbobbe commented Jan 12, 2023

gnprice commented Jan 12, 2023

compose: Don't send immediately on adding image #5474

compose: Don't send immediately on adding image #5474

Conversation

chrisbobbe commented Aug 23, 2022 • edited Loading

gnprice commented Aug 25, 2022

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 31, 2022

Choose a reason for hiding this comment

gnprice Aug 31, 2022

Choose a reason for hiding this comment

chrisbobbe commented Aug 31, 2022

chrisbobbe commented Jan 11, 2023

chrisbobbe commented Jan 11, 2023

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jan 12, 2023

Choose a reason for hiding this comment

chrisbobbe commented Jan 12, 2023

gnprice commented Jan 12, 2023

chrisbobbe commented Aug 23, 2022 •

edited

Loading