From f3980091a96f0e399598826f37206d596c1b6815 Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 1 Jan 2019 21:06:33 -0800 Subject: [PATCH 01/15] #2716 Prototype for the revamped PlaybackService to fix phantom notification. Many rough edges. Notable TODOs are marked with [2716]. --- .../adapter/DefaultActionButtonCallback.java | 19 +- .../core/receiver/MediaButtonReceiver.java | 3 +- .../service/playback/PlaybackService.java | 165 ++++++++++++------ .../util/playback/PlaybackController.java | 5 +- .../util/playback/PlaybackServiceStarter.java | 5 +- .../playback/PlaybackServiceFlavorHelper.java | 2 - 6 files changed, 123 insertions(+), 76 deletions(-) diff --git a/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java b/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java index 1286d9dc7..fda6d48a5 100644 --- a/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java +++ b/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java @@ -1,8 +1,6 @@ package de.danoeh.antennapod.adapter; import android.content.Context; -import android.support.annotation.NonNull; -import android.content.Intent; import android.widget.Toast; import com.afollestad.materialdialogs.MaterialDialog; @@ -23,7 +21,6 @@ import de.danoeh.antennapod.core.storage.DownloadRequester; import de.danoeh.antennapod.core.util.IntentUtils; import de.danoeh.antennapod.core.util.LongList; import de.danoeh.antennapod.core.util.NetworkUtils; -import de.danoeh.antennapod.core.util.playback.PlaybackServiceStarter; /** * Default implementation of an ActionButtonCallback @@ -84,16 +81,16 @@ public class DefaultActionButtonCallback implements ActionButtonCallback { } } else { // media is downloaded if (media.isCurrentlyPlaying()) { - new PlaybackServiceStarter(context, media) - .startWhenPrepared(true) - .shouldStream(false) - .start(); +// new PlaybackServiceStarter(context, media) // TODO: [2716] probably not needed but not 100% sure +// .startWhenPrepared(true) +// .shouldStream(false) +// .start(); IntentUtils.sendLocalBroadcast(context, PlaybackService.ACTION_PAUSE_PLAY_CURRENT_EPISODE); } else if (media.isCurrentlyPaused()) { - new PlaybackServiceStarter(context, media) - .startWhenPrepared(true) - .shouldStream(false) - .start(); +// new PlaybackServiceStarter(context, media) // TODO: [2716] probably not needed but not 100% sure +// .startWhenPrepared(true) +// .shouldStream(false) +// .start(); IntentUtils.sendLocalBroadcast(context, PlaybackService.ACTION_RESUME_PLAY_CURRENT_EPISODE); } else { DBTasks.playMedia(context, media, false, true, false); diff --git a/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java b/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java index b191dbf8b..3b52ac212 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java +++ b/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java @@ -3,7 +3,6 @@ package de.danoeh.antennapod.core.receiver; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; -import android.support.v4.content.ContextCompat; import android.util.Log; import android.view.KeyEvent; @@ -30,7 +29,7 @@ public class MediaButtonReceiver extends BroadcastReceiver { Intent serviceIntent = new Intent(context, PlaybackService.class); serviceIntent.putExtra(EXTRA_KEYCODE, event.getKeyCode()); serviceIntent.putExtra(EXTRA_SOURCE, event.getSource()); - ContextCompat.startForegroundService(context, serviceIntent); + context.startService(serviceIntent); // the service itself will determine if it needs to become a foreground service. } } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 66cf7d244..632d0d41b 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -25,6 +25,7 @@ import android.preference.PreferenceManager; import android.support.annotation.NonNull; import android.support.annotation.StringRes; import android.support.v4.app.NotificationCompat; +import android.support.v4.content.ContextCompat; import android.support.v4.media.MediaBrowserCompat; import android.support.v4.media.MediaBrowserServiceCompat; import android.support.v4.media.MediaDescriptionCompat; @@ -75,6 +76,7 @@ import de.greenrobot.event.EventBus; /** * Controls the MediaPlayer that plays a FeedMedia-file + * TODO [2716] add some guidelines on how to / not to start / stop the service from callers. */ public class PlaybackService extends MediaBrowserServiceCompat { /** @@ -264,9 +266,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { Log.d(TAG, "Service created."); isRunning = true; - NotificationCompat.Builder notificationBuilder = createBasicNotification(); - startForeground(NOTIFICATION_ID, notificationBuilder.build()); - registerReceiver(autoStateUpdated, new IntentFilter("com.google.android.gms.car.media.STATUS")); registerReceiver(headsetDisconnected, new IntentFilter(Intent.ACTION_HEADSET_PLUG)); registerReceiver(shutdownReceiver, new IntentFilter(ACTION_SHUTDOWN_PLAYBACK_SERVICE)); @@ -342,7 +341,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { public void onDestroy() { super.onDestroy(); Log.d(TAG, "Service is about to be destroyed"); - stopForeground(true); + stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed isRunning = false; started = false; currentMediaType = MediaType.UNKNOWN; @@ -367,7 +366,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { } private void stopService() { - stopForeground(true); + stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed stopSelf(); } @@ -465,13 +464,16 @@ public class PlaybackService extends MediaBrowserServiceCompat { Playable playable = intent.getParcelableExtra(EXTRA_PLAYABLE); if (keycode == -1 && playable == null && !castDisconnect) { Log.e(TAG, "PlaybackService was started with no arguments"); - stopService(); + // some code call startService with no arg, causing playback to stop prematurely in typical case + // temporarily comment it out for now. + // TODO: Next step - find the source of the no-arg service start +// stopService(); // TODO: find the source of starting service with no argument that causes me to comment out this line temporarily return Service.START_NOT_STICKY; } if ((flags & Service.START_FLAG_REDELIVERY) != 0) { Log.d(TAG, "onStartCommand is a redelivered intent, calling stopForeground now."); - stopForeground(true); + stopForeground(true); // TODO: unclear if stopForeground is still needed } else { if (keycode != -1) { Log.d(TAG, "Received media button event"); @@ -494,7 +496,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { } mediaPlayer.playMediaObject(playable, stream, startWhenPrepared, prepareImmediately); } - setupNotification(playable); } return Service.START_NOT_STICKY; @@ -573,7 +574,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { started = false; } - stopForeground(true); // gets rid of persistent notification + stopForeground(true); // gets rid of persistent notification // TODO: [2716] unclear if stopForeground is still needed return true; default: Log.d(TAG, "Unhandled key code: " + keycode); @@ -606,8 +607,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { public void notifyVideoSurfaceAbandoned() { mediaPlayer.pause(true, false); mediaPlayer.resetVideoSurface(); - setupNotification(getPlayable()); - stopForeground(!UserPreferences.isPersistNotify()); } private final PlaybackServiceTaskManager.PSTMCallback taskManagerCallback = new PlaybackServiceTaskManager.PSTMCallback() { @@ -670,15 +669,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { break; case PAUSED: - if ((UserPreferences.isPersistNotify() || isCasting) && - android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) { - // do not remove notification on pause based on user pref and whether android version supports expanded notifications - // Change [Play] button to [Pause] - setupNotification(newInfo); - } else if (!UserPreferences.isPersistNotify() && !isCasting) { - // remove notification on pause - stopForeground(true); - } writePlayerStatusPlaybackPreferences(); break; @@ -689,7 +679,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { case PLAYING: writePlayerStatusPlaybackPreferences(); - setupNotification(newInfo); started = true; // set sleep timer if auto-enabled if (newInfo.oldPlayerStatus != null && newInfo.oldPlayerStatus != PlayerStatus.SEEKING && @@ -701,7 +690,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { case ERROR: writePlaybackPreferencesNoMediaPlaying(); - stopService(); break; } @@ -714,7 +702,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { @Override public void shouldStop() { - stopService(); + stopService(); // TODO: [2716] unclear if stopService() call is still needed } @Override @@ -763,7 +751,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { } sendNotificationBroadcast(NOTIFICATION_TYPE_ERROR, what); writePlaybackPreferencesNoMediaPlaying(); - stopService(); return true; } @@ -848,7 +835,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { taskManager.cancelPositionSaver(); writePlaybackPreferencesNoMediaPlaying(); if (!isCasting) { - stopForeground(true); + stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed } } if (mediaType == null) { @@ -1063,6 +1050,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { private void updateMediaSession(final PlayerStatus playerStatus) { PlaybackStateCompat.Builder sessionState = new PlaybackStateCompat.Builder(); + @PlaybackStateCompat.State int state; if (playerStatus != null) { switch (playerStatus) { @@ -1126,7 +1114,9 @@ public class PlaybackService extends MediaBrowserServiceCompat { flavorHelper.mediaSessionSetExtraForWear(mediaSession); - mediaSession.setPlaybackState(sessionState.build()); + final PlaybackStateCompat sessionStateBuilt = sessionState.build(); + mediaSession.setPlaybackState(sessionStateBuilt); + serviceManager.onPlaybackStateChange(sessionStateBuilt); } private static boolean useSkipToPreviousForRewindInLockscreen() { @@ -1203,13 +1193,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { */ private Thread notificationSetupThread; - /** - * Prepares notification and starts the service in the foreground. - */ - private void setupNotification(final PlaybackServiceMediaPlayer.PSMPInfo info) { - setupNotification(info.playable); - } - private synchronized void setupNotification(final Playable playable) { if (notificationSetupThread != null) { notificationSetupThread.interrupt(); @@ -1226,7 +1209,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { @Override public void run() { - Log.d(TAG, "Starting background work"); + Log.d(TAG, "notificationSetupTask: Starting background work"); if (mediaPlayer == null) { Log.d(TAG, "notificationSetupTask: mediaPlayer is null"); @@ -1256,6 +1239,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { } PlayerStatus playerStatus = mediaPlayer.getPlayerStatus(); + Log.v(TAG, "notificationSetupTask: playerStatus=" + playerStatus); if (!Thread.currentThread().isInterrupted() && started) { String contentText = playable.getEpisodeTitle(); @@ -1349,7 +1333,22 @@ public class PlaybackService extends MediaBrowserServiceCompat { playerStatus == PlayerStatus.PREPARING || playerStatus == PlayerStatus.SEEKING || isCasting) { + Log.v(TAG, "DBG - notificationSetupTask: make service foreground"); startForeground(NOTIFICATION_ID, notification); + } else if (playerStatus == PlayerStatus.PAUSED) { + // TODO: logic originally in mediaPlayerCallback case PAUSED. + if ((UserPreferences.isPersistNotify() || isCasting) && + android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) { + // do not remove notification on pause based on user pref and whether android version supports expanded notifications + // Change [Play] button to [Pause] + // TODO LATER: logic same as outer else clause: setupNotification(info) + stopForeground(false); + NotificationManager mNotificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + mNotificationManager.notify(NOTIFICATION_ID, notification); + } else if (!UserPreferences.isPersistNotify() && !isCasting) { + // remove notification on pause + stopForeground(true); + } } else { stopForeground(false); NotificationManager mNotificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); @@ -1846,12 +1845,10 @@ public class PlaybackService extends MediaBrowserServiceCompat { void setIsCasting(boolean isCasting); - void sendNotificationBroadcast(int type, int code); + void sendNotificationBroadcast(int type, int code); // TODO: unclear if the broadcast is still needed with ServiceManager void saveCurrentPosition(boolean fromMediaPlayer, Playable playable, int position); - void setupNotification(boolean connected, PlaybackServiceMediaPlayer.PSMPInfo info); - MediaSessionCompat getMediaSession(); Intent registerReceiver(BroadcastReceiver receiver, IntentFilter filter); @@ -1890,24 +1887,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { PlaybackService.this.saveCurrentPosition(fromMediaPlayer, playable, position); } - @Override - public void setupNotification(boolean connected, PlaybackServiceMediaPlayer.PSMPInfo info) { - if (connected) { - PlaybackService.this.setupNotification(info); - } else { - PlayerStatus status = info.playerStatus; - if ((status == PlayerStatus.PLAYING || - status == PlayerStatus.SEEKING || - status == PlayerStatus.PREPARING || - UserPreferences.isPersistNotify()) && - android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) { - PlaybackService.this.setupNotification(info); - } else if (!UserPreferences.isPersistNotify()) { - PlaybackService.this.stopForeground(true); - } - } - } - @Override public MediaSessionCompat getMediaSession() { return PlaybackService.this.mediaSession; @@ -1923,4 +1902,80 @@ public class PlaybackService extends MediaBrowserServiceCompat { PlaybackService.this.unregisterReceiver(receiver); } }; + + /** + * The helper that manages PlaybackService's foreground service life cycle and the associated + * notification control. + * + * The logic is adapted from a sample app from The Android Open Source Project. + * See https://github.com/googlesamples/android-MediaBrowserService/blob/6cf01be9ef82ca2dd653f03e2a4af0b075cc0021/Application/src/main/java/com/example/android/mediasession/service/MusicService.java#L211 + * + */ + private class ServiceManager { + private boolean mServiceInStartedState; // TODO: rationalize it with parent's static started + // TODO LATER: rename the variable, do not use m prefix + + /** + * + * Entry point method for callers. Upon PlaybackState changes, + * the manager start/stop the PlaybackService as well as relevant notification + */ + void onPlaybackStateChange(PlaybackStateCompat state) { + // Report the state to the MediaSession. + + Log.v(TAG, "DBG - onPlaybackStateChange(" + state + ")"); + // Manage the started state of this service. + switch (state.getState()) { + case PlaybackStateCompat.STATE_PLAYING: + moveServiceToStartedState(state); + break; + case PlaybackStateCompat.STATE_PAUSED: + updateNotificationForPause(state); + break; + case PlaybackStateCompat.STATE_STOPPED: + moveServiceOutOfStartedState(state); + break; + case PlaybackStateCompat.STATE_ERROR: + moveServiceOutOfStartedState(state); + break; + } + } + + private void moveServiceToStartedState(PlaybackStateCompat state) { + Log.v(TAG, "DBG - ServiceManager.moveServiceToStartedState() - mServiceInStartedState:" + mServiceInStartedState); + + if (!mServiceInStartedState) { + ContextCompat.startForegroundService( + PlaybackService.this, + new Intent(PlaybackService.this, PlaybackService.class)); + mServiceInStartedState = true; + } + + doSetupNotification(); + } + + private void updateNotificationForPause(PlaybackStateCompat state) { + doSetupNotification(); + } + + private void moveServiceOutOfStartedState(PlaybackStateCompat state) { + stopForeground(true); + stopSelf(); + mServiceInStartedState = false; + } + + private void doSetupNotification() { + if (mediaPlayer != null && mediaPlayer.getPlayable() != null) { + // it updates notification and set foreground status + // based on player status (similar to PlaybackState) + setupNotification(mediaPlayer.getPlayable()); + } else { + // should not happen unless there are bugs. + Log.e(TAG, "doSetupNotification() - unexpectedly there is no playable. No notification setup done. mediaPlayer." + mediaPlayer); + } + } + } + + private final ServiceManager serviceManager = new ServiceManager(); + } diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java index 01d6812fd..81110da7b 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java @@ -13,7 +13,6 @@ import android.os.Build; import android.os.IBinder; import android.support.annotation.NonNull; import android.support.annotation.Nullable; -import android.support.v4.content.ContextCompat; import android.text.TextUtils; import android.util.Log; import android.util.Pair; @@ -105,6 +104,7 @@ public abstract class PlaybackController { } private synchronized void initServiceRunning() { + Log.v(TAG, "initServiceRunning()"); if (initialized) { return; } @@ -192,7 +192,7 @@ public abstract class PlaybackController { if (!PlaybackService.started) { if (intent != null) { Log.d(TAG, "Calling start service"); - ContextCompat.startForegroundService(activity, intent); + // ContextCompat.startForegroundService(activity, intent); // TODO: [2716] likely not needed but is not 100% sure bound = activity.bindService(intent, mConnection, 0); } else { status = PlayerStatus.STOPPED; @@ -784,6 +784,7 @@ public abstract class PlaybackController { } private void initServiceNotRunning() { + Log.v(TAG, "DBG - initServiceNotRunning()"); // TODO: review it it's still needed mediaLoader = Maybe.create((MaybeOnSubscribe) emitter -> { Playable media = getMedia(); if (media != null) { diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java index f7d2ee409..c42b6dd49 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java @@ -2,9 +2,6 @@ package de.danoeh.antennapod.core.util.playback; import android.content.Context; import android.content.Intent; -import android.media.MediaPlayer; -import android.support.annotation.NonNull; -import android.support.v4.content.ContextCompat; import de.danoeh.antennapod.core.preferences.PlaybackPreferences; import de.danoeh.antennapod.core.service.playback.PlaybackService; @@ -73,6 +70,6 @@ public class PlaybackServiceStarter { if (PlaybackService.isRunning && !callEvenIfRunning) { return; } - ContextCompat.startForegroundService(context, getIntent()); + context.startService(getIntent()); // the service itself will decide if it needs to become foreground } } diff --git a/core/src/play/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceFlavorHelper.java b/core/src/play/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceFlavorHelper.java index 0cca2ffa4..a6b732a4f 100644 --- a/core/src/play/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceFlavorHelper.java +++ b/core/src/play/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceFlavorHelper.java @@ -151,7 +151,6 @@ public class PlaybackServiceFlavorHelper { // hardware volume buttons control the local device volume mediaRouter.setMediaSessionCompat(null); unregisterWifiBroadcastReceiver(); - callback.setupNotification(false, info); } }; } @@ -181,7 +180,6 @@ public class PlaybackServiceFlavorHelper { // hardware volume buttons control the remote device volume mediaRouter.setMediaSessionCompat(callback.getMediaSession()); registerWifiBroadcastReceiver(); - callback.setupNotification(true, info); } private void switchMediaPlayer(@NonNull PlaybackServiceMediaPlayer newPlayer, From 76fbab8e82195ffa060a3d526fe1e2c90b5da04a Mon Sep 17 00:00:00 2001 From: orionlee Date: Thu, 3 Jan 2019 11:54:06 -0800 Subject: [PATCH 02/15] more #2716 - fix VideoPlayback upon hitting home button. --- .../activity/VideoplayerActivity.java | 21 +++++++++++++------ .../service/playback/PlaybackService.java | 1 + .../util/playback/PlaybackController.java | 1 + 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java b/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java index 78cc15b2c..d6983eac2 100644 --- a/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java +++ b/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java @@ -91,12 +91,22 @@ public class VideoplayerActivity extends MediaplayerActivity { @Override protected void onStop() { + stopPlaybackIfUserPreferencesSpecified(); // MUST be called before super.onStop(), while it still has member variable controller super.onStop(); if (!PictureInPictureUtil.isInPictureInPictureMode(this)) { videoControlsHider.stop(); } } + void stopPlaybackIfUserPreferencesSpecified() { + if (controller != null && !destroyingDueToReload + && UserPreferences.getVideoBackgroundBehavior() + != UserPreferences.VideoBackgroundBehavior.CONTINUE_PLAYING) { + Log.v(TAG, "stop video playback per UserPreference"); + controller.notifyVideoSurfaceAbandoned(); + } + } + @Override public void onUserLeaveHint () { if (!PictureInPictureUtil.isInPictureInPictureMode(this) && UserPreferences.getVideoBackgroundBehavior() @@ -275,13 +285,12 @@ public class VideoplayerActivity extends MediaplayerActivity { @Override public void surfaceDestroyed(SurfaceHolder holder) { - Log.d(TAG, "Videosurface was destroyed"); + Log.d(TAG, "Videosurface was destroyed." ); + Log.v(TAG, " hasController=" + (controller != null) + + " , destroyingDueToReload=" + destroyingDueToReload + + " , videoBackgroundBehavior=" + UserPreferences.getVideoBackgroundBehavior()); videoSurfaceCreated = false; - if (controller != null && !destroyingDueToReload - && UserPreferences.getVideoBackgroundBehavior() - != UserPreferences.VideoBackgroundBehavior.CONTINUE_PLAYING) { - controller.notifyVideoSurfaceAbandoned(); - } + stopPlaybackIfUserPreferencesSpecified(); } }; diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 632d0d41b..b0b3aee1c 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -605,6 +605,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { } public void notifyVideoSurfaceAbandoned() { + Log.v(TAG, "notifyVideoSurfaceAbandoned()"); mediaPlayer.pause(true, false); mediaPlayer.resetVideoSurface(); } diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java index 81110da7b..3f538da84 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java @@ -764,6 +764,7 @@ public abstract class PlaybackController { } public void notifyVideoSurfaceAbandoned() { + Log.v(TAG, "notifyVideoSurfaceAbandoned() - hasPlaybackService=" + (playbackService != null)); if (playbackService != null) { playbackService.notifyVideoSurfaceAbandoned(); } From e26a54bdbccac6eb9d9060b725192c575b3913bb Mon Sep 17 00:00:00 2001 From: orionlee Date: Sat, 5 Jan 2019 14:35:53 -0800 Subject: [PATCH 03/15] start playbackService code paths reviewed (context.startService() and ContextCompat.startForegroundService()) --- .../adapter/DefaultActionButtonCallback.java | 15 ++++++--------- .../core/service/playback/PlaybackService.java | 17 +++++++++++------ .../core/util/playback/PlaybackController.java | 4 ++-- .../util/playback/PlaybackServiceStarter.java | 7 +++++++ 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java b/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java index fda6d48a5..f54b9266e 100644 --- a/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java +++ b/app/src/main/java/de/danoeh/antennapod/adapter/DefaultActionButtonCallback.java @@ -21,13 +21,14 @@ import de.danoeh.antennapod.core.storage.DownloadRequester; import de.danoeh.antennapod.core.util.IntentUtils; import de.danoeh.antennapod.core.util.LongList; import de.danoeh.antennapod.core.util.NetworkUtils; +import de.danoeh.antennapod.core.util.playback.PlaybackServiceStarter; /** * Default implementation of an ActionButtonCallback */ public class DefaultActionButtonCallback implements ActionButtonCallback { - private static final String TAG = "DefaultActionButtonCallback"; + private static final String TAG = "DefaultActionBtnCb"; private final Context context; @@ -81,16 +82,12 @@ public class DefaultActionButtonCallback implements ActionButtonCallback { } } else { // media is downloaded if (media.isCurrentlyPlaying()) { -// new PlaybackServiceStarter(context, media) // TODO: [2716] probably not needed but not 100% sure -// .startWhenPrepared(true) -// .shouldStream(false) -// .start(); IntentUtils.sendLocalBroadcast(context, PlaybackService.ACTION_PAUSE_PLAY_CURRENT_EPISODE); } else if (media.isCurrentlyPaused()) { -// new PlaybackServiceStarter(context, media) // TODO: [2716] probably not needed but not 100% sure -// .startWhenPrepared(true) -// .shouldStream(false) -// .start(); + new PlaybackServiceStarter(context, media) // need to start the service in case it's been stopped by system. + .startWhenPrepared(true) + .shouldStream(false) + .start(); IntentUtils.sendLocalBroadcast(context, PlaybackService.ACTION_RESUME_PLAY_CURRENT_EPISODE); } else { DBTasks.playMedia(context, media, false, true, false); diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index b0b3aee1c..f34866c25 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -76,7 +76,13 @@ import de.greenrobot.event.EventBus; /** * Controls the MediaPlayer that plays a FeedMedia-file - * TODO [2716] add some guidelines on how to / not to start / stop the service from callers. + * + * Callers should connect to the service with either: + * - .bindService() + * - .startService(), optionally with arguments such as media to be played. + * + * Caller should not call startForegroundService(). The PlaybackService will make itself foreground + * when appropriate. */ public class PlaybackService extends MediaBrowserServiceCompat { /** @@ -463,11 +469,10 @@ public class PlaybackService extends MediaBrowserServiceCompat { final boolean castDisconnect = intent.getBooleanExtra(EXTRA_CAST_DISCONNECT, false); Playable playable = intent.getParcelableExtra(EXTRA_PLAYABLE); if (keycode == -1 && playable == null && !castDisconnect) { - Log.e(TAG, "PlaybackService was started with no arguments"); - // some code call startService with no arg, causing playback to stop prematurely in typical case - // temporarily comment it out for now. - // TODO: Next step - find the source of the no-arg service start -// stopService(); // TODO: find the source of starting service with no argument that causes me to comment out this line temporarily + // Typical cases when the service was started with no argument + // - when it is first bound, and then moved to startedState, as in serviceManager.moveServiceToStartedState() + // - callers (e.g., Controller) explicitly + Log.d(TAG, "PlaybackService was started with no arguments."); return Service.START_NOT_STICKY; } diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java index 3f538da84..565ef8a96 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java @@ -192,7 +192,6 @@ public abstract class PlaybackController { if (!PlaybackService.started) { if (intent != null) { Log.d(TAG, "Calling start service"); - // ContextCompat.startForegroundService(activity, intent); // TODO: [2716] likely not needed but is not 100% sure bound = activity.bindService(intent, mConnection, 0); } else { status = PlayerStatus.STOPPED; @@ -587,7 +586,8 @@ public abstract class PlaybackController { .startWhenPrepared(true) .streamIfLastWasStream() .start(); - Log.w(TAG, "Play/Pause button was pressed, but playbackservice was null!"); + Log.d(TAG, "Play/Pause button was pressed, but playbackservice was null - " + + "it is likely to have been released by Android system. Restarting it."); return; } switch (status) { diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java index c42b6dd49..a6cf500c1 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java @@ -2,11 +2,14 @@ package de.danoeh.antennapod.core.util.playback; import android.content.Context; import android.content.Intent; +import android.util.Log; import de.danoeh.antennapod.core.preferences.PlaybackPreferences; import de.danoeh.antennapod.core.service.playback.PlaybackService; public class PlaybackServiceStarter { + private static final String TAG = "PlaybackServiceStarter"; + private final Context context; private final Playable media; private boolean startWhenPrepared = false; @@ -63,6 +66,10 @@ public class PlaybackServiceStarter { launchIntent.putExtra(PlaybackService.EXTRA_SHOULD_STREAM, shouldStream); launchIntent.putExtra(PlaybackService.EXTRA_PREPARE_IMMEDIATELY, prepareImmediately); + if (media == null) { + Log.e(TAG, "getIntent() - media is unexpectedly null. intent:" + launchIntent); + } + return launchIntent; } From 3f14fac4790f54a911f5b4b5ee8a81cf2ee0eea3 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sat, 5 Jan 2019 19:12:29 -0800 Subject: [PATCH 04/15] remove static PlaybackService.started, in favor of the start state managed by inner ServiceManager. Also add a generic java8-like Optional class for use with RxJava2 where null was to be returned (RxJava2 requires non-null). --- .../service/playback/PlaybackService.java | 32 ++- .../danoeh/antennapod/core/util/Optional.java | 213 ++++++++++++++++++ .../util/playback/PlaybackController.java | 32 ++- 3 files changed, 240 insertions(+), 37 deletions(-) create mode 100644 core/src/main/java/de/danoeh/antennapod/core/util/Optional.java diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index f34866c25..10175b59c 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -200,10 +200,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { * Is true if service is running. */ public static boolean isRunning = false; - /** - * Is true if service has received a valid start command. - */ - public static boolean started = false; /** * Is true if the service was running, but paused due to headphone disconnect */ @@ -349,7 +345,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { Log.d(TAG, "Service is about to be destroyed"); stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed isRunning = false; - started = false; currentMediaType = MediaType.UNKNOWN; PreferenceManager.getDefaultSharedPreferences(this) @@ -488,7 +483,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { return Service.START_NOT_STICKY; } } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { - started = true; boolean stream = intent.getBooleanExtra(EXTRA_SHOULD_STREAM, true); boolean startWhenPrepared = intent.getBooleanExtra(EXTRA_START_WHEN_PREPARED, false); @@ -576,7 +570,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { case KeyEvent.KEYCODE_MEDIA_STOP: if (status == PlayerStatus.PLAYING) { mediaPlayer.pause(true, true); - started = false; } stopForeground(true); // gets rid of persistent notification // TODO: [2716] unclear if stopForeground is still needed @@ -595,7 +588,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { Playable playable = Playable.PlayableUtils.createInstanceFromPreferences(getApplicationContext()); if (playable != null) { mediaPlayer.playMediaObject(playable, false, true, true); - started = true; PlaybackService.this.updateMediaSessionMetadata(playable); } } @@ -685,7 +677,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { case PLAYING: writePlayerStatusPlaybackPreferences(); - started = true; // set sleep timer if auto-enabled if (newInfo.oldPlayerStatus != null && newInfo.oldPlayerStatus != PlayerStatus.SEEKING && SleepTimerPreferences.autoEnable() && !sleepTimerActive()) { @@ -1176,7 +1167,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { builder.putString(MediaMetadataCompat.METADATA_KEY_DISPLAY_ICON_URI, imageLocation); } } - if (!Thread.currentThread().isInterrupted() && started) { + if (!Thread.currentThread().isInterrupted() && isStarted()) { mediaSession.setSessionActivity(PendingIntent.getActivity(this, 0, PlaybackService.getPlayerActivityIntent(this), PendingIntent.FLAG_UPDATE_CURRENT)); @@ -1205,7 +1196,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { } if (playable == null) { Log.d(TAG, "setupNotification: playable is null"); - if (!started) { + if (!isStarted()) { stopService(); } return; @@ -1219,7 +1210,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { if (mediaPlayer == null) { Log.d(TAG, "notificationSetupTask: mediaPlayer is null"); - if (!started) { + if (!isStarted()) { stopService(); } return; @@ -1247,7 +1238,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { PlayerStatus playerStatus = mediaPlayer.getPlayerStatus(); Log.v(TAG, "notificationSetupTask: playerStatus=" + playerStatus); - if (!Thread.currentThread().isInterrupted() && started) { + if (!Thread.currentThread().isInterrupted() && isStarted()) { String contentText = playable.getEpisodeTitle(); String contentTitle = playable.getFeedTitle(); Notification notification; @@ -1909,6 +1900,10 @@ public class PlaybackService extends MediaBrowserServiceCompat { } }; + private boolean isStarted() { + return serviceManager.serviceInStartedState; + } + /** * The helper that manages PlaybackService's foreground service life cycle and the associated * notification control. @@ -1918,8 +1913,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { * */ private class ServiceManager { - private boolean mServiceInStartedState; // TODO: rationalize it with parent's static started - // TODO LATER: rename the variable, do not use m prefix + private boolean serviceInStartedState; /** * @@ -1948,13 +1942,13 @@ public class PlaybackService extends MediaBrowserServiceCompat { } private void moveServiceToStartedState(PlaybackStateCompat state) { - Log.v(TAG, "DBG - ServiceManager.moveServiceToStartedState() - mServiceInStartedState:" + mServiceInStartedState); + Log.v(TAG, "DBG - ServiceManager.moveServiceToStartedState() - serviceInStartedState:" + serviceInStartedState); - if (!mServiceInStartedState) { + if (!serviceInStartedState) { ContextCompat.startForegroundService( PlaybackService.this, new Intent(PlaybackService.this, PlaybackService.class)); - mServiceInStartedState = true; + serviceInStartedState = true; } doSetupNotification(); @@ -1967,7 +1961,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { private void moveServiceOutOfStartedState(PlaybackStateCompat state) { stopForeground(true); stopSelf(); - mServiceInStartedState = false; + serviceInStartedState = false; } private void doSetupNotification() { diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/Optional.java b/core/src/main/java/de/danoeh/antennapod/core/util/Optional.java new file mode 100644 index 000000000..0fe11ec53 --- /dev/null +++ b/core/src/main/java/de/danoeh/antennapod/core/util/Optional.java @@ -0,0 +1,213 @@ +/* + * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package de.danoeh.antennapod.core.util; + +import java.util.NoSuchElementException; +import java.util.Objects; + +// AntennaPod's stripped-down version of Java/Android platform's java.util.Optional +// so that it can be used on lower API level (API level 14) + +// Android-changed: removed ValueBased paragraph. +/** + * A container object which may or may not contain a non-null value. + * If a value is present, {@code isPresent()} will return {@code true} and + * {@code get()} will return the value. + * + *

