Merge pull request #2043 from vector-im/feature/cleanup_after_2002

Cleanup after #2002

CHANGES.md

@@ -11,7 +11,7 @@ Improvements 🙌:
 
 Bugfix 🐛:
  - Display name not shown under Settings/General (#1926)
- - Wrong markdown parsing (#350, #1375, #1939, #1982)
+ - Editing message forgets line breaks and markdown (#1939)
  - Words containing my name should not trigger notifications (#1781)
  - Fix changing language issue
  - Fix FontSize issue (#1483, #1787)

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

@@ -17,23 +17,22 @@
 package org.matrix.android.sdk.internal.session.room.send
 
 import androidx.test.ext.junit.runners.AndroidJUnit4
-import org.matrix.android.sdk.InstrumentedTest
 import org.commonmark.parser.Parser
 import org.commonmark.renderer.html.HtmlRenderer
-import org.commonmark.renderer.text.TextContentRenderer
 import org.junit.Assert.assertEquals
 import org.junit.FixMethodOrder
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.MethodSorters
+import org.matrix.android.sdk.InstrumentedTest
 
 /**
  * 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,
  * we can add more tests to cover the edge cases.
  * Some tests are suffixed with `_not_passing`, maybe one day we will fix them...
- * Riot-Web should be used as a reference for expected results, but not always. Especially Riot-Web add lots of `\n` in the
+ * Element Web should be used as a reference for expected results, but not always.
- * formatted body, which is quite useless.
+ * Also Element Web does not provide plain text body when formatted text is provided. The body contains what the user has entered. We are doing
- * Also Riot-Web does not provide plain text body when formatted text is provided. The body contains what the user has entered.
+ * the same to be able to edit messages (See #1939)
  * See https://matrix.org/docs/spec/client_server/latest#m-room-message-msgtypes
  */
 @Suppress("SpellCheckingInspection")
@@ -46,8 +45,7 @@ class MarkdownParserTest : InstrumentedTest {
      */
     private val markdownParser = MarkdownParser(
             Parser.builder().build(),
-            HtmlRenderer.builder().build(),
+            HtmlRenderer.builder().build()
-            TextContentRenderer.builder().build()
     )
 
     @Test
@@ -83,6 +81,15 @@ class MarkdownParserTest : InstrumentedTest {
         )
     }
 
+    @Test
+    fun parseBoldNewLines() {
+        testTypeNewLines(
+                name = "bold",
+                markdownPattern = "**",
+                htmlExpectedTag = "strong"
+        )
+    }
+
     @Test
     fun parseItalic() {
         testType(
@@ -92,14 +99,23 @@ class MarkdownParserTest : InstrumentedTest {
         )
     }
 
+    @Test
+    fun parseItalicNewLines() {
+        testTypeNewLines(
+                name = "italic",
+                markdownPattern = "*",
+                htmlExpectedTag = "em"
+        )
+    }
+
     @Test
     fun parseItalic2() {
-        // Riot-Web format
+        // Element Web format
-        "_italic_".let { markdownParser.parse(it) }.expect("italic", "<em>italic</em>")
+        "_italic_".let { markdownParser.parse(it).expect(it, "<em>italic</em>") }
     }
 
     /**
-     * Note: the test is not passing, it does not work on Riot-Web neither
+     * Note: the test is not passing, it does not work on Element Web neither
      */
     @Test
     fun parseStrike_not_passing() {
@@ -110,14 +126,30 @@ class MarkdownParserTest : InstrumentedTest {
         )
     }
 
+    @Test
+    fun parseStrikeNewLines() {
+        testTypeNewLines(
+                name = "strike",
+                markdownPattern = "~~",
+                htmlExpectedTag = "del"
+        )
+    }
+
     @Test
     fun parseCode() {
         testType(
                 name = "code",
                 markdownPattern = "`",
-                htmlExpectedTag = "code",
+                htmlExpectedTag = "code"
-                plainTextPrefix = "\"",
+        )
-                plainTextSuffix = "\""
+    }
+
+    @Test
+    fun parseCodeNewLines() {
+        testTypeNewLines(
+                name = "code",
+                markdownPattern = "`",
+                htmlExpectedTag = "code"
         )
     }
 
@@ -126,9 +158,16 @@ class MarkdownParserTest : InstrumentedTest {
         testType(
                 name = "code",
                 markdownPattern = "``",
-                htmlExpectedTag = "code",
+                htmlExpectedTag = "code"
-                plainTextPrefix = "\"",
+        )
-                plainTextSuffix = "\""
+    }
+
+    @Test
+    fun parseCode2NewLines() {
+        testTypeNewLines(
+                name = "code",
+                markdownPattern = "``",
+                htmlExpectedTag = "code"
         )
     }
 
@@ -137,78 +176,85 @@ class MarkdownParserTest : InstrumentedTest {
         testType(
                 name = "code",
                 markdownPattern = "```",
-                htmlExpectedTag = "code",
+                htmlExpectedTag = "code"
-                plainTextPrefix = "\"",
+        )
-                plainTextSuffix = "\""
+    }
+
+    @Test
+    fun parseCode3NewLines() {
+        testTypeNewLines(
+                name = "code",
+                markdownPattern = "```",
+                htmlExpectedTag = "code"
         )
     }
 
     @Test
     fun parseUnorderedList() {
-        "- item1".let { markdownParser.parse(it).expect(it, "<ul><li>item1</li></ul>") }
+        "- item1".let { markdownParser.parse(it).expect(it, "<ul>\n<li>item1</li>\n</ul>") }
-        "- item1\n- item2".let { markdownParser.parse(it).expect(it, "<ul><li>item1</li><li>item2</li></ul>") }
+        "- item1\n- item2".let { markdownParser.parse(it).expect(it, "<ul>\n<li>item1</li>\n<li>item2</li>\n</ul>") }
     }
 
     @Test
     fun parseOrderedList() {
-        "1. item1".let { markdownParser.parse(it).expect(it, "<ol><li>item1</li></ol>") }
+        "1. item1".let { markdownParser.parse(it).expect(it, "<ol>\n<li>item1</li>\n</ol>") }
-        "1. item1\n2. item2".let { markdownParser.parse(it).expect(it, "<ol><li>item1</li><li>item2</li></ol>") }
+        "1. item1\n2. item2".let { markdownParser.parse(it).expect(it, "<ol>\n<li>item1</li>\n<li>item2</li>\n</ol>") }
     }
 
     @Test
     fun parseHorizontalLine() {
-        "---".let { markdownParser.parse(it) }.expect("***", "<hr />")
+        "---".let { markdownParser.parse(it).expect(it, "<hr />") }
     }
 
     @Test
     fun parseH2AndContent() {
-        "a\n---\nb".let { markdownParser.parse(it) }.expect("a\nb", "<h2>a</h2><p>b</p>")
+        "a\n---\nb".let { markdownParser.parse(it).expect(it, "<h2>a</h2>\n<p>b</p>") }
     }
 
     @Test
     fun parseQuote() {
-        "> quoted".let { markdownParser.parse(it) }.expect("«quoted»", "<blockquote><p>quoted</p></blockquote>")
+        "> quoted".let { markdownParser.parse(it).expect(it, "<blockquote>\n<p>quoted</p>\n</blockquote>") }
     }
 
     @Test
     fun parseQuote_not_passing() {
-        "> quoted\nline2".let { markdownParser.parse(it) }.expect("«quoted\nline2»", "<blockquote><p>quoted<br/>line2</p></blockquote>")
+        "> quoted\nline2".let { markdownParser.parse(it).expect(it, "<blockquote><p>quoted<br />line2</p></blockquote>") }
     }
 
     @Test
     fun parseBoldItalic() {
-        "*italic* **bold**".let { markdownParser.parse(it) }.expect("italic bold", "<em>italic</em> <strong>bold</strong>")
+        "*italic* **bold**".let { markdownParser.parse(it).expect(it, "<em>italic</em> <strong>bold</strong>") }
-        "**bold** *italic*".let { markdownParser.parse(it) }.expect("bold italic", "<strong>bold</strong> <em>italic</em>")
+        "**bold** *italic*".let { markdownParser.parse(it).expect(it, "<strong>bold</strong> <em>italic</em>") }
     }
 
     @Test
     fun parseHead() {
-        "# head1".let { markdownParser.parse(it) }.expect("head1", "<h1>head1</h1>")
+        "# head1".let { markdownParser.parse(it).expect(it, "<h1>head1</h1>") }
-        "## head2".let { markdownParser.parse(it) }.expect("head2", "<h2>head2</h2>")
+        "## head2".let { markdownParser.parse(it).expect(it, "<h2>head2</h2>") }
-        "### head3".let { markdownParser.parse(it) }.expect("head3", "<h3>head3</h3>")
+        "### head3".let { markdownParser.parse(it).expect(it, "<h3>head3</h3>") }
-        "#### head4".let { markdownParser.parse(it) }.expect("head4", "<h4>head4</h4>")
+        "#### head4".let { markdownParser.parse(it).expect(it, "<h4>head4</h4>") }
-        "##### head5".let { markdownParser.parse(it) }.expect("head5", "<h5>head5</h5>")
+        "##### head5".let { markdownParser.parse(it).expect(it, "<h5>head5</h5>") }
-        "###### head6".let { markdownParser.parse(it) }.expect("head6", "<h6>head6</h6>")
+        "###### head6".let { markdownParser.parse(it).expect(it, "<h6>head6</h6>") }
     }
 
     @Test
     fun parseHeads() {
-        "# head1\n# head2".let { markdownParser.parse(it) }.expect("head1\nhead2", "<h1>head1</h1><h1>head2</h1>")
+        "# head1\n# head2".let { markdownParser.parse(it).expect(it, "<h1>head1</h1>\n<h1>head2</h1>") }
     }
 
     @Test
     fun parseBoldNewLines_not_passing() {
-        "**bold**\nline2".let { markdownParser.parse(it) }.expect("bold\nline2", "<strong>bold</strong><br />line2")
+        "**bold**\nline2".let { markdownParser.parse(it).expect(it, "<strong>bold</strong><br />line2") }
     }
 
     @Test
     fun parseLinks() {
-        "[link](target)".let { markdownParser.parse(it) }.expect(""""link" (target)""", """<a href="target">link</a>""")
+        "[link](target)".let { markdownParser.parse(it).expect(it, """<a href="target">link</a>""") }
     }
 
     @Test
     fun parseParagraph() {
-        "# head\ncontent".let { markdownParser.parse(it) }.expect("head\ncontent", "<h1>head</h1><p>content</p>")
+        "# head\ncontent".let { markdownParser.parse(it).expect(it, "<h1>head</h1>\n<p>content</p>") }
     }
 
     private fun testIdentity(text: String) {
@@ -217,60 +263,94 @@ class MarkdownParserTest : InstrumentedTest {
 
     private fun testType(name: String,
                          markdownPattern: String,
-                         htmlExpectedTag: String,
+                         htmlExpectedTag: String) {
-                         plainTextPrefix: String = "",
-                         plainTextSuffix: String = "") {
         // Test simple case
         "$markdownPattern$name$markdownPattern"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$plainTextPrefix$name$plainTextSuffix",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag>")
+                }
 
         // Test twice the same tag
         "$markdownPattern$name$markdownPattern and $markdownPattern$name bis$markdownPattern"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$plainTextPrefix$name$plainTextSuffix and $plainTextPrefix$name bis$plainTextSuffix",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag> and <$htmlExpectedTag>$name bis</$htmlExpectedTag>")
+                }
 
         val textBefore = "a"
         val textAfter = "b"
 
         // With sticked text before
         "$textBefore$markdownPattern$name$markdownPattern"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$textBefore$plainTextPrefix$name$plainTextSuffix",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "$textBefore<$htmlExpectedTag>$name</$htmlExpectedTag>")
+                }
 
         // With text before and space
         "$textBefore $markdownPattern$name$markdownPattern"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$textBefore $plainTextPrefix$name$plainTextSuffix",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "$textBefore <$htmlExpectedTag>$name</$htmlExpectedTag>")
+                }
 
         // With sticked text after
         "$markdownPattern$name$markdownPattern$textAfter"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$plainTextPrefix$name$plainTextSuffix$textAfter",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag>$textAfter")
+                }
 
         // With space and text after
         "$markdownPattern$name$markdownPattern $textAfter"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$plainTextPrefix$name$plainTextSuffix $textAfter",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag> $textAfter")
+                }
 
         // With sticked text before and text after
         "$textBefore$markdownPattern$name$markdownPattern$textAfter"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$textBefore$plainTextPrefix$name$plainTextSuffix$textAfter",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "a<$htmlExpectedTag>$name</$htmlExpectedTag>$textAfter")
+                }
 
         // With text before and after, with spaces
         "$textBefore $markdownPattern$name$markdownPattern $textAfter"
-                .let { markdownParser.parse(it) }
+                .let {
-                .expect(expectedText = "$textBefore $plainTextPrefix$name$plainTextSuffix $textAfter",
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
                                     expectedFormattedText = "$textBefore <$htmlExpectedTag>$name</$htmlExpectedTag> $textAfter")
                 }
+    }
+
+    private fun testTypeNewLines(name: String,
+                                 markdownPattern: String,
+                                 htmlExpectedTag: String) {
+        // With new line inside the block
+        "$markdownPattern$name\n$name$markdownPattern"
+                .let {
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
+                                    expectedFormattedText = "<$htmlExpectedTag>$name<br />$name</$htmlExpectedTag>")
+                }
+
+        // With new line between two blocks
+        "$markdownPattern$name$markdownPattern\n$markdownPattern$name$markdownPattern"
+                .let {
+                    markdownParser.parse(it)
+                            .expect(expectedText = it,
+                                    expectedFormattedText = "<$htmlExpectedTag>$name</$htmlExpectedTag><$htmlExpectedTag>$name</$htmlExpectedTag>")
+                }
+    }
 
     private fun TextContent.expect(expectedText: String, expectedFormattedText: String?) {
         assertEquals("TextContent are not identical", TextContent(expectedText, expectedFormattedText), this)

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

@@ -20,6 +20,8 @@ package org.matrix.android.sdk.internal.session.room
 import dagger.Binds
 import dagger.Module
 import dagger.Provides
+import org.commonmark.parser.Parser
+import org.commonmark.renderer.html.HtmlRenderer
 import org.matrix.android.sdk.api.session.file.FileService
 import org.matrix.android.sdk.api.session.room.RoomDirectoryService
 import org.matrix.android.sdk.api.session.room.RoomService
@@ -75,9 +77,6 @@ import org.matrix.android.sdk.internal.session.room.typing.DefaultSendTypingTask
 import org.matrix.android.sdk.internal.session.room.typing.SendTypingTask
 import org.matrix.android.sdk.internal.session.room.uploads.DefaultGetUploadsTask
 import org.matrix.android.sdk.internal.session.room.uploads.GetUploadsTask
-import org.commonmark.parser.Parser
-import org.commonmark.renderer.html.HtmlRenderer
-import org.commonmark.renderer.text.TextContentRenderer
 import retrofit2.Retrofit
 
 @Module
@@ -105,14 +104,6 @@ internal abstract class RoomModule {
                     .builder()
                     .build()
         }
-
-        @Provides
-        @JvmStatic
-        fun providesTextContentRenderer(): TextContentRenderer {
-            return TextContentRenderer
-                    .builder()
-                    .build()
-        }
     }
 
     @Binds

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

@@ -19,7 +19,6 @@ package org.matrix.android.sdk.internal.session.room.send
 
 import org.commonmark.parser.Parser
 import org.commonmark.renderer.html.HtmlRenderer
-import org.commonmark.renderer.text.TextContentRenderer
 import javax.inject.Inject
 
 /**
@@ -29,8 +28,7 @@ import javax.inject.Inject
  */
 internal class MarkdownParser @Inject constructor(
         private val parser: Parser,
-        private val htmlRenderer: HtmlRenderer,
+        private val htmlRenderer: HtmlRenderer
-        private val textContentRenderer: TextContentRenderer
 ) {
 
     private val mdSpecialChars = "[`_\\-*>.\\[\\]#~]".toRegex()
@@ -54,7 +52,8 @@ internal class MarkdownParser @Inject constructor(
         return if (isFormattedTextPertinent(text, 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.
-            TextContent(text, cleanHtmlText.trim())
+            // But it caused too many problems so it has been removed in #2002
+            TextContent(text, cleanHtmlText.postTreatment())
         } else {
             TextContent(text)
         }
@@ -62,4 +61,16 @@ internal class MarkdownParser @Inject constructor(
 
     private fun isFormattedTextPertinent(text: String, htmlText: String?) =
             text != htmlText && htmlText != "<p>${text.trim()}</p>\n"
+
+    /**
+     * The parser makes some mistakes, so deal with it here
+     */
+    private fun String.postTreatment(): String {
+        return this
+                // Remove extra space before and after the content
+                .trim()
+                // There is no need to include new line in an html-like source
+                // But new line can be in embedded code block, so do not remove them
+                // .replace("\n", "")
+    }
 }