Merge pull request #2336 from vector-im/feature/ons/fix_markdown_formatter

Markdown body should be pure text, see #739, #1506.

CHANGES.md

@@ -24,6 +24,7 @@ Bugfix 🐛:
  - Messages encrypted with no way to decrypt after SDK update from 0.18 to 1.0.0 (#2252)
  - Incoming call continues to ring if call is answered on another device (#1921)
  - Search Result | scroll jumps after pagination (#2238)
+ - Badly formatted mentions in body (#1506)
  - KeysBackup: Avoid using `!!` (#2262)
 
 Translations 🗣:

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

@@ -25,6 +25,8 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.MethodSorters
 import org.matrix.android.sdk.InstrumentedTest
+import org.matrix.android.sdk.internal.session.room.send.pills.MentionLinkSpecComparator
+import org.matrix.android.sdk.internal.session.room.send.pills.TextPillsUtils
 
 /**
  * It will not be possible to test all combinations. For the moment I add a few tests, then, depending on the problem discovered in the wild,
@@ -45,7 +47,8 @@ class MarkdownParserTest : InstrumentedTest {
      */
     private val markdownParser = MarkdownParser(
             Parser.builder().build(),
-            HtmlRenderer.builder().build()
+            HtmlRenderer.builder().softbreak("<br />").build(),
+            TextPillsUtils(MentionLinkSpecComparator())
     )
 
     @Test
@@ -144,12 +147,14 @@ class MarkdownParserTest : InstrumentedTest {
         )
     }
 
+    // TODO. Improve testTypeNewLines function to cover <pre><code class="language-code">test</code></pre>
     @Test
-    fun parseCodeNewLines() {
+    fun parseCodeNewLines_not_passing() {
         testTypeNewLines(
                 name = "code",
-                markdownPattern = "`",
+                markdownPattern = "```",
-                htmlExpectedTag = "code"
+                htmlExpectedTag = "code",
+                softBreak = "\n"
         )
     }
 
@@ -163,7 +168,7 @@ class MarkdownParserTest : InstrumentedTest {
     }
 
     @Test
-    fun parseCode2NewLines() {
+    fun parseCode2NewLines_not_passing() {
         testTypeNewLines(
                 name = "code",
                 markdownPattern = "``",
@@ -181,7 +186,7 @@ class MarkdownParserTest : InstrumentedTest {
     }
 
     @Test
-    fun parseCode3NewLines() {
+    fun parseCode3NewLines_not_passing() {
         testTypeNewLines(
                 name = "code",
                 markdownPattern = "```",
@@ -243,7 +248,7 @@ class MarkdownParserTest : InstrumentedTest {
     }
 
     @Test
-    fun parseBoldNewLines_not_passing() {
+    fun parseBoldNewLines2() {
         "**bold**\nline2".let { markdownParser.parse(it).expect(it, "<strong>bold</strong><br />line2") }
     }
 
@@ -334,13 +339,14 @@ class MarkdownParserTest : InstrumentedTest {
 
     private fun testTypeNewLines(name: String,
                                  markdownPattern: String,
-                                 htmlExpectedTag: String) {
+                                 htmlExpectedTag: String,
+                                 softBreak: String = "<br />") {
         // With new line inside the block
         "$markdownPattern$name\n$name$markdownPattern"
                 .let {
                     markdownParser.parse(it)
                             .expect(expectedText = it,
-                                    expectedFormattedText = "<$htmlExpectedTag>$name<br />$name</$htmlExpectedTag>")
+                                    expectedFormattedText = "<$htmlExpectedTag>$name$softBreak$name</$htmlExpectedTag>")
                 }
 
         // With new line between two blocks
@@ -348,7 +354,7 @@ class MarkdownParserTest : InstrumentedTest {
                 .let {
                     markdownParser.parse(it)
                             .expect(expectedText = it,
-                                    expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag><$htmlExpectedTag>$name</$htmlExpectedTag>")
+                                    expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag><br /><$htmlExpectedTag>$name</$htmlExpectedTag>")
                 }
     }
 

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

@@ -101,6 +101,7 @@ internal abstract class RoomModule {
         fun providesHtmlRenderer(): HtmlRenderer {
             return HtmlRenderer
                     .builder()
+                    .softbreak("<br />")
                     .build()
         }
     }

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

@@ -90,8 +90,7 @@ internal class LocalEchoEventFactory @Inject constructor(
 
     private fun createTextContent(text: CharSequence, autoMarkdown: Boolean): TextContent {
         if (autoMarkdown) {
-            val source = textPillsUtils.processSpecialSpansToMarkdown(text) ?: text.toString()
+            return markdownParser.parse(text)
-            return markdownParser.parse(source)
         } else {
             // Try to detect pills
             textPillsUtils.processSpecialSpansToHtml(text)?.let {

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

@@ -18,6 +18,7 @@ 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.send.pills.TextPillsUtils
 import javax.inject.Inject
 
 /**
@@ -27,18 +28,21 @@ import javax.inject.Inject
  */
 internal class MarkdownParser @Inject constructor(
         private val parser: Parser,
-        private val htmlRenderer: HtmlRenderer
+        private val htmlRenderer: HtmlRenderer,
+        private val textPillsUtils: TextPillsUtils
 ) {
 
     private val mdSpecialChars = "[`_\\-*>.\\[\\]#~]".toRegex()
 
-    fun parse(text: String): TextContent {
+    fun parse(text: CharSequence): TextContent {
+        val source = textPillsUtils.processSpecialSpansToMarkdown(text) ?: text.toString()
+
         // If no special char are detected, just return plain text
-        if (text.contains(mdSpecialChars).not()) {
+        if (source.contains(mdSpecialChars).not()) {
-            return TextContent(text)
+            return TextContent(source)
         }
 
-        val document = parser.parse(text)
+        val document = parser.parse(source)
         val htmlText = htmlRenderer.render(document)
 
         // Cleanup extra paragraph
@@ -48,13 +52,14 @@ internal class MarkdownParser @Inject constructor(
             htmlText
         }
 
-        return if (isFormattedTextPertinent(text, cleanHtmlText)) {
+        return if (isFormattedTextPertinent(source, cleanHtmlText)) {
             // According to https://matrix.org/docs/spec/client_server/latest#m-room-message-msgtypes:
             // The plain text version of the HTML should be provided in the body.
             // But it caused too many problems so it has been removed in #2002
-            TextContent(text, cleanHtmlText.postTreatment())
+            // See #739
+            TextContent(text.toString(), cleanHtmlText.postTreatment())
         } else {
-            TextContent(text)
+            TextContent(source)
         }
     }