Merge pull request #12606 from Stypox/do-not-startService-randomly

This should solve "Context.startForegroundService() did not then call Service.startForeground()" according to https://github.com/TeamNewPipe/NewPipe/issues/12489#issuecomment-3290318112

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

@@ -1416,10 +1416,8 @@ public final class VideoDetailFragment
                             bottomSheetBehavior.setState(BottomSheetBehavior.STATE_COLLAPSED);
                         }
                         // Rebound to the service if it was closed via notification or mini player
-                        if (!playerHolder.isBound()) {
+                        playerHolder.setListener(VideoDetailFragment.this);
-                            playerHolder.startService(
+                        playerHolder.tryBindIfNeeded(context);
-                                    false, VideoDetailFragment.this);
-                        }
                         break;
                 }
             }

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

@@ -40,6 +40,7 @@ import org.schabi.newpipe.player.mediabrowser.MediaBrowserImpl;
 import org.schabi.newpipe.player.mediabrowser.MediaBrowserPlaybackPreparer;
 import org.schabi.newpipe.player.mediasession.MediaSessionPlayerUi;
 import org.schabi.newpipe.player.notification.NotificationPlayerUi;
+import org.schabi.newpipe.player.notification.NotificationUtil;
 import org.schabi.newpipe.util.ThemeHelper;
 
 import java.lang.ref.WeakReference;
@@ -156,25 +157,24 @@ public final class PlayerService extends MediaBrowserServiceCompat {
             }
         }
 
-        if (Intent.ACTION_MEDIA_BUTTON.equals(intent.getAction())
+        if (player == null) {
-                && (player == null || player.getPlayQueue() == null)) {
+            // No need to process media button's actions or other system intents if the player is
-            /*
+            // not running. However, since the current intent might have been issued by the system
-            No need to process media button's actions if the player is not working, otherwise
+            // with `startForegroundService()` (for unknown reasons), we need to ensure that we post
-            the player service would strangely start with nothing to play
+            // a (dummy) foreground notification, otherwise we'd incur in
-            Stop the service in this case, which will be removed from the foreground and its
+            // "Context.startForegroundService() did not then call Service.startForeground()". Then
-            notification cancelled in its destruction
+            // we stop the service again.
-             */
+            Log.d(TAG, "onStartCommand() got a useless intent, closing the service");
+            NotificationUtil.startForegroundWithDummyNotification(this);
             destroyPlayerAndStopService();
             return START_NOT_STICKY;
         }
 
-        if (player != null) {
+        final PlayerType oldPlayerType = player.getPlayerType();
-            final PlayerType oldPlayerType = player.getPlayerType();
+        player.handleIntent(intent);
-            player.handleIntent(intent);
+        player.handleIntentPost(oldPlayerType);
-            player.handleIntentPost(oldPlayerType);
+        player.UIs().get(MediaSessionPlayerUi.class)
-            player.UIs().get(MediaSessionPlayerUi.class)
+                .ifPresent(ui -> ui.handleMediaButtonIntent(intent));
-                    .ifPresent(ui -> ui.handleMediaButtonIntent(intent));
-        }
 
         return START_NOT_STICKY;
     }

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

@@ -192,9 +192,11 @@ public final class PlayerHolder {
             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
+            if (playerService != null && playerService.getPlayer() != null) {
-            // open the bottom mini-player
+                // notify the main activity that binding the service has completed and that there is
-            NavigationHelper.sendPlayerStartedEvent(localBinder.getService());
+                // a player, so that it can open the bottom mini-player
+                NavigationHelper.sendPlayerStartedEvent(localBinder.getService());
+            }
         }
     }
 

app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java

@@ -5,7 +5,9 @@ import static androidx.media.app.NotificationCompat.MediaStyle;
 import static org.schabi.newpipe.player.notification.NotificationConstants.ACTION_CLOSE;
 
 import android.annotation.SuppressLint;
+import android.app.Notification;
 import android.app.PendingIntent;
+import android.content.Context;
 import android.content.Intent;
 import android.content.pm.ServiceInfo;
 import android.graphics.Bitmap;
@@ -24,6 +26,7 @@ import org.schabi.newpipe.MainActivity;
 import org.schabi.newpipe.R;
 import org.schabi.newpipe.player.Player;
 import org.schabi.newpipe.player.PlayerIntentType;
+import org.schabi.newpipe.player.PlayerService;
 import org.schabi.newpipe.player.mediasession.MediaSessionPlayerUi;
 import org.schabi.newpipe.util.NavigationHelper;
 
@@ -90,12 +93,9 @@ public final class NotificationUtil {
             Log.d(TAG, "createNotification()");
         }
         notificationManager = NotificationManagerCompat.from(player.getContext());
