Skip to content

Close call notifications when the call has been joined by the user, or the call has ended#4672

Merged
claucambra merged 1 commit into
masterfrom
bugfix/close-dead-call-notifications
Jul 1, 2022
Merged

Close call notifications when the call has been joined by the user, or the call has ended#4672
claucambra merged 1 commit into
masterfrom
bugfix/close-dead-call-notifications

Conversation

@claucambra

@claucambra claucambra commented Jun 24, 2022

Copy link
Copy Markdown
Collaborator

This PR adds a CallStateChecker that polls the state of the call being notified, much like the Talk iOS app, and stops the call notification if:

  • The call has been joined by the user
  • The call has been ended (i.e. all users have left the all)
  • The notification timer has run out (60 seconds)

Fixes #4497

@claucambra claucambra self-assigned this Jun 24, 2022
@claucambra claucambra force-pushed the bugfix/close-dead-call-notifications branch from 5db3266 to aae9e67 Compare June 24, 2022 17:56
@codecov

codecov Bot commented Jun 24, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4672 (aae9e67) into master (5546523) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head aae9e67 differs from pull request most recent head a504dbf. Consider uploading reports for the commit a504dbf to get more accurate results

@@            Coverage Diff             @@
##           master    #4672      +/-   ##
==========================================
- Coverage   56.42%   56.42%   -0.01%     
==========================================
  Files         138      138              
  Lines       17069    17071       +2     
==========================================
  Hits         9632     9632              
- Misses       7437     7439       +2
Impacted Files Coverage Δ
src/libsync/networkjobs.cpp 49.92% <0.00%> (-0.15%) ⬇️

mgallien
mgallien reviewed Jun 27, 2022
View reviewed changes
Comment thread src/gui/tray/notificationhandler.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
mgallien
mgallien reviewed Jun 30, 2022
View reviewed changes
Comment thread src/gui/callstatechecker.h Outdated
Comment thread src/gui/callstatechecker.h Outdated
Comment thread src/gui/callstatechecker.h Outdated
Comment thread src/gui/callstatechecker.h Outdated
Comment thread src/gui/callstatechecker.h Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp
Comment thread src/gui/callstatechecker.cpp Outdated
Comment thread src/gui/callstatechecker.cpp Outdated
@claucambra claucambra force-pushed the bugfix/close-dead-call-notifications branch 4 times, most recently from 2c99bb2 to 79900a1 Compare June 30, 2022 10:35
mgallien
mgallien reviewed Jun 30, 2022
View reviewed changes

@mgallien mgallien left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please squash commits and I then approve (according to our review procedure)
this time I do not forget

@mgallien

mgallien commented Jul 1, 2022

Copy link
Copy Markdown
Collaborator

/backport to stable-3.5

@claucambra
Close call notifications when the call has been joined by the user, o…
a504dbf 
…r the call has ended

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>
@claucambra claucambra force-pushed the bugfix/close-dead-call-notifications branch from 79900a1 to a504dbf Compare July 1, 2022 09:40
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2022

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

2.0% 2.0% Coverage
0.0% 0.0% Duplication

@nextcloud-desktop-bot

nextcloud-desktop-bot commented Jul 1, 2022

Copy link
Copy Markdown

AppImage file: Nextcloud-PR-4672-a504dbf9653041f1c5bf54237d380e10542044e8-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@claucambra claucambra merged commit 8ce0842 into master Jul 1, 2022
@claucambra claucambra deleted the bugfix/close-dead-call-notifications branch July 1, 2022 10:55
@backportbot-nextcloud backportbot-nextcloud Bot mentioned this pull request Jul 1, 2022
Merged
nickvergessen
nickvergessen reviewed Jul 1, 2022
View reviewed changes
Comment thread src/gui/callstatechecker.cpp
_notificationTimer.setInterval(60 * 1000);
connect(&_notificationTimer, &QTimer::timeout, this, &CallStateChecker::slotNotificationTimerElapsed);

_statusCheckTimer.setInterval(2 * 1000);

@nickvergessen nickvergessen Jul 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this basically DDoSes the server, you might use 5 secs as well like the mobile clients do?
https://github.com/nextcloud/talk-ios/blob/529bdc88341518569a7a470b6ca7a535d6e70cdf/NextcloudTalk/CallKitManager.m#L44

But also in case the user joined, you should receive a notify_push about the call notification being deleted.
Listening to that and then removing also the popup+sound would have been good enough from my POV, but now you made the effort already so it's okay to keep.

@claucambra claucambra Jul 1, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Apologies @nickvergessen I misread the 5 as a 2 and it stuck 🤦‍♂️ -- will increase to 5

Will work on the notify_push next

@mgallien mgallien added this to the 3.6.0 milestone Jul 1, 2022
@claucambra

claucambra commented Jul 12, 2022

Copy link
Copy Markdown
Collaborator Author

/backport to stable-3.5

@backportbot-nextcloud

backportbot-nextcloud Bot commented Jul 12, 2022

Copy link
Copy Markdown

The backport to stable-3.5 failed. Please do this backport manually.

@claucambra claucambra mentioned this pull request Nov 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments
@mgallien mgallien mgallien approved these changes
@camilasan camilasan Awaiting requested review from camilasan camilasan is a code owner
+1 more reviewer
@allexzander allexzander allexzander approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@claucambra claucambra

Labels

backport-request

Projects

None yet

Milestone

3.6.0

Development

Successfully merging this pull request may close these issues.

[Bug]: Call notification is not fading when the call was accepted somewhere else

5 participants

@claucambra @mgallien @nextcloud-desktop-bot @nickvergessen @allexzander