From 1062f9fb0d815d0287e6eb460343da1646b484e8 Mon Sep 17 00:00:00 2001
From: Spencer Gilson <spencer.gilson@gmail.com>
Date: Fri, 29 Oct 2021 06:01:02 -0700
Subject: [PATCH] GFM: Use Markdown syntax to render lists (#2098)

* - Add ListBuilder to PageContentBuilder.kt to make testing feasible through the builder DSL
- Switch list representation in CommonmarkRenderer.kt to use Markdown syntax instead of HTML
- Switch to non-deprecated Assert in SimpleElementsTest.kt

* Updating base.api to include new builder class and methods
---
 plugins/base/api/base.api                     |  13 ++
 .../documentables/PageContentBuilder.kt       |  58 +++++-
 .../dokka/gfm/renderer/CommonmarkRenderer.kt  |  69 ++++---
 .../renderers/gfm/SimpleElementsTest.kt       | 177 +++++++++++++++++-
 4 files changed, 283 insertions(+), 34 deletions(-)

diff --git a/plugins/base/api/base.api b/plugins/base/api/base.api
index 526e7ab452..c87d6f5ea8 100644
--- a/plugins/base/api/base.api
+++ b/plugins/base/api/base.api
@@ -1424,6 +1424,8 @@ public class org/jetbrains/dokka/base/translators/documentables/PageContentBuild
 	public final fun list (Ljava/util/List;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/util/Set;Ljava/util/Set;Ljava/util/Set;Lkotlin/jvm/functions/Function2;)V
 	public static synthetic fun list$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$DocumentableContentBuilder;Ljava/util/List;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/util/Set;Ljava/util/Set;Ljava/util/Set;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
 	public final fun operator (Ljava/lang/String;)V
+	public final fun orderedList (Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;)V
+	public static synthetic fun orderedList$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$DocumentableContentBuilder;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V
 	public final fun punctuation (Ljava/lang/String;)V
 	public final fun sourceSetDependentHint (Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;)V
 	public final fun sourceSetDependentHint (Lorg/jetbrains/dokka/links/DRI;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;)V
@@ -1438,6 +1440,17 @@ public class org/jetbrains/dokka/base/translators/documentables/PageContentBuild
 	public static synthetic fun text$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$DocumentableContentBuilder;Ljava/lang/String;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;ILjava/lang/Object;)V
 	public final fun unaryPlus (Ljava/util/Collection;)V
 	public final fun unaryPlus (Lorg/jetbrains/dokka/pages/ContentNode;)V
+	public final fun unorderedList (Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;)V
+	public static synthetic fun unorderedList$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$DocumentableContentBuilder;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V
+}
+
+public class org/jetbrains/dokka/base/translators/documentables/PageContentBuilder$ListBuilder {
+	public fun <init> (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder;ZLjava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;)V
+	public final fun build (Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;)Lorg/jetbrains/dokka/pages/ContentList;
+	public static synthetic fun build$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$ListBuilder;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;ILjava/lang/Object;)Lorg/jetbrains/dokka/pages/ContentList;
+	public final fun getOrdered ()Z
+	public final fun item (Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;)V
+	public static synthetic fun item$default (Lorg/jetbrains/dokka/base/translators/documentables/PageContentBuilder$ListBuilder;Ljava/util/Set;Ljava/util/Set;Lorg/jetbrains/dokka/pages/Kind;Ljava/util/Set;Lorg/jetbrains/dokka/model/properties/PropertyContainer;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V
 }
 
 public class org/jetbrains/dokka/base/translators/documentables/PageContentBuilder$TableBuilder {
diff --git a/plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt b/plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
index f9bc7e26dd..7d4f5c05bd 100644
--- a/plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
+++ b/plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
@@ -9,10 +9,10 @@ import org.jetbrains.dokka.model.Documentable
 import org.jetbrains.dokka.model.SourceSetDependent
 import org.jetbrains.dokka.model.doc.DocTag
 import org.jetbrains.dokka.model.properties.PropertyContainer
+import org.jetbrains.dokka.model.properties.plus
 import org.jetbrains.dokka.model.toDisplaySourceSets
 import org.jetbrains.dokka.pages.*
 import org.jetbrains.dokka.utilities.DokkaLogger
-import org.jetbrains.dokka.model.properties.plus
 
 @DslMarker
 annotation class ContentBuilderMarker
@@ -153,6 +153,26 @@ open class PageContentBuilder(
             }.build()
         }
 
