invertase / notifee

⚛️ A feature rich notifications library for React Native.
https://notifee.app
Apache License 2.0
1.88k stars 227 forks source link

feat(android): Support android call notification style #1134

Open dprevost-LMI opened 3 weeks ago

dprevost-LMI commented 3 weeks ago

PR introducing the new Android 12 CallStyle notification related to phone calls.

CallStyle can produce three different notifications:

Notes on the changes:

⚠️ Dependant projects might also need to update their androidx.core:core version or even their androidx.core:core-ktx to at least 1.10

⚠️ There is a known limitation where additional limited custom actions, as claimed in the doc, do not work right now. See this Google issue

CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

mikehardy commented 3 weeks ago

Just to note I see these commits flying by in my notification queue and I'm excited to see this when you think it's ready No rush of course, it's your effort :-) Totally fine with dependency updates as needed of course, core is actually on 1.15 now so bumping to 1.10 is arguably not even enough (but only doing what is necessary for the PR makes sense - no need to struggle on a side project of updating it)

dprevost-LMI commented 3 weeks ago

Just to note I see these commits flying by in my notification queue

😮 I can remove the draft PR; I thought the draft would not spam, sorry

core is actually on 1.15 now so bumping to 1.10 is arguably not even enough

Yeah, I struggled since on 1.10, I had some duplicate deps errors and needed to update the rooms deps too. So yeah, I'm doing the minimal since it is not the focus, as you mentioned!

While I have your attention, I struggle with the PendingIntent creation. I reached a working point, but having them in the NotificationAndroidStyleModel.java. I want to move that into its own class at some point. However, I must pass the NotificationModel pretty deep, which seems bad. If you have any suggestions about that, let me know

mikehardy commented 3 weeks ago

No worries about spam, it's a drop in the bucket and it's pleasing to see notificaitons about a feature PR vs yet another "I can't compile NNN because of ABC" issue comment 😅

I reached a working point, but having them in the NotificationAndroidStyleModel.java. I want to move that into its own class at some point. However, I must pass the NotificationModel pretty deep, which seems bad. If you have any suggestions about that, let me know

I'm a fan of "tested and working" over perfect, so I lean towards something that you know works first. Second, I'm not sure what exactly would be better, passing it down the way it is now doesn't seem so bad

The only thing I can see that makes me 🤔 is the inconsistent use of constants vs "magic numbers and strings" - I love the enums at the TS level and it would be nice to see constants or enums to match at the java level (and in the TS warning, which uses the numbers again vs the call types)

In general though, looks like it is shaping up. At first I thought this was just duplication of our full screen intent style (which is targeted for this use case as well) but as soon as I looked at the APIs in use it looks like this is just me being unaware of the problem area and that the APIs have moved on (as they always seem to for notifications...) to have specific call notification type for modern android. Nice to support it

dprevost-LMI commented 3 weeks ago

For your information, I just linked my project with my PR, and the first draft is working well in a happy path scenario! I hope to wrap this up in the upcoming weeks!

From Notifee example project it looks like this:

image
mikehardy commented 3 weeks ago

That's wonderful!

dprevost-LMI commented 3 weeks ago

Do I need to create a feature issue for this PR, or is the PR only OK?

mikehardy commented 3 weeks ago

PR only is just fine - no need for administrivia like an issue just to close it, if there is a PR in flight already

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.81%. Comparing base (ae2953b) to head (1817d5e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1134 +/- ## ========================================== + Coverage 77.08% 77.81% +0.74% ========================================== Files 32 32 Lines 1727 1771 +44 Branches 556 592 +36 ========================================== + Hits 1331 1378 +47 + Misses 395 340 -55 - Partials 1 53 +52 ```