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,49 @@ 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; + } + return llvm::StringRef(OS.str()).trim(); } -// 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 +167,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 +306,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 +314,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,10 +542,14 @@ Header.appendCode(*Type); } + bool HasMiddleSection = false; + // 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` if (Parameters && !Parameters->empty()) { + HasMiddleSection = true; markup::BulletList &L = Output.addBulletList(); for (const auto &Param : *Parameters) { std::string Buffer; @@ -556,15 +560,21 @@ } if (Value) { + HasMiddleSection = true; markup::Paragraph &P = Output.addParagraph(); P.appendText("Value ="); P.appendCode(*Value); } - if (!Documentation.empty()) + if (!Documentation.empty()) { + HasMiddleSection = true; Output.addParagraph().appendText(Documentation); + } if (!Definition.empty()) { + // No need to add an extra ruler if hovercard didn't have a middle section. + if (HasMiddleSection) + 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,32 @@ 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(); + // FIXME: Should we trim trailing rulers also in markdown? + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\n\n---"); + EXPECT_EQ(D.asPlainText(), "foo"); } TEST(Document, Heading) { @@ -182,15 +201,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 @@ -1774,7 +1777,8 @@ HI.Name = "foo"; HI.Type = "type"; - EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo` \\: `type`"); + EXPECT_EQ(HI.present().asMarkdown(), + "### variable `foo` \\: `type` \n\n---"); } } // namespace