Lifting debug overrides to their own abstraction #5361
Conversation
- rather than sharing the user facing vector data store
ouchadam
force-pushed
the
feature/adm/debug-overrides-abstraction
branch
from
a40b808
to
f6735b6
Compare
Matrix SDKIntegration Tests Results:
|
| @Provides | ||
| fun providesDefaultVectorOverrides(): DefaultVectorOverrides { | ||
| return DefaultVectorOverrides() | ||
| } |
There was a problem hiding this comment.
Optional: We can remove this entire companion inject if we add @Inject constructor on each of thse classes
It would be nice cleanup that falls well into the scope of this task, but consider it optional though, since I can see we already had this pattern in place
There was a problem hiding this comment.
It's maybe because of @InstallIn(SingletonComponent::class) anotation of the interface FeaturesModule but I am not a big specialist of the DI.
There was a problem hiding this comment.
there's two parts to this
- the default implementations avoid being directly injectable as we want to avoid leaking their implementations and ensure the consumption is only done via
VectorFeatures - we have separate debug and feature DI modules, debug configurations need to know the implementation details in order to provide fallbacks
*Both debug and release need to provide the interface VectorFeatures
// Release only provides the down scoped interface
@Provides
fun providesDefaultVectorFeatures(): VectorFeatures {
return DefaultVectorFeatures()
}// Debug
// Defaults are provided but not accessible outside debug variant (could be inlined into providesDebugVectorFeatures
@Provides
fun providesDefaultVectorFeatures(): DefaultVectorFeatures {
return DefaultVectorFeatures()
}
// debug features which make use of the of the default features
@Provides
fun providesDebugVectorFeatures(context: Context, defaultVectorFeatures: DefaultVectorFeatures): DebugVectorFeatures {
return DebugVectorFeatures(context, defaultVectorFeatures)
}
// Provides the debug instance as the VectorFeatures for the rest of the app
@Binds
fun bindFeatures(debugFeatures: DebugVectorFeatures): VectorFeatureshopefully that makes sense (and wasn't me rambling 😅 )
There was a problem hiding this comment.
I see, this does shine some light on it, thanks!
| private val forceDialPadDisplayFlow: Flow<Boolean> = context.dataStore.data.map { preferences -> | ||
| preferences[forceDialPadDisplay].orFalse() | ||
| } |
There was a problem hiding this comment.
Is there any reason we have these separate from the two override functions above? I think it would be much simpler to remove these vals and move their bodies to those functions
There was a problem hiding this comment.
the 2 members of the interface could also be vals I think.
There was a problem hiding this comment.
will inline into vals 👍
| copy( | ||
| dialPadVisible = it | ||
| ) |
There was a problem hiding this comment.
Not something to action as this is more of a personal preference thing but I never understood opting for this format rather than a single line copy(dialPadVisible = it). It just personally looks cleaner to me
There was a problem hiding this comment.
Yes, when there is only one line it's OK. I think, it's for future hypothetical code to split into 3 lines, so if for instance in the future we have:
copy(
dialPadVisible = it,
other = blah
)But this is definitely not a strong constraint.
There was a problem hiding this comment.
I don't have any strong feelings for either, I chose to stick with the consistency of the file, happy to change!
| @Provides | ||
| fun providesOverrides(): VectorOverrides { | ||
| return DefaultVectorOverrides() | ||
| } |
There was a problem hiding this comment.
Same with the above DI comment. Unfortunately I can't run builds on my machine so I can't confirm this myself but it's worth taking a look imo
There was a problem hiding this comment.
Thanks @ouchadam for the PR, look neat.
Thanks @ericdecanini for the review. I have nothing more to add.
I let you 2 manage this PR until merging on develop!
- also replaces funs with vals as the references are immutable
…vector-im/element-android into feature/adm/debug-overrides-abstraction
Type of change
Content
Lifts the debug only options to their own
VectorOverridesclass, this class is explicitly for overriding behaviour for testing and is not meant to be user facing. Thereleasebuild type will use the interface defaults, effectively the same asVectorFeatures.Motivation and context
To avoid leaking debug only content within the
VectorDataStoreand provide a single point of reference for adding feature overrides for testingScreenshots / GIFs
No UI changes
Tests
Tested devices