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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits