Skip to content

Conversation

@dturner
Copy link

@dturner dturner commented Apr 3, 2024

What I have done and why

  • Moved ScreenFlashUiState out of ScreenFlash to reduce verbosity for callers
  • Refactored ScreenFlashUiState into a sealed interface hierarchy (Applied and NotApplied) to make it clearer which states are valid, and what those states represent
  • Changed the way that the UI state flow is initialized. It is now done by mapping events from the cameraUseCase.getScreenFlashEvents flow to their relevant UI states. This avoids having an intermediate MutableStateFlow and by using .stateIn(started = WhileSubscribed) we will only receive events when we have downstream subscribers (i.e. the UI)
  • Setting the screen brightness to restore no longer triggers an event
  • CoroutineScope is only used during ScreenFlash initialization (no heap allocated coroutine scope - more info: https://www.droidcon.com/2023/10/06/coroutines-flow-android-the-good-parts/ see around 11 mins in)

@temcguir temcguir requested a review from Zeronfinity April 3, 2024 19:59
Copy link
Collaborator

@Zeronfinity Zeronfinity left a comment

Choose a reason for hiding this comment

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

The implementation looks pretty clean now, thanks!

@Zeronfinity
Copy link
Collaborator

It seems like CameraXCameraUseCase has the same problem then? Should we refactor there as well?

@temcguir
Copy link
Collaborator

temcguir commented Apr 4, 2024

It seems like CameraXCameraUseCase has the same problem then? Should we refactor there as well?

It looks like the CoroutineScope in CameraXCameraUseCase is used only to emit screen flash events into theMutableSharedFlow. Since we're already converting this to a StateFlow in ScreenFlash.kt, we could just use a StateFlow in CameraUseCase and avoid needing to launch into the CoroutineScope. We can then remove it from the CameraXCameraUseCase constructor.

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.

3 participants