ios: Fix unwanted in-app notification sound when phone is on silent.

[chrisbobbe]

(And some follow-up cleanups.)

I've kept my personal iPhone on iOS 13 just in case, and here's a case where that paid off! I was able to test this on my device, and that helped me notice that UNNotificationPresentationOptionBanner (which is iOS 14+) didn't do anything on my device, and that I should include UNNotificationPresentationOptionAlert for iOS <14 support.

A consequence is that I haven't been able to test on iOS 14 yet. I get this output from logging.warn if I try to test on a simulator running iOS 14:

Failed to register iOS push token: 3010

raw_error: {"message":"remote notifications are not supported in the simulator","details":{"NSLocalizedDescription":"remote notifications are not supported in the simulator"},"code":3010}

(Looks like we can only test push notifications on a real device.) Should I upgrade my own device to iOS 14, or does someone with a device running iOS 14 want to test, or do we think it's fine without testing on 14?

Fixes: #4897

[gnprice]

Thanks!

Testing out this area of behavior (first with just a released version, to understand the status quo), I'm actually kind of puzzled -- I can't seem to reproduce the behavior we expect.

(This is something I don't see in my own normal personal use, because I always have my devices on silent/vibrate for notifications.)

• On iOS, on an iPhone 7 running 12.4.1 and v27.163 of the app, what I find (after turning off silent mode) on getting a notification for a PM is:

  • When the app isn't running, I get a banner and a sound, the system's default notification sound. This part is expected.
  • Ditto when the app is in the background.
  • When the app is in the foreground, I get neither a banner nor any sound. This is puzzling -- the code that gets deleted at the start of this PR was expected to be playing a sound in this case, right?

• On iOS, on an iPad mini 4 running iOS 14.6 and v27.166 of the app, what I find (after turning up the volume) is:

  • When the app isn't running, I get the notification as a banner and in Notification Center, and no sound. I don't know why there's no sound.
    • I checked the system's notification settings for the Zulip app, and it's allowed to play sounds.
  • Ditto when the app is in the background.
  • When the app is in the foreground, I get neither a banner (nor anything in Notification Center) nor any sound. This is puzzling but the same as on that iPhone 7.

• On Android, on a Pixel 4 running Android 11 and v27.166 of the app, what I find (after turning up the "Ring & notification volume", and also all the other volume scales for good measure) is:

  • When the app isn't running, I get the notification in the system notifications UI at the top of the screen, but don't get any sound.
    • I also don't see the notification pop on screen as an alert. This is puzzling because I would definitely have said that I normally do; I don't know if something changed, or what. Digging into the notification settings, I have the "Pop on screen" setting enabled for the Zulip app's notifications.
  • Ditto when it's in the background.
  • Ditto also when it's in the foreground -- in particular, still no sound. This is puzzling for the same reason as on iOS: I would have expected the code that gets deleted at the start of this PR was to be playing a sound in this case.

What behavior do you see, before your changes?

[gnprice]

All the cleanups that come after the behavior changes look good!

TODO: Probably just do a CommonActions.goBack()?

This sounds pretty reasonable. I wonder how it ends up differing from StackActions.pop(), which would be the most direct simplification of what we currently have. In particular, if you happen to indeed be in a stack navigator, does it always do the same thing?

[chrisbobbe]

I can't seem to reproduce the behavior we expect

Hmm! Interesting. For the problem where you expect to hear a sound while foregrounded, before these changes:

This may have been implicit, but did you read through the code in doEventActionSideEffects and decide that you were pretty sure that playMessageSound would have been called in the released app? I think you could also check this with some logging in a debug build, without having to set up push notifications (since the playMessageSound call has nothing to do with push-notification code).

I've just tried on my iPhone, and I'm hearing our "thump" sound when I get a PM from another account and I don't have any message lists open.

Edit: I've also just tried on the office Android device, and I'm getting a visible notification and an Android notification sound, along with the "thump" sound.

[chrisbobbe]

I can't seem to reproduce the behavior we expect

It looks like this was resolved, and you managed to reproduce it: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/notification.20sound.20bug/near/1240401

[chrisbobbe]

I wonder how it ends up differing from StackActions.pop(), which would be the most direct simplification of what we currently have.

Good point—I've just pushed a revision that says StackActions.pop() in that comment; we should go for the most direct simplification, at least for now.

[gnprice]

This code looks good! And we've just tested on iOS 14 (on my iPhone 7, which I upgraded yesterday from iOS 12), and it works as intended.

Please merge at will. I believe you're in the middle of testing again on iOS 13.

[chrisbobbe]

I believe you're in the middle of testing again on iOS 13.

Yep, this worked as expected. Yay!

[chrisbobbe]

Thanks for the review! Merged.