Enhance MediaRepository with reactive flow and update ImageWell animation#531
Enhance MediaRepository with reactive flow and update ImageWell animation#531temcguir wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the media repository to expose the last captured media as a continuous StateFlow (lastCapturedMedia) instead of a suspend function, and updates the ImageWell UI component to observe this flow with smoother transition animations. It also enhances the testing infrastructure, including FakeContentProvider and LocalMediaRepositoryTest. The review feedback identifies a potential race condition in LocalMediaRepository due to unsynchronized cache access, suggests a more robust package ownership check on Android Q+, and recommends simplifying the flow implementation in FakeMediaRepository to prevent coroutine leaks.
| private var cachedUri: Uri? = null | ||
| private var cachedMediaDescriptor: MediaDescriptor? = null |
There was a problem hiding this comment.
The mutable properties cachedUri and cachedMediaDescriptor are accessed and modified concurrently within the lastCapturedMedia flow (which runs on iODispatcher / repositoryScope). Because mapLatest uses cooperative cancellation, a cancelled collection block can continue running concurrently with a new block after a suspension point. Since there is no synchronization, this can lead to race conditions and thread-visibility issues. Consider using a Mutex to synchronize access to these cached properties.
| private var cachedUri: Uri? = null | |
| private var cachedMediaDescriptor: MediaDescriptor? = null | |
| private val cacheMutex = kotlinx.coroutines.sync.Mutex() | |
| private var cachedUri: Uri? = null | |
| private var cachedMediaDescriptor: MediaDescriptor? = null |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { | ||
| val ownerColumn = | ||
| cursor.getColumnIndex(MediaStore.MediaColumns.OWNER_PACKAGE_NAME) | ||
| if (ownerColumn != -1 && !cursor.isNull(ownerColumn)) { | ||
| val owner = cursor.getString(ownerColumn) | ||
| if (owner == context.packageName) return@withContext true | ||
| } | ||
| } | ||
| val nameColumn = | ||
| cursor.getColumnIndexOrThrow(MediaStore.MediaColumns.DISPLAY_NAME) | ||
| val name = cursor.getString(nameColumn) | ||
| return@withContext name?.startsWith("JCA") == true |
There was a problem hiding this comment.
On Android Q and above, the OWNER_PACKAGE_NAME is the definitive source of truth for app ownership. If the owner package name is present but does not match our package name, we should return false immediately rather than falling back to checking the display name prefix (which could result in false positives if another app creates a file starting with "JCA").
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { | |
| val ownerColumn = | |
| cursor.getColumnIndex(MediaStore.MediaColumns.OWNER_PACKAGE_NAME) | |
| if (ownerColumn != -1 && !cursor.isNull(ownerColumn)) { | |
| val owner = cursor.getString(ownerColumn) | |
| if (owner == context.packageName) return@withContext true | |
| } | |
| } | |
| val nameColumn = | |
| cursor.getColumnIndexOrThrow(MediaStore.MediaColumns.DISPLAY_NAME) | |
| val name = cursor.getString(nameColumn) | |
| return@withContext name?.startsWith("JCA") == true | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { | |
| val ownerColumn = | |
| cursor.getColumnIndex(MediaStore.MediaColumns.OWNER_PACKAGE_NAME) | |
| if (ownerColumn != -1 && !cursor.isNull(ownerColumn)) { | |
| val owner = cursor.getString(ownerColumn) | |
| return@withContext owner == context.packageName | |
| } | |
| } | |
| val nameColumn = | |
| cursor.getColumnIndexOrThrow(MediaStore.MediaColumns.DISPLAY_NAME) | |
| val name = cursor.getString(nameColumn) | |
| return@withContext name?.startsWith("JCA") == true |
| private val _lastCapturedMedia = MutableStateFlow<MediaDescriptor>(MediaDescriptor.None) | ||
| override val lastCapturedMedia: StateFlow<MediaDescriptor> = _lastCapturedMedia.asStateFlow() | ||
| .filter { | ||
| when (it) { | ||
| is MediaDescriptor.None -> true | ||
| is MediaDescriptor.Content -> it.thumbnail != null | ||
| } | ||
| } | ||
| .distinctUntilChanged() | ||
| .stateIn( | ||
| scope = CoroutineScope(Dispatchers.Main), | ||
| started = SharingStarted.Eagerly, | ||
| initialValue = MediaDescriptor.None | ||
| ) |
There was a problem hiding this comment.
The fake repository is using a complex flow pipeline with filter, distinctUntilChanged, and stateIn using an unmanaged CoroutineScope(Dispatchers.Main). This introduces unnecessary complexity, can cause coroutine leaks, and will fail in pure JUnit tests where Dispatchers.Main is not available. Since this is a fake, we should simplify it to a direct StateFlow backed by a MutableStateFlow without unmanaged scopes or main thread dependencies.
| private val _lastCapturedMedia = MutableStateFlow<MediaDescriptor>(MediaDescriptor.None) | |
| override val lastCapturedMedia: StateFlow<MediaDescriptor> = _lastCapturedMedia.asStateFlow() | |
| .filter { | |
| when (it) { | |
| is MediaDescriptor.None -> true | |
| is MediaDescriptor.Content -> it.thumbnail != null | |
| } | |
| } | |
| .distinctUntilChanged() | |
| .stateIn( | |
| scope = CoroutineScope(Dispatchers.Main), | |
| started = SharingStarted.Eagerly, | |
| initialValue = MediaDescriptor.None | |
| ) | |
| private val _lastCapturedMedia = MutableStateFlow<MediaDescriptor>(MediaDescriptor.None) | |
| override val lastCapturedMedia: StateFlow<MediaDescriptor> = _lastCapturedMedia.asStateFlow() |
References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression, suggest the simplification. (link)
- Testing Strategy: Use Fakes Over Mocks... Fakes lead to more robust and maintainable tests by focusing on behavior rather than implementation details... (link)
…simplify fake repository
This PR refactors the media data layer to support a reactive unidirectional data flow, enhances the capture UI with improved animations, and addresses b/522926012 by supporting dynamic file prefixes.
Core Changes:
LocalMediaRepositoryto expose alastCapturedMediaStateFlow. It now automatically initializes with the latest app-captured media and reacts to MediaStore changes.MutextoLocalMediaRepositoryto synchronize access to the media cache and prevent race conditions during concurrent MediaStore updates.prefixproperty inFilePathGenerator.LocalMediaRepositorynow uses this prefix dynamically when querying the MediaStore, allowing it to correctly identify app-captured media even when a custom generator (e.g., for Google Photos) is used.OWNER_PACKAGE_NAMEon Android Q+ for identifying app-specific media, falling back to the filename prefix only on older versions.AnimatedContenttransition that performs a "vertical push" animation whenever the last captured media updates.FakeMediaRepositoryto use a directStateFlow, avoiding unmanaged coroutine scopes and potential leaks in tests.Verified:
:data:mediaand:ui:components:capturesuccessfully.spotlessApply.