diff --git a/CHANGES.md b/CHANGES.md index 501940dd18..90c2e05c65 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) - 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) diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/session/media/UrlsExtractorTest.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/session/media/UrlsExtractorTest.kt index 9ee84fdfc6..473b18b31b 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/session/media/UrlsExtractorTest.kt +++ b/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,16 +87,18 @@ 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 - result[0] shouldBeEqualTo "https://matrix.org" + result[0] shouldBeEqualTo "https://matrix.org" } } @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 + ) + ) + } } 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..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 @@ -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. - * @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. + * @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): 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 1a400ccfcf..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,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>(1_000) - override fun extractUrls(event: Event): List { + 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/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/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/DefaultRelationService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/DefaultRelationService.kt index b27cbbb0b2..b7caf62865 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/DefaultRelationService.kt +++ b/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>) { - val params = FetchEditHistoryTask.Params(roomId, cryptoSessionInfoProvider.isRoomEncrypted(roomId), eventId) + val params = FetchEditHistoryTask.Params(roomId, eventId) fetchEditHistoryTask .configureWith(params) { this.callback = callback diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/FetchEditHistoryTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/FetchEditHistoryTask.kt index 99d02b50da..854585ca29 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/relation/FetchEditHistoryTask.kt +++ b/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> { - 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 { + val isRoomEncrypted = cryptoSessionInfoProvider.isRoomEncrypted(params.roomId) val response = executeRequest(globalErrorReceiver) { - apiCall = roomAPI.getRelations(params.roomId, - params.eventId, - RelationType.REPLACE, - if (params.isRoomEncrypted) EventType.ENCRYPTED else EventType.MESSAGE) + apiCall = roomAPI.getRelations( + roomId = params.roomId, + eventId = params.eventId, + relationType = RelationType.REPLACE, + eventType = if (isRoomEncrypted) EventType.ENCRYPTED else EventType.MESSAGE + ) } val events = response.chunks.toMutableList() 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..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 @@ -16,37 +16,56 @@ 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() + 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()) // 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 + 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) + 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 { - updateState(eventId, PreviewUrlUiState.Loading) + // Already handled + null } - url } else { // Already handled null @@ -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,18 +105,20 @@ 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 - listeners[eventId].orEmpty().forEach { - it.onStateUpdated(state) + uiHandler.post { + listeners[eventId].orEmpty().forEach { + it.onStateUpdated(state) + } } } @@ -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) } }