-        final NotificationCompat.Builder builder =
-                new NotificationCompat.Builder(player.getContext(),
-                player.getContext().getString(R.string.notification_channel_id));
-        final MediaStyle mediaStyle = new MediaStyle();
 
         // setup media style (compact notification slots and media session)
+        final MediaStyle mediaStyle = new MediaStyle();
         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
             // notification actions are ignored on Android 13+, and are replaced by code in
             // MediaSessionPlayerUi
@@ -108,18 +108,9 @@ public final class NotificationUtil {
                 .ifPresent(mediaStyle::setMediaSession);
 
         // setup notification builder
-        builder.setStyle(mediaStyle)
+        final var builder = setupNotificationBuilder(player.getContext(), mediaStyle)
-                .setPriority(NotificationCompat.PRIORITY_HIGH)
-                .setVisibility(NotificationCompat.VISIBILITY_PUBLIC)
-                .setCategory(NotificationCompat.CATEGORY_TRANSPORT)
-                .setShowWhen(false)
-                .setSmallIcon(R.drawable.ic_newpipe_triangle_white)
-                .setColor(ContextCompat.getColor(player.getContext(),
-                        R.color.dark_background_color))
                 .setColorized(player.getPrefs().getBoolean(
-                        player.getContext().getString(R.string.notification_colorize_key), true))
+                        player.getContext().getString(R.string.notification_colorize_key), true));
-                .setDeleteIntent(PendingIntentCompat.getBroadcast(player.getContext(),
-                        NOTIFICATION_ID, new Intent(ACTION_CLOSE), FLAG_UPDATE_CURRENT, false));
 
         // set the initial value for the video thumbnail, updatable with updateNotificationThumbnail
         setLargeIcon(builder);
@@ -168,17 +159,17 @@ public final class NotificationUtil {
                 && notificationBuilder.mActions.get(2).actionIntent != null);
     }
 
+    public static void startForegroundWithDummyNotification(final PlayerService service) {
+        final var builder = setupNotificationBuilder(service, new MediaStyle());
+        startForeground(service, builder.build());
+    }
+
     public void createNotificationAndStartForeground() {
         if (notificationBuilder == null) {
             notificationBuilder = createNotification();
         }
         updateNotification();
-
+        startForeground(player.getService(), notificationBuilder.build());
-        // ServiceInfo constants are not used below Android Q, so 0 is set here
-        final int serviceType = Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q
-                ? ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK : 0;
-        ServiceCompat.startForeground(player.getService(), NOTIFICATION_ID,
-                notificationBuilder.build(), serviceType);
     }
 
     public void cancelNotificationAndStopForeground() {
@@ -192,6 +183,34 @@ public final class NotificationUtil {
     }
 
 
+    /////////////////////////////////////////////////////
+    // STATIC FUNCTIONS IN COMMON BETWEEN DUMMY AND REAL NOTIFICATION
+    /////////////////////////////////////////////////////
+
+    private static NotificationCompat.Builder setupNotificationBuilder(final Context context,
+                                                                       final MediaStyle style) {
+        return new NotificationCompat.Builder(context,
+                context.getString(R.string.notification_channel_id))
+                .setStyle(style)
+                .setPriority(NotificationCompat.PRIORITY_HIGH)
+                .setVisibility(NotificationCompat.VISIBILITY_PUBLIC)
+                .setCategory(NotificationCompat.CATEGORY_TRANSPORT)
+                .setShowWhen(false)
+                .setSmallIcon(R.drawable.ic_newpipe_triangle_white)
+                .setColor(ContextCompat.getColor(context, R.color.dark_background_color))
+                .setDeleteIntent(PendingIntentCompat.getBroadcast(context,
+                        NOTIFICATION_ID, new Intent(ACTION_CLOSE), FLAG_UPDATE_CURRENT, false));
+    }
+
+    private static void startForeground(final PlayerService service,
+                                        final Notification notification) {
+        // ServiceInfo constants are not used below Android Q, so 0 is set here
+        final int serviceType = Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q
+                ? ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK : 0;
+        ServiceCompat.startForeground(service, NOTIFICATION_ID, notification, serviceType);
+    }
+
+
     /////////////////////////////////////////////////////
     // ACTIONS
     /////////////////////////////////////////////////////

app/src/main/java/org/schabi/newpipe/util/external_communication/ShareUtils.java

@@ -5,10 +5,9 @@ import static org.schabi.newpipe.MainActivity.DEBUG;
 import android.content.ActivityNotFoundException;
 import android.content.ClipData;
 import android.content.ClipboardManager;
+import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
-import android.content.pm.PackageManager;
-import android.content.pm.ResolveInfo;
 import android.graphics.Bitmap;
 import android.net.Uri;
 import android.os.Build;
@@ -23,6 +22,7 @@ import androidx.core.content.FileProvider;
 
 import org.schabi.newpipe.BuildConfig;
 import org.schabi.newpipe.R;
+import org.schabi.newpipe.RouterActivity;
 import org.schabi.newpipe.extractor.Image;
 import org.schabi.newpipe.util.image.ImageStrategy;
 import org.schabi.newpipe.util.image.PicassoHelper;
@@ -62,8 +62,9 @@ public final class ShareUtils {
     }
 
     /**
-     * Open the url with the system default browser. If no browser is set as default, falls back to
+     * Open the url with the system default browser. If no browser is installed, falls back to
-     * {@link #openAppChooser(Context, Intent, boolean)}.
+     * {@link #openAppChooser(Context, Intent, boolean)} (for displaying that no apps are available
+     * to handle the action, or possible OEM-related edge cases).
      * <p>
      * This function selects the package to open based on which apps respond to the {@code http://}
      * schema alone, which should exclude special non-browser apps that are can handle the url (e.g.
@@ -77,44 +78,26 @@ public final class ShareUtils {
      * @param url     the url to browse
      **/
     public static void openUrlInBrowser(@NonNull final Context context, final String url) {
-        // Resolve using a generic http://, so we are sure to get a browser and not e.g. the yt app.
+        // Target a generic http://, so we are sure to get a browser and not e.g. the yt app.
         // Note that this requires the `http` schema to be added to `<queries>` in the manifest.
-        final ResolveInfo defaultBrowserInfo;
         final Intent browserIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://"));
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
-            defaultBrowserInfo = context.getPackageManager().resolveActivity(browserIntent,
-                    PackageManager.ResolveInfoFlags.of(PackageManager.MATCH_DEFAULT_ONLY));
-        } else {
-            defaultBrowserInfo = context.getPackageManager().resolveActivity(browserIntent,
-                    PackageManager.MATCH_DEFAULT_ONLY);
-        }
 
         final Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url))
                 .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
 
