This commit fixes one way ghost notifications could be produced (although I don't know if there are other ways). This is the call chain that would lead to ghost notifications being created:
1. the system starts `PlayerService` to query information from it, without providing `SHOULD_START_FOREGROUND_EXTRA=true`, so NewPipe does not start the player nor show any notification, as expected
2. the `PlayerHolder::serviceConnection.onServiceConnected()` gets called by the system to inform `PlayerHolder` that the player started
3. `PlayerHolder` notifies `MainActivity` that the player has started (although in fact only the service has started), by sending a `ACTION_PLAYER_STARTED` broadcast
4. `MainActivity` receives the `ACTION_PLAYER_STARTED` broadcast and brings up the mini-player, but then also tries to make `PlayerHolder` bind to `PlayerService` just in case it was not bound yet, but does so using `PlayerHolder::startService()` instead of the more passive `PlayerHolder::tryBindIfNeeded()`
5. `PlayerHolder::startService()` sends an intent to the `PlayerService` again, this time with `startForegroundService` and with `SHOULD_START_FOREGROUND_EXTRA=true`
6. the `PlayerService` receives the intent and due to `SHOULD_START_FOREGROUND_EXTRA=true` decides to start up the player and show a dummy notification
Steps 3 and 4 are wrong, and this commit fixes them:
3. `PlayerHolder` will now broadcast `ACTION_PLAYER_STARTED` when the service connects, only if the player is not-null
4. `PlayerHolder::tryBindIfNeeded()` is now used to passively try to bind, instead of `PlayerHolder::startService()`
This was always a bit weird, that clicking a timestamp would
unconditionally switch to the popup player.
With the new enum, it’s trivial to change it to always stay at the
selected player now ;)
In 063dcd41e57c46721f1691cd57d21fbde75a35ea I falsely claimed that the
fallthrough case is always degenerate, but it kinda somehow still
worked because if you long-click on e.g. the popup button, it would
call enqueue, but if nothing was running yet it would fallthrough to
the very last case and start the player with the video.
So let’s return to that and add a TODO for further refactoring in the
future.
Fixes https://github.com/TeamNewPipe/NewPipe/issues/11013
We finally are at the point where we can have good logic around
clicking on timestamps.
This is pretty straightforward:
1) if we are already playing the stream (usual case), we skip to the
correct second directly
2) If we don’t have a queue yet, create a trivial one with the stream
3) If we have a queue, we insert the video as next item and start
playing it.
The skipping logic in 1) is similar to the one further down in the old
optimization block, but will always correctly fire for timestamps now.
I copied it because it’s not quite the same code, and moving into a
separate method at this stage would complicate the code too much.
Instead of implicitely reconstructing whether the intent was
intended (lol) to be a timestamp change, we create a new kind of
intent that *only* sets the data we need to switch to a new timestamp.
This means that the logic of what to do (opening a popup player) gets
moved from `InternalUrlsHandler.playOnPopup` to the
`Player.handleIntent` method, we only pass that we want to jump to a
new timestamp. Thus, the stream is now loaded *after* sending the
intent instead of before sending.
This is somewhat messy right now and still does not fix the issue of
queue deletion, but from now on the queue logic should get more
straightforward to implement.
In the end, everything should be a giant switch. Thus we don’t
fall-through anymore, but run the post-setup code manually by calling
`handeIntentPost` and then returning.
The goal here is to convert all player intents to use a single enum
with extra data for each case. The queue ones are pretty easy, they
don’t carry any extra data. We fall through for everything else for
now.
We can do this, because:
1. if `playQueue` is not null, we return early
2. if `playQueue` is null and we need to enqueue:
- the only “proper” case that could be triggered is
the `RESUME_PLAYBACK` case, which is never `true` for the queuing
intents, see the comment in `NavigationHelper.enqueueOnPlayer`
- the generic `else` case is degenerate, because it would crash on
`playQueue` being `null`.
This makes some sense, because there is no way to trigger the
enqueueing logic via the UI currently if there is no video playing
yet, in which case `playQueue` is not `null`.
So we need to transform this whole if desaster into a big switch.
Okay, so this is the … only? branch in this if-chain that will
conditionally fire if `playQueue` *is* `null`, sometimes.
This is why the unconditional `initPlayback` in `else` is not passed a
`null` in many cases … because `RESUME_PLAYBACK` is `true` and
`playQueue` is `null`.
It’s gonna be hard to figure out which parts of that are intentional,
I say.
- ErrorInfo.getMessage() now returns an ErrorMessage instance that can be formatted into a string using a context (this allows the construction of an ErrorInfo to remain independent of a Context)
- now the service ID is used in ErrorInfo.getMessage() to customize some messages based on the currently selected service
- player HTTP invalid statuses are now included in the message
- building a custom error message for AccountTerminatedException was moved from ErrorPanelHelper to ErrorInfo
and reset them when the language is changed/changing.
This way they will be re-initialized on the next call.
Also Remove a bunch of outdated/non-thread safe code (STRING_FORMATTER)
* fix: handle nullable nextPage behavior when searching albums #12401
* feat: add nullable annotation to newPage attribute in SearchFragment
* Updated more usages of InfoItemsPage#getNextPage. Nullability is already handled in these areas so no other changes needed
---------
Co-authored-by: Siddhesh Naik <siddheshnaik20@protonmail.com>
Fixes#12400, see there for explanation. Citing from there:
So apparently the problem is onGetRoot always returning a BrowserRoot instance. Making it return null solved the issue (but again, breaks Android Auto compatibility). It turns out (see https://stackoverflow.com/q/63818988/) that onGetRoot is also used for media resumption https://developer.android.com/media/implement/surfaces/mobile#mediabrowserservice_implementation, which causes a new notification to pop up (in this case a useless notification because our onGetRoot does not return something that can be used for resumption). So what needs to be done is to check if rootHints?.getBoolean(EXTRA_RECENT) == true and if that's the case not return anything (as EXTRA_RECENT is used by the system for resumption).
The PackageValidator file is taken from 329a21b63c/common/src/main/java/com/example/android/uamp/media/PackageValidator.kt .
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.
Fixes mini-player not appearing on app start if the player service is already playing something.
The PlayerService (and the player) may be started from an external intent that does not involve the MainActivity (e.g. RouterActivity or Android Auto's media browser interface).
This PR tries to bind to the PlayerService as soon as the MainActivity starts, but only does so in a passive way, i.e. if the service is not already running it is not started.
Once the connection between PlayerHolder and PlayerService is setup, the ACTION_PLAYER_STARTED broadcast is sent to MainActivity so that it can setup the bottom mini-player.
Another important thing this commit does is to check whether the player is open before actually adding the mini-player view, since the PlayerService could be bound even without a running player (e.g. Android Auto's media browser is being used). This is a consequence of commit "Drop some assumptions on how PlayerService is started and reused".
This commit is a consequence of the commit "Drop some assumptions on how PlayerService is started and reused". Since the assumptions on how the PlayerService is started and reused have changed, we also need to adapt the way it is stopped. This means allowing the service to remain alive even after the player is destroyed, in case the system is still accessing PlayerService e.g. through the media browser interface. The foreground service needs to be stopped and the notification removed in any case.
If a playbackPreparer is set, then instead of calling `player.prepare()`, the MediaSessionConnector will call `playbackPreparer.onPrepare(true)` instead, as seen below.
This commit makes it so that playbackPreparer.onPrepare(true) restores the original behavior of just calling player.prepare().
From MediaSessionConnector -> MediaSessionCompat.Callback implementation:
```java
@Override
public void onPlay() {
if (canDispatchPlaybackAction(PlaybackStateCompat.ACTION_PLAY)) {
if (player.getPlaybackState() == Player.STATE_IDLE) {
if (playbackPreparer != null) {
playbackPreparer.onPrepare(/* playWhenReady= */ true);
} else {
player.prepare();
}
} else if (player.getPlaybackState() == Player.STATE_ENDED) {
seekTo(player, player.getCurrentMediaItemIndex(), C.TIME_UNSET);
}
Assertions.checkNotNull(player).play();
}
}
```
This class implements the media browser service interface as a standalone class for clearer separation of concerns (otherwise everything would need to go in PlayerService, since PlayerService overrides MediaBrowserServiceCompat)
Co-authored-by: Haggai Eran <haggai.eran@gmail.com>
Co-authored-by: Profpatsch <mail@profpatsch.de>
