-
-
Notifications
You must be signed in to change notification settings - Fork 521
Fix initial video player margin issue in PlayerFragment #7753
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
|
I think if we’re going to change this part, we should apply the adjustment suggested by krwow |
I think the existing design works well overall. The only concern is that the icon appears a bit too close to the progress bar — adjusting that spacing should address the issue for now. |
app/src/main/res/values/dimens.xml
Outdated
|
|
||
| <dimen name="normal_button_margin_bottom">0dp</dimen> | ||
| <dimen name="normal_button_margin_end">0dp</dimen> | ||
| <dimen name="fullscreen_button_margin_bottom">11dp</dimen> |
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.
11dp is a bit too much imo because that doesn't align anymore with the video duration and progress texts in the left.
Everything up to 5dp looks still okay, but it shouldn't be much more in my opinion.
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.
Got it done 🎉
| private fun setFullscreenButtonMargin(isFullscreen: Boolean) { | ||
| val layoutParams = playerControlsBinding.fullscreen.layoutParams as ViewGroup.MarginLayoutParams | ||
|
|
||
| if (isFullscreen) { | ||
| // Add extra bottom margin in fullscreen | ||
| layoutParams.bottomMargin = resources.getDimensionPixelSize(R.dimen.fullscreen_button_margin_bottom) | ||
| layoutParams.marginEnd = resources.getDimensionPixelSize(R.dimen.fullscreen_button_margin_end) | ||
| } else { | ||
| // Reset to default margin | ||
| layoutParams.bottomMargin = resources.getDimensionPixelSize(R.dimen.normal_button_margin_bottom) | ||
| layoutParams.marginEnd = resources.getDimensionPixelSize(R.dimen.normal_button_margin_end) | ||
| } | ||
|
|
||
| playerControlsBinding.fullscreen.layoutParams = layoutParams |
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.
Please put this logic into CustomExoPlayerView.kt and call it from there from within onFullscreenChange, see https://github.com/libre-tube/LibreTube/blob/master/app/src/main/java/com/github/libretube/ui/views/CustomExoPlayerView.kt#L824.
That way it also works if playing downloaded videos offline :)
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.
Hii @Bnyro
I’ve moved the fullscreen button margin logic into CustomExoPlayerView.kt and it’s now called inside onFullscreenChange() like you suggested. It should now work properly even for downloaded videos offline :)
All changes are pushed to the fix-video-margins branch. Excited to see it merged
Thanks for the guidance !
e8e3aca to
8cf2b65
Compare
8cf2b65 to
de5e365
Compare
Bnyro
left a comment
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.
Thanks!
Hey there ! Thanks for the feedback
This is a follow-up to my previous PR #7743 .
I’ve updated the implementation so that the margin change for the fullscreen button now occurs only when entering fullscreen mode. In normal mode, the margins remain untouched to keep the layout consistent.
Added a small tooltip for the fullscreen button as a friendly hint.
I’ve also reverted the unintended changes in app/src/main/res/values/languages.xml.
These updates should make the fullscreen layout behave smoothly and keep the UI consistent in both modes.