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 Crash on Related Items Enqueue Popup #1

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

rmtilde
Copy link
Owner

@rmtilde rmtilde commented Oct 16, 2024

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

  • Changes the InfoItemDialog popup modal for the RelatedItemsFragment to be attached to the parent context of the RelatedItemsFragment. This prevents the popup not being attached a context when the player automatically changes to the next video and the RelatedItemsFragment is re-initialized.

Fixes the following issue(s)

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

@rmtilde rmtilde self-assigned this Oct 16, 2024
@jiahengguo123
Copy link

This PR looks good to me. Developer had considered a variety of scenarios which is crucial for a robust fix. This approach of managing the InfoItemDialog context within RelatedItemsFragment is considered and effectively solves the issue.

@jiahengguo123
Copy link

This PR looks good to me. Developer had considered a variety of scenarios which is crucial for a robust fix. This approach of managing the InfoItemDialog context within RelatedItemsFragment is considered and effectively solves the issue.

I would suggest further improvements to the documentation in the code. Adding some comments explaining the need to check the context of the parent fragment can greatly help the future maintenance. Specifically, it would be helpful to detailed on how this check relates to preventing context loss during video conversion, which is essential for resolving the reported crash.

@Gc0rp
Copy link

Gc0rp commented Oct 17, 2024

Looks good to me, lovely work.

@rmtilde rmtilde changed the title Fix Crash on Related Items Modal Fix Crash on Related Items Enqueue Popup Oct 17, 2024
@rmtilde rmtilde merged commit 678f0a7 into dev Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants