Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
…simplify fake repository
58726f8 to
49304fe
Compare
…tor' into update_pr531_merge_conflicts
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| override val lastCapturedMedia: StateFlow<MediaDescriptor> = | ||
| mediaStoreChangesFlow(context.contentResolver) | ||
| .mapLatest { changedUri -> |
There was a problem hiding this comment.
Is there ever a case where the phone's media database can fail or refuse a lookup (e.g., a security exception)?
Since findLatestAppSpecificUri() runs with a CoroutineScope, this could cause either a crash or a dead image well. This can be resolved with a try/catch if it's a problem, but I'm not 100% sure if this crash is reachable in practice.
There was a problem hiding this comment.
Ah, there is a case. Since storage permission is optional, READ_EXTERNAL_STORAGE can be denied. This would cause problems on lower Android versions.
Now that findLatestAppSpecificUri() -> getLastSavedMediaUriWithDate() is running eagerly at startup, callers can't catch it early.
| ) { | ||
| viewModel.imageWellController.updateLastCapturedMedia() | ||
| } | ||
| } |
There was a problem hiding this comment.
Suppose a user gains permission mid-session. With this old code, updateLastCapturedMedia() would be called automatically. Without this, however, the thumbnail will stay blank until the next MediaStore change (e.g., a new capture).
This can cause the problem described in my previous comment
| val targetUri = when { | ||
| changedUri == null || isCollectionUri(changedUri) -> | ||
| findLatestAppSpecificUri() | ||
| isAppSpecificUri(changedUri) -> changedUri |
There was a problem hiding this comment.
I think there's an issue with treating ANY notified app-specific URI as the latest capture. From what I understand, this is saying "observe for changed items. If an changed item is owned by JCA and in DCIM/Camera, assume it's the newest capture."
(My below comment is based on the above assumption, so please let me know if I'm misunderstanding!)
What if a JCA item is edited but not the newest capture? E.g., wouldn't edits, metadata changes, etc. on an older JCA photo make the older photo show in the image well even if it's not the most recent?
The same issue would be caused by trashed rows. Take a look at https://developer.android.com/reference/android/provider/MediaStore.MediaColumns#IS_TRASHED. "Trashed items are retained until they expire" and given the IS_TRASHED flag. This means that if a user deletes a media item, JCA will see this change to the "IS_TRASHED" flag and count it as a changed item. And since the trashed item is retained and isAppSpecificUri looks up the item by URI (which includes trashed rows), it will be shown in the image well despite being deleted.
There was a problem hiding this comment.
What if we just call findLatestAppSpecificUri on every notification? I'm not sure why isAppSpecificUri is needed.
| proj | ||
| ) ?: if (values.containsKey(MediaStore.MediaColumns.DISPLAY_NAME)) { | ||
| val name = values.getAsString(MediaStore.MediaColumns.DISPLAY_NAME) | ||
| if (name?.startsWith("JCA") == true) packageName else "com.other.app" |
There was a problem hiding this comment.
nit: should this hardcoded "JCA" use the newly-defined generator prefix?
Addresses review feedback on LocalMediaRepository.lastCapturedMedia: - Catch exceptions thrown while querying the MediaStore (e.g. SecurityException when READ_EXTERNAL_STORAGE is not granted on API <= 28). Previously an exception would terminate the stateIn coroutine, leaving the flow stuck and crashing the process via the default handler. The flow now logs, falls back to the cached value, and stays alive so a later notification can recover. - When a change notification is for one of this app's items, re-query for the latest item instead of assuming the changed item is the latest. An edit or trash operation on an older item no longer surfaces it in the image well. Changes to foreign items keep the existing cheap exists() check. - Add MediaRepository.refreshLastCapturedMedia(), backed by a StateFlow counter merged into the change source. The counter's initial value also drives the initial query, replacing the callbackFlow's manual initial emission. - Use the existing ignoreResult() extension instead of leaving return values unused. Tests cover the edited-older-item case, a failing query that later recovers, and refresh without a MediaStore notification.
On API <= 28 the MediaStore cannot be read until storage permission is granted, and it does not send a change notification when that happens. Add a StoragePermissionGuard next to CameraPermissionGuard that calls MediaRepository.refreshLastCapturedMedia() when the permission becomes granted, so the image well is populated without waiting for the next capture. This restores the behavior of the permission-keyed LaunchedEffect that was removed from PreviewScreen, but keeps it at the app shell where the other permission handling lives.
FakeContentProvider inferred OWNER_PACKAGE_NAME from a hard-coded "JCA" display-name prefix. Use a configurable ownerPrefix that defaults to FakeFilePathGenerator's prefix, and share the inference between the collection filter and createRow.
Avoids an inferred Unit return type on a public declaration, which internal lint flags.
2 participants
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 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.