Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix loading StreamInfo twice on first VideoDetailFragment opening #11959

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jan 26, 2025

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

While working on #11955 I realized that when VideoDetailFragment is first started, the video loading is initiated both by VideoDetailFragment and by the NavigationHelper.openVideoDetailFragment() function, leading to the stream info loading twice in the background. From the second time openVideoDetailFragment() is invoked, the VideoDetailFragment is already running in the background, and so its initial logic does not run.

To solve this bug I made it so that NavigationHelper.openVideoDetailFragment() first opens the VideoDetailFragment as if it were in collapsed bottom sheet state (i.e. with url = null, see how VideoDetailFragment.wasCleared() is called in VideoDetailFragment.doInitialLoadLogic(). I am not sure if this is just a hack or is the actual intended behavior, but fortunately we're going to ditch all of this code soon. I think this is quite safe to merge anyway, since the change happens in NavigationHelper.openVideoDetailFragment() which is clearly defined.

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

@github-actions github-actions bot added the size/small PRs with less than 50 changed lines label Jan 26, 2025
@AudricV AudricV added the bug Issue is related to a bug label Jan 26, 2025
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try this and see if users report things :P

If you want nitpicks, then change in the comment videoUrl to url and video to stream, as we can play audio-only streams :)

@Stypox
Copy link
Member Author

Stypox commented Jan 27, 2025

Fixed in 01bf855, thanks for noticing ;-)

@Stypox Stypox enabled auto-merge January 27, 2025 13:53
@Stypox Stypox merged commit 47263f5 into TeamNewPipe:dev Jan 27, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug size/small PRs with less than 50 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants