[android] Handle receiving shares from other apps #4196

agrawal-d · 2020-07-20T10:54:25Z

Fixes: #117

Continuation of work done in #4124

Resolves several issues pointed out by @gnprice in #4124.

Unresolved issues:

~~back button appears in user picker screen for 'Share to PM'.~~ (Fixed!)
autocomplete popups cause a lot of movement in the screen as they appear/disappear/resize.

gnprice

Thanks @agrawal-d for the swift revisions!

Code comments below. Now I'll go play with the UI at this version...

src/nav/__tests__/navReducer-test.js

src/sharing/SharingScreen.js

src/sharing/send.js

gnprice · 2020-07-21T02:59:32Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+    override fun onNewIntent(intent: Intent){
+        handleIntentForSharing(intent)


Ah, well spotted! Thanks for the docs reference, too.

gnprice · 2020-07-21T03:03:22Z

src/nav/navReducer.js

+  // Dont switch to main UI if sharing screen is on.
+  if (state.routes.find(route => route.routeName === 'sharing')) {
+    return state;
+  }
+


In the chat thread you started for this question, I think I figured out the mystery of what's happening here (and why notifications don't have the same trouble), and found a cleaner solution. (And more complete! This one still leaves it the case that the sharing UI will briefly try to render in a situation where the state it has is bogus.)

android/app/src/main/java/com/zulipmobile/sharing/SharingModule.kt

gnprice · 2020-07-21T03:23:13Z

android/app/src/main/java/com/zulipmobile/sharing/SharingModule.kt

+        try{
+            promise.resolve(initialSharedData)
+        }catch(e:Exception){
+            promise.resolve(null)
+        }


Hmmm.

I'm skeptical when I see a pattern like this -- it looks like trying to shut up an error without understanding it.

In particular this description in the commit message seems like a contradiction:

Catching this exception and returning null fixes this error. This
does not change the behaviour of this function because the error
only happens when the map is re-read - and we never actually re-read
the initialSharedData map, we only ever read it once.

Doesn't the fact that we're seeing the error appear indicate that we do actually re-read the Map in question?

Or another way of looking at basically the same point: if the behavior of the function isn't affected by this change, then why are we making the change? It's certainly not simplifying or clarifying the code. If this change causes those red-screen errors with "Map already consumed" to stop appearing, then that's very much a change in the behavior of the function.

gnprice · 2020-07-21T03:30:39Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+        // Intent is reused after quitting, skip it.
+        if((getIntent().flags and Intent.FLAG_ACTIVITY_LAUNCHED_FROM_HISTORY)!=0){
+            return;
+        }


Huh, that is wacky that it behaves that way! Good find.

Took me several hours to figure this out, I tried a lot of different things 😰

android/app/src/main/java/com/zulipmobile/MainActivity.kt

gnprice · 2020-07-21T03:50:30Z

Observations from playing with the UI:

I left the Zulip app in the background, went and opened another app, and hit "share" from there and chose Zulip.
The sharing screen came up. A second or two later, I saw this happen again (like at [android] Handle receiving shares from other apps #4124 (review)):

Then a moment later it flickered and sort of redrew. Not sure what was happening underneath.

Specifically, now that I've seen this a few times: it looked like a new copy of the sharing screen was appearing and animating in, in a way that I associate with navigating to a screen. So it was as if I was on the sharing screen and then navigating to another copy of the sharing screen.
I chose PMs, chose a recipient, and sent.
This time, the app didn't close! Instead it took me back to the screen I'd been on (a particular stream narrow) when I put the app in the background in the first place. The toasts appeared in the usual sequence, over that screen.
- Seems like ideally we would navigate to the conversation I shared to, instead.
I hit the home button (this is Android 8 O, on an emulated Pixel 2) to get back to the launcher, and then hit the "recent apps" button and chose Zulip.
- The app came back to foreground showing the same screen it was on! So, that's good.
Then I hit the home button again, and this time hit the app's icon in the launcher.
- ... This time, it came back to the foreground showing that same screen... but then a fraction of a second later, took me back to the main "unreads" screen. Hmm.
- I tried a few times repeating this. It reproes 100% of the time, the first time I use the launcher icon after having shared something. (If I then use the launcher icon again, it behaves normally. And I can use the recent-apps screen instead of the launcher any number of times, and it behaves normally.)
- This calls for some further debugging, I think. What's causing this navigation back to the main screen?

gnprice · 2020-07-21T04:01:36Z

src/sharing/ShareToPm.js

@@ -169,7 +153,7 @@ class ShareToPm extends React.Component<Props, State> {

    if (choosingRecipients) {
      return (
-        <Modal>
+        <Modal onRequestClose={this.handleModalClose}>


sharing: Handle hardware back button when selecting PM recipients. Use modal's 'onRequestClose' to hide the user selection screen when the back button is pressed while choosing recipients in the share to PM tab.

Hmm, did this not work in the previous version? Certainly good to fix so that this works.

I'm less happy about relying completely on the back button here. It's acceptable as a temporary workaround because we only support this flow in the first place on Android -- but that won't work when in the future we offer sharing on iOS as well. And in the UX of this even on Android: everywhere else in the app (including other uses of this user-picker widget!), when it's possible and natural to go back, we have a "back" icon at the top left of the screen that offers that. It feels like a glitch for that to be missing here.

gnprice · 2020-07-22T01:19:02Z

Just merged 5 of these commits!
6bccf4c sharing: Switch to 'react-navigation-tabs' for creating tab bars.
38aa992 nav reducer tests: Use an accurate initial state.
63fee0f sharing: De-duplicate the 'handleSend()' function.
a914241 android: Handle new intents in 'onNewIntent' when activity is resumed.
6ca95f1 sharing: Handle hardware back button when selecting PM recipients.

Looking closer at the rest...

gnprice · 2020-07-22T04:02:11Z

OK, after some investigation in chat, ended up sending #4204 which I think will resolve at least a bit of the oddness that we've both been seeing on versions of this branch. Want to try it on top of that and see?

For anything that that branch does fix, another way to avoid the issue is to not rely on react-native run-android to start the app. Instead, perhaps after using that command to build and install the app: quit it, and start it from the launcher (or don't start it at all, to see how the sharing feature behaves when the app isn't already running.)

agrawal-d · 2020-07-22T07:48:44Z

Thanks, @gnprice!

I posted a comment on your PR thread: #4204 (comment)

gnprice · 2020-07-22T21:07:56Z

(Rebased, after having merged #4204 .)

gnprice · 2020-07-22T21:26:12Z

I reproduce the issue you describe at that comment. Let's debug in chat.

gnprice · 2020-07-22T22:13:50Z

OK, merged three more commits:
7424151 sharing: Involve a thunk action in navigateToSharing so it's buffered.
70e9ad9 android: Skip sharing intent when it's reused after quitting.
21a6713 modal search nav bar: Accept prop canGoBack.

(One of them comes from squashing two of the commits from the branch.)

That leaves just two others outstanding:
1ace60a android: Enable receiving 'shares' from other apps.
6ac549c sharing: Reset 'initialSharedData' after sending in SharingModule.

I'd really like to fix the double-initialization issue before turning this on for everyone -- i.e. the issue where we can end up with two different Android activities, and two different React component trees, in the same process and talking to the same JS engine and the same Redux store. Made some debugging progress in that chat thread linked above, and suggested a possible solution (but you should confirm whether it seems to actually work 😉 ).

gnprice

Comments below on the remaining commit, other than the one enabling the feature.

gnprice · 2020-07-22T22:14:07Z

android/app/src/main/java/com/zulipmobile/sharing/SharingModule.kt

@@ -1,6 +1,7 @@
 package com.zulipmobile.sharing

 import com.facebook.react.bridge.*
+import java.lang.Exception


now redundant

gnprice · 2020-07-22T22:15:08Z

android/app/src/main/java/com/zulipmobile/sharing/SharingModule.kt

    fun getInitialSharedContent(promise: Promise) {
        promise.resolve(initialSharedData)
+        initialSharedData = null
    }


This is certainly plausible code now. I wonder if it's clearly the behavior we really want, though. Why are we ending up trying twice to get the initial shared data? What's the situation where that happens?

agrawal-d · 2020-07-23T14:27:01Z

Thanks, @gnprice! I've updated the PR.

gnprice · 2020-07-23T20:43:05Z

This one looks like some kind of rebase error, perhaps -- the description and contents don't match:

commit 4394d6c6d745610c1bb79fb86fe844aedbfa7885
Author: Divyanshu Agrawal <agrawal.divyanshu@outlook.com>
Date:   Mon Jul 20 12:39:33 2020 +0530

    sharing: Switch to 'react-navigation-tabs' for creating tab bars.
    
    Since the RN 0.60 upgrade, using `createMaterialTopTabNavigator`
    exported by 'react-navigation' causes an error - that
    'ViewPagerAndroid has been removed from React Native'. Switching to
    the one exported by 'react-navigation-tabs' fixes this.
---
 src/sharing/SharingScreen.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/sharing/SharingScreen.js b/src/sharing/SharingScreen.js
index e69303d76..12a74206c 100644
--- a/src/sharing/SharingScreen.js
+++ b/src/sharing/SharingScreen.js
@@ -71,7 +71,12 @@ class SharingScreen extends PureComponent<Props> {
     }
 
     return (
-      <Screen canGoBack={false} title="Share on Zulip" shouldShowLoadingBanner={false}>
+      <Screen
+        canGoBack={false}
+        title="Share on Zulip"
+        shouldShowLoadingBanner={false}
+        keyboardShouldPersistTaps="always"
+      >
         <SharingTopTabNavigator navigation={navigation} />
       </Screen>
     );

gnprice · 2020-07-23T22:46:03Z

For the "Exit the process when the activity is destroyed" commit, I'm concerned about collateral damage and don't want to make that change before better understanding what's happening in the problem we're trying to solve. I have some questions in chat to help you debug more fully.

Since the RN 0.60 upgrade, using `createMaterialTopTabNavigator` exported by 'react-navigation' causes an error - that 'ViewPagerAndroid has been removed from React Native'. Switching to the one exported by 'react-navigation-tabs' fixes this.

This feature was marked as experimental because of several small bugs. These have been fixed in the preceeding commits. So, remove the comment that disables it. Fixes: zulip#117

This fixes the 'double initialization' bug.

This resolves the 'Map already consumed' bug.

gnprice · 2021-10-18T23:03:37Z

This was superseded by #4514. Thanks again @agrawal-d for your work on this!

agrawal-d requested review from gnprice and chrisbobbe July 20, 2020 10:54

agrawal-d force-pushed the sharing-2 branch 8 times, most recently from 2f74afe to 6f0a971 Compare July 20, 2020 16:41

gnprice reviewed Jul 21, 2020

View reviewed changes

agrawal-d force-pushed the sharing-2 branch from 6f0a971 to 895d221 Compare July 21, 2020 09:27

agrawal-d removed the request for review from chrisbobbe July 21, 2020 13:50

gnprice force-pushed the sharing-2 branch from 895d221 to d69d4b2 Compare July 22, 2020 01:19

gnprice force-pushed the sharing-2 branch from d69d4b2 to 1ea1791 Compare July 22, 2020 21:07

gnprice force-pushed the sharing-2 branch from 1ea1791 to 6ac549c Compare July 22, 2020 22:09

gnprice reviewed Jul 22, 2020

View reviewed changes

agrawal-d requested a review from gnprice July 23, 2020 14:26

agrawal-d force-pushed the sharing-2 branch 2 times, most recently from 3d61180 to e662d9b Compare July 23, 2020 14:33

agrawal-d added 4 commits July 24, 2020 13:24

android: Enable receiving 'shares' from other apps.

a53f336

This feature was marked as experimental because of several small bugs. These have been fixed in the preceeding commits. So, remove the comment that disables it. Fixes: zulip#117

android: Clear reactNativeHost in onDestroy if it has an instance.

72fc6b7

This fixes the 'double initialization' bug.

sharing: Reset initialSharedData after sending the map to JS.

e9d5db1

This resolves the 'Map already consumed' bug.

agrawal-d force-pushed the sharing-2 branch from e662d9b to e9d5db1 Compare July 24, 2020 07:56

gnprice mentioned this pull request Mar 6, 2021

Handle sharing from other apps (Android) #117

Closed

AkashDhiman mentioned this pull request Mar 8, 2021

[android] share to Zulip from other apps [part 1] #4514

Merged

gnprice added the a-share-to Sharing photos/etc. into Zulip label Oct 18, 2021

gnprice closed this Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Handle receiving shares from other apps #4196

[android] Handle receiving shares from other apps #4196

agrawal-d commented Jul 20, 2020 •

edited

Loading

gnprice left a comment

gnprice Jul 21, 2020

gnprice Jul 21, 2020

gnprice Jul 21, 2020

gnprice Jul 21, 2020

agrawal-d Jul 21, 2020

gnprice commented Jul 21, 2020

gnprice Jul 21, 2020

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

agrawal-d commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice left a comment

gnprice Jul 22, 2020

gnprice Jul 22, 2020

agrawal-d commented Jul 23, 2020

gnprice commented Jul 23, 2020 •

edited

Loading

gnprice commented Jul 23, 2020

gnprice commented Oct 18, 2021

		override fun onNewIntent(intent: Intent){
		handleIntentForSharing(intent)

[android] Handle receiving shares from other apps #4196

[android] Handle receiving shares from other apps #4196

Conversation

agrawal-d commented Jul 20, 2020 • edited Loading

Unresolved issues:

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jul 21, 2020

Choose a reason for hiding this comment

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

agrawal-d commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice commented Jul 22, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jul 23, 2020

gnprice commented Jul 23, 2020 • edited Loading

gnprice commented Jul 23, 2020

gnprice commented Oct 18, 2021

agrawal-d commented Jul 20, 2020 •

edited

Loading

gnprice commented Jul 23, 2020 •

edited

Loading