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

Feature/1724/missed call notification #2415

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

mahibi
Copy link
Collaborator

@mahibi mahibi commented Sep 21, 2022

this PR will:

  • resolve show notifications for missed calls (+ improve duration for ringing) #1724
    • image
  • convert some classes from java to kotlin
  • introduce refactoring of Notification handling (isolate firebase stuff from other logic)
    • All "UI-notification" logic from ChatAndCallMessagingService was moved to NotificationWorker. ChatAndCallMessagingService was renamed to NCFirebaseMessagingService because it is now only responsible for firebase stuff. This separation should make it easier for alternative push services to dock with the app (if they are incorporated in the future).

Notes

  • please note that for the next release, minSDK should be at least 23 in order to support the method notificationManager.activeNotifications. This method is already in use (which might introduced problems for older phones in the past) but it is further used with this PR.
  • this PR should not improve or worsen the reliability of notification delivery. However i expect the following impact of the "you missed a call" notifications on phones that are already known to have problems with on-time delivery:
    • if there were problems with on-time delivery and it starts to work again (e.g. after a phone restart), the user might get a bunch of too late delivered notifications at once (this might already happened for text messages, but now also for missed calls).

possible follow up PR's:

  • NotificationWorker still needs to be further simplified!

@mahibi mahibi marked this pull request as draft September 21, 2022 14:26
@mahibi mahibi force-pushed the feature/1724/missedCallNotification branch from c48a078 to 9979b06 Compare September 27, 2022 11:14
@nextcloud-android-bot
Copy link
Collaborator

Lint

TypemasterPR
Warnings112112
Errors11

SpotBugs (new)

Warning Type Number
Bad practice Warnings 4
Correctness Warnings 33
Experimental Warnings 2
Internationalization Warnings 9
Malicious code vulnerability Warnings 10
Performance Warnings 22
Security Warnings 2
Dodgy code Warnings 54
Total 136

SpotBugs (master)

Warning Type Number
Bad practice Warnings 4
Correctness Warnings 33
Experimental Warnings 2
Internationalization Warnings 9
Malicious code vulnerability Warnings 10
Performance Warnings 22
Security Warnings 2
Dodgy code Warnings 54
Total 136

@mahibi mahibi force-pushed the feature/1724/missedCallNotification branch 2 times, most recently from af532ba to a390e0a Compare October 26, 2022 09:56
@mahibi mahibi self-assigned this Oct 26, 2022
@mahibi mahibi added this to the 15.1.0 milestone Oct 26, 2022
@mahibi mahibi force-pushed the feature/1724/missedCallNotification branch 3 times, most recently from dc727fc to 257c658 Compare October 28, 2022 14:38
@mahibi
Copy link
Collaborator Author

mahibi commented Oct 28, 2022

detekt checks are not fully satisfied. Some of the checks fail with ComplexMethod. While this is true and should be resolved in general, i suggest to review/merge anyway because this PR already changed a lot and seems stable for me. Further refactoring might take place in follow up PR's (after my vacation or later).

feel free to improve minor things in this PR while reviewing @timkrueger

@mahibi mahibi added the 3. to review Waiting for reviews label Oct 28, 2022
@mahibi mahibi marked this pull request as ready for review October 28, 2022 14:55
Copy link
Contributor

@timkrueger timkrueger left a comment

Choose a reason for hiding this comment

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

The new notifications works as expected. But for the debug information I got a negative delay:

I'll try to look into it today or tomorrow.

Regarding the detekt issues I'm OK with merging them and resolve them afterwards in a separate PR.

- add missed call notifications in NotificationWorker and CallNotificationActivity

- introduce refactoring of Notification handling (isolate firebase stuff from other logic). All "UI-notification" logic from ChatAndCallMessagingService was moved to NotificationWorker. ChatAndCallMessagingService was renamed to NCFirebaseMessagingService because it is now only responsible for firebase stuff. This separation should make it easier for alternative push services to dock with the app (if they are incorporated in the future).

- for DEBUG mode: show delivery delay time in notifications (time between sending from firebase to receive on device).

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
this didn't make sense because time between firebase and devices is not synchronized, so the results were useless.

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@mahibi mahibi force-pushed the feature/1724/missedCallNotification branch from 257c658 to ca145d1 Compare November 10, 2022 12:39
@mahibi
Copy link
Collaborator Author

mahibi commented Nov 10, 2022

The new notifications works as expected. But for the debug information I got a negative delay:

i reverted the calculation of the delivery time. This doesn't make sense because time is not synchronized... in the future we may be able to trigger a test push from the device itself and measure the full roundtrip time until it is received on the device again. But calculating a difference to a timestamp from firebease server doesn't make sense because it's not time-synchronized and never will be.

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@github-actions
Copy link
Contributor

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/2415-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

@github-actions
Copy link
Contributor

Codacy

Lint

TypemasterPR
Warnings112112
Errors11

SpotBugs

CategoryBaseNew
Bad practice44
Correctness7568
Dodgy code315272
Experimental22
Internationalization97
Malicious code vulnerability5353
Performance2322
Security22
Total483430

@mahibi mahibi merged commit 263edbc into master Nov 11, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/1724/missedCallNotification branch November 11, 2022 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show notifications for missed calls (+ improve duration for ringing)
3 participants