[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Main thing here is I'm fairly sure that code blocks shouldn't be chunks in a paragraph (or I'm misunderstanding what a paragraph is)



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:95
+// Concatanates whitespace blocks into a single ` `.
+std::string canonicalizeSpaces(std::string Input) {
+  // Goes over the string and preserves only a single ` ` for any whitespace
----------------
is this equivalent to
```
SmallVector<StringRef> Words;
llvm::SplitString(Input, Words);
return llvm::join(Input, " ");
```
?

(That would trim leading/trailing whitespace, but I suspect we actually want that)


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:176
   std::string R;
-  for (const auto &C : Chunks) {
+  auto EnsureWhitespace = [&R]() {
+    // Adds a space for nicer rendering.
----------------
Can we simplify the whitespace model? A lot of this is hacks around code blocks, which shouldn't really be here.

What about:
 - the contents of each chunk never have leading/trailing ws (it's stripped when adding)
 - there's always a space between consecutive chunks


================
Comment at: clang-tools-extra/clangd/FormattedString.h:24
 
 /// A structured string representation that could be converted to markdown or
 /// plaintext upon requrest.
----------------
I don't think this comment captures what this class is about now. (Rewriting the class but leaving the comment intact is suspicious!)

Comment should allude to how blocks make up a document - a RenderableBlock holds rich text and knows how to lay it out. Blocks can be stacked to form a document, etc.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:26
 /// plaintext upon requrest.
-class FormattedString {
+class RenderableBlock {
 public:
----------------
nit: I wouldn't bother with both "renderable" prefix and the `markup` namespace, up to you


================
Comment at: clang-tools-extra/clangd/FormattedString.h:28
 public:
+  virtual std::string renderAsMarkdown() const = 0;
+  virtual std::string renderAsPlainText() const = 0;
----------------
Or just asMarkdown, up to you


================
Comment at: clang-tools-extra/clangd/FormattedString.h:28
 public:
+  virtual std::string renderAsMarkdown() const = 0;
+  virtual std::string renderAsPlainText() const = 0;
----------------
sammccall wrote:
> Or just asMarkdown, up to you
are you sure we want to create all the temporary strings?
Consider `void renderMarkdown(llvm::raw_ostream&)`, and keep `std::string asMarkdown()` on `Document`


================
Comment at: clang-tools-extra/clangd/FormattedString.h:30
+  virtual std::string renderAsPlainText() const = 0;
+
+  virtual ~RenderableBlock() = default;
----------------
renderfortests has gone. What's the plan for testing Hover rendering? (i.e. HoverInfo -> Document)
I guess asserting the plain text and markdown might be enough...

(Did we remove *all* the tests for `present()`in D70911, and I didn't notice? We should restore some based on synthetic HoverInfo)


================
Comment at: clang-tools-extra/clangd/FormattedString.h:35
+/// Represents parts of the markup that can contain strings, like inline code,
+/// code block or plain text.
+/// Only CodeBlocks guarantees conservation of new lines within text. One must
----------------
A code block doesn't go in a paragraph, as it's a block itself.

"A paragraph contains a logical line of rich text. It may be wrapped for display"


================
Comment at: clang-tools-extra/clangd/FormattedString.h:46
+
   /// Append an inline block of C++ code. This translates to the ` block in
   /// markdown.
----------------
nit: while here, I do think `inline block` is misleading - this corresponds to display: inline, not display:inline-block. "inline span"?


================
Comment at: clang-tools-extra/clangd/FormattedString.h:48
   /// markdown.
-  void appendInlineCode(std::string Code);
+  Paragraph &appendInlineCode(std::string Code);
 
----------------
just appendCode?


================
Comment at: clang-tools-extra/clangd/FormattedString.h:69
 
+/// A semantical representation for formattable strings. Allows rendering into
+/// markdown and plaintext.
----------------
semantic -> format-agnostic? This doesn't capture semantics.

formattable strings -> rich text, or structured text?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71248/new/

https://reviews.llvm.org/D71248