kadircet updated this revision to Diff 237934.
kadircet added a comment.
- Per offline discussions, do post processing to get rid of multiple ruler
occurences.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72622/new/
https://reviews.llvm.org/D72622
Files:
clang-tools-extra/clangd/FormattedString.cpp
clang-tools-extra/clangd/FormattedString.h
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1700,6 +1700,7 @@
HI.NamespaceScope.emplace();
},
R"(class foo
+
documentation
template <typename T, typename C = bool> 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
Index: clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ 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);
}
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ 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()) {
Index: clang-tools-extra/clangd/FormattedString.h
===================================================================
--- clang-tools-extra/clangd/FormattedString.h
+++ 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");
Index: clang-tools-extra/clangd/FormattedString.cpp
===================================================================
--- clang-tools-extra/clangd/FormattedString.cpp
+++ clang-tools-extra/clangd/FormattedString.cpp
@@ -113,20 +113,75 @@
return Input;
}
+enum RenderType {
+ PlainText,
+ Markdown,
+};
std::string renderBlocks(llvm::ArrayRef<std::unique_ptr<Block>> 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<Block> &C) {
+ if (C->requiresPrecedingPadding() && !HasPadding)
+ OS << '\n';
+ C->renderPlainText(OS);
+ HasPadding = C->hasTrailingPadding();
+ });
+ break;
+
+ case Markdown:
+ llvm::for_each(Children, [&](const std::unique_ptr<Block> &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<llvm::StringRef, 3> 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<Paragraph *>(Children.back().get());
}
-void Document::addSpacer() { Children.push_back(std::make_unique<Spacer>()); }
+void Document::addRuler() { Children.push_back(std::make_unique<Ruler>()); }
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() {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
