- The SubRip (.srt) specification requires subtitle numbering to begin from 1.
- Please refer to https://en.wikipedia.org/wiki/SubRip
- Previously numbering started from 0, which is accepted by most
players (tested on mpv, VLC, MPlayer, Totem) but not strictly compliant.
- Moved from androidTest to test directory
- Removed Android test runner dependencies
- Renamed to CommentSectionErrorTest
- Addresses PR feedback until Compose testing is in place
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()`
Most usages of `toggleFullscreen` are clear about whether they want to
enter or exit fullscreen, so let’s split the setup into two functions
for easier debugging.
The two remaining uses of `toggleFullscreen` *should* really toggle,
but I’m not 100% sure.
Also rename `onScreenRotationButtonClicked` to
`onFullscreenToggleButtonClicked`, since we renamed the button id
earlier.
Their names must have come from a very old version of the app, they
both toggle the fullscreen mode and rotate the screen depending on
circumstances.
So
`fullscreenButton` -> `fullscreenToggleButtonSecondaryMenu` because it
is only visible in the secondary video menu on some configurations.
and
`screenRotationButton` -> `fullscreenToggleButton` because it is the main
fullscreen button next to the video progress bar.
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.
