[android] Handle receiving shares from other apps

[agrawal-d]

Closes #117.

Two bugs exist as of now:

• uploading non-image files causes a network error
• User picker screen component shows the back button, event when canGoBack={false} is passed

Discussion on CZO

Screenshots:

[agrawal-d]

I'm not sure the commit android [nfc]: Format 'AndroidManifest.xml' acc Android Studio defaults. is required, I'll remove it if not.

[gnprice]

Thanks @agrawal-d ! Happy to see this making such rapid progress.

A number of comments below. I haven't read all of it in detail, but I've read a lot now, and I think these should be enough for a solid round of revisions.

[gnprice]

Hmm interesting. I think this call needs to happen at the top level, outside of the render() method. It's basically returning a component type, i.e. something at the same level as SharingScreen itself rather than a particular instance of it. If render gets called twice and we get two of these as a result, I expect that will cause the old instance of the old Tabs to be discarded and completely replaced by an instance of the new Tabs, rather than just have its props updated. Which would mean any UI state is lost, like which tab is selected and what text the user has entered.

I think the share value can maybe be passed through like so?

          screen: (share) => (
// ...
        <Tabs share={share} />

Not sure what the API is that react-navigation provides here, but there ought to be some way to do it.

For dispatch, subscriptions, and auth, probably simplest to have StreamScreen and PmScreen each do their own connect calls.

[agrawal-d]

I've extracted createMaterialTopTabNavigator out of render.

The snippet you commented does not seem to work though,

[gnprice]

It looks like in the new revision it's in the constructor. That means we end up getting data (for share) from props, in the constructor:

  constructor(props) {
    super(props);
    const { navigation } = this.props;
    const share: SharedData = navigation.state.params.sharedData;

    this.Tabs = createMaterialTopTabNavigator(
      {
        Stream: {
          screen: () => <ShareToStream share={share} />,

That isn't good either -- React components don't ordinarily. We should try to write this in a way that fits smoothly with the basic React model of how things work. It's always possible to do something that deviates if it's really necessary, but we should first try to make it work properly, and if we can't then understand why not.

The react-navigation library is meant to be used with React, so I'll be kind of surprised if it really doesn't have a reasonable way to pass through React props in a normal React-compatible way.

Please take a look at the react-navigation docs (https://reactnavigation.org/docs/2.x/ for the version we're currently on), and/or the web, and/or its implementation in your node_modules/react-navigation, and try to find the appropriate feature of its API for this. If you're having trouble, ask on #mobile-team and say what you've looked at and tried so far.

[chrisbobbe]

@gnprice said:

Please take a look at the react-navigation docs (https://reactnavigation.org/docs/2.x/ for the version we're currently on), and/or the web, and/or its implementation in your node_modules/react-navigation, and try to find the appropriate feature of its API for this. If you're having trouble, ask on #mobile-team and say what you've looked at and tried so far.

There's been some discussion of this, starting from Divyanshu's post here.

Taking a second look at my reply there, I actually think screenProps is the one and only way React Navigation would like us to handle it. But there's that inconvenient conclusion that, if you do things "the right way", only explicitly rendering the root navigator in your own JSX (and no descendent navigators), whatever you pass as screenProps is sort of forced to exist in a global context. Unless there's something less well documented, about screenProps, that I'm missing [1].

It would be helpful if we could work out the advantages of following this "render-only-one-navigator" guideline strictly (and bringing our existing code in line with it).

One compromise (and just because it's a compromise doesn't mean it's what we should do) might be to follow their instructions under

Alternatively, the following would also work because it exposes the router static on AuthenticationScreen and threads through the navigation prop:

at that same "Common Mistakes" doc (link). That makes it seam like we could indeed explicitly render a non-global navigator, but being sure to add some boilerplate that handles some important plumbing. Then we'd have the means to pass screenProps at a more local level. But they seem to give this example as an afterthought, so I'd wonder if we're missing out on some important design principle that means something like this should never be necessary.

---

[1]: It's not impossible: the docs on screenProps seem spotty. When I wrote that reply, I apparently felt that the most relevant bit of docs on screenProps was on a page about the stack navigator (this page), which is not the navigator we're using here. I've used screenProps in react-navigation v2 and I'm pretty sure it's quite relevant for all types of navigators.

[chrisbobbe]

I've just started a #mobile-team topic about this, here.

[gnprice]

Thanks for that suggestion and the demo! And good thought to move to chat for the detailed discussion.

That approach with the router static, and passing through the navigation prop, sounds good to me.

[gnprice]

Hmm interesting. Can you say more about why this is needed? What's the scenario in which it takes effect?

One thing that makes me curious here is that we don't have a similar condition in this spot for notifications, even though opening a notification can mean a very similar navigation at startup.

[agrawal-d]

I had mentioned earlier in the group PM that the sharing screen kept on getting closed soon after appearing. I investigated that a bit and found that It was caused due to this rehydrate function. This condition prevents that from happening,

[gnprice]

Huh! That is peculiar. It leaves me still curious what's different between this and notifications that would make it needed here and not there.

I don't see this mentioned in the group PM thread with you and me and Chris. Probably #mobile-team would be the best place for any further debugging, in any case.

One possibility I wonder about is that perhaps it's related to how in a previous version of this PR there was a separate activity. Now that that's gone, the way the rest of the system works is more like it does for notifications.

[agrawal-d]

I just realized why I'm no longer leaving line breaks after function definitions - the GitLens VS Code extension. It shows the function history above it, and it looks like a new line 🤕

I've changed some settings now, and it should not happen again.

[agrawal-d]

Thanks, @gnprice for the review. I've made the changes you suggested.

[agrawal-d]

Ouch - I didn't notice this. I remember putting it inside a conditional, it must have moved while doing a refactor.

[agrawal-d]

Fixed now.

[agrawal-d]

I've extracted createMaterialTopTabNavigator out of render.

The snippet you commented does not seem to work though,

[agrawal-d]

Thanks @gnprice for the review. I've gone through all the suggestions you mentioned. I've not made any changes for SharingScreen, as @chrisbobbe has started some discussion regarding that, and I'll wait until we've concluded that discussion.

[agrawal-d]

For the time being, this code is a close duplicate of the one in NotifyReact.kt, until we decide to either make a common function for both or keep the duplicate here.

[gnprice]

Great. The one essential thing it's missing is a comment (on each copy) mentioning it's duplicated, and pointing to the other copy.

I definitely won't want to keep this code duplicated. But it will be straightforward to factor out after things are settled and working.

[gnprice]

OK, merged those Kotlin-side changes! With a series of small tweaks on top:

0b84717 android: Kotlin-side code for receiving data from other apps.
54da31d android sharing [nfc]: Simplify a bit of null/non-null logic.
377f0d9 android sharing [nfc]: Use the handy Kotlin feature arrayListOf.
0c1b5e0 android sharing [nfc]: Tweak some formatting.
99de20d android sharing [nfc]: Flag duplicate code.
13d840c android sharing [nfc]: Align that duplicate code more closely.
43e501c android sharing [nfc]: Add some comments pointing at docs.
78360ae android sharing [nfc]: Cut redundant intent filters in manifest.

Do take a look -- some of those will contain helpful Kotlin tips, I think.

I think it should be straightforward to rebase your remaining changes on top of these. (Can drop the manifest-formatting one, I think.) Looking forward to getting the rest in!

[gnprice]

Are these extra attributes helpful and do they do things we want? We treat this activity pretty differently from our main one -- for example as soon as it opens, we go and close it again.

Hmm, in fact. I wonder if it might simplify things to simply add these intent filters to the main activity. After all, the behavior we're actually implementing here amounts to that in the end -- the user goes to send something, i.e. causes an intent with action android.intent.action.SEND, and what we end up doing is passing that all to the main activity.

[gnprice]

Note that the docs explicitly contemplate that the activity that handles these intents might be the same one that gets launched from the launcher:

Keep in mind that if this activity can be started from other parts of the system, such as the launcher, then you will need to take this into consideration when examining the intent.

[agrawal-d]

Are these extra attributes helpful and do they do things we want?

windowSoftInputMode and configChanges are not relevant here, because we don't care how the activity responds to the keyboard etc. But we do need launchMode to be singleTask - we want only one instance of the activity to exist at any given time.

I'll remove the useless attribs.

Reference: https://developer.android.com/guide/topics/manifest/activity-element

[agrawal-d]

Hmm, in fact. I wonder if it might simplify things to simply add these intent filters to the main activity. After all, the behaviour we're actually implementing here amounts to that in the end -- the user goes to send something, i.e. causes an intent with action android.intent.action.SEND, and what we end up doing is passing that all to the main activity.

One possible problem with that might be:
We always have to launch an activity. So, in case the app is already running, we will definitely launch MainActivity for every share action. We can close it soon, but I would expect the ZulipMobile component to be pretty heavy in terms of resources.

[gnprice]

(Moved to chat.)

[agrawal-d]

I've made three changes in this new revision:

1. Used the react-navigation code suggested by @chrisbobbe (Thanks!)
2. If there is no active account when receiving a share, the account picker screen is presented.
3. A spinner is shown when the message is being sent.

[chrisbobbe]

Thanks for these revisions, and I'm sorry for the delay in responding here! A few comments are below.

On CZO, I mentioned one thing I was running into, here, and I'm curious whether you've also seen symptoms like that.

One thing that I haven't been able to reproduce again, but that did happen, is an error in SharingModule.kt on the promise.resolve line in getInitialSharedContent; it said something like "Error: Map already consumed". I tried several times to get it to happen again, without success; it could have been something that only happens in development. But keep an eye out in case you see something like this; if you do, maybe you'll have better luck than me in reproducing it. I haven't found much of anything useful by Googling the error either.

@gnprice will have more to say, I'm sure. 🙂

[chrisbobbe]

I'm seeing this error:

Warning: 'keyboardShouldPersistTaps={true}' is deprecated. Use 'keyboardShouldPersistTaps="always"' instead

Here's the relevant doc: https://reactnative.dev/docs/0.60/scrollview

[agrawal-d]

Changed to 'always'.

[chrisbobbe]

How much are we relying on a proper toggle function for the choosingRecipients flag? toggleChoosingRecipients is implemented correctly as a toggle, i.e., it sets choosingRecipients to the opposite of its current value. But I'm not sure it's better than just calling this.setState({ choosingRecipients: true }) and this.setState({ choosingRecipients: false }).

In particular, at this call site in handleChooseRecipients, what we mean to do here is set the flag to false: we're finished choosing the recipients, and it's time for the modal to close.

That adds something important to the contract for handleChooseRecipients: it must be called only where the current value of this.state.choosingRecipients is true. In reading the code, there's an extra bit of work required to ensure that that contract is followed—I think it is, currently. But it feels fragile, and it leaves us open to the possibility that someone will innocently make a change to fix or improve something, and accidentally break the contract.

There's also the fact that the "Choose recipients" button doesn't really work as a toggle switch between the two states, but rather to just open the modal (i.e., change the state from false to true); it gets hidden behind the modal, and therefore effectively disabled, when the modal opens. It might actually be good to reinforce this pattern by using ZulipButton's disabled prop.

[agrawal-d]

I agree, it can be confusing. Removed the toggle function.

There's also the fact that the "Choose recipients" button doesn't really work as a toggle switch between the two states, but rather to just open the modal (i.e., change the state from false to true); it gets hidden behind the modal, and therefore effectively disabled, when the modal opens. It might actually be good to reinforce this pattern by using ZulipButton's disabled prop.

The button does not get hidden behind, it's not rendered at all when choosingRecipients is true.

    if (choosingRecipients) {
      return (
        <Modal>
          <ChooseRecipientsScreen onComplete={this.handleChooseRecipients} />
        </Modal>
      );
    }

So I think it's okay to not use the disabled prop.

[chrisbobbe]

Some of these params that we use in the request don't really correspond with the doc linked to in the code comment (https://zulip.com/api/send-message): subject, local_id, and queue_id. There must be a story behind this; maybe something changed on the server?

[agrawal-d]

Yeah, even I found this strange.

[gnprice]

subject is the old name for topic.

Dunno what the story is with local_id and queue_id -- it looks like those aren't documented, but I'd sure expect that they're still supported, as they're how we match up the eventual message / EVENT_NEW_MESSAGE event with the right outbox message to forget about. They may just never have been documented, though I thought our API-docs infrastructure was at the point now where it'd flag that. Would be good to ask in chat.

[chrisbobbe]

subject is the old name for topic.

True, I know, but it would be good to know more; maybe we can start using the new name (maybe deeper changes are needed to allow this?), when was the switch made, etc.

[chrisbobbe]

Tim responded here.

[gnprice]

One thing that I haven't been able to reproduce again, but that did happen, is an error in SharingModule.kt on the promise.resolve line in getInitialSharedContent; it said something like "Error: Map already consumed". I tried several times to get it to happen again, without success

Interesting. Here's a lead on what the error might have looked like, from some quick googling:
https://stackoverflow.com/questions/59025712/error-map-already-consumed-react-native

One guess for what that might mean is that perhaps you're sending the same WritableMap (hence the "Map" part) value over the bridge more than once, and perhaps that's not allowed. That seems quite plausible, and I think it'd explain an error message like that.

One thing that might affect reproducing it: I wouldn't particularly expect the behavior of the bridge to vary between debug and release mode... but it definitely does vary, and this is the kind of thing I could easily believe it varies in, between remote JS debugging and not remote JS debugging. The reason is that when remote JS debugging, the JS code of the RN environment actually runs in the Chrome browser you're debugging from, rather than in a JS implementation built into the app. Naturally some parts of the implementation of the bridge are written separately for talking to Chrome vs. talking to that in-app JS implementation... and so there can be subtle differences in behavior.

(Why yes, that can indeed be a very unpleasant surprise when trying to debug something! See #3588 and 5cf7f10 .)

[gnprice]

Here's some comments from a scan through this code, to add to the things Chris has already mentioned.

[gnprice]

This can be inlined, as an arrow function. Unlike in the case where we're passing a callback as a React prop, nothing's going to be looking to see if one value is === to a previous one and optimizing if it is -- which is the reason we have to have these separate named handler functions so often elsewhere.

[agrawal-d]

Okay, removed handleReceivedData and made the calls inline.

[gnprice]

Sorry, this was ambiguous -- I meant that this function handleShareReceived could be inlined 🙂

This is fine too, though; IIRC I actually waffled on whether I wanted to inline handleShareReceived, or inline handleReceivedData at its two call sites including this one.

[gnprice]

What's the reason for these fixmes?

In general where there's a $FlowFixMe, we always want at least a line or so of comment identifying why it was necessary.

[agrawal-d]

Removing the $FlowFixMe causes the following error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/sharing/SharingScreen.js:23:15

Cannot call createMaterialTopTabNavigator with object literal bound to routeConfigs because in property Stream.screen:
 • Either property navigationOptions is missing in React.ComponentType [1] but exists in
   withOptionalNavigationOptions [2].
 • Or React.ComponentType [1] is incompatible with React.StatelessFunctionalComponent [3].

     src/sharing/SharingScreen.js
      18│ |}>;
      19│
      20│ const SharingTopTabNavigator = createMaterialTopTabNavigator(
      21│   {
      22│     Stream: {
      23│       screen: ShareToStream,
      24│     },
      25│     'Private Message': {
      26│       // $FlowFixMe
      27│       screen: ShareToPm,
      28│     },
      29│   },
      30│   {
      31│     tabBarPosition: 'top',
      32│     animationEnabled: true,

     flow-typed/npm/react-navigation_v2.x.x.js
 [2] 313│     withOptionalNavigationOptions<Options>;
        :
 [3] 628│   > = React$StatelessFunctionalComponent<{
     629│     ...Props,
     630│     ...NavigationContainerProps<State, Options>,
     631│   }> &

     src/react-redux.js
 [1]  98│ ): C => ComponentType<$ReadOnly<OwnProps<C, SP>>> {

@chrisbobbe do you know what might be causing this? I can't figure out. I've added a generic comment:

      // Requires additional props that we dont need. $FlowFixMe.

[chrisbobbe]

I think #4114 (comment) is our latest knowledge on this error. Thanks, Greg, for writing that up! 🙂

[gnprice]

Ah, and that in turn leads me to this existing fixme:

$ rg -C2 FixMe.*nav 
src/nav/AppNavigator.js
34-
35-export default createStackNavigator(
36:  // $FlowFixMe react-navigation types :-/ -- see a36814e80
37-  {
38-    account: { screen: AccountPickScreen },

Which, when I try removing it, produces exactly the same kind of error as @agrawal-d quoted above.

That more recent writeup has more information than the commit mentioned in that fixme. But that certainly serves as an example of how these explanations don't need to be long 😉 .

A couple of smaller points on what goes in the fixme:

• The keyword $FlowFixMe goes at the start of the line, right after the // . That helps it stand out when scanning the code by eye. (Same thing goes for TODO, or anything else of that ilk.)
• Thinking about it a bit more, I think the key information I want in the explanation comes down to: why haven't we just already fixed it? So with "Requires additional props that we don't need", the natural next thing is: OK, let's make whatever's requiring those stop requiring them, then. But then the answer to that is: ah but it's over in the react-navigation library (or anyway its types, specified in its libdef). That doesn't mean we can't fix it -- it's just code, we can change it -- but it's more work. So identifying a specific third-party place as the location of the problem is a common kind of explanation for these.

[chrisbobbe]

Ah, and that in turn leads me to this existing fixme: [...]

One minor point about that example is that the $FlowFixMe is less precise than it could be; in this PR and #4114 (in the latter, at Greg's suggestion) we get some nice precision by putting it directly above the screen property in all those places. (I just tried that with one case in AppNavigator.js and commented out the other cases, and it was fine, as we'd expect.)

But adding 29 $FlowFixMes might be cumbersome, and we don't use anything other than the screen property. In fact, I'm pretty sure (untested though) the API would allow us to do this for all of them there:

- account: { screen: AccountPickScreen },
+ account: AccountPickScreen,

But no need to bother with that now.

[gnprice]

subject is the old name for topic.

Dunno what the story is with local_id and queue_id -- it looks like those aren't documented, but I'd sure expect that they're still supported, as they're how we match up the eventual message / EVENT_NEW_MESSAGE event with the right outbox message to forget about. They may just never have been documented, though I thought our API-docs infrastructure was at the point now where it'd flag that. Would be good to ask in chat.

[agrawal-d]

Thanks for the review, @chrisbobbe, @gnprice! I've made the changes you suggested.

One thing that I haven't been able to reproduce again, but that did happen, is an error in SharingModule.kt on the promise.resolve line in getInitialSharedContent; it said something like "Error: Map already consumed". I tried several times to get it to happen again, without success; it could have been something that only happens in development. But keep an eye out in case you see something like this; if you do, maybe you'll have better luck than me in reproducing it. I haven't found much of anything useful by Googling the error either.

Yes, I've encountered this error several times - I've only encountered it when remote debugging was enabled, and never when running the app without remote JS debugging.

[agrawal-d]

@gnprice based on your message on CZO, I've modified the code, so that the handling of sharing is done in MainActivity itself.

I've pushed to GitHub, but due to some issues with GitHub today, the PR is not getting updated, even after multiple force pushes. However, I am able to compare the changes from here: master...agrawal-d:sharing

[gnprice]

Reviewing this new version now -- sorry for the delay.

I've pushed to GitHub, but due to some issues with GitHub today, the PR is not getting updated, even after multiple force pushes. However, I am able to compare the changes from here:

Huh -- that is odd! Good to mention it and include that alternate way to see the changes.

It looks like it eventually did update, fortunately. Here's what I have locally, after using zulip:tools/reset-to-pull-request to fetch this PR:
$ git log --oneline --reverse upstream/master..pr/4124
e0df137 android: Refactor MainActivity to Kotlin.
b8b1462 android: Move sharing logic to MainActivity.
a4f7e0e topic actions [nfc]: Rename 'fetchTopicsForActiveStream' to 'fetchTopicsForStream'.
572b9a9 send message: Make certain fields optional.
d344d16 (pr/4124) android: Receive and handle shares from other apps.

And those are the same commit IDs as I see on this PR webpage, and the same as I see on the comparison page you linked to.

One GitHub bug (or class of bugs?) that I regularly run into which potentially explains your experience: when I have a PR or issue page open in a browser tab and then something happens that updates the PR or issue, the page often updates in place in the existing tab... but very often it doesn't. (In fact I was going to write that it "usually" updates, and then realized I'm not sure the success rate is even over 50%. 😞 ) So if you were watching it in an existing tab, then your experience would be another example of that.

[gnprice]

OK! The first N-1 of these commits all look good except for some quick things mentioned below -- so to help reduce round-trips, I'm going to go ahead and make those fixes and then merge them now.

Then I'll take a closer look at the last commit. One thing I've already spotted -- when I tried sharing, I got a red-screen with this error:
Invariant Violation: ViewPagerAndroid has been removed from React Native. It can now be installed and imported from 'react-native-viewpager' instead of 'react-native'. See https://github.com/react-native-community/react-native-viewpager
That's on a rebased version, because the first thing I did with the PR branch was rebase it. I imagine it corresponds to something that happened in one of the RN upgrades we've merged since this last revision (so, sorry again for the delay.)

But then I tried it un-rebased, and it worked!

There were a couple of rough edges I found in the behavior, but we can iterate on those. Specifically:

• I went and shared something from another app. I think Zulip was already running in the background, but not certain of that.
• The sharing screen appeared!
• Then a moment later it flickered and sort of redrew. Not sure what was happening underneath. This is a small polish issue, but also potentially a clue for diagnosing the thing I ran into later.
• I chose a stream and a topic, and hit send.
• Then... I'm not sure of the exact sequence here, so I guess this calls for attempting a repro and taking careful notes along the way. But:
  • IIRC, I saw the sharing screen again, as if there were two copies stacked on top of one another and I'd only completed the top one.
  • Then a thing I definitely saw was a red-screen, with an error message like "Map already consumed". (I think you've mentioned such an error before.)
  • I quit the app and relaunched, and all was well.

Separately:

• When choosing a topic, I found it hard to understand what was going on at first, as it felt like things were moving around a lot.
• Eventually I figured out it was topic autocompletion.
• I think the issue is basically that the autocomplete widget was appearing within the layout, and therefore causing the input box to jump around. Instead it should pop over everything else, so that it doesn't affect the layout and the input box doesn't jump around. (This is what we do with autocomplete in the main compose-box experience.)

[gnprice]

nit: missing final newline

[gnprice]

nit: missing final newline

[gnprice]

need to delete closing --> too -- as is, the XML doesn't parse, and so e.g. for me Gradle sync doesn't work (though the actual build seems to work fine! or at least tools/test android does)

[gnprice]

Let's make it String -- that seems more true to the intent of the method.

(I'm guessing that the Android Studio auto-conversion went for String?, which is natural because Java doesn't make the distinction. The cleanup to String can be in a followup commit -- it's fine and good for that sort of cleanup to make the Kotlin more idiomatic to be in a separate commit from the conversion.)

[gnprice]

Should just be "MainActivity", because it's global to this file so it's the natural thing that other logging calls in this file will use. Or leave it specific to sharing but name it something specific like SHARE_TAG.

[gnprice]

nit: open-brace at end of previous line, after -> (like in other examples)

[gnprice]

Ah, thanks for this cleanup! You may find 5542b01 informative for what that was about.

[gnprice]

In commit message: summary line is too long. Main solution is to drop the old or the new name (or both, and say something like "Fix misleading "...ActiveStream" name.")

[gnprice]

OK, merged the following commits!
bdce7d0 android: Refactor MainActivity to Kotlin.
c8b56e1 android [nfc]: Tighten return type on a converted-from-Java method.
f9597fa android: Move sharing logic to MainActivity.
e9ebc89 android [nfc]: Inline the sendEvent helper.
c13f889 android [nfc]: Cut a bit of redundant logging in share-receiving code.
d8be9dc android [nfc]: Push a try/catch into initializer expression.
64ca473 topic actions [nfc]: Rename fetchTopicsForStream.
b4692e3 api: Mark certain fields optional in sendMessage.

These correspond to the first N-1 commits of this PR, plus a few tiny commits of my own.

[gnprice]

nit: add items somewhere other than the end of the list, to avoid noise in the diff (because JSON doesn't permit a comma after the last item)

[gnprice]

Suggested change

	/**
	* The type of data that can be shared to this app from 3rd party apps - either
	* plain text or an image or a file.
	*/
	export type SharedData = SharedText | SharedImage | SharedFile;
	/**
	* The data we get when the user "shares" to Zulip from another app.
	*/
	export type SharedData = SharedText | SharedImage | SharedFile;

One thing I want to communicate in this jsdoc is that this is the data we got from some act of sharing. The "can be shared" sounds like it's something that might in the future be shared, or might not.

One thing that helps tighten up the jsdoc -- makes it shorter, and perhaps also a bit easier to write -- is that the jsdoc on a type focuses on a value of the type. Like how PmConversationData just above is a "Summary of a PM conversation", and Identity is "An identity belonging to this user in some Zulip org", etc. So no need for "The type of …"; and that perhaps helps with the motivation for "can be", too.

The "either plain text or …" is an implementation detail and so can just be left out. That also avoids a risk of another alternative being added later and the jsdoc not getting updated 😉

[gnprice]

Sorry, this was ambiguous -- I meant that this function handleShareReceived could be inlined 🙂

This is fine too, though; IIRC I actually waffled on whether I wanted to inline handleShareReceived, or inline handleReceivedData at its two call sites including this one.

[gnprice]

Observations from playing with the feature:

• When choosing recipients, there's a "back" button in the top left. But instead of taking me back where I got there from, it seems to discard the whole sharing interaction -- I land at the main screen with the unreads list.
• Hmm, then when I go switch back to another app and try sharing again (with Zulip still running in the background)... it brings Zulip to the foreground again but doesn't show the sharing interaction at all. Not sure what's up.
• I quit Zulip and tried sharing again, and then the sharing screen came up again as expected.

Ah OK, and I saw again a version of an issue I mentioned in my previous comment. This time, taking notes before proceeding through it:

• I'd successfully gotten to the sharing screen, chosen a PM recipient, and hit "Send".
• That promptly switched me back to the app I came from, and showed a toast saying "Sending Message..." and then one saying "Message sent".
• I switched apps back to Zulip -- curious to look at the conversation where I'd sent the message 🙂
• I got the sharing screen! With the same message text -- as if it were handling the previous share a second time.
• I hit "Cancel"... and Zulip disappeared, huh. Opening the app switcher showed it was still running, though.
• Switched back to it... and got the sharing screen again.
• Again hit "Cancel", and Zulip again disappeared; again switched back to it, and again got the sharing screen.
  • This time, I noticed that actually the main screen flashed very briefly on the screen first -- I caught the words "No unread messages".
• Again hit "Cancel" -- this time, edited the message text first. Again Zulip disappeared, again switched back, and again got the sharing screen.
  • The message text was the original version from the share, not the version with the edit I'd made. So, confirmed (as I'd figured) that it's getting the information from the share and not from the state of the old component.

Also some code comments below. Not done reading the code, but wanted to get these notes in.

[gnprice]

Mentioned this in a previous comment (#4124 (comment)), and it's small but important: $FlowFixMe goes at the start of the comment line, to help it stand out when scanning the code by eye.

[gnprice]

Also from that comment:

I think the key information I want in the explanation comes down to: why haven't we just already fixed it? So with "Requires additional props that we don't need", the natural next thing is: OK, let's make whatever's requiring those stop requiring them, then. But then the answer to that is: ah but it's over in the react-navigation library (or anyway its types, specified in its libdef). That doesn't mean we can't fix it -- it's just code, we can change it -- but it's more work. So identifying a specific third-party place as the location of the problem is a common kind of explanation for these.

So seeing this, I still have that question "OK, let's make whatever's requiring those stop requiring them, then -- why didn't we just do that?"

And here's what I wrote for similar errors elsewhere, which answers that question:

  // $FlowFixMe react-navigation types :-/ -- see a36814e80

[gnprice]

In general, better to leave a blank line than to just fill it in with new code -- they're often there for a reason, as separators.

This one separates handleInitialNotification, which looks for something that's already happened and acts on it, from the rest of this method which sets up various handlers to listen for things that happen in the future. So handleInitialShare should go above it, because it also looks for something that's already happened and acts on that.

[gnprice]

Huh! That is peculiar. It leaves me still curious what's different between this and notifications that would make it needed here and not there.

I don't see this mentioned in the group PM thread with you and me and Chris. Probably #mobile-team would be the best place for any further debugging, in any case.

One possibility I wonder about is that perhaps it's related to how in a previous version of this PR there was a separate activity. Now that that's gone, the way the rest of the system works is more like it does for notifications.

[gnprice]

There's quite a bit of nontrivial logic appearing twice, between here and ShareToStream. Rather than duplicate it, let's pull that out into a common function -- perhaps in a file like src/sharing/send.js.

Then this handleSend function will just

• deal with the component's state and props -- like collecting sharedData from the nav prop
• do the bits of logic that actually do differ between these two versions, like computing the message's type, to, and content

[gnprice]

OK, I am merging this! Leaving out just two things:

• The uncommenting of the bit of the manifest that would actually enable the feature.

  (We'll definitely need to fix that crash/red-screen mentioned above, at least, before taking that step.)

• The bit in the rehydrate handler of navReducer.

  I'm still puzzled about whether it's really necessary -- and as described in my last few comments above (not the code-inline comments but the top-level ones), there are some bits of odd nav-related behavior in the UX, and I wonder if one or more of those are related.

  But in particular it broke tests 🙂 so I wasn't going to merge it in that state. We can always add it later along with updates to those tests.

This PR thread has quite a lot in it, so hopefully merging this will give us a clean baseline for another PR, or handful of PRs, to complete the work.

Note that a number of the code comments above from this round of review still apply to the code I'm merging. So I'll want to see those taken care of in a followup PR before we consider this feature complete.

Thanks again @agrawal-d for all your work on this, and looking forward to seeing it across the finish line!