From 775cb899938c8eb7fc8873315e721a91f9f94486 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jun 2024 13:58:15 +0200 Subject: [PATCH 1/5] Fix image not rendering after clearing cache. --- .../android/appnav/root/RootNavStateFlowFactory.kt | 4 ++++ .../libraries/matrix/ui/media/ImageLoaderHolder.kt | 9 ++++++++- .../push/test/notifications/FakeImageLoaderHolder.kt | 5 +++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt index 13c0204f79..b801415f88 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt @@ -22,6 +22,7 @@ import io.element.android.appnav.di.MatrixClientsHolder import io.element.android.features.login.api.LoginUserStory import io.element.android.features.preferences.api.CacheService import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService +import io.element.android.libraries.matrix.ui.media.ImageLoaderHolder import io.element.android.libraries.sessionstorage.api.LoggedInState import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.combine @@ -39,6 +40,7 @@ class RootNavStateFlowFactory @Inject constructor( private val authenticationService: MatrixAuthenticationService, private val cacheService: CacheService, private val matrixClientsHolder: MatrixClientsHolder, + private val imageLoaderHolder: ImageLoaderHolder, private val loginUserStory: LoginUserStory, ) { private var currentCacheIndex = 0 @@ -69,6 +71,8 @@ class RootNavStateFlowFactory @Inject constructor( return cacheService.clearedCacheEventFlow .onEach { sessionId -> matrixClientsHolder.remove(sessionId) + // Ensure image loader will be recreated with the new MatrixClient + imageLoaderHolder.remove(sessionId) } .toIndexFlow(initialCacheIndex) .onEach { cacheIndex -> diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt index fc027d0e5c..ae60a44a2e 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt @@ -32,6 +32,7 @@ import javax.inject.Provider interface ImageLoaderHolder { fun get(client: MatrixClient): ImageLoader + fun remove(sessionId: SessionId) } @ContributesBinding(AppScope::class) @@ -52,7 +53,7 @@ class DefaultImageLoaderHolder @Inject constructor( override suspend fun onSessionCreated(userId: String) = Unit override suspend fun onSessionDeleted(userId: String) { - map.remove(SessionId(userId)) + remove(SessionId(userId)) } }) } @@ -68,4 +69,10 @@ class DefaultImageLoaderHolder @Inject constructor( } } } + + override fun remove(sessionId: SessionId) { + synchronized(map) { + map.remove(sessionId) + } + } } diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/notifications/FakeImageLoaderHolder.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/notifications/FakeImageLoaderHolder.kt index 57b71007e4..8fc14b2143 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/notifications/FakeImageLoaderHolder.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/notifications/FakeImageLoaderHolder.kt @@ -18,6 +18,7 @@ package io.element.android.libraries.push.test.notifications import coil.ImageLoader import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.ui.media.ImageLoaderHolder class FakeImageLoaderHolder : ImageLoaderHolder { @@ -25,4 +26,8 @@ class FakeImageLoaderHolder : ImageLoaderHolder { override fun get(client: MatrixClient): ImageLoader { return fakeImageLoader.getImageLoader() } + + override fun remove(sessionId: SessionId) { + // No-op + } } From 4faa563e810fe34b7035cf52ad8847de34b1026c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jun 2024 14:04:14 +0200 Subject: [PATCH 2/5] Move test classes to the correct package. --- .../{matrixui => matrix/ui}/messages/ToHtmlDocumentTest.kt | 3 +-- .../{matrixui => matrix/ui}/messages/ToPlainTextTest.kt | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) rename libraries/matrixui/src/test/kotlin/io/element/android/libraries/{matrixui => matrix/ui}/messages/ToHtmlDocumentTest.kt (97%) rename libraries/matrixui/src/test/kotlin/io/element/android/libraries/{matrixui => matrix/ui}/messages/ToPlainTextTest.kt (97%) diff --git a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToHtmlDocumentTest.kt b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToHtmlDocumentTest.kt similarity index 97% rename from libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToHtmlDocumentTest.kt rename to libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToHtmlDocumentTest.kt index 9dee206339..dc41b03239 100644 --- a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToHtmlDocumentTest.kt +++ b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToHtmlDocumentTest.kt @@ -14,7 +14,7 @@ * limitations under the License. */ -package io.element.android.libraries.matrixui.messages +package io.element.android.libraries.matrix.ui.messages import android.net.Uri import com.google.common.truth.Truth.assertThat @@ -24,7 +24,6 @@ import io.element.android.libraries.matrix.api.permalink.PermalinkParser import io.element.android.libraries.matrix.api.timeline.item.event.FormattedBody import io.element.android.libraries.matrix.api.timeline.item.event.MessageFormat import io.element.android.libraries.matrix.test.permalink.FakePermalinkParser -import io.element.android.libraries.matrix.ui.messages.toHtmlDocument import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner diff --git a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToPlainTextTest.kt b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToPlainTextTest.kt similarity index 97% rename from libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToPlainTextTest.kt rename to libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToPlainTextTest.kt index 74e43a9e39..2146f951c0 100644 --- a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrixui/messages/ToPlainTextTest.kt +++ b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/messages/ToPlainTextTest.kt @@ -14,14 +14,13 @@ * limitations under the License. */ -package io.element.android.libraries.matrixui.messages +package io.element.android.libraries.matrix.ui.messages import com.google.common.truth.Truth.assertThat import io.element.android.libraries.matrix.api.timeline.item.event.FormattedBody import io.element.android.libraries.matrix.api.timeline.item.event.MessageFormat import io.element.android.libraries.matrix.api.timeline.item.event.TextMessageType import io.element.android.libraries.matrix.test.permalink.FakePermalinkParser -import io.element.android.libraries.matrix.ui.messages.toPlainText import org.jsoup.Jsoup import org.junit.Test import org.junit.runner.RunWith From 8b1ca62f78694e0b095d67f623b29b6fc4a80f9c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jun 2024 14:22:46 +0200 Subject: [PATCH 3/5] Convert LoggedInImageLoaderFactory to an interface and add DefaultLoggedInImageLoaderFactory --- .../matrix/ui/media/ImageLoaderFactories.kt | 16 +++++++++++----- .../matrix/ui/media/ImageLoaderHolder.kt | 14 +++----------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt index 4c5309e2bd..95b23f54a1 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderFactories.kt @@ -22,18 +22,24 @@ import coil.ImageLoader import coil.ImageLoaderFactory import coil.decode.GifDecoder import coil.decode.ImageDecoderDecoder +import com.squareup.anvil.annotations.ContributesBinding +import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.matrix.api.MatrixClient import okhttp3.OkHttpClient import javax.inject.Inject import javax.inject.Provider -class LoggedInImageLoaderFactory( - private val context: Context, - private val matrixClient: MatrixClient, +interface LoggedInImageLoaderFactory { + fun newImageLoader(matrixClient: MatrixClient): ImageLoader +} + +@ContributesBinding(AppScope::class) +class DefaultLoggedInImageLoaderFactory @Inject constructor( + @ApplicationContext private val context: Context, private val okHttpClient: Provider, -) : ImageLoaderFactory { - override fun newImageLoader(): ImageLoader { +) : LoggedInImageLoaderFactory { + override fun newImageLoader(matrixClient: MatrixClient): ImageLoader { return ImageLoader .Builder(context) .okHttpClient { okHttpClient.get() } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt index ae60a44a2e..10f81f254d 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/ImageLoaderHolder.kt @@ -16,19 +16,15 @@ package io.element.android.libraries.matrix.ui.media -import android.content.Context import coil.ImageLoader import com.squareup.anvil.annotations.ContributesBinding import io.element.android.libraries.di.AppScope -import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.di.SingleIn import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.sessionstorage.api.observer.SessionListener import io.element.android.libraries.sessionstorage.api.observer.SessionObserver -import okhttp3.OkHttpClient import javax.inject.Inject -import javax.inject.Provider interface ImageLoaderHolder { fun get(client: MatrixClient): ImageLoader @@ -38,8 +34,7 @@ interface ImageLoaderHolder { @ContributesBinding(AppScope::class) @SingleIn(AppScope::class) class DefaultImageLoaderHolder @Inject constructor( - @ApplicationContext private val context: Context, - private val okHttpClient: Provider, + private val loggedInImageLoaderFactory: LoggedInImageLoaderFactory, private val sessionObserver: SessionObserver, ) : ImageLoaderHolder { private val map = mutableMapOf() @@ -61,11 +56,8 @@ class DefaultImageLoaderHolder @Inject constructor( override fun get(client: MatrixClient): ImageLoader { return synchronized(map) { map.getOrPut(client.sessionId) { - LoggedInImageLoaderFactory( - context = context, - matrixClient = client, - okHttpClient = okHttpClient, - ).newImageLoader() + loggedInImageLoaderFactory + .newImageLoader(client) } } } From e31bbc2b7ea2d45448f9687b4e9624ad9f6db054 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jun 2024 14:42:04 +0200 Subject: [PATCH 4/5] Add unit test on DefaultImageLoaderHolder --- libraries/matrixui/build.gradle.kts | 5 +- .../ui/media/DefaultImageLoaderHolderTest.kt | 86 +++++++++++++++++++ .../media/FakeLoggedInImageLoaderFactory.kt | 28 ++++++ .../test/observer/FakeSessionObserver.kt | 43 ++++++++++ 4 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/DefaultImageLoaderHolderTest.kt create mode 100644 libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/FakeLoggedInImageLoaderFactory.kt create mode 100644 libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/FakeSessionObserver.kt diff --git a/libraries/matrixui/build.gradle.kts b/libraries/matrixui/build.gradle.kts index ed7f3d068d..0a676e3bca 100644 --- a/libraries/matrixui/build.gradle.kts +++ b/libraries/matrixui/build.gradle.kts @@ -46,8 +46,11 @@ dependencies { ksp(libs.showkase.processor) + testImplementation(libs.coroutines.test) testImplementation(libs.test.junit) - testImplementation(libs.test.truth) testImplementation(libs.test.robolectric) + testImplementation(libs.test.truth) testImplementation(projects.libraries.matrix.test) + testImplementation(projects.libraries.sessionStorage.test) + testImplementation(projects.tests.testutils) } diff --git a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/DefaultImageLoaderHolderTest.kt b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/DefaultImageLoaderHolderTest.kt new file mode 100644 index 0000000000..46a29840b3 --- /dev/null +++ b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/DefaultImageLoaderHolderTest.kt @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2024 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 io.element.android.libraries.matrix.ui.media + +import androidx.test.platform.app.InstrumentationRegistry +import coil.ImageLoader +import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.test.A_SESSION_ID +import io.element.android.libraries.matrix.test.FakeMatrixClient +import io.element.android.libraries.sessionstorage.test.observer.FakeSessionObserver +import io.element.android.libraries.sessionstorage.test.observer.NoOpSessionObserver +import io.element.android.tests.testutils.lambda.lambdaRecorder +import io.element.android.tests.testutils.lambda.value +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class DefaultImageLoaderHolderTest { + @Test + fun `get - returns the same ImageLoader for the same client`() { + val context = InstrumentationRegistry.getInstrumentation().context + val lambda = lambdaRecorder { ImageLoader.Builder(context).build() } + + val holder = DefaultImageLoaderHolder( + loggedInImageLoaderFactory = FakeLoggedInImageLoaderFactory(lambda), + sessionObserver = NoOpSessionObserver() + ) + val client = FakeMatrixClient() + val imageLoader1 = holder.get(client) + val imageLoader2 = holder.get(client) + assert(imageLoader1 === imageLoader2) + lambda.assertions() + .isCalledOnce() + .with(value(client)) + } + + @Test + fun `when session is deleted, the image loader is deleted`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val lambda = + lambdaRecorder { ImageLoader.Builder(context).build() } + val sessionObserver = FakeSessionObserver() + val holder = DefaultImageLoaderHolder( + loggedInImageLoaderFactory = FakeLoggedInImageLoaderFactory(lambda), + sessionObserver = sessionObserver + ) + assertThat(sessionObserver.listeners.size).isEqualTo(1) + val client = FakeMatrixClient() + holder.get(client) + sessionObserver.onSessionDeleted(client.sessionId.value) + holder.get(client) + lambda.assertions() + .isCalledExactly(2) + } + + @Test + fun `when session is created, nothing happen`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val lambda = + lambdaRecorder { ImageLoader.Builder(context).build() } + val sessionObserver = FakeSessionObserver() + DefaultImageLoaderHolder( + loggedInImageLoaderFactory = FakeLoggedInImageLoaderFactory(lambda), + sessionObserver = sessionObserver + ) + assertThat(sessionObserver.listeners.size).isEqualTo(1) + sessionObserver.onSessionCreated(A_SESSION_ID.value) + } +} diff --git a/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/FakeLoggedInImageLoaderFactory.kt b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/FakeLoggedInImageLoaderFactory.kt new file mode 100644 index 0000000000..76ba75c5b9 --- /dev/null +++ b/libraries/matrixui/src/test/kotlin/io/element/android/libraries/matrix/ui/media/FakeLoggedInImageLoaderFactory.kt @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2024 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 io.element.android.libraries.matrix.ui.media + +import coil.ImageLoader +import io.element.android.libraries.matrix.api.MatrixClient + +class FakeLoggedInImageLoaderFactory( + private val newImageLoaderLambda: (MatrixClient) -> ImageLoader +) : LoggedInImageLoaderFactory { + override fun newImageLoader(matrixClient: MatrixClient): ImageLoader { + return newImageLoaderLambda(matrixClient) + } +} diff --git a/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/FakeSessionObserver.kt b/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/FakeSessionObserver.kt new file mode 100644 index 0000000000..71a5b85494 --- /dev/null +++ b/libraries/session-storage/test/src/main/kotlin/io/element/android/libraries/sessionstorage/test/observer/FakeSessionObserver.kt @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2024 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 io.element.android.libraries.sessionstorage.test.observer + +import io.element.android.libraries.sessionstorage.api.observer.SessionListener +import io.element.android.libraries.sessionstorage.api.observer.SessionObserver + +class FakeSessionObserver : SessionObserver { + private val _listeners = mutableListOf() + + val listeners: List + get() = _listeners + + override fun addListener(listener: SessionListener) { + _listeners.add(listener) + } + + override fun removeListener(listener: SessionListener) { + _listeners.remove(listener) + } + + suspend fun onSessionCreated(userId: String) { + listeners.forEach { it.onSessionCreated(userId) } + } + + suspend fun onSessionDeleted(userId: String) { + listeners.forEach { it.onSessionDeleted(userId) } + } +} From 8dc0a10a6c4eed1b2497939c92506c251b3474dc Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 24 Jun 2024 14:46:51 +0200 Subject: [PATCH 5/5] Changelog --- changelog.d/3082.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3082.bugfix diff --git a/changelog.d/3082.bugfix b/changelog.d/3082.bugfix new file mode 100644 index 0000000000..248b35ad7f --- /dev/null +++ b/changelog.d/3082.bugfix @@ -0,0 +1 @@ +Fix image rendering after clear cache