Fix crash on rapid double-clicking video record button - #541
Fix crash on rapid double-clicking video record button#541davidjiagoogle wants to merge 6 commits into
Conversation
Add CaptureButtonUiState.Enabled.Recording.Pending state to represent the window when video recording is starting. Map VideoRecordingState.Starting to Pending instead of Idle, preventing rapid double-clicks from sending duplicate StartRecordingEvents. Additionally, update CameraSession to log a warning instead of throwing an unhandled IllegalStateException when receiving duplicate StartRecordingEvents during active recording. Bug: 535645067 Test: ./gradlew :ui:uistateadapter:capture:test
There was a problem hiding this comment.
Code Review
This pull request introduces a new Pending state to CaptureButtonUiState.Enabled.Recording to represent when a video recording request has been sent but is still initializing. This state is mapped from VideoRecordingState.Starting in the adapter and handled in the capture button components and tests. Additionally, CameraSession now logs a warning instead of throwing an exception if a start recording event is received while a recording is already in progress. Feedback was provided regarding a potential bug where releasing the capture button during the Pending state would ignore the key-up event, causing the recording to continue indefinitely; a code suggestion was provided to handle this state in onKeyUp.
temcguir
left a comment
There was a problem hiding this comment.
It would be awesome to add a quick unit test to PreviewViewModelTest.kt to ensure this specific fast-release cancellation doesn't regress.
First, add a diagnostic counter to FakeCameraSystem.kt:
var numVideoRecordingStarts = 0
override suspend fun startVideoRecording(
saveLocation: SaveLocation,
onVideoRecord: (OnVideoRecordEvent) -> Unit
) {
if (!useCasesBinded) {
throw IllegalStateException("Usecases not bound")
}
numVideoRecordingStarts++
recordingInProgress = true
}
Then we can add the following test directly inside PreviewViewModelTest.kt to verify that rapidly firing "stop" immediately cancels the "start" job:
@Test
fun fastStartAndStopVideoRecording_cancelsRecording() = runTest(StandardTestDispatcher()) {
startCameraUntilRunning()
// Start and stop immediately without advancing the dispatcher
previewViewModel.captureController.startVideoRecording()
previewViewModel.captureController.stopVideoRecording()
// Let the coroutines execute
advanceUntilIdle()
// Verify that because we cancelled the job synchronously, the start implementation
// was bypassed completely to protect us from the channel race condition!
assertThat(cameraSystem.numVideoRecordingStarts).isEqualTo(0)
}
- Rename Pending state to Starting in CaptureButtonUiState - Handle key-up release during Starting state in CaptureButtonComponents - Synchronously cancel recordingJob in CaptureControllerImpl.stopVideoRecording() - Add fastStartAndStopVideoRecording_cancelsRecording unit test
Summary
Fixes an issue (b/535645067) where double-clicking or rapidly tapping the video recording button in quick succession crashed the app with
IllegalStateException: A recording is already in progress.Changes Made
CaptureButtonUiState.Enabled.Recording.Pending: Created a new UI state representing the initialization window when video recording is starting.VideoRecordingState.Starting: MappedVideoRecordingState.StartingtoCaptureButtonUiState.Enabled.Recording.Pendinginstead ofIdleinCaptureButtonUiStateAdapter.kt.CaptureButtonComponents.kt: AddedRecording.Pendingto the ignored/no-op states during click handling (onKeyUp), preventing rapid consecutive clicks from sending duplicateStartRecordingEvents.CameraSession.kt: Replaced throwing an unhandledIllegalStateExceptionwith logging a warning when receiving a duplicateStartRecordingEventduring active recording.CaptureButtonUiStateAdapterTest.ktto assert the newPendingstate.