From 602ea3327bcd69ba8a196e6fa24b956ace924444 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 25 Jan 2021 15:11:42 +0100 Subject: [PATCH] URL preview: improve fix regarding message edition --- .../sdk/api/session/media/MediaService.kt | 3 +- .../session/room/timeline/TimelineEvent.kt | 11 +++++ .../session/media/DefaultMediaService.kt | 10 ++--- .../timeline/url/PreviewUrlRetriever.kt | 44 ++++++++++++------- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/media/MediaService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/media/MediaService.kt index a7a7c93073..3b3ef57d73 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/media/MediaService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/media/MediaService.kt @@ -24,10 +24,9 @@ interface MediaService { /** * Extract URLs from a TimelineEvent. * @param event TimelineEvent to extract the URL from. - * @param forceExtract Should be used for edited events. If true, URL will be extracted again even it is already in the cache. * @return the list of URLs contains in the body of the TimelineEvent. It does not mean that URLs in this list have UrlPreview data */ - fun extractUrls(event: TimelineEvent, forceExtract: Boolean = false): List + fun extractUrls(event: TimelineEvent): List /** * Get Raw Url Preview data from the homeserver. There is no cache management for this request diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/TimelineEvent.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/TimelineEvent.kt index 73cb94b417..b10fb540e1 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/TimelineEvent.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/TimelineEvent.kt @@ -89,6 +89,17 @@ data class TimelineEvent( */ fun TimelineEvent.hasBeenEdited() = annotations?.editSummary != null +/** + * Get the latest known eventId for an edited event, or the eventId for an Event which has not been edited + */ +fun TimelineEvent.getLatestEventId(): String { + return annotations + ?.editSummary + ?.sourceEvents + ?.lastOrNull() + ?: eventId +} + /** * Get the relation content if any */ diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt index 574a97b191..9b807d03de 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt @@ -18,10 +18,10 @@ package org.matrix.android.sdk.internal.session.media import androidx.collection.LruCache import org.matrix.android.sdk.api.cache.CacheStrategy -import org.matrix.android.sdk.api.session.events.model.Event import org.matrix.android.sdk.api.session.media.MediaService import org.matrix.android.sdk.api.session.media.PreviewUrlData import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent +import org.matrix.android.sdk.api.session.room.timeline.getLatestEventId import org.matrix.android.sdk.api.util.JsonDict import org.matrix.android.sdk.internal.util.getOrPut import javax.inject.Inject @@ -35,12 +35,12 @@ internal class DefaultMediaService @Inject constructor( // Cache of extracted URLs private val extractedUrlsCache = LruCache>(1_000) - override fun extractUrls(event: TimelineEvent, forceExtract: Boolean): List { - if (forceExtract) extractedUrlsCache.remove(event.root.cacheKey()) - return extractedUrlsCache.getOrPut(event.root.cacheKey()) { urlsExtractor.extract(event) } + override fun extractUrls(event: TimelineEvent): List { + return extractedUrlsCache.getOrPut(event.cacheKey()) { urlsExtractor.extract(event) } } - private fun Event.cacheKey() = "${eventId ?: ""}-${roomId ?: ""}" + // Use the id of the latest Event edition + private fun TimelineEvent.cacheKey() = "${getLatestEventId()}-${root.roomId ?: ""}" override suspend fun getRawPreviewUrl(url: String, timestamp: Long?): JsonDict { return getRawPreviewUrlTask.execute(GetRawPreviewUrlTask.Params(url, timestamp)) diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/url/PreviewUrlRetriever.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/url/PreviewUrlRetriever.kt index 0df5dc125c..54d5fd9eb3 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/url/PreviewUrlRetriever.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/url/PreviewUrlRetriever.kt @@ -24,12 +24,19 @@ import kotlinx.coroutines.launch import org.matrix.android.sdk.api.cache.CacheStrategy import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent -import org.matrix.android.sdk.api.session.room.timeline.hasBeenEdited +import org.matrix.android.sdk.api.session.room.timeline.getLatestEventId class PreviewUrlRetriever(session: Session) { private val mediaService = session.mediaService() - private val data = mutableMapOf() + private data class EventIdPreviewUrlUiState( + // Id of the latest event in the case of an edited event, or the eventId for an event which has not been edited + val latestEventId: String, + val previewUrlUiState: PreviewUrlUiState + ) + + // Keys are the main eventId + private val data = mutableMapOf() private val listeners = mutableMapOf>() private val uiHandler = Handler(Looper.getMainLooper()) @@ -38,19 +45,22 @@ class PreviewUrlRetriever(session: Session) { fun getPreviewUrl(event: TimelineEvent, coroutineScope: CoroutineScope) { val eventId = event.root.eventId ?: return + val latestEventId = event.getLatestEventId() synchronized(data) { - val isEditedEvent = event.hasBeenEdited() - if (data[eventId] == null || isEditedEvent) { + val current = data[eventId] + if (current?.latestEventId != latestEventId) { + // The event is not known or it has been edited // Keep only the first URL for the moment - val url = mediaService.extractUrls(event, forceExtract = isEditedEvent) + val url = mediaService.extractUrls(event) .firstOrNull() ?.takeIf { it !in blockedUrl } if (url == null) { - updateState(eventId, PreviewUrlUiState.NoUrl) - url - } else if (url != (data[eventId] as? PreviewUrlUiState.Data)?.url) { - updateState(eventId, PreviewUrlUiState.Loading) + updateState(eventId, latestEventId, PreviewUrlUiState.NoUrl) + null + } else if (url != (current?.previewUrlUiState as? PreviewUrlUiState.Data)?.url) { + // There is a not known URL, or the Event has been edited and the URL has changed + updateState(eventId, latestEventId, PreviewUrlUiState.Loading) url } else { // Already handled @@ -73,15 +83,15 @@ class PreviewUrlRetriever(session: Session) { synchronized(data) { // Blocked after the request has been sent? if (urlToRetrieve in blockedUrl) { - updateState(eventId, PreviewUrlUiState.NoUrl) + updateState(eventId, latestEventId, PreviewUrlUiState.NoUrl) } else { - updateState(eventId, PreviewUrlUiState.Data(eventId, urlToRetrieve, it)) + updateState(eventId, latestEventId, PreviewUrlUiState.Data(eventId, urlToRetrieve, it)) } } }, { synchronized(data) { - updateState(eventId, PreviewUrlUiState.Error(it)) + updateState(eventId, latestEventId, PreviewUrlUiState.Error(it)) } } ) @@ -95,15 +105,15 @@ class PreviewUrlRetriever(session: Session) { // Notify the listener synchronized(data) { data[eventId] - ?.takeIf { it is PreviewUrlUiState.Data && it.url == url } + ?.takeIf { it.previewUrlUiState is PreviewUrlUiState.Data && it.previewUrlUiState.url == url } ?.let { - updateState(eventId, PreviewUrlUiState.NoUrl) + updateState(eventId, it.latestEventId, PreviewUrlUiState.NoUrl) } } } - private fun updateState(eventId: String, state: PreviewUrlUiState) { - data[eventId] = state + private fun updateState(eventId: String, latestEventId: String, state: PreviewUrlUiState) { + data[eventId] = EventIdPreviewUrlUiState(latestEventId, state) // Notify the listener uiHandler.post { listeners[eventId].orEmpty().forEach { @@ -118,7 +128,7 @@ class PreviewUrlRetriever(session: Session) { // Give the current state if any synchronized(data) { - listener.onStateUpdated(data[key] ?: PreviewUrlUiState.Unknown) + listener.onStateUpdated(data[key]?.previewUrlUiState ?: PreviewUrlUiState.Unknown) } }