-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19498][Student] Remove non-functional fullscreen button from video player #3456
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
Conversation
The fullscreen button in VideoViewActivity did nothing when tapped. Instead of trying to implement fullscreen functionality, removed the button entirely since it was non-functional. Changes: - Added View and ImageButton imports - Added setGone extension function import - Hidden fullscreen button in onCreate by setting it to GONE Test plan: - Open a video file from Global Files or course Files - Verify video plays correctly - Verify fullscreen button is no longer visible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR adds functionality to hide the fullscreen button in the video player. The change is straightforward, but there are some important concerns to address.
Issues Found
- Critical: Potential NullPointerException (line 78) -
findViewById()can return null, which would crash when callingsetGone(). Use safe call operator?.to prevent crashes. - Timing/Lifecycle Issue (line 78) - The button hiding logic runs before the PlayerView is fully initialized, which may cause the findViewById to fail even when the button exists. Consider using
post {}to defer execution. - Code Style: Import Organization (line 24) - ImageButton import breaks the logical grouping of imports. Consider organizing imports by category.
Positive Aspects
✓ The approach of using setGone() is consistent with the codebase's utility methods
✓ Minimal, focused change that addresses a specific UI requirement
✓ Proper imports added for new dependencies
Recommendations
- Add null safety: Use
binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton)?.setGone()to prevent crashes - Consider timing: Wrap the call in
binding.playerView.post { }to ensure the view hierarchy is ready - Add unit/UI test: Consider adding a test to verify the fullscreen button is properly hidden when the activity loads
Test Coverage
No test changes were included in this PR. Consider adding:
- UI test verifying the fullscreen button is not visible after activity creation
- Unit test if there's logic to conditionally show/hide the button
Overall, this is a simple change that needs defensive coding to handle potential null cases and timing issues with view initialization.
| mainHandler = Handler() | ||
| val videoTrackSelectionFactory: ExoTrackSelection.Factory = AdaptiveTrackSelection.Factory() | ||
| trackSelector = DefaultTrackSelector(applicationContext, videoTrackSelectionFactory) | ||
| binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton).setGone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: The findViewById<ImageButton>(R.id.fullscreenButton) call could return null if the button doesn't exist in the player view's layout. This would cause a crash when calling setGone() on a null object.
Consider using a safe call or null check:
binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton)?.setGone()Or verify the button exists and handle the null case:
binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton)?.let { button ->
button.setGone()
} ?: run {
// Log warning or handle missing button case
}| mainHandler = Handler() | ||
| val videoTrackSelectionFactory: ExoTrackSelection.Factory = AdaptiveTrackSelection.Factory() | ||
| trackSelector = DefaultTrackSelector(applicationContext, videoTrackSelectionFactory) | ||
| binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton).setGone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timing concern: This code runs in onCreate() before fetchMediaUri() is called. If the PlayerView hasn't fully initialized its internal layout at this point, the findViewById() might not locate the button even if it exists.
Consider moving this after the player is fully set up, or using a post handler:
binding.playerView.post {
binding.playerView.findViewById<ImageButton>(R.id.fullscreenButton)?.setGone()
}| import android.net.Uri | ||
| import android.os.Bundle | ||
| import android.os.Handler | ||
| import android.widget.ImageButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import ordering: The ImageButton import appears to be placed between OS imports and Media3 imports. Consider organizing imports by grouping (Android framework, third-party libraries, app-specific) for better readability.
📊 Code Coverage Reportℹ️ Student
ℹ️ Teacher
ℹ️ Pandautils
|
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 18 Dec 2025 10:23:36 GMT |
Test Plan
References
refs: MBL-19498
Impact
affects: Student
Release Note
release note: Removed non-functional fullscreen button from video player
Checklist