Skip to content

Refactor settings storage to Preferences DataStore to remove Protobuf#525

Open
davidjiagoogle wants to merge 16 commits into
mainfrom
david/protobufRemoval
Open

Refactor settings storage to Preferences DataStore to remove Protobuf#525
davidjiagoogle wants to merge 16 commits into
mainfrom
david/protobufRemoval

Conversation

@davidjiagoogle

Copy link
Copy Markdown
Collaborator

This PR refactors settings storage from Proto DataStore to Preferences DataStore. This allows us to completely remove the com.google.protobuf dependency and its generated Java code, saving ~134 KB of bytecode footprint in platform environments where dependencies are linked dynamically.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request migrates the application's settings storage from Proto DataStore to Preferences DataStore, removing all Protobuf dependencies, plugins, and schema files. Model enums and DebugSettings have been updated to remove proto conversion methods, with DebugSettings now using a custom string-based serialization format. LocalSettingsRepository has been refactored to read and write settings using Preferences keys. The review feedback highlights critical robustness issues where unsafe string parsing (using valueOf directly on stored preferences or parsing DebugSettings without validation) could lead to runtime crashes (e.g., IllegalArgumentException or NumberFormatException) if the data is corrupted or malformed. Implementing safe parsing helpers with fallback defaults is highly recommended to ensure app stability.

@davidjiagoogle davidjiagoogle force-pushed the david/protobufRemoval branch from 51caa20 to 4279a6b Compare June 1, 2026 18:21
@davidjiagoogle davidjiagoogle requested a review from Kimblebee June 8, 2026 22:17
Comment thread core/settings/datastore-prefs/build.gradle.kts Outdated
Comment thread core/settings/datastore-prefs/build.gradle.kts Outdated
@davidjiagoogle davidjiagoogle requested a review from temcguir June 10, 2026 23:35
testPattern = TestPattern.fromProto(proto.testPattern)
)
}
fun parseFromString(value: String): DebugSettings {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we are replacing Protobuf with a custom, hand-rolled string parser, we need unit tests to ensure this logic is robust. String parsing based on delimiters (:, ;, ,) and specific string formats (like SolidColor(...)) is prone to regressions if the model changes in the future.

Could you please add a DebugSettingsTest.kt in the core:model module? It should verify that parseFromString(encodeAsString()) correctly rebuilds the object. Specifically, we should test:

  1. Default values (all fields empty/off).
  2. Simple TestPattern variants (like ColorBars).
  3. The complex TestPattern.SolidColor variant to ensure the regex/splitting of the 4 color channels works correctly.
  4. Edge cases like missing delimiters or malformed strings (to ensure it falls back gracefully).

* Parses the URL-encoded serialized string from the navigation route.
*/
override fun parseValue(value: String): DebugSettings = DebugSettings.parseFromString(value)
override fun parseValue(value: String): DebugSettings {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similarly, we should add a quick unit test (DebugSettingsNavTypeTest.kt) to verify the URL encoding and decoding steps here. We want to ensure that parseValue(serializeAsValue(debugSettings)) returns the original object and that the resulting string is safe for Compose Navigation routes.

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