Handle player and player service separately

This is, again, a consequence of the commit "Drop some assumptions on how PlayerService is started and reused".
This commit notified VideoDetailFragment of player starting and stopping independently of the player.
Read the comments in the code changes for more information.

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

@@ -236,11 +236,14 @@ public final class VideoDetailFragment
     // Service management
     //////////////////////////////////////////////////////////////////////////*/
     @Override
-    public void onServiceConnected(final Player connectedPlayer,
-                                   final PlayerService connectedPlayerService,
-                                   final boolean playAfterConnect) {
-        player = connectedPlayer;
+    public void onServiceConnected(@NonNull final PlayerService connectedPlayerService) {
         playerService = connectedPlayerService;
+    }
+
+    @Override
+    public void onPlayerConnected(@NonNull final Player connectedPlayer,
+                                  final boolean playAfterConnect) {
+        player = connectedPlayer;
 
         // It will do nothing if the player is not in fullscreen mode
         hideSystemUiIfNeeded();
@@ -272,11 +275,18 @@ public final class VideoDetailFragment
         updateOverlayPlayQueueButtonVisibility();
     }
 
+    @Override
+    public void onPlayerDisconnected() {
+        player = null;
+        // the binding could be null at this point, if the app is finishing
+        if (binding != null) {
+            restoreDefaultBrightness();
+        }
+    }
+
     @Override
     public void onServiceDisconnected() {
         playerService = null;
-        player = null;
-        restoreDefaultBrightness();
     }
 
 

app/src/main/java/org/schabi/newpipe/player/PlayQueueActivity.java

@@ -224,7 +224,7 @@ public final class PlayQueueActivity extends AppCompatActivity
                 Log.d(TAG, "Player service is connected");
 
                 if (service instanceof PlayerService.LocalBinder) {
-                    player = ((PlayerService.LocalBinder) service).getPlayer();
+                    player = ((PlayerService.LocalBinder) service).getService().getPlayer();
                 }
 
                 if (player == null || player.getPlayQueue() == null || player.exoPlayerIsNull()) {

app/src/main/java/org/schabi/newpipe/player/PlayerService.java

@@ -46,6 +46,7 @@ import org.schabi.newpipe.util.ThemeHelper;
 
 import java.lang.ref.WeakReference;
 import java.util.List;
+import java.util.function.Consumer;
 
 
 /**
@@ -74,6 +75,13 @@ public final class PlayerService extends MediaBrowserServiceCompat {
 
     private final IBinder mBinder = new PlayerService.LocalBinder(this);
 
+    /**
+     * The parameter taken by this {@link Consumer} can be null to indicate the player is being
+     * stopped.
+     */
+    @Nullable
+    private Consumer<Player> onPlayerStartedOrStopped = null;
+
 
     //region Service lifecycle
     @Override
@@ -127,7 +135,8 @@ public final class PlayerService extends MediaBrowserServiceCompat {
         // PlayerService using startForegroundService(), will have SHOULD_START_FOREGROUND_EXTRA,
         // to ensure startForeground() is called (otherwise Android will force-crash the app).
         if (intent.getBooleanExtra(SHOULD_START_FOREGROUND_EXTRA, false)) {
-            if (player == null) {
+            final boolean playerWasNull = (player == null);
+            if (playerWasNull) {
                 // make sure the player exists, in case the service was resumed
                 player = new Player(this, mediaSession, sessionConnector);
             }
@@ -141,6 +150,13 @@ public final class PlayerService extends MediaBrowserServiceCompat {
             // shouldn't do anything.
             player.UIs().get(NotificationPlayerUi.class)
                     .ifPresent(NotificationPlayerUi::createNotificationAndStartForeground);
+
+            if (playerWasNull && onPlayerStartedOrStopped != null) {
+                // notify that a new player was created (but do it after creating the foreground
+                // notification just to make sure we don't incur, due to slowness, in
+                // "Context.startForegroundService() did not then call Service.startForeground()")
+                onPlayerStartedOrStopped.accept(player);
+            }
         }
 
         if (Intent.ACTION_MEDIA_BUTTON.equals(intent.getAction())
@@ -204,6 +220,10 @@ public final class PlayerService extends MediaBrowserServiceCompat {
 
     private void cleanup() {
         if (player != null) {
+            if (onPlayerStartedOrStopped != null) {
+                // notify that the player is being destroyed
+                onPlayerStartedOrStopped.accept(null);
+            }
             player.destroy();
             player = null;
         }
@@ -279,9 +299,31 @@ public final class PlayerService extends MediaBrowserServiceCompat {
         public PlayerService getService() {
             return playerService.get();
         }
+    }
 
-        public Player getPlayer() {
-            return playerService.get().player;
+    /**
+     * @return the current active player instance. May be null, since the player service can outlive
+     * the player e.g. to respond to Android Auto media browser queries.
+     */
+    @Nullable
+    public Player getPlayer() {
+        return player;
+    }
+
+    /**
+     * Sets the listener that will be called when the player is started or stopped. If a
+     * {@code null} listener is passed, then the current listener will be unset. The parameter taken
+     * by the {@link Consumer} can be null to indicate that the player is stopping.
+     * @param listener the listener to set or unset
+     */
+    public void setPlayerListener(@Nullable final Consumer<Player> listener) {
+        this.onPlayerStartedOrStopped = listener;
+        if (listener != null) {
+            if (player == null) {
+                listener.accept(null);
+            } else {
+                listener.accept(player);
+            }
         }
     }
     //endregion

app/src/main/java/org/schabi/newpipe/player/event/PlayerServiceExtendedEventListener.java

@@ -1,11 +1,48 @@
 package org.schabi.newpipe.player.event;
 
+import androidx.annotation.NonNull;
+
 import org.schabi.newpipe.player.PlayerService;
 import org.schabi.newpipe.player.Player;
 
+/**
+ * In addition to {@link PlayerServiceEventListener}, provides callbacks for service and player
+ * connections and disconnections. "Connected" here means that the service (resp. the
+ * player) is running and is bound to {@link org.schabi.newpipe.player.helper.PlayerHolder}.
+ * "Disconnected" means that either the service (resp. the player) was stopped completely, or that
+ * {@link org.schabi.newpipe.player.helper.PlayerHolder} is not bound.
+ */
 public interface PlayerServiceExtendedEventListener extends PlayerServiceEventListener {
-    void onServiceConnected(Player player,
-                            PlayerService playerService,
-                            boolean playAfterConnect);
+    /**
+     * The player service just connected to {@link org.schabi.newpipe.player.helper.PlayerHolder},
+     * but the player may not be active at this moment, e.g. in case the service is running to
+     * respond to Android Auto media browser queries without playing anything.
+     * {@link #onPlayerConnected(Player, boolean)} will be called right after this function if there
+     * is a player.
+     *
+     * @param playerService the newly connected player service
+     */
+    void onServiceConnected(@NonNull PlayerService playerService);
+
+    /**
+     * The player service is already connected and the player was just started.
+     *
+     * @param player the newly connected or started player
+     * @param playAfterConnect whether to open the video player in the video details fragment
+     */
+    void onPlayerConnected(@NonNull Player player, boolean playAfterConnect);
+
+    /**
+     * The player got disconnected, for one of these reasons: the player is getting closed while
+     * leaving the service open for future media browser queries, the service is stopping
+     * completely, or {@link org.schabi.newpipe.player.helper.PlayerHolder} is unbinding.
+     */
+    void onPlayerDisconnected();
+
+    /**
+     * The service got disconnected from {@link org.schabi.newpipe.player.helper.PlayerHolder},
+     * either because {@link org.schabi.newpipe.player.helper.PlayerHolder} is unbinding or because
+     * the service is stopping completely.
+     */
     void onServiceDisconnected();
 }

app/src/main/java/org/schabi/newpipe/player/helper/PlayerHolder.java

@@ -24,6 +24,9 @@ import org.schabi.newpipe.player.event.PlayerServiceExtendedEventListener;
 import org.schabi.newpipe.player.playqueue.PlayQueue;
 import org.schabi.newpipe.util.NavigationHelper;
 
+import java.util.Optional;
+import java.util.function.Consumer;
+
 public final class PlayerHolder {
 
     private PlayerHolder() {
@@ -45,7 +48,16 @@ public final class PlayerHolder {
     private final PlayerServiceConnection serviceConnection = new PlayerServiceConnection();
     private boolean bound;
     @Nullable private PlayerService playerService;
-    @Nullable private Player player;
+
+    private Optional<Player> getPlayer() {
+        return Optional.ofNullable(playerService)
+                .flatMap(s -> Optional.ofNullable(s.getPlayer()));
+    }
+
+    private Optional<PlayQueue> getPlayQueue() {
+        // player play queue might be null e.g. while player is starting
+        return getPlayer().flatMap(p -> Optional.ofNullable(p.getPlayQueue()));
+    }
 
     /**
      * Returns the current {@link PlayerType} of the {@link PlayerService} service,
@@ -55,21 +67,15 @@ public final class PlayerHolder {
      */
     @Nullable
     public PlayerType getType() {
-        if (player == null) {
-            return null;
-        }
-        return player.getPlayerType();
+        return getPlayer().map(Player::getPlayerType).orElse(null);
     }
 
     public boolean isPlaying() {
-        if (player == null) {
-            return false;
-        }
-        return player.isPlaying();
+        return getPlayer().map(Player::isPlaying).orElse(false);
     }
 
     public boolean isPlayerOpen() {
-        return player != null;
+        return getPlayer().isPresent();
     }
 
     /**
@@ -78,7 +84,7 @@ public final class PlayerHolder {
      * @return true only if the player is open and its play queue is ready (i.e. it is not null)
      */
     public boolean isPlayQueueReady() {
-        return player != null && player.getPlayQueue() != null;
+        return getPlayQueue().isPresent();
     }
 
     public boolean isBound() {
@@ -86,18 +92,11 @@ public final class PlayerHolder {
     }
 
     public int getQueueSize() {
-        if (player == null || player.getPlayQueue() == null) {
-            // player play queue might be null e.g. while player is starting
-            return 0;
-        }
-        return player.getPlayQueue().size();
+        return getPlayQueue().map(PlayQueue::size).orElse(0);
     }
 
     public int getQueuePosition() {
-        if (player == null || player.getPlayQueue() == null) {
-            return 0;
-        }
-        return player.getPlayQueue().getIndex();
+        return getPlayQueue().map(PlayQueue::getIndex).orElse(0);
     }
 
     public void setListener(@Nullable final PlayerServiceExtendedEventListener newListener) {
@@ -108,9 +107,10 @@ public final class PlayerHolder {
         }
 
         // Force reload data from service
-        if (player != null) {
-            listener.onServiceConnected(player, playerService, false);
+        if (playerService != null) {
+            listener.onServiceConnected(playerService);
             startPlayerListener();
+            // ^ will call listener.onPlayerConnected() down the line if there is an active player
         }
     }
 
@@ -181,11 +181,12 @@ public final class PlayerHolder {
             final PlayerService.LocalBinder localBinder = (PlayerService.LocalBinder) service;
 
             playerService = localBinder.getService();
-            player = localBinder.getPlayer();
             if (listener != null) {
-                listener.onServiceConnected(player, playerService, playAfterConnect);
+                listener.onServiceConnected(playerService);
+                getPlayer().ifPresent(p -> listener.onPlayerConnected(p, playAfterConnect));
             }
             startPlayerListener();
+            // ^ will call listener.onPlayerConnected() down the line if there is an active player
 
             // notify the main activity that binding the service has completed, so that it can
             // open the bottom mini-player
@@ -229,25 +230,32 @@ public final class PlayerHolder {
             bound = false;
             stopPlayerListener();
             playerService = null;
-            player = null;
             if (listener != null) {
+                listener.onPlayerDisconnected();
                 listener.onServiceDisconnected();
             }
         }
     }
 
     private void startPlayerListener() {
-        if (player != null) {
-            player.setFragmentListener(internalListener);
+        if (playerService != null) {
+            // setting the player listener will take care of calling relevant callbacks if the
+            // player in the service is (not) already active, also see playerStateListener below
+            playerService.setPlayerListener(playerStateListener);
         }
+        getPlayer().ifPresent(p -> p.setFragmentListener(internalListener));
     }
 
     private void stopPlayerListener() {
-        if (player != null) {
-            player.removeFragmentListener(internalListener);
+        if (playerService != null) {
+            playerService.setPlayerListener(null);
         }
+        getPlayer().ifPresent(p -> p.removeFragmentListener(internalListener));
     }
 
+    /**
+     * This listener will be held by the players created by {@link PlayerService}.
+     */
     private final PlayerServiceEventListener internalListener =
             new PlayerServiceEventListener() {
                 @Override
@@ -334,4 +342,23 @@ public final class PlayerHolder {
                     unbind(getCommonContext());
                 }
             };
+
+    /**
+     * This listener will be held by bound {@link PlayerService}s to notify of the player starting
+     * or stopping. This is necessary since the service outlives the player e.g. to answer Android
+     * Auto media browser queries.
+     */
+    private final Consumer<Player> playerStateListener = (@Nullable final Player player) -> {
+        if (listener != null) {
+            if (player == null) {
+                // player.fragmentListener=null is already done by player.stopActivityBinding(),
+                // which is called by player.destroy(), which is in turn called by PlayerService
+                // before setting its player to null
+                listener.onPlayerDisconnected();
+            } else {
+                listener.onPlayerConnected(player, serviceConnection.playAfterConnect);
+                player.setFragmentListener(internalListener);
+            }
+        }
+    };
 }