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

Delete notification channel when possible #1271

Merged
merged 3 commits into from
Feb 24, 2020

Conversation

joeygrover
Copy link
Member

Fixes #1267 (partially)

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested Android

Core Tests

-None specifically. This revolves around bluetooth connections.

Summary

Added the notification channel delete method call back into the router service. Some phones like the pixel 2 & 3 were only clearing the notification 5-7 seconds after the service called stopForeground. Deleting the channel removes the notification instantly. We added a check to avoid a race condition in which the channel is deleted before the notification was actually shown.

Changelog

Bug Fixes
  • Notification being displayed even after service leaves foreground
  • Added check to timer to make sure it is not going to negative numbers
  • Incremented router service version

CLA

Added the notification channel delete method call back into the router service. Some phones like the pixel 2 & 3 were only clearing the notificaiton 5-7 seconds after the service called stopForeground. Deleting the channel removes the notification instantly. We added a check to avoid a race condition in which the channel is deleted before the notifcation was actually shown.
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #1271 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1271      +/-   ##
=============================================
- Coverage      47.07%   47.04%   -0.03%     
- Complexity      4343     4344       +1     
=============================================
  Files            482      482              
  Lines          23618    23632      +14     
  Branches        2714     2718       +4     
=============================================
+ Hits           11117    11118       +1     
- Misses         11837    11850      +13     
  Partials         664      664              
Impacted Files Coverage Δ Complexity Δ
...n/java/com/smartdevicelink/proxy/SdlProxyBase.java 8.60% <0.00%> (+0.02%) 31.00% <0.00%> (+1.00%) ⬇️

@MichaelCrimando
Copy link

Sweet, thanks for the fast turn around time!

@MichaelCrimando
Copy link

@bilal-alsharifi and @joeygrover I can come up to Livio and retest on my phone this Thursday if there's a way to get the latest build on it

@bilal-alsharifi
Copy link
Contributor

Thanks, @MichaelCrimando. I don't think there is a way to test the fix with the apps that you listed on the issue. The apps will need to update to get the fix.

@bilal-alsharifi
Copy link
Contributor

@MichaelCrimando This PR should fix the bug that you mentioned in issue #1267. Are you okay with merging it?

@MichaelCrimando
Copy link

@bilal-alsharifi I'm not a code reviewer, but I'll say yes

@bilal-alsharifi bilal-alsharifi merged commit 6c24b9e into develop Feb 24, 2020
@bilal-alsharifi bilal-alsharifi deleted the bugfix/notification_removal_delay branch February 24, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants