Skip to content

Edit : fallback to room.edit when timeline item is not found. #3239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import io.element.android.libraries.matrix.api.room.Mention
import io.element.android.libraries.matrix.api.room.draft.ComposerDraft
import io.element.android.libraries.matrix.api.room.draft.ComposerDraftType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.api.timeline.TimelineException
import io.element.android.libraries.matrix.ui.messages.RoomMemberProfilesCache
import io.element.android.libraries.matrix.ui.messages.reply.InReplyToDetails
import io.element.android.libraries.matrix.ui.messages.reply.map
Expand Down Expand Up @@ -436,7 +437,14 @@ class MessageComposerPresenter @Inject constructor(
val eventId = capturedMode.eventId
val transactionId = capturedMode.transactionId
timelineController.invokeOnCurrentTimeline {
// First try to edit the message in the current timeline
editMessage(eventId, transactionId, message.markdown, message.html, message.mentions)
.onFailure { cause ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be mapFailure instead, so the result of room.editMessage is returned instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

U're right, good catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we are not doing anything with the result, so I guess it doesn't really matter

if (cause is TimelineException.EventNotFound && eventId != null) {
// if the event is not found in the timeline, try to edit the message directly
room.editMessage(eventId, message.markdown, message.html, message.mentions)
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import io.element.android.libraries.matrix.api.room.Mention
import io.element.android.libraries.matrix.api.room.RoomMembershipState
import io.element.android.libraries.matrix.api.room.draft.ComposerDraft
import io.element.android.libraries.matrix.api.room.draft.ComposerDraftType
import io.element.android.libraries.matrix.api.timeline.TimelineException
import io.element.android.libraries.matrix.api.timeline.item.event.InReplyTo
import io.element.android.libraries.matrix.test.ANOTHER_MESSAGE
import io.element.android.libraries.matrix.test.AN_EVENT_ID
Expand Down Expand Up @@ -417,6 +418,67 @@ class MessageComposerPresenterTest {
}
}

@Test
fun `present - edit sent message event not found`() = runTest {
val timelineEditMessageLambda = lambdaRecorder { _: EventId?, _: TransactionId?, _: String, _: String?, _: List<Mention> ->
Result.failure<Unit>(TimelineException.EventNotFound)
}
val timeline = FakeTimeline().apply {
this.editMessageLambda = timelineEditMessageLambda
}
val roomEditMessageLambda = lambdaRecorder { _: EventId?, _: String, _: String?, _: List<Mention> ->
Result.success(Unit)
}
val fakeMatrixRoom = FakeMatrixRoom(
liveTimeline = timeline,
typingNoticeResult = { Result.success(Unit) }
).apply {
this.editMessageLambda = roomEditMessageLambda
}
val presenter = createPresenter(
this,
fakeMatrixRoom,
)
moleculeFlow(RecompositionMode.Immediate) {
val state = presenter.present()
remember(state, state.textEditorState.messageHtml()) { state }
}.test {
val initialState = awaitFirstItem()
assertThat(initialState.textEditorState.messageHtml()).isEqualTo("")
val mode = anEditMode()
initialState.eventSink.invoke(MessageComposerEvents.SetMode(mode))
val withMessageState = awaitItem()
assertThat(withMessageState.mode).isEqualTo(mode)
assertThat(withMessageState.textEditorState.messageHtml()).isEqualTo(A_MESSAGE)
withMessageState.textEditorState.setHtml(ANOTHER_MESSAGE)
val withEditedMessageState = awaitItem()
assertThat(withEditedMessageState.textEditorState.messageHtml()).isEqualTo(ANOTHER_MESSAGE)
withEditedMessageState.eventSink.invoke(MessageComposerEvents.SendMessage)
skipItems(1)
val messageSentState = awaitItem()
assertThat(messageSentState.textEditorState.messageHtml()).isEqualTo("")

advanceUntilIdle()

assert(timelineEditMessageLambda)
.isCalledOnce()
.with(value(AN_EVENT_ID), value(null), value(ANOTHER_MESSAGE), value(ANOTHER_MESSAGE), any())

assert(roomEditMessageLambda)
.isCalledOnce()
.with(value(AN_EVENT_ID), value(ANOTHER_MESSAGE), value(ANOTHER_MESSAGE), any())

assertThat(analyticsService.capturedEvents).containsExactly(
Composer(
inThread = false,
isEditing = true,
isReply = false,
messageType = Composer.MessageType.Text,
)
)
}
}

@Test
fun `present - edit not sent message`() = runTest {
val editMessageLambda = lambdaRecorder { _: EventId?, _: TransactionId?, _: String, _: String?, _: List<Mention> ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ interface MatrixRoom : Closeable {

suspend fun sendMessage(body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit>

suspend fun editMessage(eventId: EventId, body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit>

suspend fun sendImage(
file: File,
thumbnailFile: File?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ package io.element.android.libraries.matrix.api.timeline

sealed class TimelineException : Exception() {
data object CannotPaginate : TimelineException()
data object EventNotFound : TimelineException()
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import io.element.android.libraries.matrix.impl.room.member.RoomMemberMapper
import io.element.android.libraries.matrix.impl.room.powerlevels.RoomPowerLevelsMapper
import io.element.android.libraries.matrix.impl.timeline.RustTimeline
import io.element.android.libraries.matrix.impl.timeline.toRustReceiptType
import io.element.android.libraries.matrix.impl.util.MessageEventContent
import io.element.android.libraries.matrix.impl.util.mxCallbackFlow
import io.element.android.libraries.matrix.impl.widget.RustWidgetDriver
import io.element.android.libraries.matrix.impl.widget.generateWidgetWebViewUrl
Expand Down Expand Up @@ -324,6 +325,14 @@ class RustMatrixRoom(
}
}

override suspend fun editMessage(eventId: EventId, body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit> = withContext(roomDispatcher) {
runCatching {
MessageEventContent.from(body, htmlBody, mentions).use { newContent ->
innerRoom.edit(eventId.value, newContent)
}
}
}

override suspend fun sendMessage(body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit> {
return liveTimeline.sendMessage(body, htmlBody, mentions)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import io.element.android.libraries.matrix.impl.media.toMSC3246range
import io.element.android.libraries.matrix.impl.poll.toInner
import io.element.android.libraries.matrix.impl.room.RoomContentForwarder
import io.element.android.libraries.matrix.impl.room.location.toInner
import io.element.android.libraries.matrix.impl.room.map
import io.element.android.libraries.matrix.impl.timeline.item.event.EventTimelineItemMapper
import io.element.android.libraries.matrix.impl.timeline.item.event.TimelineEventContentMapper
import io.element.android.libraries.matrix.impl.timeline.item.virtual.VirtualTimelineItemMapper
Expand All @@ -51,6 +50,7 @@ import io.element.android.libraries.matrix.impl.timeline.postprocessor.LoadingIn
import io.element.android.libraries.matrix.impl.timeline.postprocessor.RoomBeginningPostProcessor
import io.element.android.libraries.matrix.impl.timeline.postprocessor.TimelineEncryptedHistoryPostProcessor
import io.element.android.libraries.matrix.impl.timeline.reply.InReplyToMapper
import io.element.android.libraries.matrix.impl.util.MessageEventContent
import io.element.android.services.toolbox.api.systemclock.SystemClock
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.CoroutineDispatcher
Expand All @@ -70,12 +70,10 @@ import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.onStart
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import org.matrix.rustcomponents.sdk.EventTimelineItem
import org.matrix.rustcomponents.sdk.FormattedBody
import org.matrix.rustcomponents.sdk.MessageFormat
import org.matrix.rustcomponents.sdk.RoomMessageEventContentWithoutRelation
import org.matrix.rustcomponents.sdk.SendAttachmentJoinHandle
import org.matrix.rustcomponents.sdk.messageEventContentFromHtml
import org.matrix.rustcomponents.sdk.messageEventContentFromMarkdown
import org.matrix.rustcomponents.sdk.use
import timber.log.Timber
import uniffi.matrix_sdk_ui.LiveBackPaginationStatus
Expand Down Expand Up @@ -266,7 +264,7 @@ class RustTimeline(
}

override suspend fun sendMessage(body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit> = withContext(dispatcher) {
messageEventContentFromParts(body, htmlBody).withMentions(mentions.map()).use { content ->
MessageEventContent.from(body, htmlBody, mentions).use { content ->
runCatching<Unit> {
inner.send(content)
}
Expand All @@ -275,20 +273,8 @@ class RustTimeline(

override suspend fun redactEvent(eventId: EventId?, transactionId: TransactionId?, reason: String?): Result<Boolean> = withContext(dispatcher) {
runCatching {
when {
eventId != null -> {
inner.getEventTimelineItemByEventId(eventId.value).use {
inner.redactEvent(item = it, reason = reason)
}
}
transactionId != null -> {
inner.getEventTimelineItemByTransactionId(transactionId.value).use {
inner.redactEvent(item = it, reason = reason)
}
}
else -> {
error("Either eventId or transactionId must be non-null")
}
getEventTimelineItem(eventId, transactionId).use { item ->
inner.redactEvent(item = item, reason = reason)
}
}
}
Expand All @@ -302,26 +288,11 @@ class RustTimeline(
): Result<Unit> =
withContext(dispatcher) {
runCatching<Unit> {
when {
originalEventId != null -> {
inner.getEventTimelineItemByEventId(originalEventId.value).use {
inner.edit(
newContent = messageEventContentFromParts(body, htmlBody).withMentions(mentions.map()),
item = it,
)
}
}
transactionId != null -> {
inner.getEventTimelineItemByTransactionId(transactionId.value).use {
inner.edit(
newContent = messageEventContentFromParts(body, htmlBody).withMentions(mentions.map()),
item = it,
)
}
}
else -> {
error("Either originalEventId or transactionId must be non null")
}
getEventTimelineItem(originalEventId, transactionId).use { item ->
inner.edit(
newContent = MessageEventContent.from(body, htmlBody, mentions),
item = item,
)
}
}
}
Expand All @@ -334,7 +305,7 @@ class RustTimeline(
fromNotification: Boolean,
): Result<Unit> = withContext(dispatcher) {
runCatching {
val msg = messageEventContentFromParts(body, htmlBody).withMentions(mentions.map())
val msg = MessageEventContent.from(body, htmlBody, mentions)
inner.sendReply(msg, eventId.value)
}
}
Expand All @@ -361,6 +332,20 @@ class RustTimeline(
}
}

@Throws
private suspend fun getEventTimelineItem(eventId: EventId?, transactionId: TransactionId?): EventTimelineItem {
return try {
when {
eventId != null -> inner.getEventTimelineItemByEventId(eventId.value)
transactionId != null -> inner.getEventTimelineItemByTransactionId(transactionId.value)
else -> error("Either eventId or transactionId must be non-null")
}
} catch (e: Exception) {
Timber.e(e, "Failed to get event timeline item")
throw TimelineException.EventNotFound
}
}

override suspend fun sendVideo(
file: File,
thumbnailFile: File?,
Expand Down Expand Up @@ -517,13 +502,6 @@ class RustTimeline(
)
}

private fun messageEventContentFromParts(body: String, htmlBody: String?): RoomMessageEventContentWithoutRelation =
if (htmlBody != null) {
messageEventContentFromHtml(body, htmlBody)
} else {
messageEventContentFromMarkdown(body)
}

private fun sendAttachment(files: List<File>, handle: () -> SendAttachmentJoinHandle): Result<MediaUploadHandler> {
return runCatching {
MediaUploadHandlerImpl(files, handle())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* 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
*
* https://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.impl.util

import io.element.android.libraries.matrix.api.room.Mention
import io.element.android.libraries.matrix.impl.room.map
import org.matrix.rustcomponents.sdk.RoomMessageEventContentWithoutRelation
import org.matrix.rustcomponents.sdk.messageEventContentFromHtml
import org.matrix.rustcomponents.sdk.messageEventContentFromMarkdown

/**
* Creates a [RoomMessageEventContentWithoutRelation] from a body, an html body and a list of mentions.
*/
object MessageEventContent {
fun from(body: String, htmlBody: String?, mentions: List<Mention>): RoomMessageEventContentWithoutRelation {
return if (htmlBody != null) {
messageEventContentFromHtml(body, htmlBody)
} else {
messageEventContentFromMarkdown(body)
}.withMentions(mentions.map())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ class FakeMatrixRoom(
return updateUserRoleResult()
}

var editMessageLambda: (EventId, String, String?, List<Mention>) -> Result<Unit> = { _, _, _, _ -> lambdaError() }
override suspend fun editMessage(eventId: EventId, body: String, htmlBody: String?, mentions: List<Mention>): Result<Unit> {
return editMessageLambda(eventId, body, htmlBody, mentions)
}

override suspend fun sendMessage(body: String, htmlBody: String?, mentions: List<Mention>) = simulateLongTask {
sendMessageResult(body, htmlBody, mentions)
}
Expand Down
Loading