Refactor preferences by RankoR · Pull Request #691 · GrapheneOS/Camera

RankoR · 2026-08-11T22:15:35Z

Depends on #687
Closes #686

runBlocking and limited DI are intentional, as rewrite is phased.

…lockscreen

sdsantos

Code looks good.

But I found a bug on the last photo preview. It shows empty (white), and when I tap it, it shows an error message. This persists until I take a photo, then everything works fine. If I open the production Camera app, the last photo preview is fine. Video:

camera_gallery_tap_bug.mp4

sdsantos · 2026-08-12T14:48:33Z

+        update: (CameraSettings) -> CameraSettings,
+        persist: () -> Unit,
+    ): Flow<Unit> {
+        return unitFlow {


Why a Flow instead of a plain suspend method?

I personally prefer Flows because IMO chaining them looks more elegant, and things like catching exceptions / switching dispatchers are easier to read compared to suspend functions

Regarding the white preview - it's not a regression, it's pre-existing. Will be fixed during UX/UI refresh.

I think Flows work when there's a stream of events, but for one-shot updates it puts the onus on the caller to call .collect() to ensure the write is done before continuing execution. I'm not sure if there's a use-case for chaining these set* calls for a one-shot Flow<Unit> vs a sequence of suspend functions

m4pl · 2026-08-13T13:55:11Z

+        run {
+            seedCommonDefaults()
+            readCommonSettings()
+        },


The seed and read run in a field initializer, above slotted. An init {} below the properties makes that safe by construction and drops the run {}.

m4pl · 2026-08-13T14:23:14Z

+    ): SharedPreferences {
+        return commonPreferences(
+            context = context,
+            ephemeral = context is SecureActivity,


Nothing checks that lockscreen activities actually carry the SecureActivity marker. Add a test so it can't be lost?

m4pl · 2026-08-13T14:24:25Z

+
+    fun refresh()
+
+    fun setAspectRatio(value: Int): Flow<Unit>


Every call site unwraps this with runBlocking { .collect() }, and forgetting .collect() silently drops the write. Would plain fun setX(value) work here?

runBlocking is a temp solution to reduce per-PR changes, later it will look better. We should probably add a hard linter rule for that (Android Studio already highlights such things, but it should be a CI failure)

inthewaves · 2026-08-13T23:02:06Z

 app/src/main/java/app/grapheneos/camera/
  data/
+    core/
+      model/            types more than one feature stores, e.g. CameraMode
+      store/            the preferences files themselves, and the keys features share
    settings/
      model/            CameraSettings, per-mode setting values
      repository/       SettingsRepository (entry-mode-scoped, never application-scoped)
-      store/            prefs-backed stores, EphemeralSharedPrefs namespace
    camera/
      model/            CameraCapabilities, lens/extension descriptors
      repository/       CameraProviderSource
      store/            ExtensionAvailabilityStore
    media/
      model/            CapturedItem and friends
-      repository/       CapturedItemStore
-      store/            MediaStoreDataSource, SafDataSource
+      repository/       CapturedItemRepository
+      store/            CapturedItemStore, MediaStoreDataSource, SafDataSource
    location/
      repository/       LocationRepository
  domain/


Should we be expecting this to be updated by agents as it goes in general?

inthewaves · 2026-08-13T23:14:33Z

+    override fun trackSafTree(treeUri: Uri) {
+        val trees = previousSafTrees().toMutableList()
+
+        trees.remove(treeUri)
+        trees.add(0, treeUri)
+
+        while (trees.size > CapturedItems.MAX_NUMBER_OF_TRACKED_PREVIOUS_SAF_TREES) {
+            // trees.removeLast() requires API level 35 now due to Java adding it
+            trees.removeAt(trees.lastIndex)
+        }
+
+        commons.edit {
+            putPreviousSafTrees(trees, this)
+        }
+    }


Maybe a sequence would be cleaner, e.g. it would only need to take MAX_NUMBER_OF_TRACKED_PREVIOUS_SAF_TREES elements instead of having to remove n - MAX_NUMBER_OF_TRACKED_PREVIOUS_SAF_TREES elements from the removeAt loop.

val previousTrees = previousSafTrees() .asSequence() .filterNot { it == treeUri } val trees = sequenceOf(treeUri) .plus(previousTrees) .take(CapturedItems.MAX_NUMBER_OF_TRACKED_PREVIOUS_SAF_TREES)

putPreviousSafTrees doesn't seem to inherently need a list anyway, since it just does trees.joinToString

Although MAX_NUMBER_OF_TRACKED_PREVIOUS_SAF_TREES is pretty small anyway, so this is more of a nit

inthewaves · 2026-08-13T23:55:55Z

+        update: (CameraSettings) -> CameraSettings,
+        persist: () -> Unit,
+    ): Flow<Unit> {
+        return unitFlow {


I think Flows work when there's a stream of events, but for one-shot updates it puts the onus on the caller to call .collect() to ensure the write is done before continuing execution. I'm not sure if there's a use-case for chaining these set* calls for a one-shot Flow<Unit> vs a sequence of suspend functions

inthewaves · 2026-08-14T00:10:39Z

+@AndroidEntryPoint
 class InAppGallery : AppCompatActivity() {


Secure gallery mode is selected through the isSecureMode intent extra, but repository DI determines whether to use LockscreenCapturedItemRepository by checking whether the activity implements SecureActivity. Since InAppGallery does not implement that marker, a secure gallery launch receives the ordinary repository.

This does not cause a current behavior bug: the gallery only calls capturedItems(), which the lockscreen wrapper delegates unchanged. However, the injected interface also exposes persistent mutation and SAF grant-release operations. If the marker is intended to enforce the lockscreen dependency boundary, consider injecting a narrower read-only gallery repository or using a distinct secure gallery activity. Otherwise, documenting this as an intentional exception would make the split between isSecureMode and SecureActivity clear

inthewaves · 2026-08-14T00:16:40Z

    override fun onCreate(savedInstanceState: Bundle?) {
        isSecureMode = intent.getBooleanExtra(INTENT_KEY_SECURE_MODE, false)


Secure mode appears to have two representations:

SecureActivity is a type-level marker used by dependency injection to select secure/ephemeral repositories

INTENT_KEY_SECURE_MODE is runtime state used by InAppGallery to restrict UI behavior, filter media, and manage the lockscreen lifecycle

InAppGallery can therefore have isSecureMode == true without being a SecureActivity. Its UI behaves securely, but DI still supplies the ordinary CapturedItemRepository.

See the other review comment

RankoR added 12 commits August 8, 2026 21:15

Add AGENTS.md

ec05def

Add .editorconfig

25552e5

Pin CameraX to the version its internal APIs were verified against

e3d7577

Add a unit test source set and cover the ephemeral prefs used on the …

1211d50

…lockscreen

Cover the camera intent contracts and lockscreen preference isolation

c77fa88

Add detekt and ktlint, baselining the existing sources

4304bb5

Fix missing deps verification hashes

8e2bc65

Move the camera mode enum into the data layer

f1492c6

Load and save captured items through a repository

0076efc

Read and write camera settings through a repository

5b08092

Isolate every lockscreen session's preferences from the owner's

a1625f8

Document the store role and the preferences carve-out

8b000b2

RankoR requested review from inthewaves, m4pl and sdsantos August 11, 2026 22:16

sdsantos approved these changes Aug 12, 2026

View reviewed changes

m4pl approved these changes Aug 13, 2026

View reviewed changes

inthewaves suggested changes Aug 14, 2026

View reviewed changes

		@AndroidEntryPoint
		class InAppGallery : AppCompatActivity() {

		override fun onCreate(savedInstanceState: Bundle?) {
		isSecureMode = intent.getBooleanExtra(INTENT_KEY_SECURE_MODE, false)

Uh oh!

Uh oh!

Conversation

RankoR commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdsantos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inthewaves Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inthewaves Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inthewaves Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RankoR commented Aug 11, 2026 •

edited

Loading

inthewaves Aug 13, 2026 •

edited

Loading

inthewaves Aug 13, 2026 •

edited

Loading

inthewaves Aug 13, 2026 •

edited

Loading