fix: Syncing media notification getting stuck #17719
Open
+3
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We can also move it only to catch block instead of finally |
| @@ -111,6 +116,11 @@ class SyncMediaWorker( | |||
| } | |||
| } | |||
|
|
|||
| private fun clearNotification() { | |||
| Timber.i("SyncMediaWorker notification cleared") | |||
| notificationManager.cancel(NotificationId.SYNC_MEDIA) | |||
There was a problem hiding this comment.
Thanks. This seems like an unexpectedly simple fix for this issue.
I admit I don't see the point of adding clearNotification, and not just inling this funciton where it's used, unless you plan to reuse it.
You add two Timber, where once seems probably sufficient.
I don't know this part of the code. My main qustion is: when the code works normally, what cause the notification to disappear? Because I'd have assumed that this is the part of the code that could be put in a function and reused.
Except that I don't see any place that ever cancel/dlismiss the SYNC_MEDIA notification in our codebase
There was a problem hiding this comment.
Agreed with inlining it.
And you probably don't need the isStopped check
There was a problem hiding this comment.
My main qustion is: when the code works normally, what cause the notification to disappear? Because I'd have assumed that this is the part of the code that could be put in a function and reused.
Except that I don't see any place that ever cancel/dlismiss the SYNC_MEDIA notification in our codebase
The notification itself was being marked as ongoing (setOngoing(true)), which prevents the user from manually dismissing it, further exacerbating the issue.
There was a problem hiding this comment.
And you probably don't need the isStopped check
I would want to clear notifications only when the worker is stopped, adds a precaution step
There was a problem hiding this comment.
But how the work can be stopped while running? I don't see it being stopped anywhere in the code
There was a problem hiding this comment.
when the code works normally, what cause the notification to disappear?
The notification itself was being marked as ongoing (setOngoing(true)), which prevents the user from manually dismissing it, further exacerbating the issue.
I believe that Arthur's question wans't answered
BrayanDSO
requested changes
Jan 5, 2025
| @@ -111,6 +116,11 @@ class SyncMediaWorker( | |||
| } | |||
| } | |||
|
|
|||
| private fun clearNotification() { | |||
| Timber.i("SyncMediaWorker notification cleared") | |||
| notificationManager.cancel(NotificationId.SYNC_MEDIA) | |||
There was a problem hiding this comment.
Agreed with inlining it.
And you probably don't need the isStopped check
I reproduced this issue by following the steps mentioned by the OP of the issue |
criticalAY
added
the
Needs Author Reply
|
Key Problem(my investigation): ForegroundServiceStartNotAllowedException: This exception occurs because the SyncMediaWorker attempted to start a foreground service while the app was either in the background or the system disallowed starting foreground services at that moment. Modern Android versions (particularly from Android 12 onwards) impose stricter limitations on starting foreground services while the app is in the background. The system prevents such actions unless they are triggered by user interactions or explicitly whitelisted. @Arthur-Milchior the apps works normally when this sort of exc is not thrown, not sure how this works in Android background but clearing the notification seems like a reasonable solution I have logs of both the cases, i.e. when the notification was cleared and when it was stuck Logs |
criticalAY
force-pushed
the
notification-fix
branch
from
454c84c to
4cff670
Compare
BrayanDSO
reviewed
Jan 5, 2025
| @@ -84,6 +84,9 @@ class SyncMediaWorker( | |||
| setContentTitle(CollectionManager.TR.syncMediaFailed()) | |||
| } | |||
| return Result.failure() | |||
| } finally { | |||
| Timber.i("SyncMediaWorker stopped, notification cleared") | |||
There was a problem hiding this comment.
The worker stops when it returns. Just log that the notification is being cleared
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose / Description
I found out that the notification gets stuck after the exception is thrown and not cleared at times, hence force clear the notification once the worker has stopped
Fixes
Approach
Force clear the sync notification in case the worker is not running
How Has This Been Tested?
Google emulator API 35
Checklist
Please, go through these checks before submitting the PR.