Skip to content

Commit

Permalink
Fix pillification not working for non formatted message bodies (#3201)
Browse files Browse the repository at this point in the history
* Fix pillification not working for non formatted message bodies

Pure Markdown bodies weren't being 'pillified' so their mentions were turned into UI elements in the timeline.

A new `pillifiedBody` property was added to `TimelineItemTextBasedContent` to fix this.

* Use shorter version of `textWithMentions` computation
  • Loading branch information
jmartinesp authored Jul 17, 2024
1 parent 181a7a0 commit f5e866e
Show file tree
Hide file tree
Showing 20 changed files with 90 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ internal fun TimelineItemEventRowPreview() = ElementPreview {
event = aTimelineItemEvent(
senderDisplayName = "Sender with a super long name that should ellipsize",
isMine = isMine,
content = aTimelineItemTextContent().copy(
content = aTimelineItemTextContent(
body = "A long text which will be displayed on several lines and" +
" hopefully can be manually adjusted to test different behaviors."
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ internal fun TimelineItemEventRowForDirectRoomPreview() = ElementPreview {
ATimelineItemEventRow(
event = aTimelineItemEvent(
isMine = it,
content = aTimelineItemTextContent().copy(
content = aTimelineItemTextContent(
body = "A long text which will be displayed on several lines and" +
" hopefully can be manually adjusted to test different behaviors."
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ internal fun TimelineItemEventRowTimestampPreview(
event = event.copy(
content = oldContent.copy(
body = str,
pillifiedBody = str,
),
reactionsState = aTimelineItemReactions(count = 0),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ internal fun TimelineItemEventRowWithManyReactionsPreview() = ElementPreview {
ATimelineItemEventRow(
event = aTimelineItemEvent(
isMine = isMine,
content = aTimelineItemTextContent().copy(
content = aTimelineItemTextContent(
body = "A couple of multi-line messages with many reactions attached." +
" One sent by me and another from someone else."
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ internal fun TimelineItemEventRowWithRRPreview(
event = aTimelineItemEvent(
isMine = false,
sendState = null,
content = aTimelineItemTextContent().copy(
body = "A message from someone else"
),
content = aTimelineItemTextContent(body = "A message from someone else"),
timelineItemReactions = aTimelineItemReactions(count = 0),
readReceiptState = TimelineItemReadReceipts(state.receipts),
),
Expand All @@ -55,9 +53,7 @@ internal fun TimelineItemEventRowWithRRPreview(
event = aTimelineItemEvent(
isMine = true,
sendState = state.sendState,
content = aTimelineItemTextContent().copy(
body = "A message from me"
),
content = aTimelineItemTextContent(body = "A message from me"),
timelineItemReactions = aTimelineItemReactions(count = 0),
readReceiptState = TimelineItemReadReceipts(state.receipts),
),
Expand All @@ -69,9 +65,7 @@ internal fun TimelineItemEventRowWithRRPreview(
event = aTimelineItemEvent(
isMine = true,
sendState = state.sendState,
content = aTimelineItemTextContent().copy(
body = "A last message from me"
),
content = aTimelineItemTextContent(body = "A last message from me"),
timelineItemReactions = aTimelineItemReactions(count = 0),
readReceiptState = TimelineItemReadReceipts(state.receipts),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ internal fun TimelineItemEventRowWithReplyContentToPreview(
event = aTimelineItemEvent(
isMine = it,
timelineItemReactions = aTimelineItemReactions(count = 0),
content = aTimelineItemTextContent().copy(
body = "A reply."
),
content = aTimelineItemTextContent(body = "A reply."),
inReplyTo = inReplyToDetails,
displayNameAmbiguous = displayNameAmbiguous,
groupPosition = TimelineItemGroupPosition.First,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,13 @@ internal fun getTextWithResolvedMentions(content: TimelineItemTextBasedContent):
val userProfileCache = LocalRoomMemberProfilesCache.current
val lastCacheUpdate by userProfileCache.lastCacheUpdate.collectAsState()
val mentionSpanTheme = LocalMentionSpanTheme.current
val formattedBody = remember(content.formattedBody, mentionSpanTheme, lastCacheUpdate) {
content.formattedBody?.let { formattedBody ->
updateMentionSpans(formattedBody, userProfileCache)
mentionSpanTheme.updateMentionStyles(formattedBody)
formattedBody
}
val formattedBody = content.formattedBody ?: content.pillifiedBody
val textWithMentions = remember(formattedBody, mentionSpanTheme, lastCacheUpdate) {
updateMentionSpans(formattedBody, userProfileCache)
mentionSpanTheme.updateMentionStyles(formattedBody)
formattedBody
}
return SpannableString(formattedBody ?: content.body)
return SpannableString(textWithMentions)
}

private fun updateMentionSpans(text: CharSequence, cache: RoomMemberProfilesCache): Boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import io.element.android.features.messages.impl.timeline.model.event.TimelineIt
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemTextContent
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVideoContent
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVoiceContent
import io.element.android.features.messages.impl.utils.TextPillificationHelper
import io.element.android.libraries.androidutils.filesize.FileSizeFormatter
import io.element.android.libraries.core.mimetype.MimeTypes
import io.element.android.libraries.featureflag.api.FeatureFlagService
Expand Down Expand Up @@ -69,6 +70,7 @@ class TimelineItemContentMessageFactory @Inject constructor(
private val featureFlagService: FeatureFlagService,
private val htmlConverterProvider: HtmlConverterProvider,
private val permalinkParser: PermalinkParser,
private val textPillificationHelper: TextPillificationHelper,
) {
suspend fun create(
content: MessageContent,
Expand Down Expand Up @@ -126,6 +128,7 @@ class TimelineItemContentMessageFactory @Inject constructor(
val body = messageType.body.trimEnd()
TimelineItemTextContent(
body = body,
pillifiedBody = textPillificationHelper.pillify(body),
htmlDocument = null,
plainText = body,
formattedBody = null,
Expand Down Expand Up @@ -215,6 +218,7 @@ class TimelineItemContentMessageFactory @Inject constructor(
val body = messageType.body.trimEnd()
TimelineItemTextContent(
body = body,
pillifiedBody = textPillificationHelper.pillify(body),
htmlDocument = messageType.formatted?.toHtmlDocument(permalinkParser = permalinkParser),
formattedBody = parseHtml(messageType.formatted) ?: body.withLinks(),
isEdited = content.isEdited,
Expand All @@ -224,6 +228,7 @@ class TimelineItemContentMessageFactory @Inject constructor(
val body = messageType.body.trimEnd()
TimelineItemTextContent(
body = body,
pillifiedBody = textPillificationHelper.pillify(body),
htmlDocument = null,
formattedBody = body.withLinks(),
isEdited = content.isEdited,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.jsoup.nodes.Document

data class TimelineItemEmoteContent(
override val body: String,
override val pillifiedBody: CharSequence = body,
override val htmlDocument: Document?,
override val plainText: String = htmlDocument?.toPlainText() ?: body,
override val formattedBody: CharSequence?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,12 @@ fun aTimelineItemNoticeContent() = TimelineItemNoticeContent(

fun aTimelineItemRedactedContent() = TimelineItemRedactedContent

fun aTimelineItemTextContent() = TimelineItemTextContent(
body = "Text",
fun aTimelineItemTextContent(
body: String = "Text",
pillifiedBody: CharSequence = body,
) = TimelineItemTextContent(
body = body,
pillifiedBody = pillifiedBody,
htmlDocument = null,
formattedBody = null,
isEdited = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.jsoup.nodes.Document

data class TimelineItemNoticeContent(
override val body: String,
override val pillifiedBody: CharSequence = body,
override val htmlDocument: Document?,
override val plainText: String = htmlDocument?.toPlainText() ?: body,
override val formattedBody: CharSequence?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,30 @@ package io.element.android.features.messages.impl.timeline.model.event
import androidx.compose.runtime.Immutable
import org.jsoup.nodes.Document

/**
* Represents a text based content of a timeline item event (a message, a notice, an emote event...).
*/
@Immutable
sealed interface TimelineItemTextBasedContent : TimelineItemEventContent {
/** The raw body of the event, in Markdown format. */
val body: String

/** The body of the event, with mentions replaced by their pillified version. */
val pillifiedBody: CharSequence

/** The parsed HTML DOM of the formatted event body. */
val htmlDocument: Document?

/** The formatted body of the event, already parsed and with the DOM translated to Android spans. */
val formattedBody: CharSequence?

/** The plain text version of the event body. This is the Markdown version without actual Markdown formatting. */
val plainText: String

/** Whether the event has been edited. */
val isEdited: Boolean

/** The raw HTML body of the event. */
val htmlBody: String?
get() = htmlDocument?.body()?.html()
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.jsoup.nodes.Document

data class TimelineItemTextContent(
override val body: String,
override val pillifiedBody: CharSequence = body,
override val htmlDocument: Document?,
override val plainText: String = htmlDocument?.toPlainText() ?: body,
override val formattedBody: CharSequence?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package io.element.android.features.messages.impl.utils
import android.text.Spannable
import android.text.SpannableStringBuilder
import androidx.core.text.getSpans
import com.squareup.anvil.annotations.ContributesBinding
import io.element.android.libraries.di.RoomScope
import io.element.android.libraries.matrix.api.core.MatrixPatternType
import io.element.android.libraries.matrix.api.core.MatrixPatterns
import io.element.android.libraries.matrix.api.core.RoomAlias
Expand All @@ -30,14 +32,19 @@ import io.element.android.libraries.textcomposer.mentions.MentionSpan
import io.element.android.libraries.textcomposer.mentions.MentionSpanProvider
import javax.inject.Inject

class TextPillificationHelper @Inject constructor(
interface TextPillificationHelper {
fun pillify(text: CharSequence): CharSequence
}

@ContributesBinding(RoomScope::class)
class DefaultTextPillificationHelper @Inject constructor(
private val mentionSpanProvider: MentionSpanProvider,
private val permalinkBuilder: PermalinkBuilder,
private val permalinkParser: PermalinkParser,
private val roomMemberProfilesCache: RoomMemberProfilesCache,
) {
) : TextPillificationHelper {
@Suppress("LoopWithTooManyJumpStatements")
fun pillify(text: CharSequence): CharSequence {
override fun pillify(text: CharSequence): CharSequence {
val matches = MatrixPatterns.findPatterns(text, permalinkParser).sortedByDescending { it.end }
if (matches.isEmpty()) return text

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import io.element.android.features.messages.impl.timeline.model.event.TimelineIt
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVideoContent
import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemPollContent
import io.element.android.features.messages.impl.typing.TypingNotificationPresenter
import io.element.android.features.messages.impl.utils.TextPillificationHelper
import io.element.android.features.messages.impl.utils.FakeTextPillificationHelper
import io.element.android.features.messages.impl.voicemessages.composer.VoiceMessageComposerPlayer
import io.element.android.features.messages.impl.voicemessages.composer.VoiceMessageComposerPresenter
import io.element.android.features.messages.impl.voicemessages.timeline.FakeRedactedVoiceMessageManager
Expand Down Expand Up @@ -787,7 +787,7 @@ class MessagesPresenterTest {
timelineController = TimelineController(matrixRoom),
draftService = FakeComposerDraftService(),
mentionSpanProvider = mentionSpanProvider,
pillificationHelper = TextPillificationHelper(mentionSpanProvider, FakePermalinkBuilder(), FakePermalinkParser(), RoomMemberProfilesCache()),
pillificationHelper = FakeTextPillificationHelper(),
roomMemberProfilesCache = RoomMemberProfilesCache(),
).apply {
showTextFormatting = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import io.element.android.features.messages.impl.timeline.factories.event.Timeli
import io.element.android.features.messages.impl.timeline.factories.virtual.TimelineItemDaySeparatorFactory
import io.element.android.features.messages.impl.timeline.factories.virtual.TimelineItemVirtualFactory
import io.element.android.features.messages.impl.timeline.groups.TimelineItemGrouper
import io.element.android.features.messages.impl.utils.FakeTextPillificationHelper
import io.element.android.features.messages.test.timeline.FakeHtmlConverterProvider
import io.element.android.features.poll.test.pollcontent.FakePollContentStateFactory
import io.element.android.libraries.androidutils.filesize.FakeFileSizeFormatter
Expand Down Expand Up @@ -62,6 +63,7 @@ internal fun TestScope.aTimelineItemsFactory(
featureFlagService = FakeFeatureFlagService(),
htmlConverterProvider = FakeHtmlConverterProvider(),
permalinkParser = FakePermalinkParser(),
textPillificationHelper = FakeTextPillificationHelper(),
),
redactedMessageFactory = TimelineItemContentRedactedFactory(),
stickerFactory = TimelineItemContentStickerFactory(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import io.element.android.features.messages.impl.messagecomposer.MessageComposer
import io.element.android.features.messages.impl.messagecomposer.MessageComposerPresenter
import io.element.android.features.messages.impl.messagecomposer.MessageComposerState
import io.element.android.features.messages.impl.timeline.TimelineController
import io.element.android.features.messages.impl.utils.FakeTextPillificationHelper
import io.element.android.features.messages.impl.utils.TextPillificationHelper
import io.element.android.libraries.core.mimetype.MimeTypes
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
Expand Down Expand Up @@ -1362,12 +1363,7 @@ class MessageComposerPresenterTest {
permalinkParser: PermalinkParser = FakePermalinkParser(),
mentionSpanProvider: MentionSpanProvider = MentionSpanProvider(permalinkParser),
roomMemberProfilesCache: RoomMemberProfilesCache = RoomMemberProfilesCache(),
textPillificationHelper: TextPillificationHelper = TextPillificationHelper(
mentionSpanProvider = mentionSpanProvider,
permalinkBuilder = permalinkBuilder,
permalinkParser = permalinkParser,
roomMemberProfilesCache = roomMemberProfilesCache,
),
textPillificationHelper: TextPillificationHelper = FakeTextPillificationHelper(),
isRichTextEditorEnabled: Boolean = true,
draftService: ComposerDraftService = FakeComposerDraftService(),
) = MessageComposerPresenter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import io.element.android.features.messages.impl.timeline.model.event.TimelineIt
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemTextContent
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVideoContent
import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVoiceContent
import io.element.android.features.messages.impl.utils.FakeTextPillificationHelper
import io.element.android.features.messages.test.timeline.FakeHtmlConverterProvider
import io.element.android.libraries.androidutils.filesize.FakeFileSizeFormatter
import io.element.android.libraries.core.mimetype.MimeTypes
Expand Down Expand Up @@ -697,6 +698,7 @@ class TimelineItemContentMessageFactoryTest {
featureFlagService = featureFlagService,
htmlConverterProvider = FakeHtmlConverterProvider(htmlConverterTransform),
permalinkParser = permalinkParser,
textPillificationHelper = FakeTextPillificationHelper(),
)

private fun createStickerContent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import org.junit.Test
import org.junit.runner.RunWith

@RunWith(AndroidJUnit4::class)
class TextPillificationHelperTest {
class DefaultTextPillificationHelperTest {
@Test
fun `pillify - adds pills for user ids`() {
val text = "A @user:server.com"
Expand Down Expand Up @@ -119,7 +119,7 @@ class TextPillificationHelperTest {
permalinkBuilder: FakePermalinkBuilder = FakePermalinkBuilder(),
mentionSpanProvider: MentionSpanProvider = MentionSpanProvider(permalinkparser),
roomMemberProfilesCache: RoomMemberProfilesCache = RoomMemberProfilesCache(),
) = TextPillificationHelper(
) = DefaultTextPillificationHelper(
mentionSpanProvider = mentionSpanProvider,
permalinkBuilder = permalinkBuilder,
permalinkParser = permalinkparser,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* 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.features.messages.impl.utils

class FakeTextPillificationHelper(
private val pillifyLambda: (CharSequence) -> CharSequence = { it }
) : TextPillificationHelper {
override fun pillify(text: CharSequence): CharSequence {
return pillifyLambda(text)
}
}

0 comments on commit f5e866e

Please sign in to comment.