Fix player resuming from start when clicking on a timestamp

[Thompson3142]

What is it?

• Bugfix (user facing)
• Feature (user facing)
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

Opening a timestamp from a comment (or somewhere else) started the pop-up player from the beginning instead of the timestamp (more details in the linked issue). Now it always opens the pop-up player at the specified time and not from start.
I am honestly unsure if this fix is acceptable since i don't fully understand the purpose of calling setRecovery() in this spot in the first place, so i am open to suggestions and/or help regarding this topic. I have not found any behavior that breaks with these changes though.

Fixes the following issue(s)

• Fixes Tapping on timestamp causes popup player to start from beginning #11302

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

• I read the contribution guidelines.

[Profpatsch]

Why does the player change to a popup player in the first place when clicking on a timpestamp? Shouldn’t it just seek in the currently playing player?

[ShareASmile]

Why does the player change to a popup player in the first place when clicking on a timpestamp? Shouldn’t it just seek in the currently playing player?

While discussing it, We should also See the context in which it wasn't touched, like a comment from #4241 when we switched to Unified Player.

[Thompson3142]

I just assumed this was the expected behavior at this point, you can definitely make the argument to change this or give the user the option to choose between the two. I personally dislike the idea of just removing the pop-up option all together since most users are probably used to it at this point but i am not the person that should make this decision.

[ShareASmile]

i don't fully understand the purpose of calling setRecovery() in this spot in the first place, so i am open to suggestions and/or help regarding this topic.

While debugging #7427 developer litetex investigated the player code. Though I am not a developer, I am unable to help you but reading comments there(also mentioned issues/PRs) may help you understand the code around it better and may get some new insights into it.

[Thompson3142]

I have looked into it a bit and it seems like its no longer necessary to call setRecovery() in this spot. At least for me the bug did not reappear when i removed this piece of code, it seems like this was fixed somewhere along the line but never removed. Probably would be good though if more people than just me could confirm this (ideally ones that know how to reproduce this bug), i have updated the PR accordingly.

I don't know how the process for feature requests is but since the rewrite to NewPlayer is going on right now, it seems like the topic how timestamps should be opened should probably be discussed for NewPlayer but not be implemented for this version of the Player. If that is not the case i am happy to implement it differently.

[Profpatsch]

Please be aware that we are completely reworking the player at the moment so anything apart from very simple bug fixes will not be reviewed/merged.

But we’ll keep this in mind for the new player.
I think this PR might be worth merging anyway, to get the jump-to-timestamp function finally working again.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Stypox]

The setRecovery() call was introduced in c45514b, see this blame, but since that commit is giant, who knows what was the thought process behind adding that line.

I tested on my phone (Android 13), on API 34 emulated and on API 22 emulated, and I tried opening a video, switching popup/background/main multiple times and starting a new video that replaces the queue, and could not notice any issue. Therefore let's take a gamble: let's merge this and see if users report any new sort of player bug. If something happens, since this is a single-line change, it can be reverted without issues. Thank you for looking into this!

[SavSanta]

Seems the changes fix the problem.

if the playlist is open while clicking a timestamp. The entries in the playlist still exist after clicking a timestamp. It's even [badly] scrollable in the pop-up window that always happens.

However closing and re-opening the playlist via the UI button and all the entries are gone.

example
https://github.com/user-attachments/assets/6bead949-3598-43ac-8354-13fe06e1fe5b

So on clarification it DOES fix at least the portion of resetting the player timestamp back to zero. SO the above can be disregarded

[Thompson3142]

These are (imho) not related, this PR was specifically for #11302 your issue is still unfixed but not relevant in this context.

[SavSanta]

Hmm, yea that is true. It doesnt go to the beginning. My bad. +1 for timestamp reset has been fixed from my testing. Above edit commented out