Skip to content

Lifting debug overrides to their own abstraction #5361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 2, 2022
Merged

Lifting debug overrides to their own abstraction #5361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d600597
separating the debug overrides to their own abstraction
ouchadam Feb 24, 2022
f6735b6
adding changelog entry
ouchadam Feb 25, 2022
ce719b3
fixing import ordering
ouchadam Feb 25, 2022
7637ee4
inlining the debug flow getters and declarations
ouchadam Mar 1, 2022
350aceb
Merge branch 'feature/adm/debug-overrides-abstraction' of github.com.:…
ouchadam Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5361.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Creates dedicated VectorOverrides for forcing behaviour for local testing/development
16 changes: 16 additions & 0 deletions vector/src/debug/java/im/vector/app/features/debug/di/FeaturesModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ import dagger.Provides
import dagger.hilt.InstallIn
import dagger.hilt.components.SingletonComponent
import im.vector.app.features.DefaultVectorFeatures
import im.vector.app.features.DefaultVectorOverrides
import im.vector.app.features.VectorFeatures
import im.vector.app.features.VectorOverrides
import im.vector.app.features.debug.features.DebugVectorFeatures
import im.vector.app.features.debug.features.DebugVectorOverrides

@InstallIn(SingletonComponent::class)
@Module
Expand All @@ -33,6 +36,9 @@ interface FeaturesModule {
@Binds
fun bindFeatures(debugFeatures: DebugVectorFeatures): VectorFeatures

@Binds
fun bindOverrides(debugOverrides: DebugVectorOverrides): VectorOverrides

companion object {

@Provides
Expand All @@ -44,5 +50,15 @@ interface FeaturesModule {
fun providesDebugVectorFeatures(context: Context, defaultVectorFeatures: DefaultVectorFeatures): DebugVectorFeatures {
return DebugVectorFeatures(context, defaultVectorFeatures)
}

@Provides
fun providesDefaultVectorOverrides(): DefaultVectorOverrides {
return DefaultVectorOverrides()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

It's maybe because of @InstallIn(SingletonComponent::class) anotation of the interface FeaturesModule but I am not a big specialist of the DI.

Copy link
Contributor Author

@ouchadam ouchadam Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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): VectorFeatures

hopefully that makes sense (and wasn't me rambling 😅 )

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, this does shine some light on it, thanks!


@Provides
fun providesDebugVectorOverrides(context: Context): DebugVectorOverrides {
return DebugVectorOverrides(context)
}
}
}
54 changes: 54 additions & 0 deletions vector/src/debug/java/im/vector/app/features/debug/features/DebugVectorOverrides.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package im.vector.app.features.debug.features

import android.content.Context
import androidx.datastore.core.DataStore
import androidx.datastore.preferences.core.Preferences
import androidx.datastore.preferences.core.booleanPreferencesKey
import androidx.datastore.preferences.core.edit
import androidx.datastore.preferences.preferencesDataStore
import im.vector.app.features.VectorOverrides
import kotlinx.coroutines.flow.map
import org.matrix.android.sdk.api.extensions.orFalse

private val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "vector_overrides")
private val keyForceDialPadDisplay = booleanPreferencesKey("force_dial_pad_display")
private val keyForceLoginFallback = booleanPreferencesKey("force_login_fallback")

class DebugVectorOverrides(private val context: Context) : VectorOverrides {

override val forceDialPad = context.dataStore.data.map { preferences ->
preferences[keyForceDialPadDisplay].orFalse()
}

override val forceLoginFallback = context.dataStore.data.map { preferences ->
preferences[keyForceLoginFallback].orFalse()
}

suspend fun setForceDialPadDisplay(force: Boolean) {
context.dataStore.edit { settings ->
settings[keyForceDialPadDisplay] = force
}
}

suspend fun setForceLoginFallback(force: Boolean) {
context.dataStore.edit { settings ->
settings[keyForceLoginFallback] = force
}
}
}
17 changes: 9 additions & 8 deletions vector/src/debug/java/im/vector/app/features/debug/settings/DebugPrivateSettingsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import im.vector.app.core.di.MavericksAssistedViewModelFactory
import im.vector.app.core.di.hiltMavericksViewModelFactory
import im.vector.app.core.platform.EmptyViewEvents
import im.vector.app.core.platform.VectorViewModel
import im.vector.app.features.settings.VectorDataStore
import im.vector.app.features.debug.features.DebugVectorOverrides
import kotlinx.coroutines.launch

class DebugPrivateSettingsViewModel @AssistedInject constructor(
@Assisted initialState: DebugPrivateSettingsViewState,
private val vectorDataStore: VectorDataStore
private val debugVectorOverrides: DebugVectorOverrides
) : VectorViewModel<DebugPrivateSettingsViewState, DebugPrivateSettingsViewActions, EmptyViewEvents>(initialState) {

@AssistedFactory
Expand All @@ -44,11 +44,12 @@ class DebugPrivateSettingsViewModel @AssistedInject constructor(
}

private fun observeVectorDataStore() {
vectorDataStore.forceDialPadDisplayFlow.setOnEach {
copy(dialPadVisible = it)
debugVectorOverrides.forceDialPad.setOnEach {
copy(
dialPadVisible = it
)
Comment on lines +48 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any strong feelings for either, I chose to stick with the consistency of the file, happy to change!

}

vectorDataStore.forceLoginFallbackFlow.setOnEach {
debugVectorOverrides.forceLoginFallback.setOnEach {
copy(forceLoginFallback = it)
}
}
Expand All @@ -62,13 +63,13 @@ class DebugPrivateSettingsViewModel @AssistedInject constructor(

private fun handleSetDialPadVisibility(action: DebugPrivateSettingsViewActions.SetDialPadVisibility) {
viewModelScope.launch {
vectorDataStore.setForceDialPadDisplay(action.force)
debugVectorOverrides.setForceDialPadDisplay(action.force)
}
}

private fun handleSetForceLoginFallbackEnabled(action: DebugPrivateSettingsViewActions.SetForceLoginFallbackEnabled) {
viewModelScope.launch {
vectorDataStore.setForceLoginFallbackFlow(action.force)
debugVectorOverrides.setForceLoginFallback(action.force)
}
}
}
30 changes: 30 additions & 0 deletions vector/src/main/java/im/vector/app/features/DefaultVectorOverrides.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package im.vector.app.features

import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOf

interface VectorOverrides {
val forceDialPad: Flow<Boolean>
val forceLoginFallback: Flow<Boolean>
}

class DefaultVectorOverrides : VectorOverrides {
override val forceDialPad = flowOf(false)
override val forceLoginFallback = flowOf(false)
}
7 changes: 4 additions & 3 deletions vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import im.vector.app.core.di.MavericksAssistedViewModelFactory
import im.vector.app.core.di.hiltMavericksViewModelFactory
import im.vector.app.core.extensions.singletonEntryPoint
import im.vector.app.core.platform.VectorViewModel
import im.vector.app.features.VectorOverrides
import im.vector.app.features.call.dialpad.DialPadLookup
import im.vector.app.features.call.lookup.CallProtocolsChecker
import im.vector.app.features.call.webrtc.WebRtcCallManager
Expand Down Expand Up @@ -67,7 +68,8 @@ class HomeDetailViewModel @AssistedInject constructor(
private val callManager: WebRtcCallManager,
private val directRoomHelper: DirectRoomHelper,
private val appStateHandler: AppStateHandler,
private val autoAcceptInvites: AutoAcceptInvites
private val autoAcceptInvites: AutoAcceptInvites,
private val vectorOverrides: VectorOverrides
) : VectorViewModel<HomeDetailViewState, HomeDetailAction, HomeDetailViewEvents>(initialState),
CallProtocolsChecker.Listener {

Expand Down Expand Up @@ -106,8 +108,7 @@ class HomeDetailViewModel @AssistedInject constructor(
pushCounter = nbOfPush
)
}

vectorDataStore.forceDialPadDisplayFlow.setOnEach { force ->
vectorOverrides.forceDialPad.setOnEach { force ->
copy(
forceDialPadTab = force
)
Expand Down
4 changes: 3 additions & 1 deletion vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import im.vector.app.core.platform.VectorViewModel
import im.vector.app.core.resources.StringProvider
import im.vector.app.core.utils.ensureTrailingSlash
import im.vector.app.features.VectorFeatures
import im.vector.app.features.VectorOverrides
import im.vector.app.features.analytics.AnalyticsTracker
import im.vector.app.features.analytics.extensions.toTrackingValue
import im.vector.app.features.analytics.plan.UserProperties
Expand Down Expand Up @@ -81,6 +82,7 @@ class OnboardingViewModel @AssistedInject constructor(
private val vectorFeatures: VectorFeatures,
private val analyticsTracker: AnalyticsTracker,
private val vectorDataStore: VectorDataStore,
private val vectorOverrides: VectorOverrides
) : VectorViewModel<OnboardingViewState, OnboardingAction, OnboardingViewEvents>(initialState) {

@AssistedFactory
Expand All @@ -102,7 +104,7 @@ class OnboardingViewModel @AssistedInject constructor(
}

private fun observeDataStore() = viewModelScope.launch {
vectorDataStore.forceLoginFallbackFlow.setOnEach { isForceLoginFallbackEnabled ->
vectorOverrides.forceLoginFallback.setOnEach { isForceLoginFallbackEnabled ->
copy(isForceLoginFallbackEnabled = isForceLoginFallbackEnabled)
}
}
Expand Down
27 changes: 0 additions & 27 deletions vector/src/main/java/im/vector/app/features/settings/VectorDataStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ package im.vector.app.features.settings
import android.content.Context
import androidx.datastore.core.DataStore
import androidx.datastore.preferences.core.Preferences
import androidx.datastore.preferences.core.booleanPreferencesKey
import androidx.datastore.preferences.core.edit
import androidx.datastore.preferences.core.intPreferencesKey
import androidx.datastore.preferences.preferencesDataStore
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
import org.matrix.android.sdk.api.extensions.orFalse
import javax.inject.Inject

private val Context.dataStore: DataStore<Preferences> by preferencesDataStore(name = "vector_settings")
Expand All @@ -46,29 +44,4 @@ class VectorDataStore @Inject constructor(
settings[pushCounter] = currentCounterValue + 1
}
}

// For debug only
private val forceDialPadDisplay = booleanPreferencesKey("force_dial_pad_display")

val forceDialPadDisplayFlow: Flow<Boolean> = context.dataStore.data.map { preferences ->
preferences[forceDialPadDisplay].orFalse()
}

suspend fun setForceDialPadDisplay(force: Boolean) {
context.dataStore.edit { settings ->
settings[forceDialPadDisplay] = force
}
}

private val forceLoginFallback = booleanPreferencesKey("force_login_fallback")

val forceLoginFallbackFlow: Flow<Boolean> = context.dataStore.data.map { preferences ->
preferences[forceLoginFallback].orFalse()
}

suspend fun setForceLoginFallbackFlow(force: Boolean) {
context.dataStore.edit { settings ->
settings[forceLoginFallback] = force
}
}
}
7 changes: 7 additions & 0 deletions vector/src/release/java/im/vector/app/core/di/FeaturesModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import dagger.Provides
import dagger.hilt.InstallIn
import dagger.hilt.components.SingletonComponent
import im.vector.app.features.DefaultVectorFeatures
import im.vector.app.features.DefaultVectorOverrides
import im.vector.app.features.VectorFeatures
import im.vector.app.features.VectorOverrides

@InstallIn(SingletonComponent::class)
@Module
Expand All @@ -31,4 +33,9 @@ object FeaturesModule {
fun providesFeatures(): VectorFeatures {
return DefaultVectorFeatures()
}

@Provides
fun providesOverrides(): VectorOverrides {
return DefaultVectorOverrides()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}