Additional methods that depend on the presence or absence of a contained + * value are provided, such as {@link #orElse(java.lang.Object) orElse()} + * (return a default value if value not present) and + * {@link #ifPresent(java.util.function.Consumer) ifPresent()} (execute a block + * of code if the value is present). + * + * @since 1.8 + */ +public final class Optional { + /** + * Common instance for {@code empty()}. + */ + private static final Optional EMPTY = new Optional<>(); + + /** + * If non-null, the value; if null, indicates no value is present + */ + private final T value; + + /** + * Constructs an empty instance. + * + * @implNote Generally only one empty instance, {@link Optional#EMPTY}, + * should exist per VM. + */ + private Optional() { + this.value = null; + } + + /** + * Returns an empty {@code Optional} instance. No value is present for this + * Optional. + * + * @apiNote Though it may be tempting to do so, avoid testing if an object + * is empty by comparing with {@code ==} against instances returned by + * {@code Option.empty()}. There is no guarantee that it is a singleton. + * Instead, use {@link #isPresent()}. + * + * @param Type of the non-existent value + * @return an empty {@code Optional} + */ + public static Optional empty() { + @SuppressWarnings("unchecked") + Optional t = (Optional) EMPTY; + return t; + } + + /** + * Constructs an instance with the value present. + * + * @param value the non-null value to be present + * @throws NullPointerException if value is null + */ + private Optional(T value) { + this.value = Objects.requireNonNull(value); + } + + /** + * Returns an {@code Optional} with the specified present non-null value. + * + * @param the class of the value + * @param value the value to be present, which must be non-null + * @return an {@code Optional} with the value present + * @throws NullPointerException if value is null + */ + public static Optional of(T value) { + return new Optional<>(value); + } + + /** + * Returns an {@code Optional} describing the specified value, if non-null, + * otherwise returns an empty {@code Optional}. + * + * @param the class of the value + * @param value the possibly-null value to describe + * @return an {@code Optional} with a present value if the specified value + * is non-null, otherwise an empty {@code Optional} + */ + public static Optional ofNullable(T value) { + return value == null ? empty() : of(value); + } + + /** + * If a value is present in this {@code Optional}, returns the value, + * otherwise throws {@code NoSuchElementException}. + * + * @return the non-null value held by this {@code Optional} + * @throws NoSuchElementException if there is no value present + * + * @see Optional#isPresent() + */ + public T get() { + if (value == null) { + throw new NoSuchElementException("No value present"); + } + return value; + } + + /** + * Return {@code true} if there is a value present, otherwise {@code false}. + * + * @return {@code true} if there is a value present, otherwise {@code false} + */ + public boolean isPresent() { + return value != null; + } + + + /** + * Return the value if present, otherwise return {@code other}. + * + * @param other the value to be returned if there is no value present, may + * be null + * @return the value, if present, otherwise {@code other} + */ + public T orElse(T other) { + return value != null ? value : other; + } + + /** + * Indicates whether some other object is "equal to" this Optional. The + * other object is considered equal if: + *

    + *
  • it is also an {@code Optional} and; + *
  • both instances have no value present or; + *
  • the present values are "equal to" each other via {@code equals()}. + *
+ * + * @param obj an object to be tested for equality + * @return {code true} if the other object is "equal to" this object + * otherwise {@code false} + */ + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (!(obj instanceof Optional)) { + return false; + } + + Optional other = (Optional) obj; + return (value == other.value) || (value != null && value.equals(other.value)); + } + + /** + * Returns the hash code value of the present value, if any, or 0 (zero) if + * no value is present. + * + * @return hash code value of the present value or 0 if no value is present + */ + @Override + public int hashCode() { + return value != null ? value.hashCode() : 0; + } + + /** + * Returns a non-empty string representation of this Optional suitable for + * debugging. The exact presentation format is unspecified and may vary + * between implementations and versions. + * + * @implSpec If a value is present the result must include its string + * representation in the result. Empty and present Optionals must be + * unambiguously differentiable. + * + * @return the string representation of this instance + */ + @Override + public String toString() { + return value != null + ? String.format("Optional[%s]", value) + : "Optional.empty"; + } +} diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java index 565ef8a96..fd87c9905 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java @@ -12,7 +12,6 @@ import android.media.MediaPlayer; import android.os.Build; import android.os.IBinder; import android.support.annotation.NonNull; -import android.support.annotation.Nullable; import android.text.TextUtils; import android.util.Log; import android.util.Pair; @@ -36,6 +35,7 @@ import de.danoeh.antennapod.core.service.playback.PlaybackServiceMediaPlayer; import de.danoeh.antennapod.core.service.playback.PlayerStatus; import de.danoeh.antennapod.core.storage.DBTasks; import de.danoeh.antennapod.core.util.Converter; +import de.danoeh.antennapod.core.util.Optional; import de.danoeh.antennapod.core.util.playback.Playable.PlayableUtils; import io.reactivex.Maybe; import io.reactivex.MaybeOnSubscribe; @@ -187,21 +187,15 @@ public abstract class PlaybackController { serviceBinder = Observable.fromCallable(this::getPlayLastPlayedMediaIntent) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) - .subscribe(intent -> { + .subscribe(optionalIntent -> { boolean bound = false; - if (!PlaybackService.started) { - if (intent != null) { - Log.d(TAG, "Calling start service"); - bound = activity.bindService(intent, mConnection, 0); - } else { - status = PlayerStatus.STOPPED; - setupGUI(); - handleStatus(); - } + if (optionalIntent.isPresent()) { + Log.d(TAG, "Calling bind service"); + bound = activity.bindService(optionalIntent.get(), mConnection, 0); } else { - Log.d(TAG, "PlaybackService is running, trying to connect without start command."); - bound = activity.bindService(new Intent(activity, PlaybackService.class), - mConnection, 0); + status = PlayerStatus.STOPPED; + setupGUI(); + handleStatus(); } Log.d(TAG, "Result for service binding: " + bound); }, error -> Log.e(TAG, Log.getStackTraceString(error))); @@ -211,24 +205,26 @@ public abstract class PlaybackController { * Returns an intent that starts the PlaybackService and plays the last * played media or null if no last played media could be found. */ - @Nullable private Intent getPlayLastPlayedMediaIntent() { + @NonNull + private Optional getPlayLastPlayedMediaIntent() { Log.d(TAG, "Trying to restore last played media"); Playable media = PlayableUtils.createInstanceFromPreferences(activity); if (media == null) { Log.d(TAG, "No last played media found"); - return null; + return Optional.empty(); } + boolean fileExists = media.localFileAvailable(); boolean lastIsStream = PlaybackPreferences.getCurrentEpisodeIsStream(); if (!fileExists && !lastIsStream && media instanceof FeedMedia) { DBTasks.notifyMissingFeedMediaFile(activity, (FeedMedia) media); } - return new PlaybackServiceStarter(activity, media) + return Optional.of(new PlaybackServiceStarter(activity, media) .startWhenPrepared(false) .shouldStream(lastIsStream || !fileExists) - .getIntent(); + .getIntent()); } From 584865ad180e54761d5e3a4b88a4f34f162f682e Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 6 Jan 2019 12:26:06 -0800 Subject: [PATCH 05/15] review stop PlaybackService codes (stopSelf, stopForeground, etc.) --- .../service/playback/PlaybackService.java | 113 ++++++++++++------ 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 10175b59c..6c6213f2b 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -343,7 +343,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { public void onDestroy() { super.onDestroy(); Log.d(TAG, "Service is about to be destroyed"); - stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed + stopForeground(true); isRunning = false; currentMediaType = MediaType.UNKNOWN; @@ -366,11 +366,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { taskManager.shutdown(); } - private void stopService() { - stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed - stopSelf(); - } - @Override public BrowserRoot onGetRoot(@NonNull String clientPackageName, int clientUid, Bundle rootHints) { Log.d(TAG, "OnGetRoot: clientPackageName=" + clientPackageName + @@ -473,13 +468,13 @@ public class PlaybackService extends MediaBrowserServiceCompat { if ((flags & Service.START_FLAG_REDELIVERY) != 0) { Log.d(TAG, "onStartCommand is a redelivered intent, calling stopForeground now."); - stopForeground(true); // TODO: unclear if stopForeground is still needed + stopForeground(true); // TODO: [2716]-RemoveOptional legacy code from v0.9.8.1 that is no longer applicable, which used to return Service.START_REDELIVER_INTENT. See } else { if (keycode != -1) { Log.d(TAG, "Received media button event"); boolean handled = handleKeycode(keycode, true); if (!handled) { - stopService(); + serviceManager.stopService(); // TODO: [2716]-RemoveOptional why is it added in the first place in v1.7.0? return Service.START_NOT_STICKY; } } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { @@ -568,11 +563,23 @@ public class PlaybackService extends MediaBrowserServiceCompat { mediaPlayer.seekDelta(-UserPreferences.getRewindSecs() * 1000); return true; case KeyEvent.KEYCODE_MEDIA_STOP: + // The logic gives UI illusion of stop by removing notification + // In the UI within AntennaPod, including widgets, it is seen as PAUSE, e.g., + // users can still user on-screen widget to resume playing. if (status == PlayerStatus.PLAYING) { + // Implementation note: Use of a state in serviceManager to tell it to + // show stop state UI (i.e., stopForeground(true)) is a bit awkward. + // + // More intuitive API would be for mediaPlayer.pause() returns a Future that + // returns after pause, including the related async notification work completes. + // However, it has its own complication, that mediaPlayer.pause() does not + // really know when all the related work completes, as they are buried into + // (asynchronous) callbacks. + serviceManager.treatNextPauseAsStopOnUI(); mediaPlayer.pause(true, true); + } else { + serviceManager.showUIForStopState(); } - - stopForeground(true); // gets rid of persistent notification // TODO: [2716] unclear if stopForeground is still needed return true; default: Log.d(TAG, "Unhandled key code: " + keycode); @@ -672,7 +679,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { case STOPPED: //writePlaybackPreferencesNoMediaPlaying(); - //stopService(); + //serviceManager.stopService(); break; case PLAYING: @@ -699,7 +706,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { @Override public void shouldStop() { - stopService(); // TODO: [2716] unclear if stopService() call is still needed + serviceManager.stopService(); } @Override @@ -831,9 +838,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { if (stopPlaying) { taskManager.cancelPositionSaver(); writePlaybackPreferencesNoMediaPlaying(); - if (!isCasting) { - stopForeground(true); // TODO: [2716] unclear if stopForeground is still needed - } } if (mediaType == null) { sendNotificationBroadcast(NOTIFICATION_TYPE_PLAYBACK_END, 0); @@ -1190,14 +1194,18 @@ public class PlaybackService extends MediaBrowserServiceCompat { */ private Thread notificationSetupThread; - private synchronized void setupNotification(final Playable playable) { + private void setupNotification(final Playable playable) { + setupNotification(playable, false); + } + + private synchronized void setupNotification(final Playable playable, boolean treatPauseAsStop) { if (notificationSetupThread != null) { notificationSetupThread.interrupt(); } if (playable == null) { Log.d(TAG, "setupNotification: playable is null"); if (!isStarted()) { - stopService(); + serviceManager.stopService(); } return; } @@ -1211,7 +1219,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { if (mediaPlayer == null) { Log.d(TAG, "notificationSetupTask: mediaPlayer is null"); if (!isStarted()) { - stopService(); + serviceManager.stopService(); } return; } @@ -1334,7 +1342,9 @@ public class PlaybackService extends MediaBrowserServiceCompat { startForeground(NOTIFICATION_ID, notification); } else if (playerStatus == PlayerStatus.PAUSED) { // TODO: logic originally in mediaPlayerCallback case PAUSED. - if ((UserPreferences.isPersistNotify() || isCasting) && + if (treatPauseAsStop) { + stopForeground(true); + } else if ((UserPreferences.isPersistNotify() || isCasting) && android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) { // do not remove notification on pause based on user pref and whether android version supports expanded notifications // Change [Play] button to [Pause] @@ -1548,7 +1558,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { @Override public void onReceive(Context context, Intent intent) { if (TextUtils.equals(intent.getAction(), ACTION_SHUTDOWN_PLAYBACK_SERVICE)) { - stopService(); + serviceManager.stopService(); } } @@ -1914,6 +1924,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { */ private class ServiceManager { private boolean serviceInStartedState; + private boolean toTreatNextPauseAsStopOnUI = false; /** * @@ -1924,23 +1935,51 @@ public class PlaybackService extends MediaBrowserServiceCompat { // Report the state to the MediaSession. Log.v(TAG, "DBG - onPlaybackStateChange(" + state + ")"); - // Manage the started state of this service. - switch (state.getState()) { - case PlaybackStateCompat.STATE_PLAYING: - moveServiceToStartedState(state); - break; - case PlaybackStateCompat.STATE_PAUSED: - updateNotificationForPause(state); - break; - case PlaybackStateCompat.STATE_STOPPED: - moveServiceOutOfStartedState(state); - break; - case PlaybackStateCompat.STATE_ERROR: - moveServiceOutOfStartedState(state); - break; + try { + // Manage the started state of this service. + switch (state.getState()) { + case PlaybackStateCompat.STATE_PLAYING: + moveServiceToStartedState(state); + break; + case PlaybackStateCompat.STATE_PAUSED: + updateNotificationForPause(state); + break; + case PlaybackStateCompat.STATE_STOPPED: + moveServiceOutOfStartedState(state); + break; + case PlaybackStateCompat.STATE_ERROR: + moveServiceOutOfStartedState(state); + break; + } + } finally { + if (toTreatNextPauseAsStopOnUI) { + Log.v(TAG, "onPlaybackStateChange() - toTreatNextPauseAsStopOnUI enabled. The actual state (should be PAUSED, aka 2): " + state.getState()); + toTreatNextPauseAsStopOnUI = false; + } } } + /** + * Tell service manager that on the next state change, if the state is STATE_PAUSED, + * give UI treatment as if it is stopped. + * + * @see #handleKeycode(int, boolean) the use case + */ + public void treatNextPauseAsStopOnUI() { + this.toTreatNextPauseAsStopOnUI = true; + } + + public void showUIForStopState() { + Log.v(TAG, "serviceManager.showUIForStopState()"); + stopForeground(true); // gets rid of persistent notification, to give the UI illusion of STOP + } + + public void stopService() { + stopForeground(true); + stopSelf(); + serviceInStartedState = false; + } + private void moveServiceToStartedState(PlaybackStateCompat state) { Log.v(TAG, "DBG - ServiceManager.moveServiceToStartedState() - serviceInStartedState:" + serviceInStartedState); @@ -1959,16 +1998,14 @@ public class PlaybackService extends MediaBrowserServiceCompat { } private void moveServiceOutOfStartedState(PlaybackStateCompat state) { - stopForeground(true); - stopSelf(); - serviceInStartedState = false; + stopService(); } private void doSetupNotification() { if (mediaPlayer != null && mediaPlayer.getPlayable() != null) { // it updates notification and set foreground status // based on player status (similar to PlaybackState) - setupNotification(mediaPlayer.getPlayable()); + setupNotification(mediaPlayer.getPlayable(), toTreatNextPauseAsStopOnUI); } else { // should not happen unless there are bugs. Log.e(TAG, "doSetupNotification() - unexpectedly there is no playable. No notification setup done. mediaPlayer." + mediaPlayer); From f0f862393777689148851e03877a01b46480183e Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 6 Jan 2019 13:07:49 -0800 Subject: [PATCH 06/15] refactor stop PlaybackService codes - remove legacy dead codes --- .../service/playback/PlaybackService.java | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 6c6213f2b..9fd752c10 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -466,30 +466,25 @@ public class PlaybackService extends MediaBrowserServiceCompat { return Service.START_NOT_STICKY; } - if ((flags & Service.START_FLAG_REDELIVERY) != 0) { - Log.d(TAG, "onStartCommand is a redelivered intent, calling stopForeground now."); - stopForeground(true); // TODO: [2716]-RemoveOptional legacy code from v0.9.8.1 that is no longer applicable, which used to return Service.START_REDELIVER_INTENT. See - } else { - if (keycode != -1) { - Log.d(TAG, "Received media button event"); - boolean handled = handleKeycode(keycode, true); - if (!handled) { - serviceManager.stopService(); // TODO: [2716]-RemoveOptional why is it added in the first place in v1.7.0? - return Service.START_NOT_STICKY; - } - } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { - boolean stream = intent.getBooleanExtra(EXTRA_SHOULD_STREAM, - true); - boolean startWhenPrepared = intent.getBooleanExtra(EXTRA_START_WHEN_PREPARED, false); - boolean prepareImmediately = intent.getBooleanExtra(EXTRA_PREPARE_IMMEDIATELY, false); - sendNotificationBroadcast(NOTIFICATION_TYPE_RELOAD, 0); - //If the user asks to play External Media, the casting session, if on, should end. - flavorHelper.castDisconnect(playable instanceof ExternalMedia); - if (playable instanceof FeedMedia) { - playable = DBReader.getFeedMedia(((FeedMedia) playable).getId()); - } - mediaPlayer.playMediaObject(playable, stream, startWhenPrepared, prepareImmediately); + if (keycode != -1) { + Log.d(TAG, "Received media button event"); + boolean handled = handleKeycode(keycode, true); + if (!handled) { + serviceManager.stopService(); // TODO: [2716]-RemoveOptional why is it added in the first place in v1.7.0? + return Service.START_NOT_STICKY; } + } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { + boolean stream = intent.getBooleanExtra(EXTRA_SHOULD_STREAM, + true); + boolean startWhenPrepared = intent.getBooleanExtra(EXTRA_START_WHEN_PREPARED, false); + boolean prepareImmediately = intent.getBooleanExtra(EXTRA_PREPARE_IMMEDIATELY, false); + sendNotificationBroadcast(NOTIFICATION_TYPE_RELOAD, 0); + //If the user asks to play External Media, the casting session, if on, should end. + flavorHelper.castDisconnect(playable instanceof ExternalMedia); + if (playable instanceof FeedMedia) { + playable = DBReader.getFeedMedia(((FeedMedia) playable).getId()); + } + mediaPlayer.playMediaObject(playable, stream, startWhenPrepared, prepareImmediately); } return Service.START_NOT_STICKY; From 221cd4b480125566122af28893db1428d6db4cc7 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 6 Jan 2019 13:35:00 -0800 Subject: [PATCH 07/15] refactor stop PlaybackService setupNotification() - extract common code as private helper. --- .../core/service/playback/PlaybackService.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 9fd752c10..ddc52028d 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -1336,29 +1336,30 @@ public class PlaybackService extends MediaBrowserServiceCompat { Log.v(TAG, "DBG - notificationSetupTask: make service foreground"); startForeground(NOTIFICATION_ID, notification); } else if (playerStatus == PlayerStatus.PAUSED) { - // TODO: logic originally in mediaPlayerCallback case PAUSED. if (treatPauseAsStop) { stopForeground(true); } else if ((UserPreferences.isPersistNotify() || isCasting) && android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) { // do not remove notification on pause based on user pref and whether android version supports expanded notifications // Change [Play] button to [Pause] - // TODO LATER: logic same as outer else clause: setupNotification(info) - stopForeground(false); - NotificationManager mNotificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - mNotificationManager.notify(NOTIFICATION_ID, notification); + leaveNotificationAsBackground(notification); } else if (!UserPreferences.isPersistNotify() && !isCasting) { // remove notification on pause stopForeground(true); } } else { - stopForeground(false); - NotificationManager mNotificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - mNotificationManager.notify(NOTIFICATION_ID, notification); + leaveNotificationAsBackground(notification); } Log.d(TAG, "Notification set up"); } } + + private void leaveNotificationAsBackground(@NonNull Notification notification) { + stopForeground(false); + NotificationManager mNotificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + mNotificationManager.notify(NOTIFICATION_ID, notification); + } + }; notificationSetupThread = new Thread(notificationSetupTask); notificationSetupThread.start(); From 2c5db08e256afa7e56fd40461691413eab5c0dfc Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 6 Jan 2019 14:52:21 -0800 Subject: [PATCH 08/15] minor cleanup of Log codes and comments. --- .../antennapod/core/service/playback/PlaybackService.java | 8 +++----- .../antennapod/core/util/playback/PlaybackController.java | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index ddc52028d..065199f98 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -1333,7 +1333,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { playerStatus == PlayerStatus.PREPARING || playerStatus == PlayerStatus.SEEKING || isCasting) { - Log.v(TAG, "DBG - notificationSetupTask: make service foreground"); + Log.v(TAG, "notificationSetupTask: make service foreground"); startForeground(NOTIFICATION_ID, notification); } else if (playerStatus == PlayerStatus.PAUSED) { if (treatPauseAsStop) { @@ -1848,7 +1848,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { void setIsCasting(boolean isCasting); - void sendNotificationBroadcast(int type, int code); // TODO: unclear if the broadcast is still needed with ServiceManager + void sendNotificationBroadcast(int type, int code); void saveCurrentPosition(boolean fromMediaPlayer, Playable playable, int position); @@ -1930,7 +1930,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { void onPlaybackStateChange(PlaybackStateCompat state) { // Report the state to the MediaSession. - Log.v(TAG, "DBG - onPlaybackStateChange(" + state + ")"); + Log.v(TAG, "onPlaybackStateChange(" + (state != null ? state.getState() : "null") + ")"); try { // Manage the started state of this service. switch (state.getState()) { @@ -1977,8 +1977,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { } private void moveServiceToStartedState(PlaybackStateCompat state) { - Log.v(TAG, "DBG - ServiceManager.moveServiceToStartedState() - serviceInStartedState:" + serviceInStartedState); - if (!serviceInStartedState) { ContextCompat.startForegroundService( PlaybackService.this, diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java index fd87c9905..91009a196 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackController.java @@ -781,7 +781,7 @@ public abstract class PlaybackController { } private void initServiceNotRunning() { - Log.v(TAG, "DBG - initServiceNotRunning()"); // TODO: review it it's still needed + Log.v(TAG, "initServiceNotRunning()"); mediaLoader = Maybe.create((MaybeOnSubscribe) emitter -> { Playable media = getMedia(); if (media != null) { From ab78c1d4108551bb5691683bde81396612446c6e Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 7 Jan 2019 10:28:34 -0800 Subject: [PATCH 09/15] bug fix for Android8+: revert context.startService() calls back to ContextCompat.startForegroundService(), to ensure PlaybackService can be started. While PlaybackService itself ensures it will be raised to foreground when appropriate, Android 8+ forbids creating the (background) services to begin with (and throw IllegalStateException) in some situation (e.g., BroadcastReceiver). https://developer.android.com/about/versions/oreo/android-8.0-changes#atap --- .../danoeh/antennapod/core/receiver/MediaButtonReceiver.java | 3 ++- .../antennapod/core/service/playback/PlaybackService.java | 4 +--- .../antennapod/core/util/playback/PlaybackServiceStarter.java | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java b/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java index 3b52ac212..b191dbf8b 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java +++ b/core/src/main/java/de/danoeh/antennapod/core/receiver/MediaButtonReceiver.java @@ -3,6 +3,7 @@ package de.danoeh.antennapod.core.receiver; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; +import android.support.v4.content.ContextCompat; import android.util.Log; import android.view.KeyEvent; @@ -29,7 +30,7 @@ public class MediaButtonReceiver extends BroadcastReceiver { Intent serviceIntent = new Intent(context, PlaybackService.class); serviceIntent.putExtra(EXTRA_KEYCODE, event.getKeyCode()); serviceIntent.putExtra(EXTRA_SOURCE, event.getSource()); - context.startService(serviceIntent); // the service itself will determine if it needs to become a foreground service. + ContextCompat.startForegroundService(context, serviceIntent); } } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 065199f98..a0b368f86 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -79,10 +79,8 @@ import de.greenrobot.event.EventBus; * * Callers should connect to the service with either: * - .bindService() - * - .startService(), optionally with arguments such as media to be played. + * - ContextCompat.startForegroundService(), optionally with arguments, such as media to be played, in intent extras * - * Caller should not call startForegroundService(). The PlaybackService will make itself foreground - * when appropriate. */ public class PlaybackService extends MediaBrowserServiceCompat { /** diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java index a6cf500c1..64cf61457 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/playback/PlaybackServiceStarter.java @@ -2,6 +2,7 @@ package de.danoeh.antennapod.core.util.playback; import android.content.Context; import android.content.Intent; +import android.support.v4.content.ContextCompat; import android.util.Log; import de.danoeh.antennapod.core.preferences.PlaybackPreferences; @@ -77,6 +78,6 @@ public class PlaybackServiceStarter { if (PlaybackService.isRunning && !callEvenIfRunning) { return; } - context.startService(getIntent()); // the service itself will decide if it needs to become foreground + ContextCompat.startForegroundService(context, getIntent()); } } From a3389490bbc17fdf7001b0f6625621448e13c47c Mon Sep 17 00:00:00 2001 From: orionlee Date: Wed, 27 Feb 2019 14:59:31 -0800 Subject: [PATCH 10/15] ensure the service continues to run in the event it takes a long time for the service to load the media to play, e.g., streaming over a slow network. --- .../antennapod/core/service/playback/PlaybackService.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index a0b368f86..baa7a032f 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -1932,6 +1932,14 @@ public class PlaybackService extends MediaBrowserServiceCompat { try { // Manage the started state of this service. switch (state.getState()) { + case PlaybackStateCompat.STATE_CONNECTING: + // move the service to started, aka, making it foreground + // upon STATE_CONNECTING, i.e., in preparing to play a media. + // This is done so that in case the preparation takes a long time, e.g., + // streaming over a slow network, + // the service won't be killed by the system prematurely. + moveServiceToStartedState(state); + break; case PlaybackStateCompat.STATE_PLAYING: moveServiceToStartedState(state); break; From 600e0e561eb98a1a8e5f03f692c40ce858684078 Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 4 Mar 2019 13:44:53 -0800 Subject: [PATCH 11/15] coding style fix per review --- .../antennapod/core/service/playback/PlaybackService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index baa7a032f..f6fbe78cb 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -472,8 +472,7 @@ public class PlaybackService extends MediaBrowserServiceCompat { return Service.START_NOT_STICKY; } } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { - boolean stream = intent.getBooleanExtra(EXTRA_SHOULD_STREAM, - true); + boolean stream = intent.getBooleanExtra(EXTRA_SHOULD_STREAM, true); boolean startWhenPrepared = intent.getBooleanExtra(EXTRA_START_WHEN_PREPARED, false); boolean prepareImmediately = intent.getBooleanExtra(EXTRA_PREPARE_IMMEDIATELY, false); sendNotificationBroadcast(NOTIFICATION_TYPE_RELOAD, 0); From e94e95e84430f18b43899238f742c67256bb82e4 Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 4 Mar 2019 13:46:11 -0800 Subject: [PATCH 12/15] remove dead codes --- .../antennapod/core/service/playback/PlaybackService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index f6fbe78cb..44324be32 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -1186,10 +1186,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { */ private Thread notificationSetupThread; - private void setupNotification(final Playable playable) { - setupNotification(playable, false); - } - private synchronized void setupNotification(final Playable playable, boolean treatPauseAsStop) { if (notificationSetupThread != null) { notificationSetupThread.interrupt(); From 24915785eb76b57884a446e92bd24af9c5473c5e Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Mar 2019 15:01:48 -0800 Subject: [PATCH 13/15] remove the stopService() per review (which might unnecessarily stop the playback when unsupported media buttons are pressed.) --- .../antennapod/core/service/playback/PlaybackService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 44324be32..491493036 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -468,7 +468,8 @@ public class PlaybackService extends MediaBrowserServiceCompat { Log.d(TAG, "Received media button event"); boolean handled = handleKeycode(keycode, true); if (!handled) { - serviceManager.stopService(); // TODO: [2716]-RemoveOptional why is it added in the first place in v1.7.0? + // Just silently ignores unsupported keycode. Whether the service will + // continue to run is solely dependent on whether it is playing some media. return Service.START_NOT_STICKY; } } else if (!flavorHelper.castDisconnect(castDisconnect) && playable != null) { From 7a905c057025f877b56de95539da9a0768e016f9 Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Mar 2019 15:07:16 -0800 Subject: [PATCH 14/15] remove the commented stopService() call (to avoid future confusion) --- .../danoeh/antennapod/core/service/playback/PlaybackService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java index 491493036..c31dbc83d 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackService.java @@ -672,7 +672,6 @@ public class PlaybackService extends MediaBrowserServiceCompat { case STOPPED: //writePlaybackPreferencesNoMediaPlaying(); - //serviceManager.stopService(); break; case PLAYING: From 6f7b937d07642dc455e59a674a4f0d28784e0504 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sat, 9 Mar 2019 14:16:53 -0800 Subject: [PATCH 15/15] bugfix - video playback upon press back button (and pause), playback notification might reappear if one swipes it away quickly. --- .../antennapod/activity/VideoplayerActivity.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java b/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java index d6983eac2..fa5012b74 100644 --- a/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java +++ b/app/src/main/java/de/danoeh/antennapod/activity/VideoplayerActivity.java @@ -47,6 +47,7 @@ public class VideoplayerActivity extends MediaplayerActivity { */ private boolean videoControlsShowing = true; private boolean videoSurfaceCreated = false; + private boolean playbackStoppedUponExitVideo = false; private boolean destroyingDueToReload = false; private VideoControlsHider videoControlsHider = new VideoControlsHider(this); @@ -77,6 +78,7 @@ public class VideoplayerActivity extends MediaplayerActivity { @Override protected void onResume() { super.onResume(); + playbackStoppedUponExitVideo = false; if (TextUtils.equals(getIntent().getAction(), Intent.ACTION_VIEW)) { playExternalMedia(getIntent(), MediaType.VIDEO); } else if (PlaybackService.isCasting()) { @@ -99,6 +101,16 @@ public class VideoplayerActivity extends MediaplayerActivity { } void stopPlaybackIfUserPreferencesSpecified() { + // to avoid the method being called twice during leaving Videoplayer + // , which will double-pause the media + // (it is usually first called by surfaceHolderCallback.surfaceDestroyed(), + // then VideoplayerActivity.onStop() , but sometimes VideoplayerActivity.onStop() + // will first be invoked.) + if (playbackStoppedUponExitVideo) { + return; + } + playbackStoppedUponExitVideo = true; + if (controller != null && !destroyingDueToReload && UserPreferences.getVideoBackgroundBehavior() != UserPreferences.VideoBackgroundBehavior.CONTINUE_PLAYING) {