Kim/stabilization UI logic #94
Conversation
Kimblebee
force-pushed
the
kim/stabilization_ui_logic
branch
from
a64d19e
to
64078c3
Compare
domain/camera/src/main/java/com/google/jetpackcamera/domain/camera/CameraXCameraUseCase.kt
Show resolved
Hide resolved
data/settings/src/main/java/com/google/jetpackcamera/settings/test/FakeJcaSettingsSerializer.kt
Outdated
Show resolved
Hide resolved
feature/settings/src/main/java/com/google/jetpackcamera/settings/ui/SettingsComponents.kt
Outdated
Show resolved
Hide resolved
* split preview and video support into separate booleans * cameraAppSettings represents stabilization support as FULL, VIDEO_ONLY, and UNSUPPORTED * updated color of disabled UI
Kimblebee
force-pushed
the
kim/stabilization_setting
branch
from
e0a1eef
to
bd7f6da
Compare
Kimblebee
force-pushed
the
kim/stabilization_ui_logic
branch
from
9303199
to
e8aa6ce
Compare
* small preview refactor to better organize components * convert preview overlay layer to a relative/grid layout style
| ) | ||
| ) | ||
| if (previewUiState.currentCameraSettings.supportedStabilizationMode != | ||
| SupportedStabilizationMode.UNSUPPORTED && |
There was a problem hiding this comment.
Shouldn't it be the case that if the current stabilization mode is UNSUPPORTED then the stabilization mode can't be set to ON? If so, then we can get rid of the check for UNSUPPORTED, we should just go with whatever the stabilization mode is set to.
| Spacer(modifier = Modifier.height(10.dp)) | ||
|
|
||
| // on selector | ||
| SingleChoiceSelector( | ||
| text = stringResource(id = R.string.stabilization_selector_on), | ||
| secondaryText = stringResource(id = R.string.stabilization_selector_on_info), | ||
| enabled = supportedStabilizationMode == SupportedStabilizationMode.FULL, |
There was a problem hiding this comment.
Do you need to do something similar for the "high quality selector" below?
There was a problem hiding this comment.
Not necessary for the UI's current implementation. if the device can't support video stabilization at all, then the popup won't be able to open in the first place. the whole button itself would be greyed out and unclickable.
There was a problem hiding this comment.
Aren't video stabilization and preview stabilization mutually exclusive?
If preview stabilization is supported, but video stabilization isn't, then you should be able to choose between ON and OFF.
Alternatively if preview stabilization isn't supported but video stabilization is, you should be able to select between HIGH_QUALITY and OFF.
There was a problem hiding this comment.
Thats true, I assumed if the output feed couldn't be stabilized, the user may not want to bother with stabilization settings. I think to avoid confusion with ON meaning: "preview and video" OR "preview only if video isn't supported", I could add an extra option/enum PREVIEW_ONLY. It can be a hidden option only displayed when the device doesn't support video.
The setting would only be completely disabled if neither preview nor video were supported.
There was a problem hiding this comment.
I could add an extra option/enum PREVIEW_ONLY. It can be a hidden option only displayed when the device doesn't support video.
I think there's still some confusion here about preview stabilization. Preview stabilization means that Preview AND Video will be stabilized (it's WYSIWYG, hence why we call it "ON"). You don't need to enable video stabilization to stabilize the video, the preview stabilization will be sufficient to stabilize both. When you enable both preview and video stabilization at the same time, the preview stabilization takes precedence and stabilizes both, and the video stabilization setting on the VideoCapture use case will be ignored, hence why its ok to set it to UNSPECIFIED. There shouldn't be a PREVIEW_ONLY setting since preview stabilization will always apply a (lower quality) stabilization to the video.
The setting would only be completely disabled if neither preview nor video were supported.
That is the correct behavior. It means it's not possible to stabilize Preview AND Video with preview stabilization. It's also not possible to get high quality video stabilization with just video stabilization.
feature/settings/src/main/java/com/google/jetpackcamera/settings/ui/SettingsComponents.kt
Show resolved
Hide resolved
feature/preview/src/main/java/com/google/jetpackcamera/feature/preview/PreviewScreen.kt
Outdated
Show resolved
Hide resolved
* preview-only setting option/enum * stabilization icon descriptions based on current stabilization setting * adjust setting selections for preview/video stabilization depending on mode
* Fix issue where UiAutomator cannot find components * spotless
...settings/src/main/java/com/google/jetpackcamera/settings/model/SupportedStabilizationMode.kt
Outdated
Show resolved
Hide resolved
domain/camera/src/main/java/com/google/jetpackcamera/domain/camera/CameraXCameraUseCase.kt
Outdated
Show resolved
Hide resolved
changes for stabilization setting PR , i thought that it may be better to just put it into a separate PR for visibility of the changes.
here are the big changes to come of it:
CameraXCameraUseCasesets preview/video stabilization support information to datastoreOnoption if preview stabilization is unavailableHigh Qualityoption if video stabilization is unavailable