+        fun unorderedList(
+            kind: Kind = ContentKind.Main,
+            sourceSets: Set<DokkaSourceSet> = mainSourcesetData,
+            styles: Set<Style> = mainStyles,
+            extra: PropertyContainer<ContentNode> = mainExtra,
+            operation: ListBuilder.() -> Unit = {}
+        ) {
+            contents += ListBuilder(false, mainDRI, sourceSets, kind, styles, extra).apply(operation).build()
+        }
+
+        fun orderedList(
+            kind: Kind = ContentKind.Main,
+            sourceSets: Set<DokkaSourceSet> = mainSourcesetData,
+            styles: Set<Style> = mainStyles,
+            extra: PropertyContainer<ContentNode> = mainExtra,
+            operation: ListBuilder.() -> Unit = {}
+        ) {
+            contents += ListBuilder(true, mainDRI, sourceSets, kind, styles, extra).apply(operation).build()
+        }
+
         internal fun headers(vararg label: String) = contentFor(mainDRI, mainSourcesetData) {
             label.forEach { text(it) }
         }
@@ -584,4 +604,40 @@ open class PageContentBuilder(
                 extra
             )
     }
+
+    @ContentBuilderMarker
+    open inner class ListBuilder(
+        val ordered: Boolean,
+        private val mainDRI: Set<DRI>,
+        private val mainSourceSets: Set<DokkaSourceSet>,
+        private val mainKind: Kind,
+        private val mainStyles: Set<Style>,
+        private val mainExtra: PropertyContainer<ContentNode>
+    ) {
+        private val contentNodes: MutableList<ContentNode> = mutableListOf()
+
+        fun item(
+            dri: Set<DRI> = mainDRI,
+            sourceSets: Set<DokkaSourceSet> = mainSourceSets,
+            kind: Kind = mainKind,
+            styles: Set<Style> = mainStyles,
+            extra: PropertyContainer<ContentNode> = mainExtra,
+            block: DocumentableContentBuilder.() -> Unit
+        ) {
+            contentNodes += contentFor(dri, sourceSets, kind, styles, extra, block)
+        }
+
+        fun build(
+            sourceSets: Set<DokkaSourceSet> = mainSourceSets,
+            kind: Kind = mainKind,
+            styles: Set<Style> = mainStyles,
+            extra: PropertyContainer<ContentNode> = mainExtra
+        ) = ContentList(
+            contentNodes,
+            ordered,
+            DCI(mainDRI, kind),
+            sourceSets.toDisplaySourceSets(),
+            styles, extra
+        )
+    }
 }
diff --git a/plugins/gfm/src/main/kotlin/org/jetbrains/dokka/gfm/renderer/CommonmarkRenderer.kt b/plugins/gfm/src/main/kotlin/org/jetbrains/dokka/gfm/renderer/CommonmarkRenderer.kt
index c05bd07e23..786e410d07 100644
--- a/plugins/gfm/src/main/kotlin/org/jetbrains/dokka/gfm/renderer/CommonmarkRenderer.kt
+++ b/plugins/gfm/src/main/kotlin/org/jetbrains/dokka/gfm/renderer/CommonmarkRenderer.kt
@@ -55,31 +55,42 @@ open class CommonmarkRenderer(
         sourceSetRestriction: Set<DisplaySourceSet>?
     ) {
         buildParagraph()
-        buildListLevel(node, pageContext)
+        buildList(node, pageContext)
         buildParagraph()
     }
 
