-
Notifications
You must be signed in to change notification settings - Fork 59
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
base: main
Are you sure you want to change the base?
Conversation
@romanofranz |
I removed the code that locked the screen orientation when the orientation changes in the camera composable. Screen_recording_20240408_142929.webm |
Changed the animation on Activity rotation
Thanks, this works better!
Overall LGTM on the camera side, adding @donovanfm to check the rest of the refactoring. |
CameraSettings
# 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
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.
LGTM!
app/build.gradle.kts
Outdated
@@ -53,7 53,11 @@ android { | |||
} | |||
kotlinOptions { | |||
jvmTarget = "17" | |||
freeCompilerArgs = listOf("-Xcontext-receivers") | |||
freeCompilerArgs = listOf("-Xcontext-receivers") | |||
freeCompilerArgs = listOf( |
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.
is this change related to this PR on Camera UI? If not, I'd prefer it not be part of this PR.
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.
It wasn't related to the Camera UI.
Thank you for your feedback. 😀
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.
can we remove this empty file
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.
can we remove this empty file
<!--Camera Screen --> | ||
<string name="photo">Photo</string> | ||
<string name="video">Video</string> |
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.
Where are those used?
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.
Used by the CameraControls
Composable. :)
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.
Good, I found it. Thank you.
} | ||
} | ||
|
||
companion object { | ||
const val FILENAME_FORMAT = "yyyy-MM-dd-HH-mm-ss-SSS" |
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.
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) |
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.
Is there any reason to remove FILENAME_FORMAT from CameraViewModel companion object and set format here?
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.
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? 🤔
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.
You can do that simply making it as domain layer's usecase or just utils.
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.
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 :)
To-do
Example screen
fold_capture.webm