Merge pull request #2706 from vector-im/feature/ons/fix_url_preview_on_edit

Update url preview when the event is edited.

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)
  - When receiving a new pepper from identity server, use it on the next hash lookup (#2708)
  - Crashes reported by PlayStore (new in 1.0.14) (#2707)
 

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/session/media/UrlsExtractorTest.kt

@@ -26,6 +26,8 @@ import org.matrix.android.sdk.api.session.events.model.EventType
 import org.matrix.android.sdk.api.session.events.model.toContent
 import org.matrix.android.sdk.api.session.room.model.message.MessageTextContent
 import org.matrix.android.sdk.api.session.room.model.message.MessageType
+import org.matrix.android.sdk.api.session.room.sender.SenderInfo
+import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent
 
 @RunWith(AndroidJUnit4::class)
 internal class UrlsExtractorTest : InstrumentedTest {
@@ -36,6 +38,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     fun wrongEventTypeTest() {
         createEvent(body = "https://matrix.org")
                 .copy(type = EventType.STATE_ROOM_GUEST_ACCESS)
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .size shouldBeEqualTo 0
     }
@@ -43,6 +46,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun oneUrlTest() {
         createEvent(body = "https://matrix.org")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .let { result ->
                     result.size shouldBeEqualTo 1
@@ -53,6 +57,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun withoutProtocolTest() {
         createEvent(body = "www.matrix.org")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .size shouldBeEqualTo 0
     }
@@ -60,6 +65,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun oneUrlWithParamTest() {
         createEvent(body = "https://matrix.org?foo=bar")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .let { result ->
                     result.size shouldBeEqualTo 1
@@ -70,6 +76,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun oneUrlWithParamsTest() {
         createEvent(body = "https://matrix.org?foo=bar&bar=foo")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .let { result ->
                     result.size shouldBeEqualTo 1
@@ -80,6 +87,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun oneUrlInlinedTest() {
         createEvent(body = "Hello https://matrix.org, how are you?")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .let { result ->
                     result.size shouldBeEqualTo 1
@@ -90,6 +98,7 @@ internal class UrlsExtractorTest : InstrumentedTest {
     @Test
     fun twoUrlsTest() {
         createEvent(body = "https://matrix.org https://example.org")
+                .toFakeTimelineEvent()
                 .let { urlsExtractor.extract(it) }
                 .let { result ->
                     result.size shouldBeEqualTo 2
@@ -99,10 +108,26 @@ internal class UrlsExtractorTest : InstrumentedTest {
     }
 
     private fun createEvent(body: String): Event = Event(
+            eventId = "!fake",
             type = EventType.MESSAGE,
             content = MessageTextContent(
                     msgType = MessageType.MSGTYPE_TEXT,
                     body = body
             ).toContent()
     )
+
+    private fun Event.toFakeTimelineEvent(): TimelineEvent {
+        return TimelineEvent(
+                root = this,
+                localId = 0L,
+                eventId = eventId!!,
+                displayIndex = 0,
+                senderInfo = SenderInfo(
+                        userId = "",
+                        displayName = null,
+                        isUniqueDisplayName = true,
+                        avatarUrl = null
+                )
+        )
+    }
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/media/MediaService.kt

@@ -17,15 +17,16 @@
 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.
+     * Extract URLs from a TimelineEvent.
-     * @return the list of URLs contains in the body of the Event. It does not mean that URLs in this list have UrlPreview data
+     * @param event TimelineEvent to extract the URL from.
+     * @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<String>
+    fun extractUrls(event: TimelineEvent): List<String>
 
     /**
      * Get Raw Url Preview data from the homeserver. There is no cache management for this request

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
  */

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/media/DefaultMediaService.kt

@@ -18,9 +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
@@ -34,11 +35,12 @@ internal class DefaultMediaService @Inject constructor(
     // Cache of extracted URLs
     private val extractedUrlsCache = LruCache<String, List<String>>(1_000)
 
-    override fun extractUrls(event: Event): List<String> {
+    override fun extractUrls(event: TimelineEvent): List<String> {
         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))

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<String> {
+    fun extract(event: TimelineEvent): List<String> {
-        return event.takeIf { it.getClearType() == EventType.MESSAGE }
+        return event.takeIf { it.root.getClearType() == EventType.MESSAGE }
-                ?.getClearContent()
+                ?.getLastMessageContent()
-                ?.toModel<MessageContent>()
                 ?.takeIf {
                     it.msgType == MessageType.MSGTYPE_TEXT
                             || it.msgType == MessageType.MSGTYPE_NOTICE

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/DefaultRelationService.kt

@@ -141,7 +141,7 @@ internal class DefaultRelationService @AssistedInject constructor(
     }
 
     override fun fetchEditHistory(eventId: String, callback: MatrixCallback<List<Event>>) {
-        val params = FetchEditHistoryTask.Params(roomId, cryptoSessionInfoProvider.isRoomEncrypted(roomId), eventId)
+        val params = FetchEditHistoryTask.Params(roomId, eventId)
         fetchEditHistoryTask
                 .configureWith(params) {
                     this.callback = callback

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/FetchEditHistoryTask.kt

@@ -18,6 +18,7 @@ package org.matrix.android.sdk.internal.session.room.relation
 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.RelationType
+import org.matrix.android.sdk.internal.crypto.CryptoSessionInfoProvider
 import org.matrix.android.sdk.internal.network.GlobalErrorReceiver
 import org.matrix.android.sdk.internal.network.executeRequest
 import org.matrix.android.sdk.internal.session.room.RoomAPI
@@ -25,25 +26,27 @@ import org.matrix.android.sdk.internal.task.Task
 import javax.inject.Inject
 
 internal interface FetchEditHistoryTask : Task<FetchEditHistoryTask.Params, List<Event>> {
-
     data class Params(
             val roomId: String,
-            val isRoomEncrypted: Boolean,
             val eventId: String
     )
 }
 
 internal class DefaultFetchEditHistoryTask @Inject constructor(
         private val roomAPI: RoomAPI,
-        private val globalErrorReceiver: GlobalErrorReceiver
+        private val globalErrorReceiver: GlobalErrorReceiver,
+        private val cryptoSessionInfoProvider: CryptoSessionInfoProvider
 ) : FetchEditHistoryTask {
 
     override suspend fun execute(params: FetchEditHistoryTask.Params): List<Event> {
+        val isRoomEncrypted = cryptoSessionInfoProvider.isRoomEncrypted(params.roomId)
         val response = executeRequest<RelationsResponse>(globalErrorReceiver) {
-            apiCall = roomAPI.getRelations(params.roomId,
+            apiCall = roomAPI.getRelations(
-                    params.eventId,
+                    roomId = params.roomId,
-                    RelationType.REPLACE,
+                    eventId = params.eventId,
-                    if (params.isRoomEncrypted) EventType.ENCRYPTED else EventType.MESSAGE)
+                    relationType = RelationType.REPLACE,
+                    eventType = if (isRoomEncrypted) EventType.ENCRYPTED else EventType.MESSAGE
+            )
         }
 
         val events = response.chunks.toMutableList()

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)
                         }
             }
         }

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/url/PreviewUrlRetriever.kt

@@ -16,41 +16,60 @@
 
 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.getLatestEventId
 
 class PreviewUrlRetriever(session: Session) {
     private val mediaService = session.mediaService()
 
-    private val data = mutableMapOf<String, PreviewUrlUiState>()
+    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<String, EventIdPreviewUrlUiState>()
     private val listeners = mutableMapOf<String, MutableSet<PreviewUrlRetrieverListener>>()
+    private val uiHandler = Handler(Looper.getMainLooper())
 
     // In memory list
     private val blockedUrl = mutableSetOf<String>()
 
-    fun getPreviewUrl(event: Event, coroutineScope: CoroutineScope) {
+    fun getPreviewUrl(event: TimelineEvent, coroutineScope: CoroutineScope) {
-        val eventId = event.eventId ?: return
+        val eventId = event.root.eventId ?: return
+        val latestEventId = event.getLatestEventId()
 
         synchronized(data) {
-            if (data[eventId] == null) {
+            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)
                         .firstOrNull()
                         ?.takeIf { it !in blockedUrl }
                 if (url == null) {
-                    updateState(eventId, PreviewUrlUiState.NoUrl)
+                    updateState(eventId, latestEventId, PreviewUrlUiState.NoUrl)
-                } else {
+                    null
-                    updateState(eventId, PreviewUrlUiState.Loading)
+                } 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
                     null
                 }
+            } else {
+                // Already handled
+                null
+            }
         }?.let { urlToRetrieve ->
             coroutineScope.launch {
                 runCatching {
@@ -64,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))
                             }
                         }
                 )
@@ -86,20 +105,22 @@ 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) {
+    private fun updateState(eventId: String, latestEventId: String, state: PreviewUrlUiState) {
-        data[eventId] = state
+        data[eventId] = EventIdPreviewUrlUiState(latestEventId, state)
         // Notify the listener
+        uiHandler.post {
             listeners[eventId].orEmpty().forEach {
                 it.onStateUpdated(state)
             }
         }
+    }
 
     // Called by the Epoxy item during binding
     fun addListener(key: String, listener: PreviewUrlRetrieverListener) {
@@ -107,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)
         }
     }