Upload custom files of any type.

[ishammahajan]

Adding support for upload of custom files on mobile has been a
feature long avaited by users. This commit adds the feature.

Size of the expanded ComposeMenu has been increased by 4/3. Since
handleImagePickerResponse works for all types of files and not
only image files it has been renamed to handleFileUpload, and the
upload traffic from handleFilePicker has also been redirected to
handleFileUpload.

NOTE: Adequate testing hasn't been done on iOS since I don't own an iOS device. Please test and produce the results here if possible.

[ishammahajan]

Further Note: Please read the commit messages (they have some points of concern addressed in them).

[gnprice]

Thanks @ishammahajan ! I especially appreciate all the clear explanations. 😄

As I said just now over on the issue (#3184), I'd like to set aside for now iCloud or other iOS versions of this feature, and focus on the Android case. So, perhaps that simplifies things! We just need to ensure we don't break anything on iOS.

On this particular third-party library, it concerns me a little that the README has screenshots and copious description for the iOS case, and just a bare code snippet for Android. Makes me suspect that they're primarily focused on the iOS case.

Can you say more about how the UX is on Android? Some screenshots might be helpful, too.

[gnprice]

compose-menu, fetch-actions: Refactor function names.

`uploadImage` has been changed to the more appropriate `uploadFile`
since it uploads every file and not only images.

`uploadFile.js` has been renamed to `uploadToRealm.js` because
`uploadFile` name was taken up (by previous point), and because
`uploadToRealm` reflected properly of the required `auth` property.

Ah, I have an alternative solution for this! 😉 Take a look at 0c52f4a, acb979c, and b8010de .

With that style, we can use the names we think best, and not have to change the name to avoid this kind of name collision.

[gnprice]

Meanwhile I just pushed the first commit from this branch, as cab9cb5 -- thanks for that fix!

I wonder if we should do the same even when the menu isn't expanded, when there's only placeholder text or perhaps when the compose box isn't focused. Here's a max-length stream name, on an emulated Nexus 5 with display and font zoom cranked up to max:

Kind of crowds the actual message list, if you're just trying to read the messages (which is most of the time, for most users).

[gnprice]

(Propagating priority from the issue.)

[ishammahajan]

On this particular third-party library, it concerns me a little that the README has screenshots and copious description for the iOS case, and just a bare code snippet for Android. Makes me suspect that they're primarily focused on the iOS case.

I think that is because the setup process is much more complicated for iOS than for Android. Primarily that is because of Apple 'protecting' their users. Android has very little in terms of security when choosing files (which is to say they have a good understanding of where protection is necessary).

Can you say more about how the UX is on Android? Some screenshots might be helpful, too.

The UX on android would be what files app you have/use for viewing the different files on your device. Here's the code they use to achieve it.

Note the show() function used here, it uses Android native intents in order to get the file paths.

Meanwhile, here's the UI found on my OnePlus device.
https://imgur.com/ElJlcOE

[ishammahajan]

Ah, I have an alternative solution for this! 😉 Take a look at 0c52f4a, acb979c, and b8010de .

Good solution, thanks!

I wonder if we should do the same even when the menu isn't expanded,

True, that should be the way to go.

[ishammahajan]

Hmm. I think a followup to this issue would be to add progress bars to uploads -- both images and files (since I have noticed while adding large files it appears to take very long to do so, while giving no indication that the upload has started).

[gnprice]

Here's the code they use to achieve it.

BTW this would be more helpful as a link 🙂 -- then I can read the text normally in my browser, complete with Ctrl+F, copy-paste, browsing to neighboring files, etc.

[gnprice]

Generally this looks good! Thanks for that tweak on the renames, as well as the added information above.

I think the remaining things before merge are

• We need to test that it doesn't break things on iOS. I think you can do this in the iOS simulator.
  • This probably requires adding a bit of code to not show the icon on iOS, since there's nothing for it to do yet.
• I'll probably poke at the iOS build a bit to try to understand what that project.pbxproj change is doing, and/or why it happens automatically when you open Xcode. But if I can't figure it out, I'll time out on that and be OK merging as is -- with the helpful explanation in your commit message, and so long as we've tested it doesn't actually break anything.

Then:

• A small followup that'd be good to do and I think you can probably take care of quickly is to extend how we limit the compose box's height, as we discussed above. (This can actually be done now, before we even merge this PR.)
• I agree re: progress bars on uploads. See Image upload: Show progress feedback #2684 . 😄 Though see Improve the flow when uploading images #3118 (comment) -- this turns out to be more complex than it sounds.
• Another followup is to possibly add a version of this functionality on iOS, say for iCloud.

[ishammahajan]

• We need to test that it doesn't break things on iOS.

These changes actually do enable uploads for iOS. I don't know what the UI redirected towards is supposed to be though (this is on iOS 12.0). Here's the recording. Would you please test this on your physical device as well? I think it will work and it's preferable that we don't disable the specific features on iOS.

• 'll probably poke at the iOS build a bit to try to understand what that project.pbxproj change is doing, and/or why it happens automatically when you open Xcode.

Sure, thanks! 😄

• A small followup that'd be good to do and I think you can probably take care of quickly is to extend how we limit the compose box's height, as we discussed above.

Ah, I'll just push another commit which does this in this PR itself, then.

• Another followup is to possibly add a version of this functionality on iOS, say for iCloud.

Yup, although I don't think I will be able to do that. Someone with admin privilege is needed if I'm not wrong.

[gnprice]

Hmm, I see -- in that recording, it looks like there's some non-iCloud UI that this leads to, so it might actually function properly already. What I'd thought you were saying earlier is that this wouldn't work at all on iOS until we do the additional steps necessary to interact with iCloud.

OK, I'll play with it on my iPhone. Won't get to it today, though.

[gnprice]

c7e3ad3 compose box: Keep input single line when not in focus.

-              multiline={!isMenuExpanded}
+              multiline={isFocused}

Doesn't this partly undo your earlier change, by making it multiline after all in some cases?

It'll do that in any case where isFocused && isMenuExpanded. It definitely isn't obvious that that's impossible... and I think it can in fact happen, if you focus and then tap the button to expand the menu. Is it actually impossible?

[ishammahajan]

and I think it can in fact happen, if you focus and then tap the button to expand the menu. Is it actually impossible?

Well, during development I made the false assumption that someone would tap the button only when the compose box was in focus. I'll go fix that in a jiffy.

[gnprice]

7192b44 compose box: Keep input single line when not in focus.

-              multiline={!isMenuExpanded}
+              multiline={(isFocused && !isMenuExpanded) || !!message}

This still partly undoes your earlier change, making it multiline after all in some cases (though fewer of them.) What's the reasoning for that? The commit message only says it keeps it single-line in more cases.

[gnprice]

OK, I'll play with it on my iPhone. Won't get to it today, though.

OK, did this today. (It involved a couple of side quests: needed to update Xcode in order to put debug code on my newish device, and needed to update macOS in order to update Xcode.)

The experience didn't make a lot of sense to me. Here's the screen I got, after tapping past the "you have no recents" screen that also appears in your recording:

Two of those folders are empty (despite saying "1 item".)

The "Breaker" folder contains just one thing -- another folder, which is empty.

The "Slack" folder consists of some gobbledygook that looks internal to that application:

I haven't yet seen a product case for adding this feature on iOS -- as far as I recall, we've never heard from a user who wanted this feature. (The situation is very different on Android, where "files on the device filesystem" has been an important part of how many apps guide users to use them since the beginning.)

I think we've already spent more time on the iOS side here than its priority warrants, and I don't want to spend more product energy working out a version of this experience that could make sense. So at this point let's cut off pursuing the iOS work, and just find a way to make this change do nothing at all on iOS.

[ishammahajan]

Thanks for the review! :)