-    private fun StringBuilder.buildListItem(items: List<ContentNode>, pageContext: ContentPage) {
-        items.forEach {
-            if (it is ContentList) {
-                buildList(it, pageContext)
+    private fun StringBuilder.buildList(
+        node: ContentList,
+        pageContext: ContentPage
+    ) {
+        node.children.forEachIndexed { i, it ->
+            if (node.ordered) {
+                // number is irrelevant, but a nice touch
+                // period is more widely compatible
+                append("${i + 1}. ")
             } else {
-                append("<li>")
-                append(buildString { it.build(this, pageContext, it.sourceSets) }.trim())
-                append("</li>")
+                append("- ")
             }
-        }
-    }
 
-    private fun StringBuilder.buildListLevel(node: ContentList, pageContext: ContentPage) {
-        if (node.ordered) {
-            append("<ol>")
-            buildListItem(node.children, pageContext)
-            append("</ol>")
-        } else {
-            append("<ul>")
-            buildListItem(node.children, pageContext)
-            append("</ul>")
+            /*
+            Handle case when list item transitions to another complex node with no preceding text.
+            For example, the equivalent of:
+            <li>
+               <ul><li><ul>Item</ul></li></ul>
+            </li>
+
+            Would be:
+            -
+               - Item
+             */
+            if (it is ContentGroup && it.children.firstOrNull()?.let { it !is ContentText } == true) {
+                append("\n   ")
+            }
+
+            buildString { it.build(this, pageContext, it.sourceSets) }
+                .replace("\n", "\n   ") // apply indent
+                .trim().let { append(it) }
+            buildNewLine()
         }
     }
 
@@ -328,18 +339,18 @@ open class CommonmarkRenderer(
                 is RenderingStrategy.Write -> outputWriter.write(path, strategy.text, "")
                 is RenderingStrategy.Callback -> outputWriter.write(path, strategy.instructions(this, page), ".md")
                 is RenderingStrategy.DriLocationResolvableWrite -> outputWriter.write(
-                        path,
-                        strategy.contentToResolve { dri, sourcesets ->
-                            locationProvider.resolve(dri, sourcesets)
-                        },
-                        ""
+                    path,
+                    strategy.contentToResolve { dri, sourcesets ->
+                        locationProvider.resolve(dri, sourcesets)
+                    },
+                    ""
                 )
                 is RenderingStrategy.PageLocationResolvableWrite -> outputWriter.write(
-                        path,
-                        strategy.contentToResolve { pageToLocate, context ->
-                            locationProvider.resolve(pageToLocate, context)
-                        },
-                        ""
+                    path,
+                    strategy.contentToResolve { pageToLocate, context ->
+                        locationProvider.resolve(pageToLocate, context)
+                    },
+                    ""
                 )
                 RenderingStrategy.DoNothing -> Unit
             }
diff --git a/plugins/gfm/src/test/kotlin/renderers/gfm/SimpleElementsTest.kt b/plugins/gfm/src/test/kotlin/renderers/gfm/SimpleElementsTest.kt
index b4af12b2f0..e102f52a55 100644
--- a/plugins/gfm/src/test/kotlin/renderers/gfm/SimpleElementsTest.kt
+++ b/plugins/gfm/src/test/kotlin/renderers/gfm/SimpleElementsTest.kt
@@ -1,12 +1,15 @@
 package renderers.gfm
 
-import junit.framework.Assert.assertEquals
 import org.jetbrains.dokka.gfm.renderer.CommonmarkRenderer
-import org.junit.jupiter.api.Test
-import renderers.testPage
 import org.jetbrains.dokka.links.DRI
-import org.jetbrains.dokka.pages.*
+import org.jetbrains.dokka.pages.ContentEmbeddedResource
+import org.jetbrains.dokka.pages.ContentKind
+import org.jetbrains.dokka.pages.DCI
+import org.jetbrains.dokka.pages.TextStyle
+import org.junit.Assert.assertEquals
+import org.junit.jupiter.api.Test
 import renderers.RawTestPage
+import renderers.testPage
 
 class SimpleElementsTest : GfmRenderingOnlyTestBase() {
 
@@ -217,4 +220,170 @@ class SimpleElementsTest : GfmRenderingOnlyTestBase() {
         assertEquals(expect, renderedContent)
     }
 
+    @Test
+    fun `unordered list with two items`() {
+        val page = testPage {
+            unorderedList {
+                item { text("Item 1") }
+                item { text("Item 2") }
+            }
+        }
+
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |- Item 1
+                        |- Item 2""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `unordered list with styled text`() {
+        val page = testPage {
+            unorderedList {
+                item {
+                    text("Nobody", styles = setOf(TextStyle.Italic))
+                    text(" tosses a Dwarf!")
+                }
+            }
+        }
+
+        val expect = "//[testPage](test-page.md)\n\n- *Nobody* tosses a Dwarf!"
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `ordered list with two items`() {
+        val page = testPage {
+            orderedList {
+                item { text("Item 1") }
+                item { text("Item 2") }
+            }
+        }
+
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |1. Item 1
+                        |2. Item 2""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `ordered list with nested unordered list`() {
+        val page = testPage {
+            orderedList {
+                item {
+                    text("And another list:")
+                    unorderedList {
+                        item { text("Item 1") }
+                        item { text("Item 2") }
+                    }
+                }
+                item { text("Following item") }
+            }
+        }
+
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |1. And another list:
+                        |   
+                        |   - Item 1
+                        |   - Item 2
+                        |2. Following item""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `ordered list with nested table`() {
+        val page = testPage {
+            orderedList {
+                item {
+                    text("The following table is nested in a list:")
+                    table {
+                        header {
+                            text("Col1")
+                            text("Col2")
+                        }
+                        row {
+                            text("Text1")
+                            text("Text2")
+                        }
+                    }
+                }
+            }
+        }
+
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |1. The following table is nested in a list:
+                        |   | Col1 | Col2 |
+                        |   |---|---|
+                        |   | Text1 | Text2 |""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `three levels of list`() {
+        val page = testPage {
+            unorderedList {
+                item {
+                    text("Level 1")
+                    unorderedList {
+                        item {
+                            text("Level 2")
+                            unorderedList {
+                                item {
+                                    text("Level 3")
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        // Extra newlines are not pretty but do not impact formatting
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |- Level 1
+                        |   
+                        |   - Level 2
+                        |      
+                        |      - Level 3""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
+
+    @Test
+    fun `nested list with no text preceding it`() {
+        val page = testPage {
+            unorderedList {
+                item {
+                    unorderedList {
+                        item {
+                            text("Nested")
+                        }
+                    }
+                }
+            }
+        }
+
+        val expect = """|//[testPage](test-page.md)
+                        |
+                        |- 
+                        |   - Nested""".trimMargin()
+
+        CommonmarkRenderer(context).render(page)
+        assertEquals(expect, renderedContent)
+    }
 }