fix(review): close all 2026-06-03 codebase-review findings (R1-R18)
Resolves the 18 findings from the parallel codebase review, tracked in development/vimwiki-gap.md. Correctness / perf: - Wiki.config -> Arc<WikiConfig> so cloning a Wiki is a refcount bump (R5) - WorkspaceIndex::remove is no longer O(n^2): a per-source contributions map limits the scan to buckets the source actually wrote into (R6) - render_color now expands color_tag_template (__STYLE__/__CONTENT__), consuming the previously-dead field; ColorNode documented as an extension point; 3 renderer tests added (R3/R4) - wiki_root_for returns empty/nil on no-match instead of falling back to the first wiki (R2); auto_header honours links_space_char (R7) Cleanup / dedup: - Remove dead #[allow(dead_code)] stubs + uncalled pub helpers, narrow imports (R10/R11) - Dedup span_of_inline x3 -> InlineNode::span() (R12) - diary_step single read lock; page_captions single pass (R13) - Lua auto_header loop -> wiki_list(); detect_current_symbol cleanup (R14/R18) Client / docs: - :VimwikiNormalizeLink Vim cmds -> <q-args> (R17); ftplugin header fix (R16); vars.vim multi-wiki limitation documented (R15) - Document 19 config options in README.md + doc/nuwiki.txt; fix list_margin/shiftwidth doc and stale comments (R1/R9) - R8 investigated, confirmed not a real bug (documented) Verified: Neovim harness 307, Vim harness 301/18/21, Rust suite 568, all 0 failed; clippy clean; fresh parallel-agent audit found no regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -41,6 +41,31 @@ pub enum InlineNode {
|
||||
SoftBreak(SoftBreakNode),
|
||||
}
|
||||
|
||||
impl InlineNode {
|
||||
/// The source [`Span`] this node covers. Every variant wraps a node with
|
||||
/// its own `span` field; this is the one place that maps variant → span.
|
||||
pub fn span(&self) -> Span {
|
||||
match self {
|
||||
InlineNode::Text(n) => n.span,
|
||||
InlineNode::Bold(n) => n.span,
|
||||
InlineNode::Italic(n) => n.span,
|
||||
InlineNode::BoldItalic(n) => n.span,
|
||||
InlineNode::Strikethrough(n) => n.span,
|
||||
InlineNode::Code(n) => n.span,
|
||||
InlineNode::Superscript(n) => n.span,
|
||||
InlineNode::Subscript(n) => n.span,
|
||||
InlineNode::MathInline(n) => n.span,
|
||||
InlineNode::Keyword(n) => n.span,
|
||||
InlineNode::Color(n) => n.span,
|
||||
InlineNode::WikiLink(n) => n.span,
|
||||
InlineNode::ExternalLink(n) => n.span,
|
||||
InlineNode::Transclusion(n) => n.span,
|
||||
InlineNode::RawUrl(n) => n.span,
|
||||
InlineNode::SoftBreak(n) => n.span,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct TextNode {
|
||||
pub span: Span,
|
||||
@@ -106,6 +131,15 @@ pub struct KeywordNode {
|
||||
pub keyword: Keyword,
|
||||
}
|
||||
|
||||
/// A named colour span (`color` is a `color_dic` key, `children` the wrapped
|
||||
/// inline content).
|
||||
///
|
||||
/// This is an **extension point**, not a node the vimwiki parser emits: in
|
||||
/// the editor, colour comes from the `:Colorize` family writing literal
|
||||
/// `<span style="color:…">` markup, which round-trips through export via
|
||||
/// `valid_html_tags`. `ColorNode` exists so an alternate syntax (or a future
|
||||
/// `[color]` markup) can feed the renderer's `color_dic` / `color_tag_template`
|
||||
/// path directly; see `HtmlRenderer::render_color`.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct ColorNode {
|
||||
pub span: Span,
|
||||
|
||||
@@ -46,6 +46,11 @@ pub struct HtmlRenderer {
|
||||
/// names fall through to the default `class="color-<name>"`
|
||||
/// rendering. Matches vimwiki's `color_dic`.
|
||||
colors: HashMap<String, String>,
|
||||
/// vimwiki `color_tag_template`: the HTML emitted for a colour span whose
|
||||
/// name resolves in `colors`. `__STYLE__` expands to the inline style
|
||||
/// (`color:<css>`) and `__CONTENT__` to the rendered inner HTML. The
|
||||
/// default matches upstream's `<span style="…">…</span>`.
|
||||
color_template: String,
|
||||
/// vimwiki `html_header_numbering`: the heading level at which automatic
|
||||
/// section numbering begins (`0` = off, the default). When `>= 1`, every
|
||||
/// heading at that level or deeper is prefixed with a dotted section
|
||||
@@ -82,6 +87,7 @@ impl HtmlRenderer {
|
||||
template: None,
|
||||
vars: HashMap::new(),
|
||||
colors: HashMap::new(),
|
||||
color_template: "<span style=\"__STYLE__\">__CONTENT__</span>".to_string(),
|
||||
header_numbering: 0,
|
||||
header_numbering_sym: String::new(),
|
||||
valid_html_tags: Vec::new(),
|
||||
@@ -156,6 +162,18 @@ impl HtmlRenderer {
|
||||
self
|
||||
}
|
||||
|
||||
/// Override the colour-span template (vimwiki `color_tag_template`).
|
||||
/// `__STYLE__` is replaced with the resolved `color:<css>` style and
|
||||
/// `__CONTENT__` with the rendered inner HTML. An empty string keeps the
|
||||
/// built-in default.
|
||||
pub fn with_color_template(mut self, template: impl Into<String>) -> Self {
|
||||
let template = template.into();
|
||||
if !template.is_empty() {
|
||||
self.color_template = template;
|
||||
}
|
||||
self
|
||||
}
|
||||
|
||||
/// Enable vimwiki-style HTML section numbering. `level` is the heading
|
||||
/// level at which numbering starts (`0` disables it); `sym` is appended
|
||||
/// after the number. Mirrors `html_header_numbering` /
|
||||
@@ -311,7 +329,11 @@ impl HtmlRenderer {
|
||||
|
||||
fn render_paragraph(&self, n: &ParagraphNode, w: &mut dyn Write) -> io::Result<()> {
|
||||
w.write_all(b"<p>")?;
|
||||
let sb = if self.text_ignore_newline { " " } else { "<br />" };
|
||||
let sb = if self.text_ignore_newline {
|
||||
" "
|
||||
} else {
|
||||
"<br />"
|
||||
};
|
||||
self.render_inlines_break(&n.children, sb, w)?;
|
||||
w.write_all(b"</p>\n")
|
||||
}
|
||||
@@ -389,7 +411,11 @@ impl HtmlRenderer {
|
||||
}
|
||||
None => w.write_all(b"<li>")?,
|
||||
}
|
||||
let sb = if self.list_ignore_newline { " " } else { "<br />" };
|
||||
let sb = if self.list_ignore_newline {
|
||||
" "
|
||||
} else {
|
||||
"<br />"
|
||||
};
|
||||
self.render_inlines_break(&n.children, sb, w)?;
|
||||
if let Some(sublist) = &n.sublist {
|
||||
w.write_all(b"\n")?;
|
||||
@@ -664,17 +690,32 @@ impl HtmlRenderer {
|
||||
}
|
||||
|
||||
fn render_color(&self, n: &ColorNode, w: &mut dyn Write) -> io::Result<()> {
|
||||
// Render the inner content up front so it can be slotted into the
|
||||
// colour template.
|
||||
let mut content = Vec::new();
|
||||
self.render_inlines(&n.children, &mut content)?;
|
||||
let content = String::from_utf8_lossy(&content);
|
||||
|
||||
if let Some(css) = self.colors.get(&n.color) {
|
||||
w.write_all(b"<span style=\"color:")?;
|
||||
write_escaped(css, w)?;
|
||||
w.write_all(b"\">")?;
|
||||
// Known colour name: expand `color_tag_template` with the resolved
|
||||
// inline style and rendered content.
|
||||
let mut style = b"color:".to_vec();
|
||||
write_escaped(css, &mut style)?;
|
||||
let style = String::from_utf8_lossy(&style);
|
||||
let html = self
|
||||
.color_template
|
||||
.replace("__STYLE__", &style)
|
||||
.replace("__CONTENT__", &content);
|
||||
w.write_all(html.as_bytes())
|
||||
} else {
|
||||
// Unknown name: fall back to a class hook the stylesheet can target
|
||||
// (the template only models the inline-style case).
|
||||
w.write_all(b"<span class=\"color-")?;
|
||||
write_escaped(&n.color, w)?;
|
||||
w.write_all(b"\">")?;
|
||||
w.write_all(content.as_bytes())?;
|
||||
w.write_all(b"</span>")
|
||||
}
|
||||
self.render_inlines(&n.children, w)?;
|
||||
w.write_all(b"</span>")
|
||||
}
|
||||
|
||||
fn render_wikilink(&self, n: &WikiLinkNode, w: &mut dyn Write) -> io::Result<()> {
|
||||
|
||||
@@ -26,12 +26,6 @@ impl SyntaxRegistry {
|
||||
self.plugins.push(Arc::new(plugin));
|
||||
}
|
||||
|
||||
/// Register a pre-arc'd plugin. Useful when the same plugin instance
|
||||
/// also needs to be held elsewhere.
|
||||
pub fn register_arc(&mut self, plugin: Arc<dyn SyntaxPlugin>) {
|
||||
self.plugins.push(plugin);
|
||||
}
|
||||
|
||||
/// Look up a plugin by its `id`.
|
||||
pub fn get(&self, id: &str) -> Option<&dyn SyntaxPlugin> {
|
||||
self.plugins
|
||||
|
||||
@@ -37,9 +37,9 @@ use crate::ast::{
|
||||
BlockNode, BlockquoteNode, BoldNode, CodeNode, CommentNode, DefinitionItemNode,
|
||||
DefinitionListNode, DocumentNode, ErrorNode, ExternalLinkNode, HeadingNode, HorizontalRuleNode,
|
||||
InlineNode, ItalicNode, KeywordNode, LinkKind, LinkTarget, ListItemNode, ListNode, ListSymbol,
|
||||
MathBlockNode, MathInlineNode, PageMetadata, ParagraphNode, PreformattedNode, RawUrlNode, Span,
|
||||
StrikethroughNode, SubscriptNode, SuperscriptNode, TableCellNode, TableNode, TableRowNode,
|
||||
SoftBreakNode, TagNode, TagScope, TextNode, TransclusionNode, WikiLinkNode,
|
||||
MathBlockNode, MathInlineNode, PageMetadata, ParagraphNode, PreformattedNode, RawUrlNode,
|
||||
SoftBreakNode, Span, StrikethroughNode, SubscriptNode, SuperscriptNode, TableCellNode,
|
||||
TableNode, TableRowNode, TagNode, TagScope, TextNode, TransclusionNode, WikiLinkNode,
|
||||
};
|
||||
use crate::syntax::{Parser, TokenStream};
|
||||
|
||||
@@ -1426,30 +1426,9 @@ fn strip_directory(s: &str) -> (String, bool) {
|
||||
// ===== Span helpers =====
|
||||
|
||||
fn span_start_of_inline(node: &InlineNode) -> Option<crate::ast::Position> {
|
||||
Some(span_of_inline(node).start)
|
||||
Some(node.span().start)
|
||||
}
|
||||
|
||||
fn span_end_of_inline(node: &InlineNode) -> Option<crate::ast::Position> {
|
||||
Some(span_of_inline(node).end)
|
||||
}
|
||||
|
||||
fn span_of_inline(node: &InlineNode) -> Span {
|
||||
match node {
|
||||
InlineNode::Text(n) => n.span,
|
||||
InlineNode::Bold(n) => n.span,
|
||||
InlineNode::Italic(n) => n.span,
|
||||
InlineNode::BoldItalic(n) => n.span,
|
||||
InlineNode::Strikethrough(n) => n.span,
|
||||
InlineNode::Code(n) => n.span,
|
||||
InlineNode::Superscript(n) => n.span,
|
||||
InlineNode::Subscript(n) => n.span,
|
||||
InlineNode::MathInline(n) => n.span,
|
||||
InlineNode::Keyword(n) => n.span,
|
||||
InlineNode::Color(n) => n.span,
|
||||
InlineNode::WikiLink(n) => n.span,
|
||||
InlineNode::ExternalLink(n) => n.span,
|
||||
InlineNode::Transclusion(n) => n.span,
|
||||
InlineNode::RawUrl(n) => n.span,
|
||||
InlineNode::SoftBreak(n) => n.span,
|
||||
}
|
||||
Some(node.span().end)
|
||||
}
|
||||
|
||||
@@ -328,6 +328,64 @@ fn template_substitutes_vimwiki_percent_placeholders() {
|
||||
assert!(!out.contains('%'), "no placeholder left: {out}");
|
||||
}
|
||||
|
||||
// ===== Colour spans =====
|
||||
|
||||
// `ColorNode` is an extension point the vimwiki parser never emits, so build
|
||||
// a document around one by hand and render it directly.
|
||||
fn doc_with_color(color: &str) -> DocumentNode {
|
||||
use nuwiki_core::ast::{ColorNode, ParagraphNode};
|
||||
let color_node = InlineNode::Color(ColorNode {
|
||||
span: Span::default(),
|
||||
color: color.to_string(),
|
||||
children: vec![InlineNode::Text(TextNode {
|
||||
span: Span::default(),
|
||||
content: "hi".to_string(),
|
||||
})],
|
||||
});
|
||||
DocumentNode {
|
||||
children: vec![BlockNode::Paragraph(ParagraphNode {
|
||||
span: Span::default(),
|
||||
children: vec![color_node],
|
||||
})],
|
||||
..DocumentNode::default()
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn color_dic_name_uses_inline_style_via_default_template() {
|
||||
let doc = doc_with_color("red");
|
||||
let renderer =
|
||||
HtmlRenderer::new().with_colors([("red".to_string(), "crimson".to_string())].into());
|
||||
let out = renderer.render_to_string(&doc).unwrap();
|
||||
assert!(
|
||||
out.contains("<span style=\"color:crimson\">hi</span>"),
|
||||
"got: {out}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn color_tag_template_override_is_honored() {
|
||||
let doc = doc_with_color("red");
|
||||
let renderer = HtmlRenderer::new()
|
||||
.with_colors([("red".to_string(), "crimson".to_string())].into())
|
||||
.with_color_template("<em data-style=\"__STYLE__\">__CONTENT__</em>");
|
||||
let out = renderer.render_to_string(&doc).unwrap();
|
||||
assert!(
|
||||
out.contains("<em data-style=\"color:crimson\">hi</em>"),
|
||||
"got: {out}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unknown_color_name_falls_back_to_class() {
|
||||
let doc = doc_with_color("plaid");
|
||||
let out = HtmlRenderer::new().render_to_string(&doc).unwrap();
|
||||
assert!(
|
||||
out.contains("<span class=\"color-plaid\">hi</span>"),
|
||||
"got: {out}"
|
||||
);
|
||||
}
|
||||
|
||||
// ===== End-to-end smoke =====
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user