-        if (defaultBrowserInfo == null) {
+        // See https://stackoverflow.com/a/58801285 and `setSelector` documentation
-            // No app installed to open a web URL, but it may be handled by other apps so try
+        intent.setSelector(browserIntent);
-            // opening a system chooser for the link in this case (it could be bypassed by the
+        try {
-            // system if there is only one app which can open the link or a default app associated
+            context.startActivity(intent);
-            // with the link domain on Android 12 and higher)
+        } catch (final ActivityNotFoundException e) {
+            // No browser is available. This should, in the end, yield a nice AOSP error message
+            // indicating that no app is available to handle this action.
+            //
+            // Note: there are some situations where modified OEM ROMs have apps that appear
+            // to be browsers but are actually app choosers. If starting the Activity fails
+            // related to this, opening the system app chooser is still the correct behavior.
+            intent.setSelector(null);
             openAppChooser(context, intent, true);
-            return;
-        }
-
-        final String defaultBrowserPackage = defaultBrowserInfo.activityInfo.packageName;
-
-        if (defaultBrowserPackage.equals("android")) {
-            // No browser set as default (doesn't work on some devices)
-            openAppChooser(context, intent, true);
-        } else {
-            try {
-                intent.setPackage(defaultBrowserPackage);
-                context.startActivity(intent);
-            } catch (final ActivityNotFoundException e) {
-                // Not a browser but an app chooser because of OEMs changes
-                intent.setPackage(null);
-                openAppChooser(context, intent, true);
-            }
         }
     }
 
@@ -190,6 +173,18 @@ public final class ShareUtils {
             chooserIntent.putExtra(Intent.EXTRA_TITLE, context.getString(R.string.open_with));
         }
 
+        // Avoid opening in NewPipe
+        // (Implementation note: if the URL is one for which NewPipe itself
+        // is set as handler on Android >= 12, we actually remove the only eligible app
+        // for this link, and browsers will not be offered to the user. For that, use
+        // `openUrlInBrowser`.)
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+            chooserIntent.putExtra(
+                    Intent.EXTRA_EXCLUDE_COMPONENTS,
+                    new ComponentName[]{new ComponentName(context, RouterActivity.class)}
+            );
+        }
+
         // Migrate any clip data and flags from the original intent.
         final int permFlags = intent.getFlags() & (Intent.FLAG_GRANT_READ_URI_PERMISSION
                 | Intent.FLAG_GRANT_WRITE_URI_PERMISSION