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

[MM-57486] [MM-58008] Calls: Mobile ringing for incoming calls #7984

Merged
merged 26 commits into from
Jul 3, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented May 31, 2024

Summary

  • Adds ringing and vibration for incoming calls
    • Will ring only when not ringing and not in current call (same as desktop)
    • When silent mode is on, phone only vibrates
    • This was a bit of a trip. Initially was using a new sound playing library because I couldn't get incall-manager to work. That external library had some edge cases I wasn't happy with. Tried a different one, same issues. Went back to incall-manager and ended up having to write some native code to make it work properly.
  • Settings panel allows the user to choose the ring tone
  • Designs: https://www.figma.com/design/WVbRdxA4aRFdw410j2DxxU/MM-51513-Ringing-in-DMs%2C-GMs-(Mobile)?node-id=1573-20814&m=dev

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.

Device Information

This PR was tested on:

Screenshots

IMG_1614 IMG_1615 IMG_1616

Release Note

Calls: Incoming calls on DMs and GMs ring; ringtone is selectable in the settings menu

@cpoile cpoile added 2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer labels May 31, 2024
@matthewbirtch matthewbirtch added the Build Apps for PR Build the mobile app for iOS and Android to test label May 31, 2024
Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

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

Looking good @cpoile.

  1. In the settings, can we make it play the preview sound when you tap it the first time and then tapping again would stop it? Right now we're requiring an extra tap here.
  2. When a call comes in while my phone is locked or if the app is in the background, I get a regular notification along with persistent vibration, but the sound plays in the phone speaker (which I can only hear when I raise to my ear) rather than out loud. Is this intended? Or should the sound play out loud as long as I have my volume on?
  3. There is not an easy way to silence the persistent ringing/vibration if the app is in the background or my phone is locked. I have to tap the notification and enter the app to stop it from ringing. We need to figure out a way to make this better. I'm wondering what options we have to provide users the ability to silence an incoming call ring while not in the app. Are we able to add 'join' and 'dismiss' buttons within the notification? Also, if I press the volume down button on my phone while there is a ring happening, it should stop the ringing as well.

We may need to discuss 3 in more detail.

@cpoile
Copy link
Member Author

cpoile commented Jun 5, 2024

@matthewbirtch Thanks for the review. 1 is addressed in c0a1240. As for 2 and 3, my guess is you're using Android for that test, right? And you must be receiving the call within ~15 seconds of the app going into the background. On iOS there's no sound the moment the app goes into the background. I've added code to prevent Android from doing that too. The reason is both to keep it consistent with iOS, and because after ~15 seconds the websocket disconnects and it's not possible for the app to receive the incoming call ws event.

@streamer45
Copy link
Contributor

@cpoile I suppose it would be challenging to have the visual notification show on top of every screen?

@cpoile
Copy link
Member Author

cpoile commented Jun 6, 2024

@streamer45 Do you mean the green incoming call notification bar? It's where it is supposed to be, according to the designs. :) We would need new designs for where to put it if we wanted to put it on any other screens. But not impossible, just more work.

@streamer45
Copy link
Contributor

@streamer45 Do you mean the green incoming call notification bar? It's where it is supposed to be, according to the designs. :) We would need new designs for where to put it if we wanted to put it on any other screens. But not impossible, just more work.

Okay, because this functionality becomes increasingly less effective if we don't show it everywhere. Not a blocker of course, just a thought.

@cpoile cpoile requested a review from matthewbirtch June 6, 2024 17:46
@cpoile
Copy link
Member Author

cpoile commented Jun 6, 2024

Totally open to adding them in more screens. But I would caution about starting down a path where effort-to-reward ratio might get low -- there's dozens of screens that the user is in only briefly. Design team -- please feel free to open tickets or raise it in channel and we can add them in as needed.

@matthewbirtch matthewbirtch added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Jun 7, 2024
@matthewbirtch
Copy link
Contributor

this functionality becomes increasingly less effective if we don't show it everywhere. Not a blocker of course, just a thought

@streamer45 I get what you mean. I noticed this too while I was on my profile screen and a ring started, but I had no banner on this screen to join or dismiss the call. Maybe for the main screens that are accessible from the main bottom tab navigation, we should show the banner on the bottom like we do on the homescreen (could do this in a follow-up ticket). It would be challenging to consider every other possible screen though.

As for 2 and 3, my guess is you're using Android for that test, right?

@cpoile I was using iOS actually.

I just tested again on my iPhone and after I put Mattermost in the background while it's ringing, sound does indeed stop, so number 2 above seems to be resolved.

However, here's some other things I notices:

  • As mentioned, the sound stops if I put the app in the background, but the persistent vibration continues. If sound stops when put in the background, the vibration should also.
  • If the app is in the background while a DM call comes in, if I open the app, the ringing sound doesn't come on (but the banners show).
  • When the ringing sound happens, I also get the standard notification sound. Can we suppress the standard notification sound when the app is in the foreground and just have the ringing sound?

@cpoile
Copy link
Member Author

cpoile commented Jun 10, 2024

