[android] share to Zulip from other apps [part 1] #4514
Conversation
AkashDhiman
force-pushed
the
share-to-zulip
branch
5 times, most recently
from
3ad6d47 to
5dccc78
Compare
|
Regarding the double initialization issue: To identify this issue I was mainly looking for the log
(@gnprice adds: Akash mentioned at #4514 (comment) that this was observed originally on Android 10, and then also on Android 7.) |
AkashDhiman
force-pushed
the
share-to-zulip
branch
2 times, most recently
from
b91f125 to
91b117a
Compare
AkashDhiman
force-pushed
the
share-to-zulip
branch
2 times, most recently
from
9fe8de1 to
b0cc436
Compare
|
@gnprice please review this, and let me know what can be improved upon. |
There was a problem hiding this comment.
Thanks, @AkashDhiman! I appreciate your work on this difficult issue.
I haven't yet read through all of the commits, but I ran git rebase --exec 'tools/test' and saw that they all pass tests. I also haven't tested the new feature manually by running the app.
In addition to the more specific comments below, here are some broader thoughts:
- I see some commits that address some important-looking bugs—great! But we should try to get those fixes in before exposing the feature to users.
- As I understand it, we do that in the commit that uncomments lines in
android/app/src/main/AndroidManifest.xml. Is that right? I'm basing that on this part of 17f73d8: "To enable the feature as it currently exists, it's enough to uncomment the commented-out bit of the manifest". - If so, then that commit will introduce a feature that we know has major user-facing bugs. We want to avoid that. 🙂 This will make commits easier to review, and easier to think about reverting post-merge if that turns out to be necessary.
- Could just move that uncommenting commit to the tip of the branch, I think. (Though, enhancements that aren't major bugfixes are fine to put after that commit, like using a new PNG in the current tip commit.)
- As I understand it, we do that in the commit that uncomments lines in
- Just one of these commits should be marked with
Fixes: #117, not most/all of them. Probably the one that exposes the feature to users. - Please feel free to expand freely on the changes in the commits from the old PR that you've borrowed here. I've mentioned that some of those commits deserve more detail in their commit messages. 🙂 You could remove those commits and rewrite them from scratch, or add a line like
Co-authored-by: Chris Bobbe <[email protected]>(except with you instead of me) if you're leaving Divyanshu in the commits'authorfield.
src/sharing/SharingScreen.js
Outdated
| } | ||
|
|
||
| export default connect(state => ({ | ||
| export default connect((state) => ({ |
There was a problem hiding this comment.
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.
This commit looks out-of-place, hmm. Ah, I see Greg said something similar on the old PR: 670ac28#r457804303.
Probably just drop the commit.
There was a problem hiding this comment.
dropped this commit.
| super.onNewIntent(intent) | ||
| } | ||
|
|
||
| override fun onDestroy() { |
There was a problem hiding this comment.
What's the 'double initialization' bug? 🙂 Let's add to the commit message a summary of symptoms, diagnosis, and why this is the right solution.
There was a problem hiding this comment.
Before anything I have chosen to drop this commit and changes introduced by it. As to the reason for this - it is explained below
What's the 'double initialization' bug?
From what I could infer double initialization bug is a bug where another instance of the app gets started while the previous one is running, this leads to sharing of the redux store that tends to introduce inconsistencies.
More discussion on this can be found at CZO
Possible steps of reproduction
These are Listed here
I can't seem to run that branch locally (unrelated react native error) so I couldn't verify it there, but I did test it out on the rebased version of the app, at the point where I introduced initialParams f0428a7
I tried the steps of repro with the changes reverted and with the changes present. The symptoms reported were not noticed in the user interface.
However I did notice in debug console two symptoms that do correspond to this issue, occurrence of action persist/REHYDRATE as well as the log Running "ZulipMobile" with {"rootTag":21} two times.
These occurred when I reverted the changes introduced in 'fix re initialization issue` commit, and disappeared when the changes were present.
This was the reason I kept this commit before as it indicated double initialization.
Why this specific change was supposed to fix this issue?
It was discussed in CZO that when the app were quit, it didn't fully quit, it was discovered that react native host was still present and on starting the app again in the way the repro steps demanded, the react native host would mount another react root view, which would trigger rehydrate and we would have a double initialization issue.
Except this may not be the complete story. When an app quits android doesn't kill it's process entirely, this is true for every app be it react app or not (possibly for optimization purposes for when the app is restarted). This is verifiable as the profiler still indicates the app process as alive even after the app's onDestroy method is triggered (Up until the point when the app is available in recent apps view.)
The Symptoms that we discover then are not because our app is reinitialized, while a previous instance is running; but instead when we do quit the app (and whenever it's onDestroy is called), it also destroys the react root view that houses our app without killing the react native host so then when we do bring the app to foreground once again a new react root view is mounted.
it is certain rootview is destroyed when onDestroy is called, I discovered this by following super.onDestroy by pressing CTRL+B in android studio, and it never kills react native host, but does unmount react root view. This was also discussed in CZO
This behavior is also in agreement with other react apps. react native host doesn't die but the root view is unmounted. So the double logs we were seeing is because root view was mounted before (it got unmounted without any log) and then mounts again. The dispatch to rehydrate happens after as well and we report it as having seen rehydrate twice, but at any one moment we only have 1 root view and 1 rehydrate dispatch.
So what can be the cause of the user interface symptoms discovered in the reproduction step?
I suspect it had something to do with implementation of onNewIntent. Checking in the dates of the repro comment and the commit editing onNewIntent. The comment comes before.
Why do I suspect it to be onNewIntent? because the implementation at the time of writing that commit was not the same as what it is today, and the repro steps trigger onNewIntent method. Although It seems a bit hard to verify, I will have to manually revert changes.
In any case the symptoms of double console log are also present in the current master as well - if one quits the app by pressing back button, and restarts the app from recent app, another rootView gets mounted while we can see a Running "ZulipMobile" with {"rootTag":21} before.
| @ReactMethod | ||
| fun getInitialSharedContent(promise: Promise) { | ||
| promise.resolve(initialSharedData) | ||
| initialSharedData = null |
There was a problem hiding this comment.
(Same comment as I made on the previous commit: let's add some useful detail to the commit message. 🙂)
There was a problem hiding this comment.
Added a much more verbose commit message regarding this, I am also a bit skeptical about the placement of this code and have written about an alternative in the commit message itself, please share your views on it.
There was a problem hiding this comment.
commit ded9b49
sharing: Reset initialSharedData after sending the map to JS.
This resolves the 'Map already consumed' bug.
(and much more detail, which is excellent, thank you!)
So effectively this solution makes getInitialSharedContent no longer a getter, but instead something that consumes its input. If we call it twice, the second time will get nothing, because the first call already consumed it.
I think that's probably the behavior we actually want in any case, even if there weren't a crash involved. When you take an action to share something, that's a single request to do so -- we don't want to be later picking it up from the middle and trying to act on it again.
Framed that way, I think this is the perfect place to implement that behavior -- right where the data is being handed off to the JS that's asked for it, we cross it off as having been consumed.
I have two remarks from there:
-
Because this method now has an important side effect, it should have a name that sounds less like a plain old getter. Just replacing "get" with "read" might be good -- that makes the name similar to reading data from an input stream, which has exactly the same kind of side effect.
-
There's another place in the existing code where we handle some data that has a lot in common with this structurally: when you start the app by opening a notification. That code currently has a different behavior:
@ReactMethod public void getInitialNotification(Promise promise) { if (null == initialNotification) { promise.resolve(null); } else { promise.resolve(Arguments.fromBundle(initialNotification)); } }
So if we call
getInitialNotificationseveral times, we get the same data each time. I think theArguments.fromBundlethere basically avoids this "Map already consumed" error by creating a new map on each call, as a copy ofinitialNotification.Could you try applying the same repro recipe with opening a notification, instead of sharing text? I am curious what the behavior is. From what you've discovered, I'm guessing that on the last step when you try starting the app from recent apps, we'll end up re-navigating to the conversation where the notification was -- which is a bug.
If we do have that bug in notifications, then we should fix it by probably doing the same thing as this commit does, and that will also be helpful confirmation that this commit is the right direction for fixing the sharing issue. (Or if we turn out not to, then that will be interesting and I'll be curious to understand better what's different between the two cases.)
Oh and I guess also a small comment on the commit message:
But it seems that react native discovers that the data in
initialSharedIntent is outdated and hence is unsafe to use thereby
giving us the exception and the bug.
I think it's not exactly that it's "outdated", so much as it is that the memory underlying that WritableMap has been re-used for handing that data over to the JS code.
There was a problem hiding this comment.
Could you try applying the same repro recipe with opening a notification, instead of sharing text?
I did it and we end up re navigating to the conversation where the notification pointed.
Setting up initialNotification to null fixes it.
I should probably make a different PR to fix it.
There was a problem hiding this comment.
Sounds good -- please do! You might first file a quick issue for it.
| android:host="login" | ||
| android:scheme="zulip" /> | ||
| </intent-filter> | ||
| <!-- Disabled while the feature is experimental. See #117 and #4124. |
There was a problem hiding this comment.
Interesting; if I understand correctly, this is the "big reveal" or the "moment of truth", where we take the feature's implementation and finally make it available to the user! 🎉
Enhancements aside (those are probably fine to do in later commits), can we expect that this change is OK to make at this point in the branch? In particular, there aren't any known bugs, crashes, test failures (well I know there aren't any test failures 🙂), etc., at this point?
One reason I ask is that I see several commits after this. So I'll be keeping an eye out for bug-fixes in those commits to see if they might better go before this one.
Reading...
There was a problem hiding this comment.
I have moved this commit to the point in PR where there are no longer any fatal issues (issues that would crash the app). But it's still at a point after which the feature is enhanced a lot (example: addition of multiple file share). So let me know if it is placed at the correct position now or should it be preferred at the end of those enhancements.
I have also made sure to mark issue resolved in only one commit (this commit). My thought process was that the entire PR was aimed towards fixing the issue, and each commit solved part of the issue, but I understand your point it is better to mark it in the place where we release the feature.
| } | ||
|
|
||
| export default connect((state) => ({ | ||
| export default connect(state => ({ |
There was a problem hiding this comment.
nit: This pure-formatting fix belongs in its own small NFC commit; it's not interesting alongside the main work being done in this commit.
src/sharing/send.js
Outdated
| if (sharedData.type === 'image' || sharedData.type === 'file') { | ||
| const url = sharedData.type === 'image' ? sharedData.sharedImageUrl : sharedData.sharedFileUrl; | ||
| const fileName = url.split('/').pop(); | ||
| let fileName; |
There was a problem hiding this comment.
sharing: Set appropriate filename of shared content.
previously filename was the last element of sharedDataUrl.split('/'),
this did not always correspond to the actual filename, sometimes
this element did not contain a file extension causing the uploaded
data to be stored as a binary file. this made shared images fail to
render and on downloading the shared content it was downloaded as
a binary file, this commit gives the files generic names with
appropriate extensions.
Fixes: #117
(Same thing about the Fixes: #117 line. Which commit fixes the issue? 🙂)
It seems like you've found a bug that makes file-sharing fail, and this is a fix for it! Glad to have a fix, but best for it to go before the exciting commit that exposes the feature to users, if I understand correctly that this is possible.
sharedDataUrl.split('/')
I don't find the identifier sharedDataUrl anywhere previous to this commit (or after it). I think you meant sharedImageUrl and sharedFileUrl (right?), but this makes it a bit harder to follow. 🙂
There was a problem hiding this comment.
Can you help me understand why the new way to choose fileName is OK and what we want? Some concerns are,
- It can now give the same name for different files, even if those files' original names were different. What was once
cat1.jpegandcat2.jpegwould both get namedimage.jpeg. Is that right? That sounds like something we should be careful about. - It's not obvious to me that the second part of a MIME type will be an appropriate file extension.
- If the MIME type is
text/plain, it looks like the new code will give usfile.plainforfileName. - If the MIME type is
image/svg+xml, it looks like the new code will give usimage.svg+xmlforfileName(from a look at MDN). - (Those don't look right, and I'm sure we could find others that also don't look right.)
- If the MIME type is
There was a problem hiding this comment.
[...] sometimes
this element did not contain a file extension causing the uploaded
data to be stored as a binary file
Ah, and: could this be solved at a different layer? I see that we're passing fileName to api.uploadFile. One thing that api.uploadFile does is call getMimeTypeFromFileExtension to get the MIME type. But that step seems unnecessary for us here, since we already know what the MIME type is. Could the bug you're seeing have been caused by getMimeTypeFromFileExtension giving the wrong MIME type? That sounds plausible to me.
src/sharing/send.js
Outdated
| const { content } = sharedData; | ||
| for (let i = 0; i < content.length; i++) { | ||
| const name = `shared-content-${i + 1}`; | ||
| const ext = content[i].type.split('/').pop(); |
There was a problem hiding this comment.
There's a lot going on in this commit! I'm a little lost when examining each individual change here.
As you've seen with at least one bug-fixing commit I've read so far, it's an area that's pretty prone to bugs, so it'll be vital—as much as possible without introducing regressions or test failures—to isolate the individual changes into their own commits and explain each one clearly, both to help focus review and to maintain a clean project history. 🙂
I'll take a closer look at more individual changes later, but one looks interesting to me so far: after this change, the .type property has a different shape (and meaning?) from the nearby .type property before this change. That one used to take a literal "image", "text", or "file". Now (if I've understood correctly), it's computed with cr.getType(). That seems like a change that should be reasonably separable from everything else going on here; is that right? Probably there are more, too.
| import android.content.Intent | ||
| import android.content.ClipData | ||
| import android.provider.MediaStore | ||
| import android.content.ContentResolver |
There was a problem hiding this comment.
sharing: Set file name to actual name of the file.
Previously files shared to zulip were renamed to have generic file
name, this made it difficult to identify shared files. Now the name
of shared files aren't altered.
Fixes: #117
Ah—so this reverts the change earlier in the PR where we started using generic names? Let's change that earlier commit so that we don't use generic names ever, then, since we don't intend to keep that behavior.
There was a problem hiding this comment.
Changed according to this suggestion
| if ((getIntent().flags and Intent.FLAG_ACTIVITY_LAUNCHED_FROM_HISTORY) != 0) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
sharing: Fix bug when app is launched from history.
this was required because of the following bug:
1. share from any app to zulip.
2. cancel the share.
3. return back to zulip by selecting it from minimized apps.
4. now go back to the previous app used to share to zulip.
5. share something to zulip again.
6. the sharing screen will not be visible.
intended behavior: Sharing screen should be visible in step 6
Fixes: #117
Could you talk in the commit message about the diagnosis that led to the change in this commit, and why the particular change is the right way to respond to that diagnosis? So far, I just see a description of the bug's symptoms.
(This also seems good to go earlier in the branch, before we expose the feature to users.)
There was a problem hiding this comment.
made it more verbose in the commit message.
AkashDhiman
force-pushed
the
share-to-zulip
branch
from
b0cc436 to
8299a05
Compare
|
Sorry for the delay, I was not in the condition to work. I have made changes that the review requested, tried to make each commit more verbose and the
@chrisbobbe this PR is ready for a re-review. Of particular attention is this comment I made since it concerns a big decision, and would really like some input on this while you review it. Thankfully it is independent of other commits and can be safely be examined outside of other commits in this PR. |
AkashDhiman
force-pushed
the
share-to-zulip
branch
from
8299a05 to
dbb1e90
Compare
|
Thanks @AkashDhiman ! Like @chrisbobbe said, this is a tough issue, and it's great to see all this progress on it. I especially appreciate the detailed new commit messages on these bugfixes. Reading through now, and commenting on the relevant threads above. |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
OK, it's getting late here and I should set this down for right now. 🙂 I've read through these commits:
ded9b49 sharing: Reset initialSharedData after sending the map to JS.
ee02551 sharing: Provide params to Tab.Screen, as initialParams.
7c07ddc android: Fix issue where another instance of app would start on sharing.
3daa381 android: Enable receiving 'shares' from other apps.
caf2c69 sharing: Fix bug when app is launched from history.
e0b0b41 sharing: Make the stream/topic input list scrollable.
e6bcb0e sharing: Capture error via sentry if sending shared data fails.
and I am excited to get these fixes in! And thanks again for all the detailed explanations added in this revision.
Comments below, and others in threads above:
#4514 (comment)
#4514 (comment)
I haven't yet read the remaining commits:
19e542c sharing: Make the app not quit after share to zulip concludes.
9c1524a sharing: Set appropriate filename of shared content.
0ef352b sharing: Add functionality to share multiple files to zulip android.
0100150 sharing: Display file name under content preview.
25506d4 sharing: Show preview for non image files.
dbb1e90 sharing: Enable users to delete attachments before sending.
Also I don't have a firm opinion yet on whether the "enable" commit should go in at its current spot in the branch; but at a quick glance some of the later fixes look like they're pretty significant user-visible issues too, which would be a reason to put them before the "enable" commit.
| <activity | ||
| android:name=".HandleSendIntent" | ||
| android:label="@string/app_name" | ||
| android:launchMode="standard" | ||
| > |
There was a problem hiding this comment.
Then just when I thought the problem is solved I noticed that when content is shared from the app
Files by Googlezulip's activity started within the task holdingFiles by Googleapp. Even if zulip is in background, its main activity loads on top ofFiles by Googleand I do see a second"Running with {"rootTag": # }"andaction persist/REHYDRATE. I can even see the two instances of the app running, one previously present in recent Task, and one newly spawned within the app that called the share intent.
Very odd! The Android docs on launchMode seem to really clearly say that shouldn't happen:
https://developer.android.com/guide/topics/manifest/activity-element#lmode
"
singleTask" and "singleInstance" activities can only begin a task. They are always at the root of the activity stack. Moreover, the device can hold only one instance of the activity at a time — only one such task.
One detail for reproducibility: what version of Android did you observe this on?
(I think this shouldn't differ by Android version -- I don't think the documented meaning of that manifest attribute has changed in a long time. But if it's behaving in a surprising way, then the Android version is one thing that's likely to affect it.)
To deduce as to why this happened I created dummy app in android whose only purpose was to send intent and discovered the following: […]
- if the intent is shared via calling the
startActivityForResultmethod:
- If zulip is in background, it doesn't come to foreground, it remains in background and a new zulip MainActivity is created within the dummy app.
- If zulip is not in background, It doesn't get started in a new task separate from the calling app, instead the same thing happens - a new zulip MainActivity is created within the dummy app. […]
- I checked how other apps function in similar scenario there are mainly two ways apps deal with sharing:
- They start within the calling app and if share is successful they exit and redirect the user to their previous task within the recent activity (if they had any) or spawn themself in a new task separate from calling app.
- They immediately redirect to their previous task (if they had any) or spawns itself in a new task, never do they open in another app even if they are called from
startActivityForResult. This is probably the result that we require.
Fascinating. Thanks for that debugging and these detailed notes, and I agree with your conclusion.
Please add a link in the commit message pointing to #4514 (comment) where you wrote those detailed notes down -- those were very helpful for me to understand what this commit is doing.
There was a problem hiding this comment.
One detail for reproducibility: what version of Android did you observe this on?
Android 10. Although I just tested out Android 7 and it behaves like this as well.
There was a problem hiding this comment.
Android 10. Although I just tested out Android 7 and it behaves like this as well.
Cool, thanks. I've also just taken the liberty of adding a line to your comment at #4514 (comment) with that information.
| return ( | ||
| <Popup> | ||
| <FlatList | ||
| nestedScrollEnabled |
There was a problem hiding this comment.
Huh interesting. Are you finding that this makes a difference in the behavior you see?
The docs say this defaults to true, so passing it shouldn't be needed.
They also a bit concerningly say:
Enables nested scrolling for Android API level 21+.
which makes me worry about whether the issue you were seeing will be there on iOS, when we add sharing support there (which I'd expect to use just about exactly the same JS code.) OTOH reportedly it works on iOS anyway:
https://stackoverflow.com/questions/59828595/react-native-what-does-nestedscrollenabled-do
On the other other hand, the people on that thread found that it worked on Android already without the flag, so that's different from what it sounds like you were seeing.
There was a problem hiding this comment.
Strangely yes, the change in this commit does make a difference. I have added it in 3 places and all of them are relevant for scrolling to work properly (scrolling of stream selection popup and topic selection popup) inside the sharing ui.
There was a problem hiding this comment.
So be it, then. Thanks!
I added a few words about this in this commit message, including a link to this thread.
Are you referring to the decision to drop 72fc6b7 (permalink 72fc6b7 ) from the branch? (I.e., the commit in #4196 that was meant to fix the "double initialization" issue.) As discussed at #4514 (comment) , I find your investigation of the issue convincing and I think your fix for it makes sense. Given that investigation, if you're no longer seeing the symptoms after that fix, then I'm perfectly happy not including any other changes meant to fix it. PS -- A longstanding GitHub bug is that links like But there is a workaround: just delete from the URL the reference to the PR (and also the "s" from "commits"), to make |
AkashDhiman
force-pushed
the
share-to-zulip
branch
from
dbb1e90 to
7a0058a
Compare
This would occur whenever zulip was started by another Activity using startActivityForResult. Ideally with launchMode="singleTask" zulip should always launch in a seperate task, this however is not the case, when it is started via startActivityForResult by another app. With startActivityForResult another instance of zulip would be initialized inside the calling app, regardless of the fact that zulip was in background or not. This caused problems especially for share to zulip intents and this commit addresses those cases. Now SEND intent is not captured by MainActivity, a seperate native activity is created to capture them and it redirects the intents unchanged to MainActivity by calling startActivity, hence always ensuring that zulip is launched as a seperate task. (If zulip was already running and in background, it will be used instead of creating a new task.) Details on how I came to this conclusion: - zulip#4514 (comment)
AkashDhiman
force-pushed
the
share-to-zulip
branch
from
7a0058a to
ae3989e
Compare
This would occur whenever zulip was started by another Activity using startActivityForResult. Ideally with launchMode="singleTask" zulip should always launch in a seperate task, this however is not the case, when it is started via startActivityForResult by another app. With startActivityForResult another instance of zulip would be initialized inside the calling app, regardless of the fact that zulip was in background or not. This caused problems especially for share to zulip intents and this commit addresses those cases. Now SEND intent is not captured by MainActivity, a seperate native activity is created to capture them and it redirects the intents unchanged to MainActivity by calling startActivity, hence always ensuring that zulip is launched as a seperate task. (If zulip was already running and in background, it will be used instead of creating a new task.) Details on how I came to this conclusion: - zulip#4514 (comment)
AkashDhiman
force-pushed
the
share-to-zulip
branch
from
ae3989e to
fc7612c
Compare
Thanks for this, it always bugged me.
Yes, but the "double initialization" that commit was meant to fix was different than what I discovered in #4514 (comment). And for whatever reason I wasn't able to replicate it (even before my fix) so I dropped it. (I have explained about it more at #4514 (comment)). In any case the solution for the "double init" issue at 5bed0a3 (introduced in this PR) should fix any such related issues concerning share to zulip flow. @gnprice I have made the changes you requested, I think the PR is ready for another review. |
Expanded from a couple of PR comments I made recently: #4514 (comment) #4590 (comment)
This resolves the 'Map already consumed' bug. `Map already Consumed` is a fatal JS exception that occurs when react bridge gives `ObjectAlreadyConsumedException`. This exception according to the react-native documentation [1] at the time of writing this commit is given when: "caller attempts to modify or use a `WritableNativeArray` or `WritableNativeMap` after it has already been added to a parent array or map. This is unsafe since we reuse the native memory so the underlying array/map is no longer valid." One possible way to reproduce this issue is: - revert changes made in this commit (comment out line `initialSharedData = null` in `android/**/sharing/SharingModule.kt`) - start the app using `tools/run-android` - kill the app - start another app and share text to zulip (It is important that zulip starts using share intent, hence we do this step. It is also important to share text, data with content doesn't leave the app in recent apps view (discussed in next step), share playstore links as an example.) - cancle the share and quit zulip (use back presses if necessary), make sure to "quit" the app and not "kill" the app. - notice the app is still in the recent apps view (accessible by long pressing home) - start the app from recent apps. - map already consumed should popup. In this scenario it is assumed that when we backpress to quit zulip, android system does not flush the entire memory of the app (so that it can boot up faster, which is also noticable here). This also involves the static variable `initialSharedIntent` we set when the app was started previously using share intent. But it seems that react native discovers that the data in initialSharedIntent is reused and hence is unsafe to use thereby giving us the exception and the bug. The change introduced in this commit will fix any `map already consumed` issue concerning `initialSharedIntent`, since we reset its value just after its use. A more specific place to reset initialSharedData is within `MainActivity.kt` inside condition that checks for `FLAG_ACTIVITY_LAUNCHED_FROM_HISTORY`. This will only resolve the steps of reproduction discussed above, but the error may pop up somewhere else. [1] https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/ObjectAlreadyConsumedException.java Commit message authored-by: Akash Dhiman <[email protected]>
The app was crashing on mounting ShareToStream, with error suggesting `props.route.params` is undefined. It seems that no params were provided to the Tab.Screen. To fix this we now use initialParams. If no `params` are specified `initialParams` take its place. It is also shallow merged with any params we do pass. more details: https://reactnavigation.org/docs/params/#initial-params Possibly the 'blocker bug' at: 17f73d8
This would occur whenever zulip was started by another Activity using startActivityForResult. Ideally with launchMode="singleTask" zulip should always launch in a seperate task, this however is not the case, when it is started via startActivityForResult by another app. With startActivityForResult another instance of zulip would be initialized inside the calling app, regardless of the fact that zulip was in background or not. This caused problems especially for share to zulip intents and this commit addresses those cases. Now SEND intent is not captured by MainActivity, a seperate native activity is created to capture them and it redirects the intents unchanged to MainActivity by calling startActivity, hence always ensuring that zulip is launched as a seperate task. (If zulip was already running and in background, it will be used instead of creating a new task.) Details on how I came to this conclusion: - zulip#4514 (comment)
This was required because of the following bug: 1. share from any app to zulip. 2. cancel the share. 3. return back to zulip by selecting it from minimized apps. 4. now go back to the previous app used to share to zulip. 5. share something to zulip again. 6. the sharing screen will not be visible. Intended behavior: Sharing screen should be visible in step 6 This bug was possibly introduced in: 70e9ad9 The condition checking `FLAG_ACTIVITY_LAUNCHED_FROM_HISTORY` in the code that commit introduces, checks MainActivity's intent (intent the app was originally started with). Instead it should check the intent handleSend was called with. This caused the intents coming via `onNewIntent` to be ignored.
This conditional existed to fix a bug where a quit app, that originally was started with SEND_INTENT, when restarted from the recent app drawer reused the SEND_INTENT (thereby opening on sharing ui once again). introduced at: 70e9ad9 `onCreate` seems like a good place to have it compared to `handleSend` as it simplifies the code and we avoid the cases where we need to check for intents coming from other places (such as `onNewIntent`).
Previously the scroll event was getting registered by the ScrollView enclosing these input list, hence users were not able to scroll through the input list provided in the sharing screen. Upstream docs: https://reactnative.dev/docs/scrollview#nestedscrollenabled-android say this defaults to true, which should mean this is redundant. But empirically, we seem to need it. Discussion at: zulip#4514 (comment)
|
Thanks @AkashDhiman for the revisions! These commits look great and I'm merging them now: bb1bba0 sharing: Reset initialSharedData after sending the map to JS. The only edit I've made is to add to one commit message the answer to the question I asked here: #4514 (comment) That leaves these commits, which mostly I just haven't yet read properly (I didn't get to them in my last round, at #4514 (review) ): 2b3519d sharing: Make the app not quit after share to zulip concludes. Because this thread has gotten long, I think the cleanest thing will be to give those their own thread. Would you send a new PR with those changes? In case it's at all helpful, I'll post them as a branch; but I've made no edits to them, so you can also just rebase what you have locally and get an equivalent result. |
|
Posted those commits here: |
|
(Also when you send that new PR, please include a link to @chrisbobbe 's review above: #4514 (review) because it has some comments about a previous version of the changes that'll be in that new PR.) |
Done, changes not merged in this PR can be found in #4757 |
AkashDhiman
changed the title
4 participants
Works towards fixing #117
Continuation of work done in #4196
screen capture:
work done (in both parts):
See more details here.
According to me the section of code that was causing the problem in previous point was present in case someone opens the app from minimized apps after they have cancelled sharing, if it wasn't present they would still see the share screen since the SEND_INTENT that originally started the app persists in the minimized app, the placement of the code however was wrong (inside the handleSend method), this caused intents coming from onNewIntent methods to be ignored, when user has already cancelled/shared content once, and wanted to share again.
not addressed: