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 issue #20 and fix full screen padding #26

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

AnandJeyapal
Copy link
Contributor

This is for the following issue

#20

What was the issue?

Dialog window has extra padding around it. Hence if there is any content around the video player (top/bottom/right/left), VideoPlayerFullScreenDialog shows the content behind it.

What is the fix?

We should update the layout properties of the dialog window so that it full occupies the screen's size.

I have copied the attributes from activityWindow and applied them to the dialogWindow.

Read more at: https://www.droidcon.com/2024/01/15/camouflage-the-status-bar-with-edge-to-edge-jetpack-compose-screens-and-dialogs/

@AnandJeyapal AnandJeyapal requested a review from dsa28s as a code owner May 29, 2024 09:00
Copy link
Owner

@dsa28s dsa28s left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've written down some suggestions, please correct them.

@@ -77,6 +83,9 @@ internal fun VideoPlayerFullScreenDialog(
PlayerView(context)
.also(fullScreenPlayerView)
}
var fullScreenModeEntered by remember {
Copy link
Owner

Choose a reason for hiding this comment

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

@AnandJeyapal I think need to change this variable name fullScreenModeEntered to isFullScreenModeEntered. This is my habit, and it is customary to add is to boolean type variables and has to boolean return type functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that makes sense. I have updated it :)

if (activityWindow != null && dialogWindow != null && !fullScreenModeEntered) {
activityWindow.setFullScreen(true)
dialogWindow.setFullScreen(true)
dialogWindow
Copy link
Owner

Choose a reason for hiding this comment

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

@AnandJeyapal Only variables have been called, but if you don't intend to, it looks better to clear them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I removed it (also the redundant view variable).

internal fun getDialogWindow(): Window? = (LocalView.current.parent as? DialogWindowProvider)?.window

@Composable
internal fun getActivityWindow(): Window? = LocalView.current.context.findActivity()?.window
Copy link
Owner

Choose a reason for hiding this comment

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

@AnandJeyapal Please add EOF (End of file). All files in this project must always have a space at the end :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I missed it. Thanks :)

}
}

private fun Window.hideSystemBars() {
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, awesome!

@dsa28s dsa28s assigned dsa28s and AnandJeyapal and unassigned dsa28s Jun 1, 2024
@dsa28s dsa28s added bug Something isn't working enhancement New feature or request labels Jun 1, 2024
@dsa28s dsa28s added this to the 1.3.0 milestone Jun 1, 2024
@dsa28s dsa28s self-requested a review June 5, 2024 03:38
Copy link
Owner

@dsa28s dsa28s left a comment

Choose a reason for hiding this comment

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

Thank you!

@dsa28s
Copy link
Owner

dsa28s commented Jun 5, 2024

This PR will release version 1.3.0

@dsa28s dsa28s merged commit c96ea6d into dsa28s:main Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants