Merge pull request #4541 from vector-im/feature/dla/fix_reply_and_quote_newlines

Fix reply and quote newlines

changelog.d/4540.bugfix

@@ -0,0 +1 @@
+Fix message replies/quotes to respect newlines.

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/session/room/send/MarkdownParserTest.kt

@@ -49,6 +49,7 @@ class MarkdownParserTest : InstrumentedTest {
      * Create the same parser than in the RoomModule
      */
     private val markdownParser = MarkdownParser(
+            Parser.builder().build(),
             Parser.builder().build(),
             HtmlRenderer.builder().softbreak("<br />").build(),
             TextPillsUtils(

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/send/SendService.kt

@@ -56,6 +56,15 @@ interface SendService {
      */
     fun sendFormattedTextMessage(text: String, formattedText: String, msgType: String = MessageType.MSGTYPE_TEXT): Cancelable
 
+    /**
+     * Method to quote an events content.
+     * @param quotedEvent The event to which we will quote it's content.
+     * @param text the text message to send
+     * @param autoMarkdown If true, the SDK will generate a formatted HTML message from the body text if markdown syntax is present
+     * @return a [Cancelable]
+     */
+    fun sendQuotedTextMessage(quotedEvent: TimelineEvent, text: String, autoMarkdown: Boolean): Cancelable
+
     /**
      * Method to send a media asynchronously.
      * @param attachment the media to send

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

@@ -21,6 +21,7 @@ import dagger.Module
 import dagger.Provides
 import org.commonmark.Extension
 import org.commonmark.ext.maths.MathsExtension
+import org.commonmark.node.BlockQuote
 import org.commonmark.parser.Parser
 import org.commonmark.renderer.html.HtmlRenderer
 import org.matrix.android.sdk.api.session.file.FileService
@@ -100,6 +101,21 @@ import org.matrix.android.sdk.internal.session.room.version.DefaultRoomVersionUp
 import org.matrix.android.sdk.internal.session.room.version.RoomVersionUpgradeTask
 import org.matrix.android.sdk.internal.session.space.DefaultSpaceService
 import retrofit2.Retrofit
+import javax.inject.Qualifier
+
+/**
+ * Used to inject the simple commonmark Parser
+ */
+@Qualifier
+@Retention(AnnotationRetention.RUNTIME)
+internal annotation class SimpleCommonmarkParser
+
+/**
+ * Used to inject the advanced commonmark Parser
+ */
+@Qualifier
+@Retention(AnnotationRetention.RUNTIME)
+internal annotation class AdvancedCommonmarkParser
 
 @Module
 internal abstract class RoomModule {
@@ -123,11 +139,23 @@ internal abstract class RoomModule {
         }
 
         @Provides
+        @AdvancedCommonmarkParser
         @JvmStatic
-        fun providesParser(): Parser {
+        fun providesAdvancedParser(): Parser {
             return Parser.builder().extensions(extensions).build()
         }
 
+        @Provides
+        @SimpleCommonmarkParser
+        @JvmStatic
+        fun providesSimpleParser(): Parser {
+            // The simple parser disables all blocks but quotes.
+            // Inline parsing(bold, italic, etc) is also enabled and is not easy to disable in commonmark currently.
+            return Parser.builder()
+                    .enabledBlockTypes(setOf(BlockQuote::class.java))
+                    .build()
+        }
+
         @Provides
         @JvmStatic
         fun providesHtmlRenderer(): HtmlRenderer {

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/DefaultSendService.kt

@@ -97,6 +97,12 @@ internal class DefaultSendService @AssistedInject constructor(
                 .let { sendEvent(it) }
     }
 
+    override fun sendQuotedTextMessage(quotedEvent: TimelineEvent, text: String, autoMarkdown: Boolean): Cancelable {
+        return localEchoEventFactory.createQuotedTextEvent(roomId, quotedEvent, text, autoMarkdown)
+                .also { createLocalEcho(it) }
+                .let { sendEvent(it) }
+    }
+
     override fun sendPoll(question: String, options: List<String>): Cancelable {
         return localEchoEventFactory.createPollEvent(roomId, question, options)
                 .also { createLocalEcho(it) }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/LocalEchoEventFactory.kt

@@ -198,20 +198,23 @@ internal class LocalEchoEventFactory @Inject constructor(
                                  eventReplaced: TimelineEvent,
                                  originalEvent: TimelineEvent,
                                  newBodyText: String,
-                                 newBodyAutoMarkdown: Boolean,
+                                 autoMarkdown: Boolean,
                                  msgType: String,
                                  compatibilityText: String): Event {
         val permalink = permalinkFactory.createPermalink(roomId, originalEvent.root.eventId ?: "", false)
         val userLink = originalEvent.root.senderId?.let { permalinkFactory.createPermalink(it, false) } ?: ""
 
         val body = bodyForReply(originalEvent.getLastMessageContent(), originalEvent.isReply())
-        val replyFormatted = REPLY_PATTERN.format(
+        // As we always supply formatted body for replies we should force the MarkdownParser to produce html.
+        val newBodyFormatted = markdownParser.parse(newBodyText, force = true, advanced = autoMarkdown).takeFormatted()
+        // Body of the original message may not have formatted version, so may also have to convert to html.
+        val bodyFormatted = body.formattedText ?: markdownParser.parse(body.text, force = true, advanced = autoMarkdown).takeFormatted()
+        val replyFormatted = buildFormattedReply(
                 permalink,
                 userLink,
                 originalEvent.senderInfo.disambiguatedDisplayName,
-                // Remove inner mx_reply tags if any
+                bodyFormatted,
-                body.takeFormatted().replace(MX_REPLY_REGEX, ""),
+                newBodyFormatted
-                createTextContent(newBodyText, newBodyAutoMarkdown).takeFormatted()
         )
         //
         // > <@alice:example.org> This is the original body
@@ -391,13 +394,17 @@ internal class LocalEchoEventFactory @Inject constructor(
         val userLink = permalinkFactory.createPermalink(userId, false) ?: return null
 
         val body = bodyForReply(eventReplied.getLastMessageContent(), eventReplied.isReply())
-        val replyFormatted = REPLY_PATTERN.format(
+
+        // As we always supply formatted body for replies we should force the MarkdownParser to produce html.
+        val replyTextFormatted = markdownParser.parse(replyText, force = true, advanced = autoMarkdown).takeFormatted()
+        // Body of the original message may not have formatted version, so may also have to convert to html.
+        val bodyFormatted = body.formattedText ?: markdownParser.parse(body.text, force = true, advanced = autoMarkdown).takeFormatted()
+        val replyFormatted = buildFormattedReply(
                 permalink,
                 userLink,
                 userId,
-                // Remove inner mx_reply tags if any
+                bodyFormatted,
-                body.takeFormatted().replace(MX_REPLY_REGEX, ""),
+                replyTextFormatted
-                createTextContent(replyText, autoMarkdown).takeFormatted()
         )
         //
         // > <@alice:example.org> This is the original body
@@ -415,6 +422,16 @@ internal class LocalEchoEventFactory @Inject constructor(
         return createMessageEvent(roomId, content)
     }
 
+    private fun buildFormattedReply(permalink: String, userLink: String, userId: String, bodyFormatted: String, newBodyFormatted: String): String {
+        return REPLY_PATTERN.format(
+                permalink,
+                userLink,
+                userId,
+                // Remove inner mx_reply tags if any
+                bodyFormatted.replace(MX_REPLY_REGEX, ""),
+                newBodyFormatted
+        )
+    }
     private fun buildReplyFallback(body: TextContent, originalSenderId: String?, newBodyText: String): String {
         return buildString {
             append("> <")
@@ -498,6 +515,38 @@ internal class LocalEchoEventFactory @Inject constructor(
         localEchoRepository.createLocalEcho(event)
     }
 
+    fun createQuotedTextEvent(
+            roomId: String,
+            quotedEvent: TimelineEvent,
+            text: String,
+            autoMarkdown: Boolean,
+    ): Event {
+        val messageContent = quotedEvent.getLastMessageContent()
+        val textMsg = messageContent?.body
+        val quoteText = legacyRiotQuoteText(textMsg, text)
+        return createFormattedTextEvent(roomId, markdownParser.parse(quoteText, force = true, advanced = autoMarkdown), MessageType.MSGTYPE_TEXT)
+    }
+
+    private fun legacyRiotQuoteText(quotedText: String?, myText: String): String {
+        val messageParagraphs = quotedText?.split("\n\n".toRegex())?.dropLastWhile { it.isEmpty() }?.toTypedArray()
+        return buildString {
+            if (messageParagraphs != null) {
+                for (i in messageParagraphs.indices) {
+                    if (messageParagraphs[i].isNotBlank()) {
+                        append("> ")
+                        append(messageParagraphs[i])
+                    }
+
+                    if (i != messageParagraphs.lastIndex) {
+                        append("\n\n")
+                    }
+                }
+            }
+            append("\n\n")
+            append(myText)
+        }
+    }
+
     companion object {
         // <mx-reply>
         //     <blockquote>

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/MarkdownParser.kt

@@ -18,6 +18,8 @@ package org.matrix.android.sdk.internal.session.room.send
 
 import org.commonmark.parser.Parser
 import org.commonmark.renderer.html.HtmlRenderer
+import org.matrix.android.sdk.internal.session.room.AdvancedCommonmarkParser
+import org.matrix.android.sdk.internal.session.room.SimpleCommonmarkParser
 import org.matrix.android.sdk.internal.session.room.send.pills.TextPillsUtils
 import javax.inject.Inject
 
@@ -27,22 +29,30 @@ import javax.inject.Inject
  * If any change is required, please add a test covering the problem and make sure all the tests are still passing.
  */
 internal class MarkdownParser @Inject constructor(
-        private val parser: Parser,
+        @AdvancedCommonmarkParser private val advancedParser: Parser,
+        @SimpleCommonmarkParser private val simpleParser: Parser,
         private val htmlRenderer: HtmlRenderer,
         private val textPillsUtils: TextPillsUtils
 ) {
 
     private val mdSpecialChars = "[`_\\-*>.\\[\\]#~$]".toRegex()
 
-    fun parse(text: CharSequence): TextContent {
+    /**
+     * Parses some input text and produces html.
+     * @param text An input CharSequence to be parsed.
+     * @param force Skips the check for detecting if the input contains markdown and always converts to html.
+     * @param advanced Whether to use the full markdown support or the simple version.
+     * @return TextContent containing the plain text and the formatted html if generated.
+     */
+    fun parse(text: CharSequence, force: Boolean = false, advanced: Boolean = true): TextContent {
         val source = textPillsUtils.processSpecialSpansToMarkdown(text) ?: text.toString()
 
         // If no special char are detected, just return plain text
-        if (source.contains(mdSpecialChars).not()) {
+        if (!force && source.contains(mdSpecialChars).not()) {
             return TextContent(source)
         }
 
-        val document = parser.parse(source)
+        val document = if (advanced) advancedParser.parse(source) else simpleParser.parse(source)
         val htmlText = htmlRenderer.render(document)
 
         // Cleanup extra paragraph

vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerViewModel.kt

@@ -39,8 +39,6 @@ import im.vector.app.features.settings.VectorPreferences
 import im.vector.app.features.voice.VoicePlayerHelper
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
-import org.commonmark.parser.Parser
-import org.commonmark.renderer.html.HtmlRenderer
 import org.matrix.android.sdk.api.query.QueryStringValue
 import org.matrix.android.sdk.api.session.Session
 import org.matrix.android.sdk.api.session.content.ContentAttachmentData
@@ -408,23 +406,7 @@ class MessageComposerViewModel @AssistedInject constructor(
                     popDraft()
                 }
                 is SendMode.Quote   -> {
-                    val messageContent = state.sendMode.timelineEvent.getLastMessageContent()
+                    room.sendQuotedTextMessage(state.sendMode.timelineEvent, action.text.toString(), action.autoMarkdown)
-                    val textMsg = messageContent?.body
-
-                    val finalText = legacyRiotQuoteText(textMsg, action.text.toString())
-
-                    // TODO check for pills?
-
-                    // TODO Refactor this, just temporary for quotes
-                    val parser = Parser.builder().build()
-                    val document = parser.parse(finalText)
-                    val renderer = HtmlRenderer.builder().build()
-                    val htmlText = renderer.render(document)
-                    if (finalText == htmlText) {
-                        room.sendTextMessage(finalText)
-                    } else {
-                        room.sendFormattedTextMessage(finalText, htmlText)
-                    }
                     _viewEvents.post(MessageComposerViewEvents.MessageSent)
                     popDraft()
                 }