diff --git a/clang-tools-extra/clangd/FormattedString.h b/clang-tools-extra/clangd/FormattedString.h --- a/clang-tools-extra/clangd/FormattedString.h +++ b/clang-tools-extra/clangd/FormattedString.h @@ -33,6 +33,12 @@ std::string asMarkdown() const; std::string asPlainText() const; + /// Padding information to imitate spacing while rendering for plaintext. As + /// markdown renderers should already add appropriate padding between + /// different blocks. + virtual bool hasTrailingPadding() const { return false; } + virtual bool requiresPrecedingPadding() const { return false; } + virtual ~Block() = default; }; @@ -82,8 +88,8 @@ public: /// Adds a semantical block that will be separate from others. Paragraph &addParagraph(); - /// Inserts a vertical space into the document. - void addSpacer(); + /// Inserts a horizontal separator to the document. + void addRuler(); /// Adds a block of code. This translates to a ``` block in markdown. In plain /// text representation, the code block will be surrounded by newlines. void addCodeBlock(std::string Code, std::string Language = "cpp"); diff --git a/clang-tools-extra/clangd/FormattedString.cpp b/clang-tools-extra/clangd/FormattedString.cpp --- a/clang-tools-extra/clangd/FormattedString.cpp +++ b/clang-tools-extra/clangd/FormattedString.cpp @@ -113,20 +113,75 @@ return Input; } +enum RenderType { + PlainText, + Markdown, +}; std::string renderBlocks(llvm::ArrayRef> Children, - void (Block::*RenderFunc)(llvm::raw_ostream &) const) { + const RenderType RT) { std::string R; llvm::raw_string_ostream OS(R); - for (auto &C : Children) - ((*C).*RenderFunc)(OS); - return llvm::StringRef(OS.str()).trim().str(); + // We initially start with true to not introduce redundant paddings at the + // beginning. + bool HasPadding = true; + switch (RT) { + case PlainText: + llvm::for_each(Children, [&](const std::unique_ptr &C) { + if (C->requiresPrecedingPadding() && !HasPadding) + OS << '\n'; + C->renderPlainText(OS); + HasPadding = C->hasTrailingPadding(); + }); + break; + + case Markdown: + llvm::for_each(Children, [&](const std::unique_ptr &C) { + C->renderMarkdown(OS); + }); + break; + } + + // Post process to get rid of leading/trailing/multiple occurences of vertical + // spaces. + llvm::StringRef Output(OS.str()); + llvm::SmallVector Sections; + + if (RT == PlainText) { + // In plaintext we don't want more than a single empty line between + // sections. Trimming is enough to get rid of leading/trailing ones. + for (auto Secs = Output.trim().split("\n\n"); !Output.empty(); + Secs = Output.split("\n\n")) { + Sections.push_back(Secs.first); + Output = Secs.second.ltrim(); + } + return llvm::join(Sections, "\n\n"); + } + + // For markdown we dont' want multiple rulers. It is safe to search for `---` + // as non-ruler occurences should've been escaped. + Output.split(Sections, "\n---\n", -1, false); + if (!Sections.empty()) { + // We trim in the end so that splitting works for trailing/leading rulers as + // well. + Sections.front() = Sections.front().ltrim(); + Sections.back() = Sections.back().rtrim(); + } + return llvm::join(Sections, "\n---\n"); } -// Puts a vertical space between blocks inside a document. -class Spacer : public Block { +// Seperates two blocks with extra spacing. Note that it might render strangely +// in vscode if the trailing block is a codeblock, see +// https://github.com/microsoft/vscode/issues/88416 for details. +class Ruler : public Block { public: - void renderMarkdown(llvm::raw_ostream &OS) const override { OS << '\n'; } + void renderMarkdown(llvm::raw_ostream &OS) const override { + // Note that we need an extra new line before the ruler, otherwise we might + // make previous block a title instead of introducing a ruler. + OS << "\n---\n"; + } void renderPlainText(llvm::raw_ostream &OS) const override { OS << '\n'; } + + bool hasTrailingPadding() const override { return true; } }; class CodeBlock : public Block { @@ -138,10 +193,15 @@ } void renderPlainText(llvm::raw_ostream &OS) const override { - // In plaintext we want one empty line before and after codeblocks. - OS << '\n' << Contents << "\n\n"; + // In plaintext we print one extra new line to imitate markdown padding. + OS << Contents << "\n\n"; } + // Code blocks requires a padding before the previous block, and has a padding + // afterwards. + bool requiresPrecedingPadding() const override { return true; } + bool hasTrailingPadding() const override { return true; } + CodeBlock(std::string Contents, std::string Language) : Contents(std::move(Contents)), Language(std::move(Language)) {} @@ -272,7 +332,7 @@ return *static_cast(Children.back().get()); } -void Document::addSpacer() { Children.push_back(std::make_unique()); } +void Document::addRuler() { Children.push_back(std::make_unique()); } void Document::addCodeBlock(std::string Code, std::string Language) { Children.emplace_back( @@ -280,11 +340,11 @@ } std::string Document::asMarkdown() const { - return renderBlocks(Children, &Block::renderMarkdown); + return renderBlocks(Children, Markdown); } std::string Document::asPlainText() const { - return renderBlocks(Children, &Block::renderPlainText); + return renderBlocks(Children, PlainText); } BulletList &Document::addBulletList() { diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -542,6 +542,8 @@ Header.appendCode(*Type); } + // Put a linebreak after header to increase readability. + Output.addRuler(); // For functions we display signature in a list form, e.g.: // - `bool param1` // - `int param2 = 5` @@ -565,6 +567,7 @@ Output.addParagraph().appendText(Documentation); if (!Definition.empty()) { + Output.addRuler(); std::string ScopeComment; // Drop trailing "::". if (!LocalScope.empty()) { diff --git a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp --- a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp +++ b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp @@ -136,13 +136,37 @@ EXPECT_EQ(D.asPlainText(), ExpectedText); } -TEST(Document, Spacer) { +TEST(Document, Ruler) { Document D; D.addParagraph().appendText("foo"); - D.addSpacer(); + D.addRuler(); + + // Ruler followed by paragraph. D.addParagraph().appendText("bar"); - EXPECT_EQ(D.asMarkdown(), "foo \n\nbar"); + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\nbar"); + EXPECT_EQ(D.asPlainText(), "foo\n\nbar"); + + D = Document(); + D.addParagraph().appendText("foo"); + D.addRuler(); + D.addCodeBlock("bar"); + // Ruler followed by a codeblock. + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\n```cpp\nbar\n```"); EXPECT_EQ(D.asPlainText(), "foo\n\nbar"); + + // Ruler followed by another ruler + D = Document(); + D.addParagraph().appendText("foo"); + D.addRuler(); + D.addRuler(); + EXPECT_EQ(D.asMarkdown(), "foo"); + EXPECT_EQ(D.asPlainText(), "foo"); + + // Multiple rulers between blocks + D.addRuler(); + D.addParagraph().appendText("foo"); + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\nfoo"); + EXPECT_EQ(D.asPlainText(), "foo\n\nfoo"); } TEST(Document, Heading) { @@ -182,15 +206,11 @@ foo ```)md"; EXPECT_EQ(D.asMarkdown(), ExpectedMarkdown); - // FIXME: we shouldn't have 2 empty lines in between. A solution might be - // having a `verticalMargin` method for blocks, and let container insert new - // lines according to that before/after blocks. ExpectedPlainText = R"pt(foo bar baz - foo)pt"; EXPECT_EQ(D.asPlainText(), ExpectedPlainText); } diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp --- a/clang-tools-extra/clangd/unittests/HoverTests.cpp +++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp @@ -1700,6 +1700,7 @@ HI.NamespaceScope.emplace(); }, R"(class foo + documentation template class Foo {})", @@ -1723,6 +1724,7 @@ HI.Definition = "ret_type foo(params) {}"; }, R"(function foo → ret_type + - - type - type foo @@ -1741,6 +1743,7 @@ HI.Definition = "def"; }, R"(variable foo : type + Value = value // In test::bar @@ -1777,6 +1780,30 @@ EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo` \\: `type`"); } +// This is a separate test as rulers behave differently in markdown vs +// plaintext. +TEST(Hover, PresentRulers) { + HoverInfo HI; + HI.Kind = index::SymbolKind::Variable; + HI.Name = "foo"; + HI.Value = "val"; + HI.Definition = "def"; + + EXPECT_EQ(HI.present().asMarkdown(), R"md(### variable `foo` + +--- +Value \= `val` + +--- +```cpp +def +```)md"); + EXPECT_EQ(HI.present().asPlainText(), R"pt(variable foo + +Value = val + +def)pt"); +} } // namespace } // namespace clangd } // namespace clang