diff --git a/CHANGES.md b/CHANGES.md index b2483df78d..9dd6b3a894 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,7 @@ Improvements 🙌: Bugfix 🐛: - Fix clear cache issue: sometimes, after a clear cache, there is still a token, so the init sync service is not started. - Sidebar too large in horizontal orientation or tablets (#475) + - UrlPreview should be updated when the url is edited and changed (#2678) Translations 🗣: - 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 9040ec7d5c..a7a7c93073 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 @@ -17,15 +17,17 @@ package org.matrix.android.sdk.api.session.media 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.room.timeline.TimelineEvent import org.matrix.android.sdk.api.util.JsonDict interface MediaService { /** - * Extract URLs from an Event. - * @return the list of URLs contains in the body of the Event. It does not mean that URLs in this list have UrlPreview data + * 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: Event): List + fun extractUrls(event: TimelineEvent, forceExtract: Boolean = false): 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/internal/session/media/DefaultMediaService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt index 1a400ccfcf..574a97b191 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 @@ -21,6 +21,7 @@ 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.util.JsonDict import org.matrix.android.sdk.internal.util.getOrPut import javax.inject.Inject @@ -34,8 +35,9 @@ internal class DefaultMediaService @Inject constructor( // Cache of extracted URLs private val extractedUrlsCache = LruCache>(1_000) - override fun extractUrls(event: Event): List { - return extractedUrlsCache.getOrPut(event.cacheKey()) { urlsExtractor.extract(event) } + override fun extractUrls(event: TimelineEvent, forceExtract: Boolean): List { + if (forceExtract) extractedUrlsCache.remove(event.root.cacheKey()) + return extractedUrlsCache.getOrPut(event.root.cacheKey()) { urlsExtractor.extract(event) } } private fun Event.cacheKey() = "${eventId ?: ""}-${roomId ?: ""}" diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/UrlsExtractor.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/UrlsExtractor.kt index e531d6af9f..6137b4152c 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/UrlsExtractor.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/UrlsExtractor.kt @@ -17,21 +17,19 @@ package org.matrix.android.sdk.internal.session.media import android.util.Patterns -import org.matrix.android.sdk.api.session.events.model.Event import org.matrix.android.sdk.api.session.events.model.EventType -import org.matrix.android.sdk.api.session.events.model.toModel -import org.matrix.android.sdk.api.session.room.model.message.MessageContent import org.matrix.android.sdk.api.session.room.model.message.MessageType +import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent +import org.matrix.android.sdk.api.session.room.timeline.getLastMessageContent import javax.inject.Inject internal class UrlsExtractor @Inject constructor() { // Sadly Patterns.WEB_URL_WITH_PROTOCOL is not public so filter the protocol later private val urlRegex = Patterns.WEB_URL.toRegex() - fun extract(event: Event): List { - return event.takeIf { it.getClearType() == EventType.MESSAGE } - ?.getClearContent() - ?.toModel() + fun extract(event: TimelineEvent): List { + return event.takeIf { it.root.getClearType() == EventType.MESSAGE } + ?.getLastMessageContent() ?.takeIf { it.msgType == MessageType.MSGTYPE_TEXT || it.msgType == MessageType.MSGTYPE_NOTICE diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt index ecfd1f85d3..09179a9458 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt @@ -1396,7 +1396,7 @@ class RoomDetailViewModel @AssistedInject constructor( snapshot .takeIf { state.asyncRoomSummary.invoke()?.isEncrypted == false } ?.forEach { - previewUrlRetriever.getPreviewUrl(it.root, viewModelScope) + previewUrlRetriever.getPreviewUrl(it, viewModelScope) } } } 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 695661feeb..0df5dc125c 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 @@ -16,37 +16,46 @@ package im.vector.app.features.home.room.detail.timeline.url +import android.os.Handler +import android.os.Looper import im.vector.app.BuildConfig import kotlinx.coroutines.CoroutineScope 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.events.model.Event +import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent +import org.matrix.android.sdk.api.session.room.timeline.hasBeenEdited class PreviewUrlRetriever(session: Session) { private val mediaService = session.mediaService() private val data = mutableMapOf() private val listeners = mutableMapOf>() + private val uiHandler = Handler(Looper.getMainLooper()) // In memory list private val blockedUrl = mutableSetOf() - fun getPreviewUrl(event: Event, coroutineScope: CoroutineScope) { - val eventId = event.eventId ?: return + fun getPreviewUrl(event: TimelineEvent, coroutineScope: CoroutineScope) { + val eventId = event.root.eventId ?: return synchronized(data) { - if (data[eventId] == null) { + val isEditedEvent = event.hasBeenEdited() + if (data[eventId] == null || isEditedEvent) { // Keep only the first URL for the moment - val url = mediaService.extractUrls(event) + val url = mediaService.extractUrls(event, forceExtract = isEditedEvent) .firstOrNull() ?.takeIf { it !in blockedUrl } if (url == null) { updateState(eventId, PreviewUrlUiState.NoUrl) - } else { + url + } else if (url != (data[eventId] as? PreviewUrlUiState.Data)?.url) { updateState(eventId, PreviewUrlUiState.Loading) + url + } else { + // Already handled + null } - url } else { // Already handled null @@ -96,8 +105,10 @@ class PreviewUrlRetriever(session: Session) { private fun updateState(eventId: String, state: PreviewUrlUiState) { data[eventId] = state // Notify the listener - listeners[eventId].orEmpty().forEach { - it.onStateUpdated(state) + uiHandler.post { + listeners[eventId].orEmpty().forEach { + it.onStateUpdated(state) + } } }