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

Refactor - Camera UI Adaptation for Foldable Devices and Camera Initialization #52

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

Conversation

yongsuk44
Copy link

@yongsuk44 yongsuk44 commented Mar 28, 2024

To-do

  • : UI adjustment based on screen rotation and foldable state
  • : Generating files to fit the capture and video screen size

Example screen

fold_capture.webm

@yongsuk44 yongsuk44 marked this pull request as draft March 28, 2024 13:23
@yongsuk44 yongsuk44 marked this pull request as ready for review April 2, 2024 14:49
@romanofranz
Copy link

Thanks for doing this!
There are 2 issues when running it on the Pixel Fold (physical device):

  • The camera viewfinder is sideways after rotation
  • The tabletop mode UI is not in sync with the fold orientation
    socialite-fold

@yongsuk44
Copy link
Author

@romanofranz
Thanks, I'll check back. 😀

@yongsuk44
Copy link
Author

@romanofranz

I removed the code that locked the screen orientation when the orientation changes in the camera composable.

Screen_recording_20240408_142929.webm

@romanofranz
Copy link

Thanks, this works better!
There are still few issues:

  • Performance issues on rotation (it takes ~1 second to render the preview after rotation). I changed the rotation animation to jumpcut to mitigate it, but we need to check the cause of the hiccup
  • If you rotate the device upside down (display rotation = 2), the preview is sideways. We may want to add a display listener to catch this corner case.

Overall LGTM on the camera side, adding @donovanfm to check the rest of the refactoring.
Good job!

# Conflicts:
#	app/src/main/java/com/google/android/samples/socialite/MainActivity.kt
#	app/src/main/java/com/google/android/samples/socialite/ui/camera/Camera.kt
#	app/src/main/java/com/google/android/samples/socialite/ui/camera/CameraViewModel.kt
#	app/src/main/java/com/google/android/samples/socialite/ui/camera/ViewFinder.kt
#	app/src/main/java/com/google/android/samples/socialite/ui/camera/viewfinder/surface/SurfaceTransformationUtil.kt
#	app/src/main/java/com/google/android/samples/socialite/ui/camera/viewfinder/surface/Texture.kt
# Conflicts:
#	app/src/main/java/com/google/android/samples/socialite/MainActivity.kt
@yongsuk44 yongsuk44 changed the title Refactoring - Camera (Capture and Video) UI Adaptation for Foldable Devices and Camera Initialization Refactor - Camera UI Adaptation for Foldable Devices and Camera Initialization May 16, 2024
@romanofranz romanofranz self-requested a review June 21, 2024 14:49
Copy link

@romanofranz romanofranz left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -53,7 53,11 @@ android {
}
kotlinOptions {
jvmTarget = "17"
freeCompilerArgs = listOf("-Xcontext-receivers")
freeCompilerArgs = listOf("-Xcontext-receivers")
freeCompilerArgs = listOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change related to this PR on Camera UI? If not, I'd prefer it not be part of this PR.

Copy link
Author

Choose a reason for hiding this comment

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

It wasn't related to the Camera UI.
Thank you for your feedback. 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this empty file

Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this empty file

Comment on lines 69 to 71
<!--Camera Screen -->
<string name="photo">Photo</string>
<string name="video">Video</string>

Choose a reason for hiding this comment

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

Where are those used?

Copy link
Author

Choose a reason for hiding this comment

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

Used by the CameraControls Composable. :)

Choose a reason for hiding this comment

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

Good, I found it. Thank you.

}
}

companion object {
const val FILENAME_FORMAT = "yyyy-MM-dd-HH-mm-ss-SSS"

Choose a reason for hiding this comment

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

Is there any reason to remove FILENAME_FORMAT companion object?

@@ -150,7 149,7 @@ class VideoEditScreenViewModel @Inject constructor(
.build()

val editedVideoFileName = "Socialite-edited-recording-"
SimpleDateFormat(CameraViewModel.FILENAME_FORMAT, Locale.US)
SimpleDateFormat("yyyy-MM-dd-HH-mm-ss-SSS", Locale.US)

Choose a reason for hiding this comment

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

Is there any reason to remove FILENAME_FORMAT from CameraViewModel companion object and set format here?

Copy link
Author

Choose a reason for hiding this comment

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

If you look at CameraModule, you can see that file names, paths, and types are created separately to be injected externally. In the future, When we refactor VideoEditScreen, we plan to change it to the same injection method and use separate constants to declare the format internally.

Do you have any good suggestions? 🤔

Choose a reason for hiding this comment

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

You can do that simply making it as domain layer's usecase or just utils.

Copy link
Author

Choose a reason for hiding this comment

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

Personally, I believe that it is appropriate to create UseCases only when handling the business logic of the program (such as chat, camera, video, etc.). On the other hand, providing only DateFormat as a Util seems like a good approach.
I'll refactor and apply this after the current PR work is completed. Thank you for your suggestions :)

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.

None yet

4 participants