Skip to content

feat: Android - Implementation of Picture in Picture for the youtube pl… - #217

Open
dsandeepkumar2830 wants to merge 71 commits into
2U/developfrom
sandeepd/Learner-11101
Open

feat: Android - Implementation of Picture in Picture for the youtube pl…#217
dsandeepkumar2830 wants to merge 71 commits into
2U/developfrom
sandeepd/Learner-11101

Conversation

@dsandeepkumar2830

Copy link
Copy Markdown

@kalalmounesh kalalmounesh Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this class i didn't find any usage.i found 4 files remove all

@dsandeepkumar2830 dsandeepkumar2830 Jul 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If we remove this run test is failed.
Screenshot 2026-07-17 at 1 13 41 PM

@dsandeepkumar2830 dsandeepkumar2830 Jul 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Screenshot 2026-07-17 at 2 36 00 PM Screenshot 2026-07-17 at 2 34 58 PM @kalalmounesh you can check the screenshots in 2u/develop branch already there are 3 json files , and if you remove these json files run tests will be failing in PR.

@dsandeepkumar2830 dsandeepkumar2830 Jul 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

and Kindly refer this document
json

}
})


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this extra lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed.

Comment thread course/src/main/AndroidManifest.xml Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this class i didn't find any usage.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed

Comment thread Documentation/# PipInteractor_Fixes.md Outdated

@kalalmounesh kalalmounesh Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regarding the documents remove all and add one or max two valid doc in confluence.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed

}

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this extra lines

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed


private val constraintContainer: ConstraintLayout
get() = binding.rootLayout as ConstraintLayout

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this extra lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed


viewModel { (courseId: String?, infoType: String?) ->
WhatsNewViewModel(
WhatsNewViewModel(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are a few extra spaces and empty lines added here. Please remove them to keep the formatting clean and consistent.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed

* Repository for managing PiP player state.
*
* Single source of truth for:
* - Which player is currently registered for PiP

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think we need to add comments/docstrings for every function and class in these newly added PIP-related files. We don't follow this pattern in the existing codebase, so it would be better to keep it consistent.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed

Comment thread Documentation/ARCHITECTURE_FINDINGS.md Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think these documentation files (Architecture Review, Changes, and Fixes) are required. We don't have similar documentation files elsewhere in the project, so it would be better to keep the repository consistent.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is fixed

@maheshchathani maheshchathani left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed.

I have a few suggestions:

  • Please remove the extra spaces and unnecessary empty lines that were added.

  • For the newly added PIP-related files, I don't think we need comments/docstrings for every function and class. We don't usually add them throughout the existing codebase, so it would be better to keep it consistent.

  • I also don't think we need to add the PIP documentation files (Architecture Review, Changes, and Fixes). There are no similar documentation files in the project so far, so I don't think these are required.

Other than these suggestions, the changes look good to me.

@dsandeepkumar2830

Copy link
Copy Markdown
Author

Review completed.

I have a few suggestions:

  • Please remove the extra spaces and unnecessary empty lines that were added.
  • For the newly added PIP-related files, I don't think we need comments/docstrings for every function and class. We don't usually add them throughout the existing codebase, so it would be better to keep it consistent.
  • I also don't think we need to add the PIP documentation files (Architecture Review, Changes, and Fixes). There are no similar documentation files in the project so far, so I don't think these are required.

Other than these suggestions, the changes look good to me.

@dsandeepkumar2830

Copy link
Copy Markdown
Author

@kalalmounesh @maheshchathani I have fixed the review comments and regarding the .md files I have removed the most of the files. Please review and approve and let me know if any changes need to be fixed.

@kalalmounesh kalalmounesh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We are able to see some code related to crash fix in CL 461de61, impact of the changes should be validated by QA.
Rest of the fixes are good to go.

@roy-sumanta roy-sumanta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved, based on the changes done.

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.

5 participants