Decouple ui:controller:impl and ui:uistateadapter:capture from ui:components:capture#507
Decouple ui:controller:impl and ui:uistateadapter:capture from ui:components:capture#507davidjiagoogle wants to merge 17 commits into
Conversation
…ponents:capture to reduce library footprint for external embedding apps
There was a problem hiding this comment.
Code Review
This pull request refactors the project by moving capture-related test tags and the DisabledReason enum from the ui:components:capture module to the ui:uistate module, while also adding corresponding localized string resources. Review feedback identifies a missing R class import in CaptureControllerImpl.kt required for string resource access and a bug where HDR_VIDEO_UNSUPPORTED_ON_LENS_TAG was assigned a duplicate string value.
…eControllerImpl.kt and fix duplicate string value assigned to HDR_VIDEO_UNSUPPORTED_ON_LENS_TAG
…_CAPTURE_EXTERNAL_UNSUPPORTED_TAG from com.google.jetpackcamera.ui.uistate module
…ocated test tag constants across instrumentation source sets
temcguir
left a comment
There was a problem hiding this comment.
Decoupling ui:controller:impl and ui:uistateadapter:capture from ui:components:capture is the right goal for allowing JCA to be embedded externally.
However, moving test tags and UI string resources into ui:uistate highlights a deeper architectural issue: we are passing test tags through our state layer. Test tags in Compose are meant to identify structural UI nodes (e.g., a "Snackbar" component), not the data passing through them.
The cleanest architecture is to remove the testTag property from both SnackbarData and DisableRationale.
- Our UI tests can find the UI element using a single static tag (like
SNACKBAR_NODE_TAGdefined inui:components) and then inspect its text or semantics to verify the specific message or severity. ui:controllershould stop constructingSnackbarDataand just emit domain events.ui:uistateadapter:captureshould evaluate constraints and provide the string resources for the rationales.
This severs the dependencies on test tags across the board and makes the state layer generic. I have left a few inline comments showing how we can distribute these changes.
…son and strings, drop testTag from SnackbarData
…tead of deleted test tags
…rom DisableRationale
…flict in ComposeTestRuleExt.kt
…ponents # Conflicts: # app/src/androidTest/java/com/google/jetpackcamera/ConcurrentCameraTest.kt
There was a problem hiding this comment.
I noticed that the UI tests were updated to use waitForNodeWithText to globally search for the success/failure strings. This defeats the purpose of adding the SNACKBAR_NODE_TAG and could lead to flaky tests or false positives if that same text ever appears elsewhere on the screen.
Could you update the testing approach to ensure we are specifically inspecting the Snackbar? I recommend creating a new utility function in ComposeTestRuleExt.kt (e.g., waitForSnackbarWithText) that explicitly waits for a node matching both the SNACKBAR_NODE_TAG and the expected text.
Here is an example of what that could look like:
fun ComposeTestRule.waitForSnackbarWithText(
@StringRes textResId: Int,
timeoutMillis: Long = DEFAULT_TIMEOUT_MILLIS
) {
val expectedText = getResString(textResId)
waitUntil(timeoutMillis = timeoutMillis) {
onAllNodes(hasTestTag(SNACKBAR_NODE_TAG) and hasText(expectedText))
.fetchSemanticsNodes().isNotEmpty()
}
}You can then replace the waitForNodeWithText calls with waitForSnackbarWithText where appropriate in the test files. This will make the UI tests much more robust.
Breaks the transitive dependency chain to exclude heavy development/tooling dependencies when JCA is embedded as an external library.