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

Smooth rotation on orientation change #162

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Conversation

JolandaVerhoef
Copy link
Collaborator

@JolandaVerhoef JolandaVerhoef commented Mar 29, 2024

Allow for a smooth rotation while running the app. Replaced the quick-settings lazy grid with a FlowRow as the grid was depending on screenWidth instead of constraints.

Open issues:

  • Surface aspect ratio breaks when in non-initial orientation.
  • small flash sometimes when doing orientation change
  • UI tests are failing on CI but passing locally 🤷

@JolandaVerhoef JolandaVerhoef force-pushed the jv/rotation branch 2 times, most recently from 19f0d8b to ecac330 Compare March 29, 2024 15:52
…-settings lazy grid with a FlowRow as the grid was depending on screenWidth instead of constraints.

Open issues:
- Surface aspect ratio breaks when in non-initial orientation.
- small flash sometimes when doing orientation change
@@ -145,28 +147,46 @@ fun PreviewDisplay(
currentOnFlipCamera()
}
)
},
}
.layout { measurable, constraints ->

Choose a reason for hiding this comment

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

Can this .layout be replaced with .aspectRatio(aspectRatio.ratio.toFloat())?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, no. The aspectRatio argument is actually an enum that can represent an aspect ratio and its reciprocal, depending on the orientation of the screen. What this layout modifier is doing is calculating the correct aspect ratio that maximizes the screen space used. I'm still not sure if it's the correct thing to do for all screens (specifically foldables), so I'll look into it more, but this does eliminate a recomposition that would be required by changing the aspectRatio argument to its reciprocal.

context.getActivity()?.window?.let { window ->
if (currentRotationAnimation != null) {
window.attributes = window.attributes.apply {
rotationAnimation = WindowManager.LayoutParams.ROTATION_ANIMATION_ROTATE

Choose a reason for hiding this comment

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

Should this be setting the rotationAnimation back to currentRotationAnimation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've modified this to now restore the rotationAnimation to the old rotation animation before the DisposableEffect was applied. This seems to work pretty well, but I am not quite sure using a DisposableEffect is the right thing to do here. Based on this documentation it's possible it may not apply correctly since we are getting the window during composition.

I'll continue to look at this to see if there is a better way using Compose's existing inset modifiers.

Base automatically changed from jv/cleanup to main April 15, 2024 19:17
temcguir added 18 commits April 18, 2024 23:37
…ect orientatin

 Sets the target rotation on ImageCapture and VideoCapture use cases.
 Preview is always set to natural orientation since the rotation will be
 applied in the Viewfinder.
 This class is representing the physical rotation of the entire device.
 We don't want to confuse this with how the UI must rotate to compensate
 for the device being rotated. The UI must rotate in the opposite
 direction to compensate for the device being rotated.
 Always initialize the orientation in CameraXCameraUseCase to the value
 coming from the app settings. This may need to be updated in the future
 so it is initialized during the initialization call rather than just
 using the default from CameraAppSettings (Natural)

 Ensures we don't update the orientation until the camera is ready by
 adding the collector to the orientation flow after the device is
 displaying preview.
temcguir added 11 commits June 12, 2024 16:25
 The previous code required a recomposition due to changing
 MutableState in a LaunchedEffect. Now we calculate orientation
 during composition, and only recompose on configuration changes.
 Modifier.rotatedLayout will now keep the layout rotated to match what
 the layout would look like in the natural orientation of the device
 rather than trying to keep it rotated to match the layout where
 width <= height.
 Ensures the bottom camera controls will not overlap with any insets
 such as the navigation bar or the gesture navigation areas using
 the "safeContent" insets.

 This also requires that the activity be set to edge-to-edge, as
 described at: https://developer.android.com/develop/ui/views/layout/edge-to-edge#enable-edge-to-edge-display
 Sets some icons in the camera controls overlay to rotate to an
 upright position depending on device orientation.
 Quicksettings won't remain in place, and will re-layout whenever
 the device orientation changes. This PR allows that relayout to
 be animated with AnimatedContent.

 To achieve this, we need to use a counter-rotated layout while
 the animation is in progress. We re-use the `rotatedLayout`
 modifier and make it more generic so we can set a base orientation.
 Previously, the base orientation was always the natural orientation
 of the device.
 Sets the FlowRow max number of items per row to 3
 and ensures quicksettings items have a consistent
 width.
 This will not rotate while a recording is in progress as the
 display (and recording) are locked to the orientation of
 when the recording was started.
@@ -535,7 +558,7 @@ fun ToggleButton(
} else {
Modifier
}
),
).then(modifier),

Choose a reason for hiding this comment

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

This is incorrect, since the modifier has already been applied above and we shouldn't apply it twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. I think this was added accidentally. I'll remove it.

Comment on lines +125 to +128
key(LocalConfiguration.current) {
prevSurfaceRotation = newSurfaceRotation
newSurfaceRotation = LocalView.current.display.rotation
}

Choose a reason for hiding this comment

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

I think this is assuming that the rotation of the display can only change when the Configuration also changes, but I don't think that's true in the general case. In free-form windowing, the device (and display) can rotate without the configuration changing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you recommend a way to test this? I'll try to address it.

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.

3 participants