From 2c2fbb09b20cfcc1ebf2fe2b0ab8332e9be252ae Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Fri, 11 Jun 2021 17:10:48 +0200 Subject: [PATCH 1/6] refactor: move playbackCallbacks in MusicService to own anonymous class --- .../gramophone/service/MusicService.java | 66 ++++++++++--------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 4539ec15..2dfcea44 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -74,7 +74,7 @@ import static com.google.android.exoplayer2.Player.MEDIA_ITEM_TRANSITION_REASON_ import static com.google.android.exoplayer2.Player.MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED; import static com.google.android.exoplayer2.Player.PLAY_WHEN_READY_CHANGE_REASON_END_OF_MEDIA_ITEM; -public class MusicService extends Service implements SharedPreferences.OnSharedPreferenceChangeListener, Playback.PlaybackCallbacks { +public class MusicService extends Service implements SharedPreferences.OnSharedPreferenceChangeListener { public static final String PACKAGE_NAME = BuildConfig.APPLICATION_ID; public static final String ACTION_TOGGLE = PACKAGE_NAME + ".toggle"; @@ -159,6 +159,38 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP } }; + private final Playback.PlaybackCallbacks playbackCallbacks = new Playback.PlaybackCallbacks() { + @Override + public void onStateChanged(int state) { + notifyChange(STATE_CHANGED); + } + + @Override + public void onReadyChanged(boolean ready, int reason) { + notifyChange(STATE_CHANGED); + + if (ready) { + progressHandler.sendEmptyMessage(TRACK_STARTED); + prepareNext(); + } else if (reason == PLAY_WHEN_READY_CHANGE_REASON_END_OF_MEDIA_ITEM) { + progressHandler.sendEmptyMessage(TRACK_ENDED); + } + } + + @Override + public void onTrackChanged(int reason) { + acquireWakeLock(30000); + + if (reason == MEDIA_ITEM_TRANSITION_REASON_AUTO) { + playerHandler.sendEmptyMessage(TRACK_CHANGED); + progressHandler.sendEmptyMessage(TRACK_CHANGED); + } else if (reason == MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED) { + progressHandler.sendEmptyMessage(TRACK_CHANGED); + prepareNext(); + } + } + }; + private final BroadcastReceiver becomingNoisyReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, @NonNull Intent intent) { @@ -205,7 +237,7 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP wakeLock.setReferenceCounted(false); playback = new LocalPlayer(this); - playback.setCallbacks(this); + playback.setCallbacks(playbackCallbacks); queueManager = new QueueManager(this, queueCallbacks); @@ -760,36 +792,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP } } - @Override - public void onStateChanged(int state) { - notifyChange(STATE_CHANGED); - } - - @Override - public void onReadyChanged(boolean ready, int reason) { - notifyChange(STATE_CHANGED); - - if (ready) { - progressHandler.sendEmptyMessage(TRACK_STARTED); - prepareNext(); - } else if (reason == PLAY_WHEN_READY_CHANGE_REASON_END_OF_MEDIA_ITEM) { - progressHandler.sendEmptyMessage(TRACK_ENDED); - } - } - - @Override - public void onTrackChanged(int reason) { - acquireWakeLock(30000); - - if (reason == MEDIA_ITEM_TRANSITION_REASON_AUTO) { - playerHandler.sendEmptyMessage(TRACK_CHANGED); - progressHandler.sendEmptyMessage(TRACK_CHANGED); - } else if (reason == MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED) { - progressHandler.sendEmptyMessage(TRACK_CHANGED); - prepareNext(); - } - } - private static final class PlaybackHandler extends Handler { private final WeakReference mService; From f351429049250f297bbc1d3e79bf622cf262b0d4 Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Fri, 11 Jun 2021 17:33:50 +0200 Subject: [PATCH 2/6] refactor: remove duplicate MusicServie#setPostion(int position) --- .../gramophone/helper/MusicPlayerRemote.java | 18 ++++----------- .../gramophone/service/MusicService.java | 22 +------------------ .../gramophone/service/QueueManager.java | 4 ++-- 3 files changed, 7 insertions(+), 37 deletions(-) diff --git a/app/src/main/java/com/dkanada/gramophone/helper/MusicPlayerRemote.java b/app/src/main/java/com/dkanada/gramophone/helper/MusicPlayerRemote.java index f6e19356..61838fb4 100644 --- a/app/src/main/java/com/dkanada/gramophone/helper/MusicPlayerRemote.java +++ b/app/src/main/java/com/dkanada/gramophone/helper/MusicPlayerRemote.java @@ -103,12 +103,6 @@ public class MusicPlayerRemote { } } - public static void setPosition(final int position) { - if (musicService != null) { - musicService.setPosition(position); - } - } - public static void pauseSong() { if (musicService != null) { musicService.pause(); @@ -148,7 +142,7 @@ public class MusicPlayerRemote { } public static void openQueue(final List queue, final int startPosition, final boolean startPlaying) { - if (!tryToHandleOpenPlayingQueue(queue, startPosition, startPlaying) && musicService != null) { + if (!tryToHandleOpenPlayingQueue(queue, startPosition) && musicService != null) { musicService.openQueue(queue, startPosition, startPlaying); if (!PreferenceUtil.getInstance(musicService).getRememberShuffle()){ setShuffleMode(QueueManager.SHUFFLE_MODE_NONE); @@ -162,19 +156,15 @@ public class MusicPlayerRemote { startPosition = new Random().nextInt(queue.size()); } - if (!tryToHandleOpenPlayingQueue(queue, startPosition, startPlaying) && musicService != null) { + if (!tryToHandleOpenPlayingQueue(queue, startPosition) && musicService != null) { openQueue(queue, startPosition, startPlaying); setShuffleMode(QueueManager.SHUFFLE_MODE_SHUFFLE); } } - private static boolean tryToHandleOpenPlayingQueue(final List queue, final int startPosition, final boolean startPlaying) { + private static boolean tryToHandleOpenPlayingQueue(final List queue, final int startPosition) { if (getPlayingQueue() == queue) { - if (startPlaying) { - playSongAt(startPosition); - } else { - setPosition(startPosition); - } + playSongAt(startPosition); return true; } diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 2dfcea44..57365f5f 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -106,7 +106,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP public static final int RELEASE_WAKELOCK = 0; public static final int PLAY_SONG = 3; public static final int PREPARE_NEXT = 4; - public static final int SET_POSITION = 5; public static final int SAVE_QUEUE = 0; public static final int LOAD_QUEUE = 9; @@ -638,11 +637,7 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP position = 0; } - if (startPlaying) { - playSongAt(position); - } else { - setPosition(position); - } + playSongAt(position); notifyChange(QUEUE_CHANGED); } @@ -654,16 +649,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP playerHandler.obtainMessage(PLAY_SONG, position, 0).sendToTarget(); } - public void setPosition(final int position) { - // handle this on the handlers thread to avoid blocking the ui thread - playerHandler.removeMessages(SET_POSITION); - playerHandler.obtainMessage(SET_POSITION, position, 0).sendToTarget(); - } - - private void playSongAtImpl(int position) { - openTrackAndPrepareNextAt(position); - } - public void pause() { if (playback.isPlaying()) { playback.pause(); @@ -843,11 +828,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP break; case PLAY_SONG: - service.playSongAtImpl(msg.arg1); - service.notifyChange(STATE_CHANGED); - break; - - case SET_POSITION: service.openTrackAndPrepareNextAt(msg.arg1); service.notifyChange(STATE_CHANGED); break; diff --git a/app/src/main/java/com/dkanada/gramophone/service/QueueManager.java b/app/src/main/java/com/dkanada/gramophone/service/QueueManager.java index 3ddedf1b..46d6cd4a 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/QueueManager.java +++ b/app/src/main/java/com/dkanada/gramophone/service/QueueManager.java @@ -215,9 +215,9 @@ public class QueueManager { position = currentPosition - 1; } else if (deletedPosition == currentPosition) { if (playingQueue.size() > deletedPosition) { - MusicPlayerRemote.setPosition(position); + MusicPlayerRemote.playSongAt(position); } else { - MusicPlayerRemote.setPosition(position - 1); + MusicPlayerRemote.playSongAt(position - 1); } } } From a429198833c1258285e7b16f3b792c8e98e179e8 Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Mon, 15 Nov 2021 23:53:13 +0100 Subject: [PATCH 3/6] refactor: move synchronized keyword in funtion header --- .../gramophone/service/MusicService.java | 66 ++++++++----------- 1 file changed, 28 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 57365f5f..0dba7f62 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -497,24 +497,20 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP playSongAt(queueManager.getNextPosition(force)); } - private void openTrackAndPrepareNextAt(int position) { - synchronized (this) { - queueManager.position = position; + private synchronized void openTrackAndPrepareNextAt(int position) { + queueManager.position = position; - openCurrent(); - playback.start(); + openCurrent(); + playback.start(); - notifyChange(META_CHANGED); - notHandledMetaChangedForCurrentTrack = false; - } + notifyChange(META_CHANGED); + notHandledMetaChangedForCurrentTrack = false; } - private void openCurrent() { - synchronized (this) { - if (queueManager.getCurrentSong() == null) return; + private synchronized void openCurrent() { + if (queueManager.getCurrentSong() == null) return; - playback.setDataSource(queueManager.getCurrentSong()); - } + playback.setDataSource(queueManager.getCurrentSong()); } private void prepareNext() { @@ -522,13 +518,11 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP playerHandler.obtainMessage(PREPARE_NEXT).sendToTarget(); } - private void prepareNextImpl() { - synchronized (this) { - if (queueManager.getCurrentSong() == null) return; + private synchronized void prepareNextImpl() { + if (queueManager.getCurrentSong() == null) return; - queueManager.nextPosition = queueManager.getNextPosition(false); - playback.queueDataSource(queueManager.getSongAt(queueManager.nextPosition)); - } + queueManager.nextPosition = queueManager.getNextPosition(false); + playback.queueDataSource(queueManager.getSongAt(queueManager.nextPosition)); } public void initNotification() { @@ -656,20 +650,18 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP } } - public void play() { - synchronized (this) { - if (!playback.isPlaying()) { - if (!playback.isReady()) { - playSongAt(queueManager.position); - } else { - playback.start(); - if (notHandledMetaChangedForCurrentTrack) { - handleChangeInternal(META_CHANGED); - notHandledMetaChangedForCurrentTrack = false; - } - - notifyChange(STATE_CHANGED); + public synchronized void play() { + if (!playback.isPlaying()) { + if (!playback.isReady()) { + playSongAt(queueManager.position); + } else { + playback.start(); + if (notHandledMetaChangedForCurrentTrack) { + handleChangeInternal(META_CHANGED); + notHandledMetaChangedForCurrentTrack = false; } + + notifyChange(STATE_CHANGED); } } } @@ -694,12 +686,10 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP return playback.getDuration(); } - public int seek(int millis) { - synchronized (this) { - playback.setProgress(millis); - throttledSeekHandler.notifySeek(); - return millis; - } + public synchronized int seek(int millis) { + playback.setProgress(millis); + throttledSeekHandler.notifySeek(); + return millis; } private void notifyChange(@NonNull final String what) { From d59f63b0bfd6f629d2f470bcd0ee1c65621ce817 Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Mon, 15 Nov 2021 23:55:32 +0100 Subject: [PATCH 4/6] refactor: mark TRACK_ENDED as unused (with FIXME) --- .../java/com/dkanada/gramophone/service/MusicService.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 0dba7f62..293a804d 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -103,7 +103,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP public static final int TRACK_CHANGED = 1; public static final int TRACK_ENDED = 2; - public static final int RELEASE_WAKELOCK = 0; public static final int PLAY_SONG = 3; public static final int PREPARE_NEXT = 4; @@ -797,6 +796,8 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP break; case TRACK_ENDED: + // FIXME This isn't used anywhere. This means releaseWakeLock() is never called + // if there is a timer finished, don't continue if (service.pendingQuit || service.queueManager.getRepeatMode() == QueueManager.REPEAT_MODE_NONE && service.queueManager.isLastTrack()) { service.notifyChange(STATE_CHANGED); @@ -810,10 +811,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP service.playNextSong(false); } - sendEmptyMessage(RELEASE_WAKELOCK); - break; - - case RELEASE_WAKELOCK: service.releaseWakeLock(); break; From 588759c6d35fdeee9ed3b03ce27273a52d5a5803 Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Tue, 16 Nov 2021 00:07:09 +0100 Subject: [PATCH 5/6] refactor: notify changes in MusicService with callbacks and reduce duplicate notifyChange() calls --- .../gramophone/service/MusicService.java | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 293a804d..0fe17f98 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -121,7 +121,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP public QueueManager queueManager; - private boolean notHandledMetaChangedForCurrentTrack; private boolean queuesRestored; private PlayingNotification playingNotification; @@ -186,6 +185,9 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP progressHandler.sendEmptyMessage(TRACK_CHANGED); prepareNext(); } + + notifyChange(STATE_CHANGED); + notifyChange(META_CHANGED); } }; @@ -453,7 +455,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP if (restoredProgress > 0) seek(restoredProgress); - notHandledMetaChangedForCurrentTrack = true; handleChangeInternal(META_CHANGED); handleChangeInternal(QUEUE_CHANGED); } @@ -501,9 +502,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP openCurrent(); playback.start(); - - notifyChange(META_CHANGED); - notHandledMetaChangedForCurrentTrack = false; } private synchronized void openCurrent() { @@ -645,7 +643,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP public void pause() { if (playback.isPlaying()) { playback.pause(); - notifyChange(STATE_CHANGED); } } @@ -655,12 +652,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP playSongAt(queueManager.position); } else { playback.start(); - if (notHandledMetaChangedForCurrentTrack) { - handleChangeInternal(META_CHANGED); - notHandledMetaChangedForCurrentTrack = false; - } - - notifyChange(STATE_CHANGED); } } } @@ -786,11 +777,9 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP if (service.queueManager.getRepeatMode() == QueueManager.REPEAT_MODE_NONE && service.queueManager.isLastTrack()) { service.pause(); service.seek(0); - service.notifyChange(STATE_CHANGED); } else { service.queueManager.position = service.queueManager.nextPosition; service.prepareNextImpl(); - service.notifyChange(META_CHANGED); service.notifyChange(QUEUE_CHANGED); } break; @@ -816,7 +805,6 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP case PLAY_SONG: service.openTrackAndPrepareNextAt(msg.arg1); - service.notifyChange(STATE_CHANGED); break; case PREPARE_NEXT: From d1b7b5fe46664fcaece368d6e16231b1f216db1a Mon Sep 17 00:00:00 2001 From: Jakob Kukla Date: Mon, 15 Nov 2021 21:55:24 +0100 Subject: [PATCH 6/6] decouple ThrottledSeekHandler from PlayerHandler --- .../main/java/com/dkanada/gramophone/service/MusicService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java index 0fe17f98..c2ae60af 100644 --- a/app/src/main/java/com/dkanada/gramophone/service/MusicService.java +++ b/app/src/main/java/com/dkanada/gramophone/service/MusicService.java @@ -253,7 +253,7 @@ public class MusicService extends Service implements SharedPreferences.OnSharedP queueHandlerThread.start(); queueHandler = new QueueHandler(this, queueHandlerThread.getLooper()); - throttledSeekHandler = new ThrottledSeekHandler(playerHandler); + throttledSeekHandler = new ThrottledSeekHandler(new Handler()); uiThreadHandler = new Handler(); registerReceiver(widgetIntentReceiver, new IntentFilter(INTENT_EXTRA_WIDGET_UPDATE));