Skip to content

Decoupling Matrix creation from Application #5185

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
bmarty merged 15 commits into from
Feb 17, 2022
Merged

Decoupling Matrix creation from Application #5185

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2d80c6b
avoiding using the matrix getInstance internally within the sdk to al…
ouchadam Feb 8, 2022
c160f5d
always providing matrix via the singleton module -
ouchadam Feb 8, 2022
a63e79b
initialising a new matrix instance for android tests, ideally this wo…
ouchadam Feb 8, 2022
8dc8c20
using injected matrix instance for the bug reporting to avoid directl…
ouchadam Feb 8, 2022
0390994
removing unused import
ouchadam Feb 8, 2022
5ff420f
marking the matrix provision as a singleton to avoid reinitialising
ouchadam Feb 8, 2022
c62b994
adding changelog entry
ouchadam Feb 8, 2022
674aea9
injecting the room name provider as we're within hilts scope
ouchadam Feb 14, 2022
fd2d928
creating separate creator for the matrix instance to avoid ambiguous …
ouchadam Feb 14, 2022
2f7f86b
Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Ma…
ouchadam Feb 14, 2022
2eb417a
Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Ma…
ouchadam Feb 14, 2022
d05af1c
providing more alternatives when Matrix.getInstance fails
ouchadam Feb 14, 2022
95df3e7
deprecating the Matrix.initialize and Matrix.getInstance entry points…
ouchadam Feb 14, 2022
8e0f580
Merge branch 'feature/adm/decouple-matrix-creation' of github.com:vec…
ouchadam Feb 14, 2022
5b851f1
suppressing deprecated warning on an unused method
ouchadam Feb 14, 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/5185.sdk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Avoids using Matrix.getInstance() within the SyncService and instead relies on the SDK client to provide the matrix instance
4 changes: 3 additions & 1 deletion ...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/job/SyncService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,14 @@ abstract class SyncService : Service() {
}
}

abstract fun provideMatrix(): Matrix

private fun initialize(intent: Intent?): Boolean {
if (intent == null) {
Timber.d("## Sync: initialize intent is null")
return false
}
val matrix = Matrix.getInstance(applicationContext)
val matrix = provideMatrix()
val safeSessionId = intent.getStringExtra(EXTRA_SESSION_ID) ?: return false
syncTimeoutSeconds = intent.getIntExtra(EXTRA_TIMEOUT_SECONDS, getDefaultSyncTimeoutSeconds())
syncDelaySeconds = intent.getIntExtra(EXTRA_DELAY_SECONDS, getDefaultSyncDelaySeconds())
Expand Down
6 changes: 2 additions & 4 deletions vector/src/androidTest/java/im/vector/app/SecurityBootstrapTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import androidx.test.espresso.matcher.ViewMatchers.withText
import androidx.test.ext.junit.rules.ActivityScenarioRule
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.LargeTest
import androidx.test.platform.app.InstrumentationRegistry
import im.vector.app.core.utils.getMatrixInstance
import im.vector.app.features.MainActivity
import im.vector.app.features.crypto.recover.SetupMode
import im.vector.app.features.home.HomeActivity
Expand All @@ -47,7 +47,6 @@ import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.session.Session

@RunWith(AndroidJUnit4::class)
Expand All @@ -61,8 +60,7 @@ class SecurityBootstrapTest : VerificationTestBase() {

@Before
fun createSessionWithCrossSigning() {
val context = InstrumentationRegistry.getInstrumentation().targetContext
val matrix = Matrix.getInstance(context)
val matrix = getMatrixInstance()
val userName = "foobar_${System.currentTimeMillis()}"
existingSession = createAccountAndSync(matrix, userName, password, true)
stubAllExternalIntents()
Expand Down
6 changes: 2 additions & 4 deletions vector/src/androidTest/java/im/vector/app/VerifySessionInteractiveTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@ import androidx.test.espresso.matcher.ViewMatchers.withText
import androidx.test.ext.junit.rules.ActivityScenarioRule
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.LargeTest
import androidx.test.platform.app.InstrumentationRegistry
import im.vector.app.core.utils.getMatrixInstance
import im.vector.app.features.MainActivity
import im.vector.app.features.home.HomeActivity
import org.hamcrest.CoreMatchers.not
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.auth.UIABaseAuth
import org.matrix.android.sdk.api.auth.UserInteractiveAuthInterceptor
import org.matrix.android.sdk.api.auth.UserPasswordAuth
Expand All @@ -66,8 +65,7 @@ class VerifySessionInteractiveTest : VerificationTestBase() {

@Before
fun createSessionWithCrossSigning() {
val context = InstrumentationRegistry.getInstrumentation().targetContext
val matrix = Matrix.getInstance(context)
val matrix = getMatrixInstance()
val userName = "foobar_${System.currentTimeMillis()}"
existingSession = createAccountAndSync(matrix, userName, password, true)
doSync<Unit> {
Expand Down
6 changes: 3 additions & 3 deletions vector/src/androidTest/java/im/vector/app/VerifySessionPassphraseTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.LargeTest
import androidx.test.platform.app.InstrumentationRegistry
import im.vector.app.core.resources.StringProvider
import im.vector.app.core.utils.getMatrixInstance
import im.vector.app.features.MainActivity
import im.vector.app.features.crypto.quads.SharedSecureStorageActivity
import im.vector.app.features.crypto.recover.BootstrapCrossSigningTask
Expand All @@ -45,7 +46,6 @@ import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.auth.UIABaseAuth
import org.matrix.android.sdk.api.auth.UserInteractiveAuthInterceptor
import org.matrix.android.sdk.api.auth.UserPasswordAuth
Expand All @@ -67,7 +67,7 @@ class VerifySessionPassphraseTest : VerificationTestBase() {
@Before
fun createSessionWithCrossSigningAnd4S() {
val context = InstrumentationRegistry.getInstrumentation().targetContext
val matrix = Matrix.getInstance(context)
val matrix = getMatrixInstance()
val userName = "foobar_${System.currentTimeMillis()}"
existingSession = createAccountAndSync(matrix, userName, password, true)
doSync<Unit> {
Expand All @@ -90,7 +90,7 @@ class VerifySessionPassphraseTest : VerificationTestBase() {

runBlocking {
task.execute(Params(
userInteractiveAuthInterceptor = object : UserInteractiveAuthInterceptor {
userInteractiveAuthInterceptor = object : UserInteractiveAuthInterceptor {
override fun performStage(flowResponse: RegistrationFlowResponse, errCode: String?, promise: Continuation<UIABaseAuth>) {
promise.resume(
UserPasswordAuth(
Expand Down
31 changes: 31 additions & 0 deletions vector/src/androidTest/java/im/vector/app/core/utils/TestMatrixHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* 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.core.utils

import androidx.test.platform.app.InstrumentationRegistry
import im.vector.app.features.room.VectorRoomDisplayNameFallbackProvider
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.MatrixConfiguration

fun getMatrixInstance(): Matrix {
val context = InstrumentationRegistry.getInstrumentation().targetContext
val configuration = MatrixConfiguration(
roomDisplayNameFallbackProvider = VectorRoomDisplayNameFallbackProvider(context)
)
Matrix.initialize(context, configuration)
return Matrix.getInstance(context)
}
13 changes: 2 additions & 11 deletions vector/src/main/java/im/vector/app/VectorApplication.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,13 @@ import im.vector.app.features.pin.PinLocker
import im.vector.app.features.popup.PopupAlertManager
import im.vector.app.features.rageshake.VectorFileLogger
import im.vector.app.features.rageshake.VectorUncaughtExceptionHandler
import im.vector.app.features.room.VectorRoomDisplayNameFallbackProvider
import im.vector.app.features.settings.VectorLocale
import im.vector.app.features.settings.VectorPreferences
import im.vector.app.features.themes.ThemeUtils
import im.vector.app.features.version.VersionProvider
import im.vector.app.push.fcm.FcmHelper
import org.jitsi.meet.sdk.log.JitsiMeetDefaultLogHandler
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.auth.AuthenticationService
import org.matrix.android.sdk.api.legacy.LegacySessionImporter
import timber.log.Timber
Expand All @@ -77,7 +75,6 @@ import androidx.work.Configuration as WorkConfiguration
@HiltAndroidApp
class VectorApplication :
Application(),
MatrixConfiguration.Provider,

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe delete this interface? Or can it still be used by other application?

@ouchadam ouchadam Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it could be used by other clients of the SDK, if we agree that the Matrix singleton API should avoid querying the application then we could mark the interface as deprecated and schedule removing it? potentially with some migration steps~

what do you think?

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I defer to @ganfra :)

@ganfra ganfra Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes I think we should remove this and the inner singleton to avoid mistakes!
Regarding the process, we could indeed mark them as deprecated in a first place.

@ouchadam ouchadam Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added deprecations 95df3e7

WorkConfiguration.Provider {

lateinit var appContext: Context
Expand All @@ -100,6 +97,7 @@ class VectorApplication :
@Inject lateinit var autoRageShaker: AutoRageShaker
@Inject lateinit var vectorFileLogger: VectorFileLogger
@Inject lateinit var vectorAnalytics: VectorAnalytics
@Inject lateinit var matrix: Matrix

// font thread handler
private var fontThreadHandler: Handler? = null
Expand Down Expand Up @@ -220,16 +218,9 @@ class VectorApplication :
}
}

override fun providesMatrixConfiguration(): MatrixConfiguration {

@ouchadam ouchadam Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the main downside to using MatrixConfiguration.Provider is that it occurs during super.onCreate which means the dependencies declared in the application haven't been injected yet, making the object creation lifecycle a bit misleading

return MatrixConfiguration(
applicationFlavor = BuildConfig.FLAVOR_DESCRIPTION,
roomDisplayNameFallbackProvider = VectorRoomDisplayNameFallbackProvider(this)
)
}

override fun getWorkManagerConfiguration(): WorkConfiguration {
return WorkConfiguration.Builder()
.setWorkerFactory(Matrix.getInstance(this.appContext).workerFactory())
.setWorkerFactory(matrix.workerFactory())
.setExecutor(Executors.newCachedThreadPool())
.build()
}
Expand Down
15 changes: 14 additions & 1 deletion vector/src/main/java/im/vector/app/core/di/SingletonModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import dagger.Module
import dagger.Provides
import dagger.hilt.InstallIn
import dagger.hilt.components.SingletonComponent
import im.vector.app.BuildConfig
import im.vector.app.EmojiCompatWrapper
import im.vector.app.EmojiSpanify
import im.vector.app.core.dispatchers.CoroutineDispatchers
Expand All @@ -42,12 +43,14 @@ import im.vector.app.features.navigation.DefaultNavigator
import im.vector.app.features.navigation.Navigator
import im.vector.app.features.pin.PinCodeStore
import im.vector.app.features.pin.SharedPrefPinCodeStore
import im.vector.app.features.room.VectorRoomDisplayNameFallbackProvider
import im.vector.app.features.ui.SharedPreferencesUiStateRepository
import im.vector.app.features.ui.UiStateRepository
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.api.MatrixConfiguration
import org.matrix.android.sdk.api.auth.AuthenticationService
import org.matrix.android.sdk.api.auth.HomeServerHistoryService
import org.matrix.android.sdk.api.legacy.LegacySessionImporter
Expand Down Expand Up @@ -107,7 +110,17 @@ object VectorStaticModule {
}

@Provides
fun providesMatrix(context: Context): Matrix {
fun providesMatrixConfiguration(context: Context): MatrixConfiguration {

@ouchadam ouchadam Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

extra dependencies can be added here, such as the VectorFeatures!

return MatrixConfiguration(
applicationFlavor = BuildConfig.FLAVOR_DESCRIPTION,
roomDisplayNameFallbackProvider = VectorRoomDisplayNameFallbackProvider(context)

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could probably also provide RoomDisplayNameFallbackProvider as a param.

@ouchadam ouchadam Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will do! 👍

@ouchadam ouchadam Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

674aea9

)
}

@Provides
@Singleton
fun providesMatrix(context: Context, configuration: MatrixConfiguration): Matrix {
Matrix.initialize(context, configuration)
return Matrix.getInstance(context)

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this work since the context is not implementing MatrixConfiguration.Provider anymore?

https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Matrix.kt#L108

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, just read again the description, sorry

@ouchadam ouchadam Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no worries! yep it'll work but... it's only because of the exact order of these calls

initialise() // sets the private singleton instance
getInstance() // the instance is not null so we avoid attempting to read from (application as Provider). providesMatrixConfiguration()

I could add another documented function to the companion object to only create an instance without making use of the internal singleton (in our case dagger/hilt will handle the singleton instance for us)

/**
* Creates an instance of Matrix, it's recommend to manage this instance as a singleton.
* To make use of the built in singleton use Matrix.initialise() or Matrix.getInstance(context) instead   
**/
fun createInstance(context: Context, matrixConfiguration: MatrixConfiguration) : Matrix {
  return Matrix(context.applicationContext, matrixConfiguration)
}

@ouchadam ouchadam Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did it 😄 having the separate method really helps clarify the ambiguity fd2d928

Matrix.initialize and Matrix.getInstance are now unused in Element

}

Expand Down
4 changes: 4 additions & 0 deletions vector/src/main/java/im/vector/app/core/services/VectorSyncService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import im.vector.app.R
import im.vector.app.core.platform.PendingIntentCompat
import im.vector.app.features.notifications.NotificationUtils
import im.vector.app.features.settings.BackgroundSyncMode
import org.matrix.android.sdk.api.Matrix
import org.matrix.android.sdk.internal.session.sync.job.SyncService
import timber.log.Timber
import javax.inject.Inject
Expand Down Expand Up @@ -75,6 +76,9 @@ class VectorSyncService : SyncService() {
}

@Inject lateinit var notificationUtils: NotificationUtils
@Inject lateinit var matrix: Matrix

override fun provideMatrix() = matrix

override fun getDefaultSyncDelaySeconds() = BackgroundSyncMode.DEFAULT_SYNC_DELAY_SECONDS

Expand Down
5 changes: 3 additions & 2 deletions vector/src/main/java/im/vector/app/features/rageshake/BugReporter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class BugReporter @Inject constructor(
private val versionProvider: VersionProvider,
private val vectorPreferences: VectorPreferences,
private val vectorFileLogger: VectorFileLogger,
private val systemLocaleProvider: SystemLocaleProvider
private val systemLocaleProvider: SystemLocaleProvider,
private val matrix: Matrix

@bmarty bmarty Feb 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like that!

) {
var inMultiWindowMode = false

Expand Down Expand Up @@ -265,7 +266,7 @@ class BugReporter @Inject constructor(
val builder = BugReporterMultipartBody.Builder()
.addFormDataPart("text", text)
.addFormDataPart("app", rageShakeAppNameForReport(reportType))
.addFormDataPart("user_agent", Matrix.getInstance(context).getUserAgent())
.addFormDataPart("user_agent", matrix.getUserAgent())
.addFormDataPart("user_id", userId)
.addFormDataPart("can_contact", canContact.toString())
.addFormDataPart("device_id", deviceId)
Expand Down