-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(android): add back option to remove notification #1730
Conversation
@@ -379,6 +387,7 @@ class MusicService : HeadlessJsTaskService() { | |||
} | |||
is NotificationState.CANCELLED -> { | |||
stopForeground(true) | |||
stopSelf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though Android does not completely kill the app process, it's still good to call this.
android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt
Show resolved
Hide resolved
android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid overall. Left a couple of comments
android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt
Outdated
Show resolved
Hide resolved
Just merged rebased to |
repositories { | ||
mavenLocal() | ||
mavenCentral() | ||
google() | ||
} | ||
dependencies { | ||
classpath 'com.android.tools.build:gradle:4.2.2' | ||
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" | ||
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:1.7.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using 1.7.10 features? What will happen now if someone is not on 1.7.10 yet? Will it cause compilation errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, they'll just have to update Kotlin
@@ -0,0 +1,17 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to only have migration docs for the major versions, but not for the minor. I added this for that reason. Not sure if there is a better way to go about this.
@mpivchev I tested this in my app with v3.2.0 using |
@mmmoussa what version of Android and device are you on? |
Moto G Fast running Android 11 |
@mmmoussa fascinating... I'm on a Moto G Power running Android 11 and I'm not getting the issue you mentioned. Can you try clearing your cache and rebuilding? |
Cleared cache and did a fresh install but still having the same issue. I am entirely unfamiliar with Kotlin but will try to debug. |
@mmmoussa if you create a repro based on a fork of this lib in the example project I can take a look at it too (but I encourage you to do so as well) |
With the example app, when I run the app in debug mode after modifying the
In the case of my app, the release build does stop the audio playback but the notification remains. However, the debug build behaviour also has the notification get removed like with the example app. |
Seems that the old functionality was to unbind the service based on 3c292a8 and that is no longer the approach, so I'm not sure where to start looking for the issue. What would be helpful is if the example app could be bundled for release and tested to determine if the expected functionality even works in that app correctly. |
@mmmoussa you can execute a release build locally and install it on your phone I believe if that's a concern. At the moment I'm skeptical that this is something related to release or dev mode. I'm going to try and test the example app to see if I get the same results as you. |
@jspizziri that is what I tried to do but I encountered the above issue for the release build. I am looking into resolving it. Is the release build of the example app working for you? |
I got the release build of the example app working and found the behaviour to be the same as debug, which is still broken. You can see this yourself if you take a look at #1758. |
This adds an
AndroidOptions
interface to RN that adds optional flags specifically for Android.It also adds the android flag
stoppingAppRemovesNotification
which will remove the notification when the app is killed.