Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mail-composer][android] fix composeAsync not resolving after send/ discard #20869

Merged
merged 3 commits into from Feb 2, 2023

Conversation

keith-kurak
Copy link
Contributor

@keith-kurak keith-kurak commented Jan 17, 2023

Why

Fixes #20509

How

TL;DR - this fix restores the documented behavior, albeit with a visual bug that seems to be peculiar to Gmail.

I first tried to compare the send/ resolve flow to (what I thought) was the closest analog, expo-sms. The onHostResume lifecycle listener event is trigged in the SMS package, but not mail composer. Nothing along these lines worked (e.g., matching the Intent flags).

It turns out that expo-sharing is perhaps a more appropriate comparison, because they both use Intent.createChooser to create a share sheet (the mail composer's sheet mainly filters on email apps and can pass additional email-related keys). It seems like onHostResume never responds to returning from a share sheet. Sharing uses the ActivityEventListener instead.

Anyway, this works, with one visual quirk that seems to be peculiar to the implementation of the Gmail client's new mail activity. Pressing back does not discard the email/ return to the app. It takes you back to your inbox. Once you do this, your only choice is to use the task switcher/ launcher to leave the email, and, in the task switcher, the image of the app doesn't update, continuing to show a picture of the email that you were composing.

Other apps demonstrate the same issue with sharing to Gmail (e.g., Twitter), though others don't (e.g., Facebook). Seems possible that this is related to task/ activity settings outside the control of this package, e.g., android:launchMode.

Comparing to external packages that fulfill a similar purpose, I found some (e.g., react-native-mail) that resolve the promise as soon as the share sheet opens, which is an option, but would break the documented behavior and not match iOS.

Test Plan

Scenarios:

  • single email client (bypasses chooser)
  • multiple email clients (shows chooser)
  • send email (returns to app, promise resolves)
  • discard email (returns to app, promise resolves)
  • share email, open task switcher, return to app (promise resolves, but task switcher freezes on email client)
  • test with latest Expo Go unversioned as of 2/1/2023

See video (including visual bug at the end)

Checklist

@linear
Copy link

linear bot commented Jan 17, 2023

CX-189 Android Mail Composer not returning after finishing

Seems like SMS does it sometimes on Snack, but not always (odd), even though it uses the same code.

Would also be nice if these used the same/ similar return types #20509

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 17, 2023
@keith-kurak keith-kurak force-pushed the keith/cx-189-android-mail-composer-not-returning branch from 4e3146d to 665ba5c Compare February 1, 2023 21:43
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Feb 1, 2023
@keith-kurak keith-kurak marked this pull request as ready for review February 1, 2023 22:09
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅

…/mailcomposer/MailComposerModule.kt

Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
@keith-kurak keith-kurak merged commit 6e026ca into main Feb 2, 2023
@keith-kurak keith-kurak deleted the keith/cx-189-android-mail-composer-not-returning branch February 2, 2023 15:13
jakobo added a commit to jakobo/expo that referenced this pull request Feb 2, 2023
…flipper

* upstream/main: (47 commits)
  [docs] Update Hermes guide to state that Hermes is the new default engine (expo#21047)
  chore: don't mark issues with the "Issue accepted" label as stale (expo#21058)
  Switch default JS engine to Hermes (expo#21001)
  [mail-composer][android] fix composeAsync not resolving after send/ discard (expo#20869)
  Update CHANGELOG.md (expo#21061)
  [core][iOS] Fix expo modules aren't added to global (expo#21037)
  [test-suite] Fix import in the Image example (expo#21043)
  [test-suite] fix video hanging (expo#21057)
  [av][ncl][go] fix audio and video qa issues (expo#21055)
  [tools] Selecting pull requests to label in the publish command (expo#20991)
  [document-picker] fill missing descriptions in `DocumentResult` type (expo#21040)
  [tools] Bump http-cache-semantics from 4.1.0 to 4.1.1 (expo#21049)
  Bump http-cache-semantics from 4.1.0 to 4.1.1 in /docs (expo#21050)
  [apps][yarn-workspace] replace deprecated activateKeepAwake
  update changelogs for react-native 0.71 upgrade (expo#20858)
  Upgrade react native to 0.71.2 (expo#21045)
  [go] update @shopify/react-native-skia to 0.1.172 (expo#21014)
  [stripe] Upgrade stripe to 0.23.1 (expo#20964)
  [expo-firebase-*] Remove libraries (expo#20979)
  [docs] Update expo-secure-store to add info about Export compliance (expo#21021)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MailComposer.composeAsync doesn't return result on Android
3 participants