Merge pull request #2182 from domingos86/fix-episodes-deleted

fix unskipped episodes being removed from queue

4 files changed, 46 insertions, 34 deletions

diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java
index cad67539b..dd7bba964 100644
--- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java
+++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java
@@ -146,7 +146,7 @@ public class LocalPSMP extends PlaybackServiceMediaPlayer {
 
                 if (!media.getIdentifier().equals(playable.getIdentifier())) {
                     final Playable oldMedia = media;
-                    executor.submit(() -> callback.onPostPlayback(oldMedia, false, true));
+                    executor.submit(() -> callback.onPostPlayback(oldMedia, false, false, true));
                 }
 
                 setPlayerStatus(PlayerStatus.INDETERMINATE, null);
@@ -758,7 +758,8 @@ public class LocalPSMP extends PlaybackServiceMediaPlayer {
 
 
     @Override
-    protected Future<?> endPlayback(final boolean wasSkipped, final boolean shouldContinue, final boolean toStoppedState) {
+    protected Future<?> endPlayback(final boolean hasEnded, final boolean wasSkipped,
+                                    final boolean shouldContinue, final boolean toStoppedState) {
         return executor.submit(() -> {
             playerLock.lock();
             releaseWifiLockIfNecessary();
@@ -816,7 +817,7 @@ public class LocalPSMP extends PlaybackServiceMediaPlayer {
                 }
                 final boolean hasNext = nextMedia != null;
 
-                executor.submit(() -> callback.onPostPlayback(currentMedia, !wasSkipped, hasNext));
+                executor.submit(() -> callback.onPostPlayback(currentMedia, hasEnded, wasSkipped, hasNext));
             } else if (isPlaying) {
                 callback.onPlaybackPause(currentMedia, currentMedia.getPosition());
             }
@@ -883,7 +884,7 @@ public class LocalPSMP extends PlaybackServiceMediaPlayer {
             mp -> genericOnCompletion();
 
     private void genericOnCompletion() {
-        endPlayback(false, true, true);
+        endPlayback(true, false, true, true);
     }
 
     private final MediaPlayer.OnBufferingUpdateListener audioBufferingUpdateListener =
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 33aec0ee0..ee6c46130 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
@@ -685,8 +685,9 @@ public class PlaybackService extends MediaBrowserServiceCompat {
         }
 
         @Override
-        public void onPostPlayback(@NonNull Playable media, boolean ended, boolean playingNext) {
-            PlaybackService.this.onPostPlayback(media, ended, playingNext);
+        public void onPostPlayback(@NonNull Playable media, boolean ended, boolean skipped,
+                                   boolean playingNext) {
+            PlaybackService.this.onPostPlayback(media, ended, skipped, playingNext);
         }
 
         @Override
@@ -784,16 +785,20 @@ public class PlaybackService extends MediaBrowserServiceCompat {
      * Even though these tasks aren't supposed to be resource intensive, a good practice is to
      * usually call this method on a background thread.
      *
-     * @param playable the media object that was playing. It is assumed that its position property
-     *                 was updated before this method was called.
-     * @param ended if true, it signals that {@param playable} was played until its end.
-     *              In such case, the position property of the media becomes irrelevant for most of
-     *              the tasks (although it's still a good practice to keep it accurate).
-     * @param playingNext if true, it means another media object is being loaded in place of this one.
+     * @param playable    the media object that was playing. It is assumed that its position
+     *                    property was updated before this method was called.
+     * @param ended       if true, it signals that {@param playable} was played until its end.
+     *                    In such case, the position property of the media becomes irrelevant for
+     *                    most of the tasks (although it's still a good practice to keep it
+     *                    accurate).
+     * @param skipped     if the user pressed a skip >| button.
+     * @param playingNext if true, it means another media object is being loaded in place of this
+     *                    one.
      *                    Instances when we'd set it to false would be when we're not following the
      *                    queue or when the queue has ended.
      */
-    private void onPostPlayback(final Playable playable, boolean ended, boolean playingNext) {
+    private void onPostPlayback(final Playable playable, boolean ended, boolean skipped,
+                                boolean playingNext) {
         if (playable == null) {
             Log.e(TAG, "Cannot do post-playback processing: media was null");
             return;
@@ -824,7 +829,7 @@ public class PlaybackService extends MediaBrowserServiceCompat {
 
         if (item != null) {
             if (ended || smartMarkAsPlayed ||
-                    !UserPreferences.shouldSkipKeepEpisode()) {
+                    (skipped && !UserPreferences.shouldSkipKeepEpisode())) {
                 // only mark the item as played if we're not keeping it anyways
                 DBWriter.markItemPlayed(item, FeedItem.PLAYED, ended);
                 try {
@@ -845,7 +850,7 @@ public class PlaybackService extends MediaBrowserServiceCompat {
             }
         }
 
-        if (ended || playingNext) {
+        if (ended || skipped || playingNext) {
             DBWriter.addItemToPlaybackHistory(media);
         }
     }
diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceMediaPlayer.java b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceMediaPlayer.java
index aec059ca0..3b9a11610 100644
--- a/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceMediaPlayer.java
+++ b/core/src/main/java/de/danoeh/antennapod/core/service/playback/PlaybackServiceMediaPlayer.java
@@ -228,32 +228,35 @@ public abstract class PlaybackServiceMediaPlayer {
     protected abstract void setPlayable(Playable playable);
 
     public void skip() {
-        endPlayback(true, true, true);
+        endPlayback(false, true, true, true);
     }
 
     /**
      * Ends playback of current media (if any) and moves into INDETERMINATE state, unless
      * {@param toStoppedState} is set to true, in which case it moves into STOPPED state.
      *
-     * @see #endPlayback(boolean, boolean, boolean)
+     * @see #endPlayback(boolean, boolean, boolean, boolean)
      */
     public Future<?> stopPlayback(boolean toStoppedState) {
-        return endPlayback(true, false, toStoppedState);
+        return endPlayback(false, false, false, toStoppedState);
     }
 
     /**
      * Internal method that handles end of playback.
      *
-     * Currently, it has 4 use cases:
+     * Currently, it has 5 use cases:
      * <ul>
-     * <li>Media playback has completed: call with (false, true, true)</li>
-     * <li>User asks to skip to next episode: call with (true, true, true)</li>
-     * <li>Stopping the media player: call with (true, false, true)</li>
-     * <li>We want to change the media player implementation: call with (true, false, false)</li>
+     * <li>Media playback has completed: call with (true, false, true, true)</li>
+     * <li>User asks to skip to next episode: call with (false, true, true, true)</li>
+     * <li>Skipping to next episode due to playback error: call with (false, false, true, true)</li>
+     * <li>Stopping the media player: call with (false, false, false, true)</li>
+     * <li>We want to change the media player implementation: call with (false, false, false, false)</li>
      * </ul>
      *
-     * @param wasSkipped       If true, we assume the current media's playback has ended, for
+     * @param hasEnded         If true, we assume the current media's playback has ended, for
      *                         purposes of post playback processing.
+     * @param wasSkipped       Whether the user chose to skip the episode (by pressing the skip
+     *                         button).
      * @param shouldContinue   If true, the media player should try to load, and possibly play,
      *                         the next item, based on the user preferences and whether such item
      *                         exists.
@@ -264,7 +267,8 @@ public abstract class PlaybackServiceMediaPlayer {
      *
      * @return a Future, just for the purpose of tracking its execution.
      */
-    protected abstract Future<?> endPlayback(boolean wasSkipped, boolean shouldContinue, boolean toStoppedState);
+    protected abstract Future<?> endPlayback(boolean hasEnded, boolean wasSkipped,
+                                             boolean shouldContinue, boolean toStoppedState);
 
     /**
      * @return {@code true} if the WifiLock feature should be used, {@code false} otherwise.
@@ -346,7 +350,7 @@ public abstract class PlaybackServiceMediaPlayer {
 
         boolean onMediaPlayerError(Object inObj, int what, int extra);
 
-        void onPostPlayback(@NonNull Playable media, boolean ended, boolean playingNext);
+        void onPostPlayback(@NonNull Playable media, boolean ended, boolean skipped, boolean playingNext);
 
         void onPlaybackStart(@NonNull Playable playable, int position);
 
diff --git a/core/src/play/java/de/danoeh/antennapod/core/service/playback/RemotePSMP.java b/core/src/play/java/de/danoeh/antennapod/core/service/playback/RemotePSMP.java
index ea95ea894..79e058d0c 100644
--- a/core/src/play/java/de/danoeh/antennapod/core/service/playback/RemotePSMP.java
+++ b/core/src/play/java/de/danoeh/antennapod/core/service/playback/RemotePSMP.java
@@ -123,7 +123,8 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
             }
             if (playbackEnded) {
                 // This is an unconventional thing to occur...
-                endPlayback(true, true, true);
+                Log.w(TAG, "Somehow, Chromecast went from playing directly to standby mode");
+                endPlayback(false, false, true, true);
             }
         }
 
@@ -239,7 +240,7 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
                             if (position >= 0) {
                                 oldMedia.setPosition(position);
                             }
-                            callback.onPostPlayback(oldMedia, false, false);
+                            callback.onPostPlayback(oldMedia, false, false, false);
                         }
                         // onPlaybackEnded pretty much takes care of updating the UI
                         return;
@@ -261,13 +262,13 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
                         if (mediaChanged && currentMedia != null) {
                             media = currentMedia;
                         }
-                        endPlayback(false, true, true);
+                        endPlayback(true, false, true, true);
                         return;
                     case MediaStatus.IDLE_REASON_ERROR:
                         Log.w(TAG, "Got an error status from the Chromecast. Skipping, if possible, to the next episode...");
                         callback.onMediaPlayerInfo(CAST_ERROR_PRIORITY_HIGH,
                                 R.string.cast_failed_media_error_skipping);
-                        endPlayback(true, true, true);
+                        endPlayback(false, false, true, true);
                         return;
                 }
                 break;
@@ -282,7 +283,7 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
         if (mediaChanged) {
             callback.onMediaChanged(true);
             if (oldMedia != null) {
-                callback.onPostPlayback(oldMedia, false, currentMedia != null);
+                callback.onPostPlayback(oldMedia, false, false, currentMedia != null);
             }
         }
     }
@@ -334,7 +335,7 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
                 }
                 if (!media.getIdentifier().equals(playable.getIdentifier())) {
                     final Playable oldMedia = media;
-                    callback.onPostPlayback(oldMedia, false, true);
+                    callback.onPostPlayback(oldMedia, false, false, true);
                 }
 
                 setPlayerStatus(PlayerStatus.INDETERMINATE, null);
@@ -599,7 +600,8 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
     }
 
     @Override
-    protected Future<?> endPlayback(boolean wasSkipped, boolean shouldContinue, boolean toStoppedState) {
+    protected Future<?> endPlayback(boolean hasEnded, boolean wasSkipped, boolean shouldContinue,
+                                    boolean toStoppedState) {
         Log.d(TAG, "endPlayback() called");
         boolean isPlaying = playerStatus == PlayerStatus.PLAYING;
         if (playerStatus != PlayerStatus.INDETERMINATE) {
@@ -647,7 +649,7 @@ public class RemotePSMP extends PlaybackServiceMediaPlayer {
             }
             if (shouldPostProcess) {
                 // Otherwise we rely on the chromecast callback to tell us the playback has stopped.
-                callback.onPostPlayback(currentMedia, !wasSkipped, nextMedia != null);
+                callback.onPostPlayback(currentMedia, hasEnded, wasSkipped, nextMedia != null);
             }
         } else if (isPlaying) {
             callback.onPlaybackPause(currentMedia,