[android] Share to Zulip from other apps [part 2]

[AkashDhiman]

Fixes: #117
Continuation of work done in #4514

The previous PR thread was getting too long, so as suggested by @gnprice It is now broken into two parts.
To see the screen capture of the demonstration of this feature checkout #4514.
Reviews on that thread by @chrisbobbe: #4514 (review)
Reviews on that thread by @gnprice: #4514 (review)

Note that the specific commit that fixes #117 is present in this PR and not #4514.

[gnprice]

Thanks @AkashDhiman for these changes!

I've now read through the first 4/7 commits:

db8038d sharing: Make the app not quit after share to zulip concludes.
1a0be67 sharing: Set appropriate filename of shared content.
bd52bce android: Enable receiving 'shares' from other apps.
61c1e45 sharing: Add functionality to share multiple files to zulip android.

and I'll leave it there for this round of review.

That leaves these commits for a future round:

1ddc8ac sharing: Display file name under content preview.
41975a5 sharing: Show preview for non image files.
536f0e3 sharing: Enable users to delete attachments before sending.

[gnprice]

This should pass editMessage: null, same as the existing navigateToChat does.

The reason is that ChatScreen expects that:

// ChatScreen.js
type Props = $ReadOnly<{|
  navigation: AppNavigationProp<'chat'>,
  route: RouteProp<'chat', {| narrow: Narrow, editMessage: EditMessage | null |}>,
|}>;

Note editMessage is a required property there.

This is exactly the kind of thing that in general the type-checker is supposed to mean you don't have to worry about 🙂 -- if Flow accepts the code, then it's good. All these actions in navActions.js represent an exception to that happy general rule, because react-navigation doesn't make it easy to connect the dots between the argument 'chat' here and the route-params type expected by ChatScreen over there. So whenever adding or changing something in navActions.js, or changing the route-params type on a screen, we have to manually check that the types line up.

I should add a comment in navActions.js saying that, because I see there isn't one; perhaps also something in the style guide or other docs. Also I should try to just solve that problem -- react-navigation doesn't make it easy, but I think it still can be done.

[gnprice]

nit: keep the blank line -- it visually separates the nav props from the rest.

