mattttvaughn / chronicle

GNU General Public License v3.0
216 stars 58 forks source link

Custom jump intervals #51

Closed lks-nbg closed 2 years ago

lks-nbg commented 2 years ago

First of all, I am not a native Android developer. Therefore, the submitted code may not be the best solution to the issue and may contain bugs. Therefore, thorough review and testing is necessary, which I cannot provide.

This pull request tries to solve issue #6 by adding custom intervals for jumping back and forth. The user can choose between 10, 15, 20, 30, 60 and 90 seconds in the settings menu. The player then displays the appropriate icon and jumps to the new position accordingly.

I tested the code on two devices and it worked as expected. However, there are still some to-dos before this code can be released.

mattttvaughn commented 2 years ago

And for updating the icons on the currently playing view, see this gist here: https://gist.github.com/mattttvaughn/6879649ba9ee4b179070ab1d3f721dd8

lks-nbg commented 2 years ago

Thanks for your review. I incorporated your suggestions and the open todos are now resolved. The functions now work as expected.

mattttvaughn commented 2 years ago

I'm not seeing the icons update in the notification when jump intervals are changed. I'm okay with changing those skip forwards/backwards buttons to not have any text in them, but if we wanted text in them we'd probably want to update the notification when the prefs change. @lks-nbg

lks-nbg commented 2 years ago

Sorry I missed that.

I tried to adapt the prefsChangeListener solution from CurrentlyPlayingViewModel but failed since (Mutable)LiveData can't be used in NotificationCompat.Action. Then I tried to store a local copy of the jumpIntervals to detect changes in the build method and then replace the icon if necessary (see latest commit). This works but only when the notification is rebuild (e.g. after tapping on play/pause).

Do you know a simple solution to rebuild the notification? Otherwise I would switch back to my first attempt with generic icons :-/

mattttvaughn commented 2 years ago

@lks-nbg we want rebuild the notification when the prefs are updated from inside of the MediaPlayerService (which is active only when media is playing), so we aren't accidentally creating a notification outside of media playback and so we have a reference to the media session token. gist below to explain

https://gist.github.com/mattttvaughn/221767c2613ddc1848ea48b00b9377b6

lks-nbg commented 2 years ago

@mattttvaughn Thanks for your patience! I implemented your suggestions and the notification icons are now also updated when the settings change.