fix: Small Android memory safety improvements #1930
Conversation
… player when it is no longer strongly reachable
gnarhard
changed the title
There was a problem hiding this comment.
Thank you for the work done and making aware of these memory leaks.
Note that we also have the package audioplayers_android_exo which should also have the changes included. But we can do this after finishing the review for audioplayers_android, so no unnecessary duplicate work needs to be done.
...s/audioplayers_android/android/src/main/kotlin/xyz/luan/audioplayers/player/WrappedPlayer.kt
Show resolved
Hide resolved
| mediaPlayer.setOnCompletionListener(null) | ||
| mediaPlayer.setOnSeekCompleteListener(null) | ||
| mediaPlayer.setOnErrorListener(null) | ||
| mediaPlayer.setOnBufferingUpdateListener(null) |
There was a problem hiding this comment.
I guess I would prefer its own (interface) method dispose which is only called, when the player object is newly set or set to null. Otherwise it is hard to follow and this would implicate that the listeners are not used any more when released, which should not implicitly be the case.
...s/audioplayers_android/android/src/main/kotlin/xyz/luan/audioplayers/player/WrappedPlayer.kt
Show resolved
Hide resolved
|
|
||
| fun dispose() { | ||
| if (isDisposed) return | ||
| isDisposed = true |
There was a problem hiding this comment.
Can we somehow test this? What is the desired outcome? This should be aligned then on all platforms. I would expect it to throw, if a player is disposed twice, as this most likely is a mistake of the developer (?)
|
@Gustl22 I'm trying to implement what you suggested, but I have no idea what I'm doing in Java. 🤣 TBH, I just noticed a place where memory leaks might occur and used AI to help me write the Java code that could possibly help. If you feel like the memory leaks are valid concerns, please push this through the finish line, otherwise, you can close this PR and I won't be offended! Your call man, just trying to help. :) |
2 participants
Description
I'm not well-versed in Java, but these changes might benefit the garbage collection and memory-safety of the plugin. I've double checked this functionality by running it in my game.
Checklist
fix:,feat:,refactor:,docs:,chore:,test:,ci:etc).///, where necessary.Breaking Change
Related Issues