(In general, where there's an existing blank line it's best to keep it unless you have a reason it shouldn't be there anymore.)

[gnprice]

nit: keep blank line between third-party imports and ours

Also an example of the same general rule as in the previous comment 😉

[gnprice]

nit: keep auth and ownUserId in the same order here as in the connect call (probably alphabetical).

Also keep dispatch first, as it currently is; that's where we typically put it among props gotten from connect. The dispatch: Dispatch is super boring, and writing it the same way every time helps let the boring thing fade appropriately into the background 🙂

[gnprice]

Hmm, the name finishShare no longer seems appropriate -- this now isn't something we do after successfully sending, to finish off the sharing, but instead is something we do if sharing failed.

Perhaps a name like onSendError?

[gnprice]

Oh gosh I see, and this exception case shouldn't even happen, because here's the spot in handleSend where we make the API call:

  try {
    await sendMessage(auth, messageData);
  } catch (err) {
    showToast(_('Failed to send message'));
    logging.error(err);
    return;
  }
  showToast(_('Message sent'));

(Where that sendMessage should be api.sendMessage -- it's an API call. I probably hadn't written this style-guide entry yet when that line was added, and I guess I let it slip through in code review.)

That explains the existing behavior you describe in the commit message:

Previously after content was shared to zulip the app would quit,
regardless of the fact that the content was shared successfully or
failed to share, …

That code is, on its own, doing a fine job of the pattern I described in this style-guide entry I just wrote yesterday:
https://github.com/zulip/zulip-mobile/blob/master/docs/style.md#catch-at-ui
i.e. of catching the exception at the level of UI code, and informing the user accordingly.

But then basically what's happening in this commit is that you want to introduce another way that the UI responds to the difference between success and failure: you want to navigate one way on success, and another way on failure.

Does the new logic in this version actually work as intended? It looks like if the API request fails, the handleSend call will show an error toast but then return, and so this code will proceed to this.shareSuccess() which will navigate to the relevant narrow.

To implement this logic correctly, we probably want a single try/catch. Both this function and its callee handleSend are plausible sites for that -- they're both UI code that's responsible for this interaction. I think probably the cleanest way would be to have handleSend take care of it; this caller can pass in a Narrow object for it to use.

[gnprice]

(One reason it's probably cleanest to have handleSend take care of this is that as I was looking at this code, another thing I was thinking is that more of this logic should be pushed into code that's shared between ShareToPm and ShareToStream -- there's a fair amount that looks duplicate. That's mostly pre-existing before this PR, but would be good not to regress on.)

[AkashDhiman]

Does the new logic in this version actually work as intended? It looks like if the API request fails, the handleSend call will show an error toast but then return, and so this code will proceed to this.shareSuccess() which will navigate to the relevant narrow.

Yeah, it works as you describe, I must have not tested it.

In the new revision I have significantly changed this code, (as part of de-duplicating some of the code b/w ShareToPm and ShareToStream) and so effectively this part of the code now uses only 1 try catch.

[gnprice]

Hmm interesting, so this commit changes the way we detect whether the item should be called an "image" or a "file".

Previously we'd look at the type property of the Intent, which I guess is a MIME type, and see if it looked like image/foo.

Now we instead take each URL, and go ask the content resolver about it with getType, and pass that on.

Can you make that change first, before adding the SEND_MULTIPLE case? I think that would help split up this commit's changes so they can individually be understood.

Do you believe that for the SEND case, the new logic has the exact same behavior? I.e. that intent.type is exactly the same as contentResolver.getType(url). That would be helpful to know if so (and you can mark that commit as NFC to express that), or if not it'd be helpful to know what changes are expected.

[AkashDhiman]

contentResolver.getType has the potential to return null, so in the new revision I am handling this in kotlin code itself although I am not sure if this is the right way, it returns null only when the url provided is broken, this shouldn't really happen with SEND_INTENT but just in case it does, I give a fallback mimeType for octet-stream.

implementation in master branch deduces types by directly analyzing the send intent, but this won't be possible in case of multiple files, it will just be / in that case, wonder what will happen there if a broken url is provided.

[gnprice]

Hmm, what if neither url nor urls is non-null? Or what if both of them are?

Is the point here that url should always be present on SEND, and url should always be present on SEND_MULTIPLE? If so, it'd be best to make that explicit -- instead of conditioning on which of these extras are present on the intent, we can switch on intent.action and then assert that the expected extra is present, throwing an exception like the existing code does.

[gnprice]

This is exactly the same logic as in the url != null case above, right? Let's deduplicate it. Can just make a little helper method that takes url and contentResolver as arguments, and returns singleContent.

[gnprice]

Hmm, it's a shame to have to be making all these changes twice. And it makes it likely that as we keep making changes, things will unintentionally diverge.

Would you try to deduplicate some of this code, in commits that come before the ones where you extend it?

It might be as simple as making a component with a name like SharedContentPreview, which would contain (initially, before your change to handle multiple files) just the <Image … /> line and the conditional on sharedData.type that controls it.

Basically any code you're going to add more logic to, and that's already duplicated, would be good to deduplicate first so you can then add the logic only once. (Ideally it should have been deduplicated in the first place; oh well 🤷 )

[AkashDhiman]

I've made a ShareWrapper method, that deduplicates this and a lot more. Initially I went with just SharedContentPreview but there was a lots of de-duplication potential in both the screens. I wonder if this can have any downside though 🤔.

[AkashDhiman]

@gnprice I've made the revision you asked, and did a bit more.

• I've done as much of deduplication as possible between ShareToStream and ShareToPm.
• I've moved 1 commit about displaying file name under content preview, earlier than the previous revision.
• I've noticed that getFileName and getType have the potential to return null, this will take place only when provided with an invalid url and here I am essentially falling back to displaying unknown.bin file in such a case.
• Then there is 1 concern that I will make an inline comment for.

[AkashDhiman]

This can result in an unhandled promise rejection right? should this be wrapped in a try catch? I am using this inside handleSend of the ShareWrapper introduced in the current revision as well and haven't wrapped it in try catch just yet, wanted someone's opinion on this.

[gnprice]

Yeah, thanks for catching that.

In the current revision, it looks like there are two awaits in handleSend. You have a try/catch around the one for api.sendMessage, so I think that one is taken care of. It'd be good to add another try/catch around the await line that does api.uploadFile, too.

[gnprice]

nit: when there's a type-tag like this type property, put it first in the list. That's because that's always the property you want to look at first, to determine which other properties will even be present and what they'd mean.

[gnprice]

Oh I see, this is based on types that were previously in src/sharing/send.js. Well, reordering these would be a nice tiny cleanup to tack on, even at the end of the branch.

[AkashDhiman]

done, made a separate commit together with making a SendTo type.

[gnprice]

Hmm, yikes, is the existing code here relying on stream names instead of IDs? ... Yep, it is. Well, fixing that is out of scope for this PR. But it would be a good followup while you're in this code.

[AkashDhiman]

will keep this in mind for followup issues.

[gnprice]

I think we don't even need names for these two subtypes -- we can just have a single definition for their union.

Then you can write it real compactly:

type SendTo =
  | {| type: 'stream', stream: string, topic: string |}
  | {| type: 'pm', selectedRecipients: $ReadOnlyArray<UserId> |};

Oh, but this is based on types from send.js. Yeah, for moving those it's definitely best to leave them in a similar format -- there's plenty happening in that commit, so best not to add more changes than necessary. Would be a nice small cleanup later; could squash it with reordering type to go first.

[gnprice]

This nestedScrollEnabled looks like potentially a change that we care about one way or the other. It wasn't set on the ShareToPm side, and isn't set in the new wrapper. Is this something we do want set, or don't?

[AkashDhiman]

it is necessary. Fixed in the new revision.

[gnprice]

Cool. The new revision sets that prop in the new wrapper.

That does still make this change not NFC, because previously it wasn't set for ShareToPm and now it is. Two ways to fix that:

• The cleanest is to have a small commit first that adds it on ShareToPm and says why. Then the main commit can be NFC.
  • In this case ([android] Share to Zulip from other apps [part 2] #4757 (comment)), it's OK if the "why" doesn't involve really investigating how or whether the old code worked -- if the story is "we want it for the stream case, and it seems fine here", that's enough.
• Alternatively the main commit could drop the "[nfc]", and mention this change in the commit message. That wouldn't normally meet our standards for highly readable commits, but in this case it'd be OK.

I think the cleaner thing is actually pretty quick to do, though, with the right tools. (Much easier than splitting up the bigger changes in this commit into several steps would be.) So I'd encourage you to try it. I'd use git rebase -i, with something like:

$ gitk --all HEAD &  # to see what you're doing
$ git rebase -i
  # insert a 'break' line after this "Deduplicate code" commit
  # save and exit; now you're mid-rebase, and at this commit
$ git reset --hard @~  # reset to before this commit
  # now edit ShareToPm.js to change that one line; save that
$ git commit -am 'wip add nestedScrollEnabled'
$ git restore -s @{2} .  # get to the same state as after this commit
$ git commit -aC @{2}  # borrow commit message, and author name+date, from this commit
$ git rebase --continue

The two references to @{2} there are a way of mentioning the "Deduplicate code" commit -- @{0} is the current HEAD, @{1} is what it was before this (before the git commit), and @{2} is what it was before that (before the git reset). Alternatively, you could look at the gitk --all HEAD or a git log command, and copy-paste the commit ID.

Then after that:

• refresh in gitk, to check your work;
• use git rebase -i and just a reword on the new commit, to replace the placeholder message with a proper one.

[gnprice]

Cool, this is a good way to unpack this object.

[gnprice]

This case should be impossible, right? It'd be a bug in the app, in fact a bug within the sharing code, if we were to reach this case.

I think treating it as failure is a fine response in the UI, since that's handy. (It'd also be OK to just throw an exception, if there weren't something more specific to do instead that was handy.) But then the other thing we should do is log an error with logging.error, so that we see it in Sentry.

Separately, the first thing this case should do is call ensureUnreachable. That way Flow will confirm that our types say this case really is impossible.

Here's an example at the end of handleOutboundEvents.js:

    default:
      ensureUnreachable(event);
      logging.error(`WebView event of unknown type: ${event.type}`);
      break;

[gnprice]

Thanks!

on a side note this makes me wonder why notification has its own separate types.js (why not include it in the main types?)

Good question. Two reasons:

• It's generally more helpful to have the code organized primarily by what general subsystem it's part of -- like notifications, or sharing, or the server API -- rather than by what kind of code it is, like is it type definitions. For example when you're working on notifications, you might be touching a bunch of different notifications-related files; but it's less common that you'd be working on "types, in general", and be touching a bunch of different types-related files.
• Having a single global "types" file tends to create big import cycles. Those can be bad for a variety of reasons; they tend to make the codebase harder for both tools and people to work with. Here's an example we ran into: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3745.20Server.20version.20to.20Sentry/near/930319

[gnprice]

Partway through reviewing, but dinnertime here 🙂

[AkashDhiman]

@gnprice made the changes, you may see them.

[gnprice]

Thanks for the fast revision! Reading now.

One thought I meant to put in the overall review comment for yesterday's review (but then didn't think of when I was submitting the partly-done review):

The second commit here, the one deduplicating the sharing code into the new SharingWrapper, has kind of a lot of things going on. Typically I'd ask for a commit like that to be broken up into smaller pieces, because that makes it easier to read all the changes closely and really verify they're not changing anything, or changing only things we intend to change.

• For example, one commit could make SharingWrapper and give it a handleSend that still looks like the handleSend on the two old components, and then a later commit does the part where the separate handleSend helper function gets inlined into that.

When I'm doing a refactor myself, I'll generally write it more or less that way in the first place. I find that helps a lot for being pretty sure I'm changing only the things I intend to change -- and especially for doing large or complex refactors. This involves doing git commit -a frequently, at basically every step, often with short placeholder commit messages; and then doing a fair amount of git rebase -i at the end, to reorder the changes, to squash later changes that were fixes to earlier ones, and to fill out the commit messages that I didn't fully write the first time.

• The frequent git commit -am 'wip add WidgetFrobber' is easy, and often very helpful even just for writing the code. I think I would do that even if I were ultimately going to squash everything into one commit.
• The git rebase -i takes some effort, though it gets faster with practice. We do this for the sake of people reading the changes.

But... the existing code isn't live code that people are using, so it's not like we have a presumption that it currently works despite being tangly. So it's actually OK if the behavior changes, as long as we think the new behavior is fine. It's not even important (like it usually would be) that we understand what the old behavior was and why it was that way.

So I have some questions where I do notice what look like changes. But in this case I won't ask you to do the work to split the commit up into smaller pieces; and for those changes, it's fine for the answer to be that you tested that aspect of the new behavior and it seems good.

[gnprice]

👍

[gnprice]

Cool. (Continues this thread: #4757 (comment) .)

We do actually want the onShareCancelled call too, though. In general, because our types aren't guarantees, when we explicitly get into a case that should be impossible, we want to do something to bail out of the part of the code that's clearly confused. Throwing an exception is the default solution, but here onShareCancelled is handy and would be better.

[gnprice]

Cool, sounds good.

As described at #4757 (comment) , for this particular refactor, if this sort of thing seems to work in the new version, then that's good enough. If we find some issue in the layout later, we can fix it then.

[gnprice]

OK, finished re-reviewing the parts you just revised! (Including a couple of replies in the threads above.) That covers the first 6 commits:

af4e9a9 translation: Add some english words used in sharing screen.
91ef465 sharing [nfc]: Deduplicate code b/w Pm and Stream sharing screens.
beb81e7 sharing [nfc]: Write type of sendTo more aptly.
1c8a526 sharing [nfc]: Move helper kotlin code to a new file.
f8f2c15 sharing: Make the app not quit after share to zulip concludes.
ba0c1e4 sharing [nfc]: Add doc about synchronising native and js data type.

There are a few small things left to fix; I think we're quite close to merging those.

Next I'll take a look at the next few commits, corresponding to the remaining commits I reviewed the first time and that you revised this week.

[gnprice]

OK, read the next few. Just a few comments -- see below and replies above.

That covers the next four commits:
5695197 sharing: Set appropriate filename of shared content.
fe9463d sharing: Show preview for non image files.
1d09fd1 sharing: Display file name under content preview.
63a6154 android: Enable receiving 'shares' from other apps.

including actually enabling the feature! 🎉 Excited to be close to merging that.

Then I think probably the easiest workflow is I'll merge this PR with just the changes up to that point, and please post the remaining changes:
889d73c sharing: Change SharedFile.
d022622 sharing: Add functionality to share multiple files to zulip android.
787bd00 sharing: Enable users to delete attachments before sending.

as a followup PR -- basically a PR adding the feature of sharing multiple files. There are a couple of subthreads above that will continue on that PR, but that'll just help keep the GitHub interface more manageable as this thread gets long.

[AkashDhiman]

@gnprice done resolving, you can check. I will also make the PR for Multiple Share once this gets merged.

I see from this review that I should focus on splitting up my commits some more, and I will keep that in mind. One reason for doing this is that I do not notice this sometimes (which I will try to improve upon) but also that my thought process was that associated code should change together to convey the meaning of why we did that change in the first place; moreover sometimes (less frequently though) it can lead to doing extra changes to accommodate the changes introduced in the prep commit that will be changed in the next commit regardless, which feels unnecessary.

But I understand your point that this increases the chances of us catching side effects we may have from this prep code and it makes for a better code review experience so I will try to do this as much as possible. Thanks!

[gnprice]

Thanks for the revision! All looks good, except that it looks like send.js got revived in this one (i.e. it's no longer deleted in the deduplication commit.) I'll fix that and merge.

[gnprice]

my thought process was that associated code should change together to convey the meaning of why we did that change in the first place

I started writing some thoughts in reply to this, and then realized it'd be better as a chat thread 🙂 So I'll do that.

edit: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/splitting.2Fsquashing.20commits/near/1234248