Made the changes to Remove the PiP implementation #395
Conversation
Summary of ChangesHello @praneethatchana, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically removes the Picture-in-Picture (PiP) functionality from the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request disables Picture-in-Picture (PiP) functionality for PiPSampleActivity and PiPMovieActivity by setting android:supportsPictureInPicture="false" in the AndroidManifest.xml. Correspondingly, a substantial amount of PiP-related code, including method overrides (onPictureInPictureModeChanged, onPictureInPictureUiStateChanged), helper functions (updatePictureInPictureParams), and calls to enterPictureInPictureMode, trackPipAnimationHintView, and ActivityCompat.registerReceiver, has been commented out across PiPMovieActivity.kt and PiPSampleActivity.kt. The review comments consistently request that all this commented-out code be removed completely to enhance code clarity, readability, and maintainability, noting that version control systems are the appropriate place for historical code.
.../user-interface/picture-in-picture/src/main/java/com/example/android/pip/PiPMovieActivity.kt
Show resolved
Hide resolved
.../user-interface/picture-in-picture/src/main/java/com/example/android/pip/PiPMovieActivity.kt
Show resolved
Hide resolved
.../user-interface/picture-in-picture/src/main/java/com/example/android/pip/PiPMovieActivity.kt
Show resolved
Hide resolved
...user-interface/picture-in-picture/src/main/java/com/example/android/pip/PiPSampleActivity.kt
Show resolved
Hide resolved
| if (started) R.drawable.ic_pause_24dp else R.drawable.ic_play_arrow_24dp, | ||
| ) | ||
| updatePictureInPictureParams(started) | ||
| // updatePictureInPictureParams(started) |
There was a problem hiding this comment.
This commented-out line should be removed. Keeping commented-out code can be confusing for other developers and reduces code quality.
| /* lifecycleScope.launch { | ||
| repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| trackPipAnimationHintView(binding.stopwatchBackground) | ||
| } | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
This lifecycleScope.launch block is commented out. It's better to remove dead code entirely rather than commenting it out, to improve readability and maintainability.
| /*ActivityCompat.registerReceiver( | ||
| this, | ||
| broadcastReceiver, | ||
| IntentFilter(ACTION_STOPWATCH_CONTROL), | ||
| ContextCompat.RECEIVER_NOT_EXPORTED | ||
| ) | ||
| )*/ |
There was a problem hiding this comment.
This ActivityCompat.registerReceiver call is commented out. Please remove it to avoid code clutter.
| /*override fun onPictureInPictureModeChanged( | ||
| isInPictureInPictureMode: Boolean, | ||
| newConfig: Configuration, | ||
| ) { | ||
| super.onPictureInPictureModeChanged(isInPictureInPictureMode, newConfig) | ||
| // Toggle visibility of in-app buttons. They cannot be interacted in the picture-in-picture | ||
| // mode, and their features are provided as the action icons. | ||
| toggleControls(if (isInPictureInPictureMode) View.GONE else View.VISIBLE) | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
The onPictureInPictureModeChanged method is commented out. It should be removed completely to keep the code clean.
| /* @RequiresApi(35) | ||
| override fun onPictureInPictureUiStateChanged(pipState: PictureInPictureUiState) { | ||
| super.onPictureInPictureUiStateChanged(pipState) | ||
| if (pipState.isTransitioningToPip) { | ||
| toggleControls(View.GONE) | ||
| } | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
The onPictureInPictureUiStateChanged method is commented out. Please remove it instead of commenting it out.
|
@praneethatchana can we create this change for merging into a new branch, rather than into main. e.g. no_pip Also, please remove the code entirely vs commenting it out. Thanks! |
2 participants
Made the changes to Remove the PiP implementation