This still partly undoes your earlier change, making it multiline after all in some cases (though fewer of them.) What's the reasoning for that? The commit message only says it keeps it single-line in more cases.

I see the problem, and I think I'll just remove this commit for now. How does just !isMenuExpanded || !!message sound for solving this issue? That way, if there is no message, or if the menu is expanded, the ComposeBox will remain single line.

I haven't yet seen a product case for adding this feature on iOS -- as far as I recall, we've never heard from a user who wanted this feature.

I think I remember one recent issue where a user was asking for custom file uploads from iOS. It might have been sharing from another app, I don't really remember. Tried finding it and couldn't. Perhaps they were asking for another feature altogether.

So at this point let's cut off pursuing the iOS work, and just find a way to make this change do nothing at all on iOS.

That makes sense, given the time constraint I'm facing in my university. I've removed iOS support using basic platform checking.

[gnprice]

I'm sorry, but this is still not ready for merge.

First, in this version it doesn't actually work! The icon never appears, because this condition is always false:

+            {Platform.OS === 'iOS' && (

The value is ios when on iOS. I was confused why it wasn't showing up, and then grepped our code for Platform.OS to see other examples -- then it was immediately clear why.

Moreover, if the spelling were fixed, that would make it show up only on iOS -- the reverse of the intent.

Then when it doesn't show up, the menu is still widened to make room for it -- so there's a buggy-looking blank space.

So it looks like you didn't really test this version, either on iOS or on Android.

---

OK, that's not too hard to fix once it's debugged, and I fixed it. I fixed a few other issues, and was about to merge a version of it. Then I noticed that running tests was extremely noisy -- if I run tools/test, I get about 8 copies of a warning generated by this library:

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "RNDocumentPicker: Native module is not available, make sure you have finished the installation process and rebuilt your app".

      at BufferedConsole.warn (node_modules/@jest/console/build/BufferedConsole.js:224:10)
      at Timeout._onTimeout (node_modules/react-native-document-picker/index.js:8:13)

Looking at the library's source, it decides at import time to emit this warning when the native module isn't present. The native module is indeed not present, because that's part of Jest doing its work so quickly. Then it emits it inside a setTimeout, which is why it comes to us wrapped by Jest in a further warning.

We can't accept that level of warning noise as routine -- it makes it much too hard to spot a new warning.

I'm going to try to merge as much of this as I can, given the work I just did to clean it up.

[gnprice]

OK, merged! Thanks again for all your work on this PR.

I was able to work around the obnoxious repeating warning by moving the import to a require just before we actually invoke the library; see b20dbcf .

Do take a look at the other commits in the series as well.

[ishammahajan]

🎉

About the testing, I had updated the PR locally, but because of another bug I was trying to resolve (which was the tools/test warnings you discovered later), hadn't pushed it. So sorry about that, should have put up an update about it here.

I was able to work around the obnoxious repeating warning by moving the import to a require just before we actually invoke the library; see b20dbcf .

That's a little confusing at first. Makes sense though.

Do take a look at the other commits in the series as well.

I have done that now, thanks for the help :)

[ishammahajan]

This also saves us from the common Promise API mistake of invoking
.catch() on the return value of .then when we really only
intend to handle errors from the original Promise. As written,
this code would invoke this.handleImagePickerResponse a second
time if for some reason there was an exception thrown when we
called it the first time.

That's a good insight, thanks!

This library prints an obnoxious warning right at import time,
if its native module isn't present. (Actually worse: at import
time it calls setTimeout with a callback to print the warning.)

I wonder if we should add a comment here explaining this (in case the git blame gets overwritten for this piece).

[gnprice]

This library prints an obnoxious warning right at import time,
if its native module isn't present. (Actually worse: at import
time it calls setTimeout with a callback to print the warning.)

I wonder if we should add a comment here explaining this

Sure, good thought, thanks -- done, as c61702e.

(in case the git blame gets overwritten for this piece).

Though when you are looking in the history to see how and why something happened, I would highly recommend skimming through git log --stat -p with the /^c trick, filtered to the relevant file and perhaps also with git log -S or git log -G. I find that generally gets to a much more complete and precise answer faster than git blame does -- partly because it's much less easily obscured by later changes.