Restore datastore-proto module and decouple via SettingsDataSource#534
Restore datastore-proto module and decouple via SettingsDataSource#534temcguir wants to merge 12 commits into
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 introduces a new :data:settings:datastore-proto module to encapsulate Proto DataStore settings, refactors LocalSettingsRepository to use a new SettingsDataSource interface, and migrates snackbar/toast testing from custom test tags to string resources. Feedback focuses on a critical channel competition and deadlock bug in PreviewViewModel, missing Dispatchers.resetMain() in test teardown, and several style guide violations regarding missing @RunWith annotations, missing KDoc documentation, and insufficiently restrictive visibility modifiers.
temcguir
force-pushed
the
temcguir/restore-proto-datastore-module
branch
from
cf954d8 to
4fe8a7f
Compare
This PR restores the `datastore-proto` module that was removed in PR #525. It brings back the `.proto` files, mappers, and serializers, but in a separate module under `:core:settings:datastore-proto`. Key architectural changes: - The `datastore-proto` module encapsulates `DataStoreFactory.create()` so `androidx.datastore` is no longer exposed to `:app`. - The `datastore-proto` module is completely Hilt-free. - Updates `:app`'s `AppSettingsModule` to manually provide the `ProtoDataStoreSettingsDataSource`.
temcguir
force-pushed
the
temcguir/restore-proto-datastore-module
branch
from
4fe8a7f to
190c006
Compare
temcguir
force-pushed
the
temcguir/restore-proto-datastore-module
branch
from
a3a5cf3 to
6004e3f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a proto-based DataStore settings implementation across new modules (datastore-proto, model-proto, and settings-proto) and configures the app to use it. Feedback highlights a critical architectural issue where the new ProtoDataStoreSettingsDataSource is set as the primary settings provider but lacks support for concurrentCameraMode, which would cause a silent functional regression. Additionally, the reviewer noted that :app still directly depends on androidx.datastore (violating encapsulation), unused dead code was introduced in concurrent_camera_mode.proto, and several style guide violations exist regarding missing @RunWith annotations on test classes and missing KDoc documentation on public APIs.
temcguir
force-pushed
the
temcguir/restore-proto-datastore-module
branch
2 times, most recently
from
f85a242 to
b219771
Compare
temcguir
force-pushed
the
temcguir/restore-proto-datastore-module
branch
from
b219771 to
97f9e98
Compare
|
@gemini-code-assist We have addressed the architectural and functional issues!
For the KDoc comments on the simple mapping functions, I left inline comments asking if they are strictly necessary. Let us know if anything else is needed! |
|
Great work on addressing these architectural concerns and restoring the functionality! Decoupling the DataStore implementation from the Regarding your question about the KDoc comments on the mapping functions: While they might seem simple, I recommend keeping them. They serve as useful documentation for other developers, provide helpful context in IDE tooltips, and ensure consistency across the codebase. Even for straightforward mappings, documenting the intent—especially if there are default values or specific mapping logic—is a good practice for long-term maintainability. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the application settings from Preferences DataStore to Proto DataStore by introducing the core:model-proto, core:settings:settings-proto, and core:settings:datastore-proto modules, along with corresponding data mappers and tests. Feedback on the changes includes ensuring that all default values (such as targetFrameRate and lowLightBoostPriority) are explicitly defined in the serializer, documenting public create functions with KDoc, removing an unused TAG constant, and properly resetting the main dispatcher in tests to avoid test pollution.
Added <ENUM_NAME>_UNSPECIFIED = 0 to all proto enums to comply with official proto3 style guide requirements. Updated ModelMappers.kt to handle the new UNSPECIFIED values and safely fallback to defaults. Also added a rule enforcing this to the local styleguide.
- Set targetFrameRate and lowLightBoostPriority explicitly in ProtoCameraAppSettingsSerializer's defaultValue. - Added KDoc for create() in ProtoDataStoreSettingsDataSource and PrefsDataStoreSettingsDataSource. - Removed unused TAG from ProtoDataStoreSettingsDataSource. - Added Dispatchers.resetMain() to tearDown in ProtoDataStoreSettingsDataSourceTest.
2 participants
This PR restores the
datastore-protomodule that was removed in PR #525. It brings back the.protofiles, mappers, and serializers, but in a separate module under:data:settings:datastore-proto.Key architectural changes:
SettingsDataSourceto abstract away the datastore implementation details fromLocalSettingsRepository.datastore-protomodule encapsulatesDataStoreFactory.create()soandroidx.datastoreis no longer exposed to:app.datastore-protomodule is completely Hilt-free.:app'sAppSettingsModuleto manually provide theProtoDataStoreSettingsDataSource.<ENUM_NAME>_UNSPECIFIED = 0element to all proto enums, mapping them to safe defaults in the mappers..toDomain()to.toModel()in the proto mappers for semantic consistency..gemini/styleguide.mdwith a rule to enforce proto3 zero-value best practices.targetFrameRateandlowLightBoostPriority).