Improve Image Upload Workflow

[armaanahluwalia]

This PR does a few different things in relation to improving the user experience for uploading image attachments along with messages:

1. Enables seeing preview thumbnails of image attachments while composing a message.
2. Enables sending a message along with an image attachment.
3. Enables sending multiple image attachments in the same message.
4. Enables setting a topic on a message with only an image
5. Enables downsampling of image quality before uploading an image attachment to save on data usage.
6. Enables downscaling of images before uploading to save on data usage. See Downsample images before uploading them #2749

This is done by the following changes -

1. Adding a new category of state called draftImages ( analogous with drafts ).
2. Adding image thumbnails to ComposeBox
3. Changing the library used for uploading and selecting images from react-native-image-picker to react-native-image-crop-picker

To Do:

• Write tests for DraftImages
• Make sure dependency has been added correctly and remove old dependency
• Copy changes from ComposeBox.ios.js to ComposeBox.android.js
• Changing markdown parsing on zulip/zulip to add space between multiple images

Screenshots:

Adding Single Image

Adding Multiple Images - Delete one, add one, hit max limit

Network Failure during upload. Recovery from failure.

[gnprice]

Awesome! Looking forward to getting this in. This is a fairly complex feature to build -- and that interaction in those screencaps looks pretty reasonable and this code and the commits look quite clean, so this is an excellent start.

• CI is failing with a type error, so you'll want to fix that.
• Also, be sure to format your code with our prettier+eslint; that helps avoid formatting-related noise in diffs. You can fix up this branch by running yarn prettier on each commit.
  • I highly recommend setting up VS Code with our recommended extensions, notably prettier-vscode -- then it'll automatically get formatted whenever you save a file, which makes it quite seamless.
• This doesn't seem to build for me on Android. I get this error message:

$ yarn
[...]
Done in 4.91s.

$ time react-native run-android
[...]

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':app'.
> Could not resolve all dependencies for configuration ':app:_debugApk'.
   > A problem occurred configuring project ':react-native-image-crop-picker'.
      > Could not resolve all dependencies for configuration ':react-native-image-crop-picker:_debugPublishCopy'.
         > Could not find com.github.yalantis:ucrop:2.2.2-native.
           Required by:
               project :react-native-image-crop-picker

Sounds like the library isn't properly declaring one of its dependencies, perhaps. If you try an Android build, do you reproduce the error?

• A couple of exceptions to these commits being clean :-) :

  • The "Change implementation of ImagePicker" does several things, and I'm not sure they correspond entirely to the commit message. The multiple images feature comes in the next commit, right? Definitely good to separate those, but the commit message should reflect that.
  • Similarly, if it's possible to separate the change of which library we use from the change of how we upload the images, into two separate commits, that would be very helpful.
  • See https://zulip.readthedocs.io/en/latest/contributing/version-control.html for our guide to writing clear and coherent commits. This makes a big difference for people's ability to read and understand the code -- starting with the reviewer, like me. 😉

• This makes handleSend return immediately with a Promise, having started the upload; and the outbox message gets added only after the sequence of uploads is complete. What happens in the UI while that's going on?

  • Previously, we created the outbox message immediately, then started the uploads.

• Let's put an upper bound on the number of images; I don't think the UI will scale well past about two rows (so, four images? or is it more complicated than that?)

[gnprice]

Also, sorry this code is duplicated :-/ . That's a hack we did in #2595 , to work around two different React Native bugs at the same time (one that affected Android, one iOS, and triggered in opposite circumstances.)

With #2886 we'll merge them into one file again soon. The new one will be based mainly on the current .android.js version; shouldn't be a big deal, but heads up that the rebase might be a little annoying when it happens.

[armaanahluwalia]

@gnprice Thanks for the feedback! And for making me aware of those two PRs. I'll be working on updating this PR after tomorrow so will pull in master before I do ( in trying to avoid too many future conflicts ).

Looking forward to getting this in too!

[armaanahluwalia]

Hey @gnprice -- I've updated this PR with tests, lint changes and all the feedback you had.
Also added a bit of extra care to network failure situations.
Let me know if this looks mergeable now.

[gnprice]

Thanks @armaanahluwalia ! Re-reviewing this now; apologies for the delay.

First, a few comments from playing with this version of the app. These are all from an emulated Pixel 2 XL running Android 8.1.0 Oreo.

• It builds! 😄
• When I hit the "plus" icon on the left of the compose box to get to the compose menu, it takes a long time -- the icon pretty promptly changes to a left-chevron, but then it sits that way for like 300-600ms before the other icons show up. This capture doesn't seem to quite match the timing I see live, but it gives the idea:
  
  • Ditto when I hit the left-chevron to collapse the compose menu back.
• When I hit the "picture" icon to select an image to upload, I get this screen:
  
  Seems like it's looking for a "file" generically. If I poke around in that UI: hamburger icon -> left drawer -> "Photos" -> "Device folders > Camera", I get to a screen where I actually see some photos and can select them.

Do you have an Android development environment set up? It looks like this is going to be a change where you'll need to play with it on both platforms in order to get to something really good. Setting one up is fortunately very straightforward -- there are a fair number of steps involved, but there are instructions and they should work reliably. See our guide and the upstream instructions it links to.

Separately, some comments from reading the code.

• There's a lot of stuff happening in the first commit. Let's make it easier to understand (=> easier for me to merge!) by breaking it up.
  • A lot of it is about adding the draftImages subtree to the Redux state, and the associated actions / reducers / selectors. That can be a commit on its own, not yet touching ComposeBox or ComposeMenu. It's OK that the code won't yet actually get used; the commit message can just mention that we plan to use it soon.
  • Then commits changing those components. I'm not yet 100% sure what all these changes are doing; I suspect there are some pieces that can be separated out to make it clearer.
  • This looks potentially expensive: <Image style={styles.composeImage} source={{ uri: 'data:' + draftImages[id].content }} /> Is that... the entire image contents, being manipulated as a string? Can we avoid doing that?
  • Ditto this, I guess: 'image/png;base64,' + response.data
• In the second commit, switching libraries:
  • There are some miscellaneous changes here that should definitely be explained in the commit message, and probably should be their own commits, or maybe we don't need some of them at all: compileSdkVersion, vectorDrawables.useSupportLibrary, two maven repos, IDEWorkspaceChecks.plist.
  • We use yarn, not npm; skip the package-lock.json.
  • The project.pbxproj file unfortunately doesn't make very self-explanatory diffs. So it's important that when making a change to it, you explain in the commit message what you did (just ran a certain react-native link command? did something in the Xcode UI?)
  • Something's off in the whitespace in MainApplication.java.
• The third commit, "Change implementation":
  • The second commit doesn't actually work without this one, does it?
  • There's a lot going on in this third commit. As with the first, this makes it hard to tell which changes are related to which, and what each change is intended to do.
  • What's the absolute minimum that you can change in ComposeBox and friends in order to make things work with the new library? Let's break that out as its own commit. (Ultimately we'll squash it into the "Change dependency" commit so no commit breaks things, but that's easy to do at the end.) It's OK if the UI doesn't look great at that step -- just something that works at all would be a big help for understanding what's going on here.

[gnprice]

@armaanahluwalia Is this meant to be ready for a re-review?

It looks like you've pushed a new version of the branch. The way GitHub works, it doesn't send notifications when you do that, so a reviewer won't necessarily ever notice -- so the recommended workflow is that when responding to a review, you always make a comment when the new version is ready.

(This one, I noticed because the PR is one I'm eager to get merged 😄 , and have on our priority list at https://github.com/zulip/zulip-mobile/projects/7 .)

[armaanahluwalia]

@gnprice I'm sorry I wanted to be sure it passed the tests before I notified you. I addressed all the changes you requested except for the "slowness" while clicking the plus icon. That one I'm having a hard time figuring out but will try to debug further. The rest is ready to be reviewed.

Edit: The other thing to note is that this depends on compileSdkVersion 26 so it would follow the RN 56 upgrade i guess.

[gnprice]

Great! Nothing to apologize for 😄

Happy to re-review the other aspects, with your warning that that slowness issue is still there. I'll take a look now.

On the slowness issue: have you been able to reproduce it and you're still debugging, or have you not been able to reproduce?

[gnprice]

• Playing with this UI, this part of the interaction is still the way I described above:

  When I hit the "picture" icon to select an image to upload, I get this screen:

  Have you been able to get an Android development environment set up?

• Thanks for the reply in the commit message about these bits!

  compileSdkVersion, vectorDrawables.useSupportLibrary, two maven repos

  If they're required, it's fine to make those changes. What I'd like to be different is to have them in their own separate commits, coming before the commit that switches to the new library. That helps the reader (i.e. me now, and others reading this in the future) notice they're being added, and see them without being mixed in with all these other changes. It also gives you a good place -- the new commits' commit messages -- to explain what those changes are doing and why.

  In particular: I have no idea what the vectorDrawables.useSupportLibrary setting does. And what are we downloading from each of those two Maven repositories?

• Also still curious about this question:

  The project.pbxproj file unfortunately doesn't make very self-explanatory diffs. So it's important that when making a change to it, you explain in the commit message what you did (just ran a certain react-native link command? did something in the Xcode UI?)

• In the new "Change ImagePicker Library Implementation" commit:

  • Thanks for breaking this change out! I appreciate how much smaller it is than the previous "change implementation" commit. 😄
  • Is it the case that after just these two commits, before any of the changes in later commits, everything works correctly again? That's what we're aiming for. (As I mentioned above: Ultimately we'll actually squash it into the "Change dependency" commit so no commit breaks things, but that's easy to do at the end.)
  • There's still a couple of different things happening here. A further change that would help is to take the changes to chooseUploadImageFilename and break those out as their own commit at the beginning of the PR.
  • In chooseUploadImageFilename:
    • The type signature should be updated to say fileName: ?string, if you're expecting undefined or null as arguments.
    • The existing code has some code handling HEIC files, and a long comment about that code right before it. You're adding new code that isn't about HEIC files -- so it shouldn't go between that comment and that code. That's just confusing. 😛
  • It looks like there's a fair amount of logic duplicated now between handleImageUpload and handleCameraCapture. Let's avoid duplicating all that. Instead, write all that logic once, in its own method, and then those two methods can be very short and just invoke that one.
  • You don't have to keep the structure with the handleImagePickerResponse callback.
    • I think both this change, and the state of the code after it, would be simpler if you just handled the success and error cases separately, rather than making up a "response" to pass to the old callback that was written for the old library.
    • Also, try using async / await -- I think that will be simpler than the .then / .catch.

(cont'd)

[gnprice]

On the draftImages commit:

• Thanks for breaking this out! This is very helpful for reading and understanding it.
• It's not obvious to me what all these five different actions are supposed to mean -- so this is an example of a time where it'd be super helpful to write that down, in jsdoc form.
  • See src/types.js for a lot of examples of jsdoc comments -- they're the ones starting with /**.
  • Here, let's put them on the types in src/actionTypes.js.
• The action types (and creators) say "images"; should be "image" to match the constants.
• Actually even more important than documenting the action types (though that's still important too): I'd very much want to see documentation on the DraftImagesState type.
  • Starting with giving its values a clearer type than Object -- that's pretty vacuous. There's clearly a bunch of fields here.
  • And then as a followup question: what are each of those fields intended to mean? What do these three states loading, loaded, error mean?
  • I have a feeling that e.g. "loaded" here is intended to mean "uploaded to the server"? If so, let's use the verb "upload" instead of "load" -- the meaning to me is quite different.

OK, I'll leave it there for now. Thanks again -- looking forward to the next version!

[armaanahluwalia]

@gnprice thanks for those comments I’ll follow up with those changes.

I did manage to get an emulator running with the same configuration as you ( barely with my machine RAM ) and also tested on a physical android device.

Regarding the screen when you click the picture icon —
That’s something we don’t have control over. It starts out as that screen but the next time you select an image it goes to whatever folder you picked from last so it’s really only the first time you need to find the right folder. It also depends on android version.

[gnprice]

I did manage to get an emulator running with the same configuration as you ( barely with my machine RAM ) and also tested on a physical android device.

OK, good to hear. Were you able to reproduce the slowness issue and you're still debugging, or have you not been able to reproduce it?

If you have a physical Android device and you're at all short of RAM on your laptop/desktop, I recommend using the device. Things are more fun as well as more efficient when your computers respond quickly 😉

(When using my laptop, I prefer to do development on my physical device in order to save screen space, as well as RAM and also CPU and GPU load. The emulator is convenient on my desktop because I have a large monitor, a good GPU, and lots of RAM.)

It starts out as that screen but the next time you select an image it goes to whatever folder you picked from last so it’s really only the first time you need to find the right folder.

Hrm. That behavior would be suboptimal but OK. But that's not what I'm seeing (on Android 8.1 in my simulated Pixel 2 XL) -- I hit the picture icon, get that screen, go through the flow to choose an image, send a message with it... and when I then hit the picture icon again while composing another message, I get the same "Recent -> No items" screen again.

In what environment are you seeing the behavior you're seeing?

[armaanahluwalia]

@gnprice This PR is ready for review.

I addressed all the points in your feedback ( I hope ).

As for the following 2 points:

1. Folder for selecting an Image:
   I ran and tested this on an emulator with the same configuration you mentioned. The behavior is consistent (mostly) with what I described earlier. If you pick an image from a specific folder, the next time you pick an image, that folder will be selected. Here when I say folder, I think this may be where the inconsistency arises between different android versions. On my newer device, even if I select an image from top-level folders, it seems to go back to that ( ex: Downloads ). Whereas on the Pixel 2, it goes back to Recents ( which seems quite logical as a default ), unless I specifically pick from an album within the Camera folder.

2. Slowness while clicking the plus icon:
   This is a bit strange. I actually saw this behavior myself on my physical android device a few days ago after your initial comments. The funny thing is I can't reproduce it now on both physical device and an emulated Pixel 2. My feeling is some code change between first review and now has helped fix the issue ( maybe the change where we were causing re-renders too often ). If you see it and are able to reproduce let me know I'll take another look.

Side Note:
You mentioned you wanted more description on the changes to project.pbxproj. I'm not really sure what kind of description you were looking for and I fear I may not know enough to provide a better description. Those are changes that come when you link and unlink a library as described in the commit. Perhaps you could have a readthrough and see if you can be a bit more descriptive? Would love to know.

[gnprice]

Thanks @armaanahluwalia ! This is very helpful.

• more description on the changes to project.pbxproj

  Yep, what I want is just the story of what you did that caused those changes. (For most files, the answer is like "I typed this code in my editor"... this is an exception where it's not so obvious.) So mentioning react-native link and react-native unlink is quite helpful.

  You also refer to upstream's README for install instructions. That has a few more steps; did you do those as well, or were those not needed?

• Speaking of which, in the commit before that you refer to their "Production build" instructions. Taking a look here... I am not really sure exactly what these instructions mean or how to go about them. Do you understand them / can you try adding a commit that does those?

• Nit, but one thing you can do to simplify reading the diff that changes which library we use is to make diffs like this one:

                     new MainReactPackage(),
+                    new PickerPackage(),
                     new RNTextInputResetPackage(),
-                    new ImagePickerPackage(),
                     new OrientationPackage(),

put the new thing on the same line instead of a different one.

• I like the much simplified handleImageRequest code!

  • Can the "cancelled" case be moved to tightly enclose just the ImagePicker.... call? I think that'd be a bit cleaner.

  • I'm not sure whether we need the other, showErrorAlert case. In fact... if the upload fails, does that even throw an exception here? I think it doesn't -- I think dispatch will have promptly returned, and so will we.

  • Hmm, in fact, even if we did want to change something about the error-handling when upload fails: that should be a separate commit. 😄 We'll keep this one tightly focused, the better to understand it.

• Draft images: Thanks for these changes! The explanations are helpful.

  • I think the three booleans will be clearer as one enum-style property, like uploadStatus: 'uploading' | 'uploaded' | 'error'.

---

I have to head off shortly, but a few thoughts on the later commits:

• When handleImageRequest gets longer, my desire to keep the try / catch tightly focused gets stronger. :-) If there are several things that could legitimately throw an error, I'd prefer to handle each of them separately so we can make explicit choices about them.

• This can be just one line:

+    const { topic } = this.state;
+    const { lastMessageTopic } = this.props;
+    const newTopic = topic || lastMessageTopic;

as const newTopic = this.state.topic || this.props.lastMessageTopic.

• This looks like tricky code:

+      const uriPromise = new Promise(async (resolve, reject) => {
+        try {
+          const remoteUri = await uploadFile(auth, imageObj.uri, imageObj.fileName);
+          resolve(`[${imageObj.fileName}](${remoteUri})`);
+          dispatch(draftImageRemove(id));
+        } catch (e) {
+          reject(e);
+        }
+      });

Why do we resolve the one promise, and then go on to take another step?

Also, is that try / catch -> reject doing something? I'd have to puzzle through it carefully to be sure, but I think it may behave the same as if it weren't there (and the callback was just the body of that try.)

• A product/UX question: it looks like this starts uploading all the images when you're ready to send the message. What if we started uploading each image eagerly, as soon as you select it?

• Between uploadAllDrafts and its call site in handleSend, there's some very tight intermingling of (a) code that says how to write Markdown about the images; (b) code that handles tricky management of asynchronous I/O activity. I'd much prefer to arrange for those kinds of code to be separate.

• In handleSend, why mutate the local variable message? Seems confusing.

• Folder for selecting an Image

  Hmm, interesting. Thanks for nailing down that pattern!

  I guess I'll have to try this flow out on my real device, with lots of actual photos, and see how natural it feels there. Wouldn't be surprising if it makes sense there even though it feels wrong in the artificial environment I have in the emulator.

[armaanahluwalia]

@gnprice I've updated this PR with the changes you requested. Here are some notes on the update in response to some of your comments and queries:

Dependency Change: I added some more detail to the commit and re-did the commit from scratch just to make sure I wasn't doing anything unnecessary. The one caveat is that I can't actually test this properly since the "Archive" action is disabled for me on XCode so I can't be sure how this will build for production code. I think this is because I'm not technically on the "team" under apple developers or whatever. Perhaps you could verify this by doing the archive step?

Draft Images: I renamed the property in reducer to being one key as you suggested. I do think this makes sense. I kept them as separate actions because I feel thats kind of a best practice which I completely agree with. Makes for better logging, signatures and general readability. This way we don't need to click into a log object each time we want to see the details of an action.

Try Catches: I refactored a bunch of code and the remaining try-catches in there actually do have specific functions. One to verify a corrupt image is not being selected. Another to verify a file is uploaded correctly etc.

Slowness of the Plus Icon: This one I'm puzzled by. I was actually seeing this while developing for a while. I switched out to the tip of master and it appeared there as well so its not a bug specific to this PR. The problem is I can't reliably reproduce this on either branch now. I do know that between the time the plus icon is clicked and the menu actually animates there are no actions fired. This has to do with some kind of slowness elsewhere in the code that shares the same thread as the animation.

[armaanahluwalia]

@gnprice I've updated this branch with code changes. I believe merging in #3118 first makes sense since after thats done I can incorporate that new withPreview action into the code. Either ways take a look.

Couple of things I haven't been able to tackle:

1. Changing the file manager: This is something that comes out of the box with the plugin and there's no real way to change what folder it looks for. Most newer androids will have a pretty sane flow I believe and most likely your images will be recent anyway.

2. Camera on android 8.1 emulator: For some reason the internet on my android emulator will absolutely not work. I tried debugging it for a good hour with no luck so I gave up. Not sure what else to do.

3. Production build: I'm not authorized to archive this bundle so it would have to be someone on the Kandra Labs developer team.

Let me know if you see anything else.

[armaanahluwalia]

Build is failing due to a possibly unrelated cause: react-native-vector-icons ?

[armaanahluwalia]

FYI @gnprice I believe my other changes for uploading images with a loading spinner, waiting till its uploaded and then sending should also be incorporated ( I left them out at your request here because you said you'd like to merge e stripped down version ). With all the issues i've been facing for image uploads recently, I really believe that flow is a much more fool proof way to go. It also kind of trains the user to wait for images to upload before -- say for example, closing the app, going to another stream, taking other actions. These are all flows that are from what I can tell untested and a cause for bugs in the code you have currently. I'm not saying this is a dealbreaker issue but thats my view on things.

Maybe I'm also just a little frustrated at how many issues I'm facing with uploading pictures in the app right now. Let me know thoughts.

[armaanahluwalia]

I'm sure @borisyankov has thoughts on this since you worked on #3118

[armaanahluwalia]

I also think we might be underestimating how many people are actually on imperfect networks. The code I had handled a bunch of cases around network failure which, barring some other future PRs working on retries, network failure, queueing etc. has not really been taken into account.

On a side note: I'd really love to get this merged in for the upcoming release whenever that is.

[armaanahluwalia]

@gnprice Just wanted to ping and see if there's any plan to integrate this? If yes, I'll fix up conflicts.

[borisyankov]

@armaanahluwalia Just wanted to reassure you that we consider the problem this PR addresses to be important and that we want to merge this work soon :)

I think we got to the position where a lot of work is tied together and lives and dies together.
Would be better to split it because some of it looks ready, some of it looks like it needs some more work or rethinking of the solution itself.

[borisyankov]

Some comments:

Enables seeing preview thumbnails of image attachments while composing a message.

That is certainly needed!

Enables sending a message along with an image attachment.

That I think both me and Greg think is not necessarily important.
It becomes tricky to implement correctly, is possibly not that valued by the user and most other apps don't offer that, possibly because of these reasons.

Enables sending multiple image attachments in the same message.

That is good. I am not sure we initially were decided on how important that is, but with future improvements of the retry logic of image uploads (I could work on this soon) this will be even more viable. I know I do send multiple images sometimes and appreciate it being made easier.

Enables setting a topic on a message with only an image

That is important, but maybe our last commits addressed that at least partially (it was a bug that the wrong topic got set).

Enables downsampling of image quality before uploading an image attachment to save on data usage.
Enables downscaling of images before uploading to save on data usage. See #2749

Very important, though this can easily be added even with the current image picker. But good to have it done!

[borisyankov]

Just to clarify: most of my comments include my thoughts and what I remember Greg's opinions were when we did review the work together last. I had some time to think after that on the way we approach this.

I think it is very valuable to upload the images in two steps:

• select images
• confirm send

We miss step 2 right now and your work is addressing that.
But I am not too keep on the image drafts approach.

We can implement this without any drafts, as mixing images and text is not going to work well anyway.

[armaanahluwalia]

@borisyankov Thanks for those comments. It certainly is reassuring to hear the majority of this work won't go fruitless.

I'm just a little confused as to what next steps should be now. I also don't fully understand why you would want to treat images as a special case. Since we're creating drafts out of text -- I'm not exactly sure why we wouldn't do the same with image uploads. I feel like a user would expect his selections to be saved just like they are for text.

I also disagree that sending images and text together is not important. I think its that kind of functionality that really sets a decent app apart from a really good app. I do understand though if thats something you want to implement as a v2. To be clear, I think having drafts for images makes a lot of sense, both from a user experience perspective as well as since most ( if not all ) of the work for it has already been done : )

Would love to get some direction on next steps.

[borisyankov]

Images are treated as a special case in your code. In fact, it is not necessary to treat them as such, if you include the link in the message body as markdown. This will completely skip the need for a separate reducer.

Also, I think this is not adding too much control or a difference between what would happen if you send the images and the text separately as you can't position the messages.

[armaanahluwalia]

@borisyankov I'm not sure what you mean. Having images saved as drafts has the following advantages -

1. It allows you to save the local url of the image ( before upload ) so if you leave the chat and come back your selected images are still selected as drafts just like text is
2. It allows you to save upload state for the selected images in the case that the upload failed, was cancelled, connectivity dropped, or it was uploaded successfully.
3. This code has already been fleshed out, tested and is working.
4. Allows for being able to order images by dragging them in the future ( by potentially changing the structure of the image draft reducer )
   I'm not sure I see very good reason to change the implementation at this point. ( I could understand if this discussion was at the beginning of the PR). In any case this approach is superior for all the reasons mentioned above.

[borisyankov]

I wanted to make clear that different parts of this PR are at different stages of being ready and since this is implementing several related but different features it will help if we merge them independently.

While I do not have comments or objections about many of the parts, I do about others, and this drags the whole PR (I am sure Greg has similar or different ones, this is a big PR)

For example, we could use the drafts we already do like this:

![](image!.png)
![](image2.png)

And do not need to create another reducer separately.

After testing the current branch:

• it seems I can not add more than one image?
• once an image is added I can not add more, since the buttons are hidden
• the draft text seems to not be restored (a regression or something weird?)

I didn't go too deep in how the code would handle retries of draft images that did not send etc. but at this point we are stretching the feature set that is reasonable to merge at once.

[armaanahluwalia]

@borisyankov Yep the code was simplified on request from greg. I'd like to get @gnprice comments on what to do to move this forward. I feel like its been sitting around long enough : )

[gnprice]

Thanks @armaanahluwalia for all your work on this, and for the latest revisions!

I think my main high-level piece of feedback on this PR is: you need to set up for yourself a working Android development environment, so you can try the UI out on Android. This is a very UX-oriented PR, and it's one that (inevitably) has a lot of platform-specific aspects. It's not possible to adequately develop it without trying it on both platforms.

I remember this was a major source of problems in the early history of this PR. Trying the branch out just now in Android O on an emulated Pixel 2, it does basically work (:tada:), but there are still some UX bugs that jump out at me. I could describe them in detail, which is the tack we took early in this PR... but really it is less work in total -- and I think you will find it a much shorter and less frustrating iteration cycle! -- if you see them firsthand by playing with it yourself.

In fact, I see now that I made this point back in September:

Do you have an Android development environment set up? It looks like this is going to be a change where you'll need to play with it on both platforms in order to get to something really good. Setting one up is fortunately very straightforward -- there are a fair number of steps involved, but there are instructions and they should work reliably. See our guide and the upstream instructions it links to.

Having a working Android development environment will be helpful for you again and again beyond this PR too.

---

You mention that the emulator isn't working for you:

For some reason the internet on my android emulator will absolutely not work. I tried debugging it for a good hour with no luck so I gave up. Not sure what else to do.

This sounds like a Zulip-independent problem, so there should be useful advice for it on the Web.

The first thing I'd suggest: wipe that emulated device (that "AVD"), and make a fresh one. Maybe make a fresh one from a different system image -- a different OS version, for example.

If you're continuing to have trouble and aren't finding useful advice on the Web, let's debug in chat.

[gnprice]

Some more specific thoughts:

Most newer androids will have a pretty sane flow I believe and most likely your images will be recent anyway.

Just to be clear: I described above my experience on my personal device, running Android 9. That is as new as it gets. 😛 The flow was not a good one.

But as I said here, I think it's not clearly worse than the existing flow, so I'll be OK with merging even with that issue.

I believe my other changes for uploading images with a loading spinner, waiting till its uploaded and then sending should also be incorporated ( I left them out at your request here because you said you'd like to merge e stripped down version ). With all the issues i've been facing for image uploads recently, I really believe that flow is a much more fool proof way to go. It also kind of trains the user to wait for images to upload before -- say for example, closing the app, going to another stream, taking other actions.

This is what #3118 also does, right? Or is there a subtler distinction you have in mind?

I agree that that functionality will be important.

---

I also agree generally with @borisyankov 's replies. I'm not sure about the question of whether we can avoid the new concept of "draft images"; that is interesting and I'll want to think about it more.

One way of looking at what "draft images" are doing would be: they're tracking image uploads that we're currently doing, or have attempted and need to retry, etc. I think that's the piece that isn't covered in this example:

![](image!.png)
![](image2.png)

From that perspective, in order to do nice retrying of image uploads, and also to show in the UI when an image is uploaded that you're planning to include in a draft message, we need some kind of tracking like this. But perhaps "draft" isn't the right framing for it. In some ways they're more like our "outbox" messages than our draft messages. @borisyankov , what do you think of that explanation?

[chrisbobbe]

Thanks everyone for your work writing and reviewing this PR! I'm closing it as mostly superseded by #5474 and #5638, which fixed these issues:

• [Android] Multiple file selection when uploading pictures. #2366
• Don't immediately send attachment on upload #4540

I see that this PR was also meant to fix these issues:

• Downsample images before uploading them #2749
• Image upload: Show preview before upload #1903

And those issues are still open. But so many merge conflicts have gathered that I think it would be more productive to address those issues with a new PR freshly written from the current main.

I encourage @armaanahluwalia and anyone else interested in this PR to try out the new UI (first released in the v27.198 beta on 2023-01-12) and see what you think. 🙂