@matthewbirtch Thanks again, that's helpful. 1) fixed, everything stops when going into the background;

  1. fixed; I changed how we're triggering and tracking the sound, so now it will appear whenever the notification appears. Note: to simplify, if the sound appears once, it won't appear again. However, there is one tough edge case: if you receive a call immediately (within 15 seconds) of putting the phone into the background. Here the issue is that we do show the notification banner (you just don't see it), but we can't start the sound (since we're in the background). There's no simple way to start the sound when you come back, since we've recorded that the sound couldn't be played. We could introduce a queue, or some kind of retry logic, but that's really overcomplicating something I'd like to keep straightforward. The danger is that we make a mistake and start playing music or get into a state where we can't stop the music, and that would mean the user would have to kill the app and restart it. That's the very worst case scenario.
  1. I'm not sure what you mean by "the standard notification sound", can you describe it with a repro step?

@matthewbirtch
Copy link
Contributor

I'm not sure what you mean by "the standard notification sound", can you describe it with a repro step?

I'm getting two notification sounds on top of each other: the standard iOS notification sound (that I get for normal MM messages) plays on top of the ringing notification sound.

@cpoile
Copy link
Member Author

cpoile commented Jun 10, 2024

@matthewbirtch Hmm, I've never heard that before, I didn't even know we had an in-product notification sound. This is different from the push notification sound?

@matthewbirtch
Copy link
Contributor

@matthewbirtch Hmm, I've never heard that before, I didn't even know we had an in-product notification sound. This is different from the push notification sound?

Sorry, the push notification sound is what I mean

@enahum
Copy link
Contributor

enahum commented Jun 21, 2024

@cpoile I've build the app locally and is working as expected, I can hear 4 different tones as I select the values. I built locally with bundle exec fastlane android build command under the fastlane directory.

give it a try and let's see if it builds correctly from the CI

@enahum enahum added Build App for Android Build the mobile app for Android and removed Build App for Android Build the mobile app for Android labels Jun 21, 2024
@cpoile
Copy link
Member Author

cpoile commented Jun 21, 2024

@enahum True, bundle exec fastlane android build does work, but the ci's fastlane doesn't... :( The ci's fastlane seems to be doing other things, like filename munging and it doesn't have the same dir structure.

@mvitale1989
Copy link
Member

mvitale1989 commented Jun 21, 2024

@cpoile after some testing and digging, I see that it's possible to reproduce the behaviour of CI locally by utilizing the same env vars used for PR builds:

cd fastlane/
set -a
source .env.android.pr
CI=true
set +a
bundle exec fastlane android build

(or as I later found, you can just run bundle exec fastlane android build --env android.pr instead, which is what CI does 😄 )

Among the variables used for PR builds, the culprit for the name mangling (e.g. for renaming ./res/raw/calls_urgent.mp3 into ./res/ZG.mp3) seems to be BUILD_FOR_RELEASE=true. So the most minimal way to reproduce the issue locally is BUILD_FOR_RELEASE=true bundle exec fastlane android build.

I'm not familiar with the internals of the build scripts though, perhaps Elias would be able to best assist you with those.

@cpoile
Copy link
Member Author

cpoile commented Jun 21, 2024

@mvitale1989 Awesome, thank you for this! So, building locally with bundle exec fastlane android build --env android.pr does bundle the mp3s, and even mangled it works. 🎉

But why doesn't the ci step bundle the mp3s?

@mvitale1989
Copy link
Member

@cpoile CI seems to embed them in the same ways as the bundle exec fastlane android build --env android.pr command does locally.

E.g. if you look at the build-pr/build-android-pr status check's workflow and download the artifact android-build-pr-9609907829, if I extract the zip and then extract the embedded apk, i can see that the mp3s are also there with the same name:

mario@localhost:~/android-build-pr-9609907829/Mattermost_Beta$ find . -name '*.mp3'
./res/ZG.mp3
./res/ia.mp3
./res/tt.mp3
./res/xB.mp3

Do the ringtones work with that build?

@cpoile
Copy link
Member Author

cpoile commented Jun 21, 2024

@mvitale1989 Ahh, that artifact seems to be different from the one that was posted to the Mobile: Test Builds channel. I wonder why? The test channel build does not have the mp3s, but the artifact you pointed to does. And it works on my device. 🎉
Let's try building the test build again, see if it works this time.

Thanks so much Mario, you're a life saver.

@cpoile cpoile added Build App for Android Build the mobile app for Android Build Apps for PR Build the mobile app for iOS and Android to test and removed Build App for Android Build the mobile app for Android labels Jun 21, 2024
@cpoile
Copy link
Member Author

cpoile commented Jun 21, 2024

@enahum I've built it twice, and both times the Android apk sent to the QA test build channel link doesn't include the mp3 files, but the android apk in the build artifacts does include the mp3 files link. Any idea why there's a difference?

@cpoile
Copy link
Member Author

cpoile commented Jun 24, 2024

@DHaussermann Please take a look now -- most recent android QA build should be working, thanks!

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed
Since the last round of testing, The only remaining issue was the mp3 files.

I have confirmed on the more recent build the ring tones are now available
Also did a quick spot check over the feature again.

Tested and passed based on testing above #7984 (comment)

LGTM!
Thanks @cpoile!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed Build Apps for PR Build the mobile app for iOS and Android to test 3: QA Review Requires review by a QA tester labels Jul 2, 2024
@cpoile cpoile merged commit 92bdb28 into main Jul 3, 2024
36 checks passed
@cpoile cpoile deleted the MM-58008-mobile-ringing branch July 3, 2024 14:22
@amyblais amyblais added this to the v2.19.0 milestone Jul 3, 2024
@amyblais amyblais added the Docs/Needed Requires documentation label Jul 4, 2024
@amyblais amyblais added Docs/Done Required documentation has been written Docs/Needed Requires documentation and removed Docs/Needed Requires documentation Docs/Done Required documentation has been written labels Jul 12, 2024
@cwarnermm cwarnermm added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Docs/Done Required documentation has been written release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants