diff --git a/CHANGELOG.md b/CHANGELOG.md index 36ec9d2..45a4122 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ matching the pushed tag as that GitHub release's notes, so add an entry here bef ## Unreleased +- Comment cards on a table now sit beside the row they belong to in Live Preview instead of stacking at the top of the table, and the new-comment composer opens beside its row too ([#79](https://github.com/kylemcd/obsidian-document-comments/issues/79)). +- Commenting on a whole table row, a whole table, a selection that crosses a cell border, or a selection that runs in from the text around a table no longer breaks the table. Comment markers are kept inside the cell borders, off the blank line a table needs above it, and on both sides of an escaped pipe (`\|`), where they can't stop the block rendering as a table; a selection holding nothing but borders is now refused with an explanation. +- Hovering a table comment's card highlights its text, and hovering the text raises its card — both of which previously did nothing inside a Live Preview table. +- Added a repair for tables an older comment already broke. The comment's card, which sits beside the damage, says so and offers a one-click **Repair**; the **Repair table comments in this note** command fixes every one at once, which is the way out when several anchors broke the same table. Both land as a single undo step, and nothing is rewritten unless you ask. + ## 0.1.15 - Fixed emoji reactions added to a reply being attached to the first comment in the thread. Reply reactions now remain with the thread entry where they were added, while existing reaction data remains compatible. diff --git a/src/editor/commands.ts b/src/editor/commands.ts index 6ccd543..9f966d6 100644 --- a/src/editor/commands.ts +++ b/src/editor/commands.ts @@ -16,6 +16,7 @@ import { computeToggleReaction, findHighlightAtSelection, } from "./edits"; +import { computeRepairTableAnchors } from "./table-repair"; /** Create a comment/highlight, or update an exact matching highlight. Ok carries * the affected id, or an empty string when a disabled blank submit writes nothing. */ @@ -46,6 +47,14 @@ export const addComment = ( }); }; +/** Move broken table anchors back inside their cells. `only` limits it to one + * comment (the card's Repair action); without it, the whole note. */ +export const repairTableAnchors = (view: EditorView, only?: ReadonlySet): Result => { + return computeRepairTableAnchors(view.state.doc.toString(), only).map((changes) => { + if (changes.length > 0) view.dispatch({ changes }); + }); +}; + export const appendReply = (view: EditorView, id: string, text: string, author: string): Result => { return computeAppendReply(view.state.doc.toString(), id, { createdAt: now(), author, text }).map((changes) => { view.dispatch({ changes }); diff --git a/src/editor/edits.ts b/src/editor/edits.ts index b42585f..52bdd8b 100644 --- a/src/editor/edits.ts +++ b/src/editor/edits.ts @@ -1,8 +1,9 @@ import { Result } from "better-result"; -import { CommentData, ParsedComment, Reaction, ReactionTarget } from "../format/types"; +import { CommentData, ParsedComment, Reaction, ReactionTarget, TextRange } from "../format/types"; import { anchorRange, isAnchored, isHighlight, isInFencedCode, parseComments } from "../format/parse"; import { codeSelectionTarget, isCodeComment, resolveCodeAnchor } from "../format/code-anchor"; import { closeMarker, openMarker, serializeBody } from "../format/serialize"; +import { clampToTableCells } from "../format/table"; /** A document edit in original coordinates (matches CodeMirror's ChangeSpec shape). */ export type Change = { @@ -80,7 +81,8 @@ export const computeAddComment = ( if (isInFencedCode(doc, from) || isInFencedCode(doc, to - 1)) { return computeAddCodeComment(doc, from, to, input); } - ({ from, to } = expandInlineCodeSelection(doc, from, to)); + ({ from, to } = anchorSelection(doc, from, to)); + if (to === from) return Result.err("Select the text inside a table cell, not its borders."); const quote = doc.slice(from, to); const data: CommentData = { @@ -117,7 +119,7 @@ export const findHighlightAtSelection = (doc: string, from: number, to: number): ); } - ({ from, to } = expandInlineCodeSelection(doc, from, to)); + ({ from, to } = anchorSelection(doc, from, to)); return ( comments.find((comment) => { if (isCodeComment(comment)) return false; @@ -156,6 +158,14 @@ const computeAddCodeComment = ( ]); }; +/** Normalize a raw selection to the range we actually wrap in markers. Both the + * write and the "is this already a highlight?" lookup have to agree, or running + * Add comment twice on the same text stops finding the comment it just made. */ +const anchorSelection = (doc: string, from: number, to: number): TextRange => { + const cell = clampToTableCells(doc, from, to); + return expandInlineCodeSelection(doc, cell.from, cell.to); +}; + /** HTML comments inside a Markdown code span render as literal code. When a * selection is within one inline-code token, anchor the whole token so the * comment markers remain invisible outside its backtick delimiters. */ diff --git a/src/editor/margin.ts b/src/editor/margin.ts index ccecb5d..0569310 100644 --- a/src/editor/margin.ts +++ b/src/editor/margin.ts @@ -1,10 +1,13 @@ import { Notice, editorInfoField } from "obsidian"; import { Result } from "better-result"; +import type { Text } from "@codemirror/state"; import { EditorView, PluginValue, ViewPlugin } from "@codemirror/view"; -import { ParsedComment } from "../format/types"; +import { ParsedComment, TextRange } from "../format/types"; import { anchorRange, hasMarginAnchor } from "../format/parse"; import { isCodeComment, resolveCodeAnchor } from "../format/code-anchor"; import { commentField } from "./state"; +import { setActiveTableComment, tableCellsForRanges, tableCommentAtPoint } from "./table-highlights"; +import { brokenTableAnchors } from "./table-repair"; import { commentConfig } from "./config"; import { Draft, clearDraft, draftField } from "./draft"; import { Card, CardCallbacks, CardView } from "../ui/card"; @@ -15,6 +18,7 @@ import { deleteComment, deleteEntry, editEntry, + repairTableAnchors, setResolved, toggleReaction, } from "./commands"; @@ -24,6 +28,9 @@ import { CARD_GAP, FLASH_MS } from "../ui/constants"; import { buildDraftComposer } from "../ui/draft-composer"; import { EmptySubmitAction } from "../ui/draft-behavior"; +/** Key for the draft composer's anchor in the table-cell lookup. */ +const DRAFT_ANCHOR = "draft:composer"; + /** Editor-margin writes go through a live CodeMirror view (no I/O), so the only * failure is a compute error — surface it as a notice rather than swallowing it. */ const notifyErr = (result: Result): Result => { @@ -48,6 +55,9 @@ class MarginView implements PluginValue { private animFrames = 0; private animatingLoop = false; private destroyed = false; + /** Broken table anchors and the document they were found in. Only an edit can + * change them, while reconcile runs on every cursor move and scroll too. */ + private brokenAnchors: { doc: Text; ids: Set } | null = null; constructor(private view: EditorView) { this.container = view.dom.createDiv("doc-comment-margin"); @@ -85,6 +95,7 @@ class MarginView implements PluginValue { toggleReaction: ({ id, entry, emoji }) => notifyErr(toggleReaction({ view, id, entry, emoji, author: this.cb.getAuthor() })), openInSidebar: (id) => view.state.facet(commentConfig).openInSidebar?.(id), + repairTableAnchor: (id) => notifyErr(repairTableAnchors(view, new Set([id]))), }; view.scrollDOM.addEventListener("scroll", this.scrollHandler, { passive: true }); @@ -93,6 +104,7 @@ class MarginView implements PluginValue { view.contentDOM.addEventListener("mousedown", this.onContentMouseDown); view.contentDOM.addEventListener("mouseover", this.onContentMouseOver); view.contentDOM.addEventListener("mouseout", this.onContentMouseOut); + view.contentDOM.addEventListener("mousemove", this.onContentMouseMove); this.reconcile(); this.requestReposition(); @@ -137,6 +149,7 @@ class MarginView implements PluginValue { this.view.contentDOM.removeEventListener("mousedown", this.onContentMouseDown); this.view.contentDOM.removeEventListener("mouseover", this.onContentMouseOver); this.view.contentDOM.removeEventListener("mouseout", this.onContentMouseOut); + this.view.contentDOM.removeEventListener("mousemove", this.onContentMouseMove); this.removeDraftOutside(); for (const card of this.cards.values()) card.destroy(); this.cards.clear(); @@ -170,19 +183,30 @@ class MarginView implements PluginValue { } const cardView = this.cardView(); + const broken = comments.length > 0 ? this.brokenTableAnchorIds() : new Set(); for (const c of comments) { const existing = this.cards.get(c.id); if (!existing) { const card = new Card(c, this.cb, cardView); this.cards.set(c.id, card); this.container.appendChild(card.el); + card.setTableAnchorBroken(broken.has(c.id)); } else { if (existing.signature !== cardSignature(c)) existing.update(c); existing.refreshAuthorColors(); + existing.setTableAnchorBroken(broken.has(c.id)); } } } + private brokenTableAnchorIds(): Set { + const doc = this.view.state.doc; + if (this.brokenAnchors?.doc === doc) return this.brokenAnchors.ids; + const comments = this.view.state.field(commentField, false)?.comments ?? []; + this.brokenAnchors = { doc, ids: brokenTableAnchors(doc.toString(), comments) }; + return this.brokenAnchors.ids; + } + private cardView(): CardView { const cfg = this.view.state.facet(commentConfig); // Resolve links/embeds in comment text against THIS editor's file, not the @@ -209,28 +233,44 @@ class MarginView implements PluginValue { // absolutely positioned, so a top write can't change any height. const placements: Array<{ el: HTMLElement; top: number; height: number }> = []; - const place = (el: HTMLElement, pos: number) => { - const coords = this.view.coordsAtPos(pos); - if (!coords) { + // Live Preview renders a whole table as ONE block widget, and coordsAtPos + // reports that widget's rect for every position inside it — so measuring a + // table anchor that way piles every card in the table onto its top edge + // (issue #79). Where the anchor resolves to a rendered cell, measure the + // cell; everywhere else coordsAtPos is still the right answer. + const place = (el: HTMLElement, pos: number, cell: HTMLElement | undefined) => { + const rect = cell?.getBoundingClientRect() ?? this.view.coordsAtPos(pos); + if (!rect) { el.addClass("dc-offscreen"); return; } el.removeClass("dc-offscreen"); if (el.offsetHeight === 0) return; // hidden (e.g. resolved) - placements.push({ el, top: coords.top - editorTop, height: el.offsetHeight }); + placements.push({ el, top: rect.top - editorTop, height: el.offsetHeight }); }; const doc = this.view.state.doc.toString(); - for (const c of this.comments()) { + const comments = this.comments(); + const anchors = new Map( + comments.flatMap((c): Array<[string, TextRange]> => { + // A code comment's card aligns to its target line, not the block top. + const range = isCodeComment(c) ? resolveCodeAnchor(doc, c) : anchorRange(c); + return range ? [[c.id, range]] : []; + }), + ); + // Comment ids are alphanumeric, so this key can never collide with one. + if (draft) anchors.set(DRAFT_ANCHOR, { from: draft.from, to: draft.to }); + const cells = tableCellsForRanges(this.view, doc, anchors); + + for (const c of comments) { const card = this.cards.get(c.id); if (!card) continue; - // A code comment's card aligns to its target line, not the block top. - const range = isCodeComment(c) ? resolveCodeAnchor(doc, c) : anchorRange(c); - if (range) place(card.el, range.from); + const range = anchors.get(c.id); + if (range) place(card.el, range.from, cells.get(c.id)); else card.el.addClass("dc-offscreen"); // orphaned (e.g. the commented code changed) } - if (draft && this.draftEl) place(this.draftEl, draft.from); + if (draft && this.draftEl) place(this.draftEl, draft.from, cells.get(DRAFT_ANCHOR)); const tops = stackTops(placements, CARD_GAP); placements.forEach((p, i) => p.el.setCssStyles({ top: `${tops[i]}px` })); @@ -318,6 +358,10 @@ class MarginView implements PluginValue { this.cards.get(id)?.setActive(true); this.markHighlight(id, true); } + // Text inside a Live-Preview table is painted with the CSS Custom Highlight + // API, which has no element to carry `is-active` — the painter re-registers + // the range under its active name instead. + setActiveTableComment(this.view, id); } private markHighlight(id: string, active: boolean): void { @@ -357,7 +401,7 @@ class MarginView implements PluginValue { } private onContentMouseDown = (e: MouseEvent): void => { - const id = closestSpanId(e.target); + const id = closestSpanId(e.target) ?? this.tableIdAt(e); if (id) this.setActive(id); }; @@ -366,12 +410,27 @@ class MarginView implements PluginValue { if (id) this.setActive(id); }; + /** A table cell is usually one text node, so `mouseover` never fires as the + * pointer crosses into the commented words inside it. Track the pointer while + * it is over a table and hit-test the painted ranges directly. */ + private onContentMouseMove = (e: MouseEvent): void => { + if (!(e.target instanceof Element) || !e.target.closest(".cm-table-widget")) return; + this.setActive(this.tableIdAt(e)); + }; + + private tableIdAt(e: MouseEvent): string | null { + if (!(e.target instanceof Element) || !e.target.closest(".cm-table-widget")) return null; + return tableCommentAtPoint(this.view, e.clientX, e.clientY); + } + private onContentMouseOut = (e: MouseEvent): void => { - const span = e.target instanceof Element ? e.target.closest(".doc-comment-span") : null; - if (!span) return; + // A table widget counts as one hover region: its highlights are painted + // ranges rather than elements, so leaving the table is what ends the hover. + const region = e.target instanceof Element ? e.target.closest(".doc-comment-span, .cm-table-widget") : null; + if (!region) return; // Ignore moves that stay within the same highlight element (avoids flicker). const to = e.relatedTarget; - if (to instanceof Node && span.contains(to)) return; + if (to instanceof Node && region.contains(to)) return; this.setActive(null); }; } diff --git a/src/editor/table-highlights.ts b/src/editor/table-highlights.ts index 01b63d7..d895b6b 100644 --- a/src/editor/table-highlights.ts +++ b/src/editor/table-highlights.ts @@ -1,15 +1,33 @@ import { ViewPlugin, ViewUpdate } from "@codemirror/view"; import type { EditorView } from "@codemirror/view"; import { anchorRange } from "../format/parse"; -import type { ParsedComment } from "../format/types"; +import type { ParsedComment, TextRange } from "../format/types"; +import { + type SourceLine, + type SourceTable, + lineIndexAt, + sourceLines, + sourceTables, + tableColumnAt, +} from "../format/table"; import { commentConfig, type CommentConfig } from "./config"; import { getComments } from "./state"; import { authorColorCss, creatorForComment, type ResolvedAuthorColor } from "../author-colors"; -export type TableHighlightTarget = { +/** A rendered table cell, addressed the way the widget's DOM is laid out. */ +export type TableCellTarget = { table: number; row: number; column: number; +}; + +/** The cell a source range starts in. `whole` says whether the range also ENDS + * there — an anchor spanning two rows has no single cell to paint, but its card + * still belongs beside the row it starts on rather than at the top of the table. */ +export type AnchorCell = TableCellTarget & { whole: boolean }; + +export type TableHighlightTarget = TableCellTarget & { + id: string; quote: string; resolved: boolean; author: string | null; @@ -18,11 +36,11 @@ export type TableHighlightTarget = { type TableColorRanges = { color: ResolvedAuthorColor; resolved: boolean; + active: boolean; ranges: Range[]; }; type TableRanges = Map; type BrowserWindow = NonNullable; -type SourceTable = { start: number; end: number; from: number; to: number }; // `CSS.highlights` is a per-DOCUMENT global registry, so every editor view in a // window must merge its ranges before we set it. Keyed by document (pop-out @@ -31,14 +49,18 @@ const rangesByDocument = new WeakMap>(); const namesByDocument = new WeakMap>(); const stylesByDocument = new WeakMap(); -export const tableHighlightName = (color: ResolvedAuthorColor, resolved: boolean): string => { - return `document-comments-table-${resolved ? "resolved" : "open"}-${color ? color.slice(1) : "default"}`; +export const tableHighlightName = (color: ResolvedAuthorColor, resolved: boolean, active = false): string => { + const state = `${resolved ? "resolved" : "open"}${active ? "-active" : ""}`; + return `document-comments-table-${state}-${color ? color.slice(1) : "default"}`; }; -export const tableHighlightRule = (color: ResolvedAuthorColor, resolved: boolean): string => { - const name = tableHighlightName(color, resolved); +export const tableHighlightRule = (color: ResolvedAuthorColor, resolved: boolean, active = false): string => { + const name = tableHighlightName(color, resolved, active); const cssColor = authorColorCss(color); - const background = resolved ? "transparent" : `color-mix(in srgb, ${cssColor} 18%, transparent)`; + // Mirror the DOM highlight's 18% / 38% pair, so hovering a card emphasizes + // table text exactly as much as it emphasizes prose. + const mix = (percent: number) => `color-mix(in srgb, ${cssColor} ${percent}%, transparent)`; + const background = active ? mix(38) : resolved ? "transparent" : mix(18); const decoration = resolved ? "dashed" : "solid"; return `::highlight(${name}) { background-color: ${background}; text-decoration-line: underline; text-decoration-style: ${decoration}; text-decoration-color: ${cssColor}; }`; }; @@ -46,40 +68,113 @@ export const tableHighlightRule = (color: ResolvedAuthorColor, resolved: boolean /** Map source comment anchors to the rendered table/cell that owns them. */ export const tableHighlightTargets = (doc: string, comments: ParsedComment[]): TableHighlightTarget[] => { const lines = sourceLines(doc); - const targets: TableHighlightTarget[] = []; const tables = sourceTables(lines); - - for (const [table, { start, end }] of tables.entries()) { - for (const comment of comments) { - const range = anchorRange(comment); - if (!range) continue; - const lineIndex = lines.findIndex((line, index) => { - if (index === start + 1 || index < start || index >= end) return false; - return range.from >= line.from && range.to <= line.to; - }); - const line = lines[lineIndex]; - if (!line) continue; - - const quote = doc.slice(range.from, range.to); - if (!quote.trim()) continue; - targets.push({ - table, - row: lineIndex === start ? 0 : lineIndex - start - 1, - column: tableColumnAt(line.text, range.from - line.from), + if (tables.length === 0) return []; + + return comments.flatMap((comment): TableHighlightTarget[] => { + const range = anchorRange(comment); + if (!range) return []; + const quote = doc.slice(range.from, range.to); + if (!quote.trim()) return []; + const cell = tableCellTarget(lines, tables, range); + // Only an anchor wholly inside one cell has text there to paint. + if (!cell?.whole) return []; + return [ + { + table: cell.table, + row: cell.row, + column: cell.column, + id: comment.id, quote, resolved: comment.status === "resolved", author: creatorForComment(comment), - }); + }, + ]; + }); +}; + +/** The table cell a source range starts in, or null when it starts in no table. */ +export const tableCellForRange = (doc: string, range: TextRange): AnchorCell | null => { + const lines = sourceLines(doc); + return tableCellTarget(lines, sourceTables(lines), range); +}; + +const tableCellTarget = ( + lines: readonly SourceLine[], + tables: readonly SourceTable[], + range: TextRange, +): AnchorCell | null => { + if (tables.length === 0) return null; + const lineIndex = lineIndexAt(lines, range.from); + const line = lines[lineIndex]; + // The delimiter row renders no cells, so a range starting there has none. + const table = tables.findIndex( + ({ start, end }) => lineIndex >= start && lineIndex < end && lineIndex !== start + 1, + ); + const owner = tables[table]; + if (!line || !owner) return null; + return { + table, + // With the delimiter row rendering nothing, body rows shift up by one. + row: lineIndex === owner.start ? 0 : lineIndex - owner.start - 1, + column: tableColumnAt(line.text, range.from - line.from), + whole: range.to <= line.to, + }; +}; + +/** + * The rendered ``/`` that owns each source range, keyed the way the + * caller keyed the ranges. A key is absent when its range isn't inside a table, + * or when Live Preview hasn't mounted that table's widget. + * + * The margin needs this because a Live-Preview table is a single block widget: + * `coordsAtPos` reports the widget's own rect for every position inside it, so + * measuring a card's anchor that way puts every card in a table on the table's + * top edge instead of beside its row (issue #79). + */ +export const tableCellsForRanges = ( + view: EditorView, + doc: string, + ranges: ReadonlyMap, +): Map => { + if (ranges.size === 0) return new Map(); + const lines = sourceLines(doc); + const tables = sourceTables(lines); + if (tables.length === 0) return new Map(); + + const widgets = mountedTableWidgets(view, doc); + return new Map( + Array.from(ranges).flatMap(([key, range]): Array<[string, HTMLElement]> => { + const target = tableCellTarget(lines, tables, range); + const cell = target && cellElement(widgets, target); + return cell ? [[key, cell]] : []; + }), + ); +}; + +/** Mounted table widgets, keyed by their index in the source's table order. */ +const mountedTableWidgets = (view: EditorView, doc: string): Map => { + const widgets = Array.from(view.dom.querySelectorAll(".cm-table-widget")); + return mapTableWidgets(doc, widgets, (widget) => { + try { + return view.posAtDOM(widget); + } catch { + return null; } - } + }); +}; - return targets; +const cellElement = (widgets: ReadonlyMap, target: TableCellTarget): HTMLElement | null => { + const row = widgets.get(target.table)?.querySelectorAll("tr").item(target.row); + return row?.querySelectorAll("th, td").item(target.column) ?? null; }; class TableHighlights { private observer: MutationObserver; private scheduled = false; private generation = 0; + private activeId: string | null = null; + private painted = new Map(); private renderedQuotes = new Map>(); constructor(private view: EditorView) { @@ -115,6 +210,7 @@ class TableHighlights { const cfg = this.view.state.facet(commentConfig); const renderMarkdown = cfg.renderMarkdown; if (!cfg.showComments()) { + this.painted = new Map(); setViewRanges(this.view, new Map()); return; } @@ -124,22 +220,13 @@ class TableHighlights { (comment) => cfg.showResolved() || comment.status !== "resolved", ); const targets = tableHighlightTargets(doc, comments); - const widgets = Array.from(this.view.dom.querySelectorAll(".cm-table-widget")); - const widgetsByTable = mapTableWidgets(doc, widgets, (widget) => { - try { - return this.view.posAtDOM(widget); - } catch { - return null; - } - }); + const widgetsByTable = mountedTableWidgets(this.view, doc); const ranges: TableRanges = new Map(); + const painted = new Map(); const nextMatch = new WeakMap(); for (const target of targets) { - const rows = widgetsByTable.get(target.table)?.querySelectorAll("tr"); - const row = rows?.item(target.row); - const cells = row?.querySelectorAll("th, td"); - const cell = cells?.item(target.column); + const cell = cellElement(widgetsByTable, target); if (!cell) continue; const content = @@ -157,13 +244,35 @@ class TableHighlights { if (!match) continue; nextMatch.set(content, match.next); const color = (cfg.highlightColorForAuthor ?? cfg.colorForAuthor)(target.author ?? cfg.author()); - const name = tableHighlightName(color, target.resolved); - const entry = ranges.get(name) ?? { color, resolved: target.resolved, ranges: [] }; + const active = target.id === this.activeId; + const name = tableHighlightName(color, target.resolved, active); + const entry = ranges.get(name) ?? { color, resolved: target.resolved, active, ranges: [] }; entry.ranges.push(match.range); ranges.set(name, entry); + painted.set(target.id, [...(painted.get(target.id) ?? []), match.range]); } - if (generation === this.generation) setViewRanges(this.view, ranges); + if (generation !== this.generation) return; + this.painted = painted; + setViewRanges(this.view, ranges); + } + + /** Emphasize one comment's table text (or none). Hovering a margin card and + * hovering the text itself both land here. */ + setActiveComment(id: string | null): void { + if (this.activeId === id) return; + this.activeId = id; + this.schedule(); + } + + /** The comment whose painted table text covers a point. A CSS Custom Highlight + * has no element to hit-test, so ask its ranges for their rects instead. */ + commentAtPoint(x: number, y: number): string | null { + const covers = (rect: DOMRect) => x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom; + const hit = Array.from(this.painted).find(([, ranges]) => + ranges.some((range) => Array.from(range.getClientRects()).some(covers)), + ); + return hit?.[0] ?? null; } private renderedQuote( @@ -192,6 +301,16 @@ class TableHighlights { export const tableHighlightPlugin = ViewPlugin.fromClass(TableHighlights); +/** Emphasize a comment's text inside this view's Live-Preview tables, or clear it. */ +export const setActiveTableComment = (view: EditorView, id: string | null): void => { + view.plugin(tableHighlightPlugin)?.setActiveComment(id); +}; + +/** The comment whose table text covers a viewport point, or null. */ +export const tableCommentAtPoint = (view: EditorView, x: number, y: number): string | null => { + return view.plugin(tableHighlightPlugin)?.commentAtPoint(x, y) ?? null; +}; + const setViewRanges = (view: EditorView, ranges: TableRanges, remove = false): void => { const doc = view.dom.ownerDocument; let viewRanges = rangesByDocument.get(doc); @@ -243,7 +362,7 @@ const updateHighlightStyles = (doc: Document, ranges: ReadonlyMap tableHighlightRule(entry.color, entry.resolved)) + .map(([, entry]) => tableHighlightRule(entry.color, entry.resolved, entry.active)) .join("\n"); }; @@ -302,38 +421,6 @@ const textContent = (root: HTMLElement): string => { return text; }; -const sourceLines = (doc: string): Array<{ text: string; from: number; to: number }> => { - const lines: Array<{ text: string; from: number; to: number }> = []; - let from = 0; - for (const text of doc.split("\n")) { - lines.push({ text, from, to: from + text.length }); - from += text.length + 1; - } - return lines; -}; - -const sourceTables = (lines: Array<{ text: string; from: number; to: number }>): SourceTable[] => { - // Scanner that consumes a variable run of rows per table and advances `start` - // past it — a for loop is the natural fit, not an array method. - const tables: SourceTable[] = []; - for (let start = 0; start + 1 < lines.length; start++) { - const head = lines[start]; - const delimiter = lines[start + 1]; - if (!head || !delimiter || !isTableRow(head.text) || !isDelimiterRow(delimiter.text)) continue; - let end = start + 2; - let row = lines[end]; - while (row && isTableRow(row.text)) { - end++; - row = lines[end]; - } - const lastRow = lines[end - 1]; - if (!lastRow) continue; - tables.push({ start, end, from: head.from, to: lastRow.to }); - start = end - 1; - } - return tables; -}; - export const mapTableWidgets = ( doc: string, widgets: readonly T[], @@ -349,28 +436,3 @@ export const mapTableWidgets = ( } return result; }; - -const isTableRow = (line: string): boolean => unescapedPipes(line).length > 0; - -const isDelimiterRow = (line: string): boolean => { - const cells = line.trim().replace(/^\|/, "").replace(/\|$/, "").split("|"); - return cells.length > 0 && cells.every((cell) => /^\s*:?-{3,}:?\s*$/.test(cell)); -}; - -const tableColumnAt = (line: string, offset: number): number => { - const pipes = unescapedPipes(line); - const firstNonSpace = line.search(/\S/); - const leadingPipe = pipes[0] === firstNonSpace ? pipes[0] : null; - return pipes.filter((pipe) => pipe < offset && pipe !== leadingPipe).length; -}; - -const unescapedPipes = (line: string): number[] => { - const pipes: number[] = []; - for (let i = 0; i < line.length; i++) { - if (line[i] !== "|") continue; - let slashes = 0; - for (let j = i - 1; j >= 0 && line[j] === "\\"; j--) slashes++; - if (slashes % 2 === 0) pipes.push(i); - } - return pipes; -}; diff --git a/src/editor/table-repair.ts b/src/editor/table-repair.ts new file mode 100644 index 0000000..ee2c952 --- /dev/null +++ b/src/editor/table-repair.ts @@ -0,0 +1,224 @@ +import { Result } from "better-result"; +import { isCodeComment } from "../format/code-anchor"; +import { anchorRange, parseComments } from "../format/parse"; +import type { ParsedComment, TextRange } from "../format/types"; +import { closeMarker, openMarker } from "../format/serialize"; +import { + clampToTableCells, + isDelimiterLine, + lineIndexAt, + sourceLines, + tableCoverage, + type SourceLine, +} from "../format/table"; +import type { Change } from "./edits"; + +/** + * Finding and undoing the damage a comment written before the anchor clamp can + * do to a table. + * + * A marker outside a row's outer pipes stops Obsidian reading that line as a + * row, so the table truncates there — or, on the header or delimiter row, stops + * rendering as a table at all. Nothing about the comment itself is wrong; the + * markers just sit a few characters too far out, and moving them inside the + * pipes restores the table. + */ + +/** A blank-line-delimited block and the comments whose markers sit in it. */ +type BlockAnchors = { block: TextRange; ids: Set }; +type Placement = TextRange & { id: string }; + +const ANCHOR_MARKER = //; +const ANCHOR_MARKERS = //g; + +/** Every marker-shaped string in `text`. Only the cheap per-line gate uses this. */ +const stripMarkers = (text: string): string => text.replace(ANCHOR_MARKERS, ""); + +/** The anchor markers the parser recognized, in document order. */ +const parsedMarkers = (comments: readonly ParsedComment[]): TextRange[] => { + return comments + .flatMap((comment) => [comment.open, comment.close]) + .filter((marker): marker is TextRange => marker !== null) + .sort((a, b) => a.from - b.from); +}; + +/** + * `text` with just these markers taken out. Marker-shaped text the parser skipped + * — written out inside a code span, say, or hidden by a stray backtick — is + * ordinary text; stripping it too would delete it, because nothing puts it back. + */ +const withoutMarkers = (text: string, markers: readonly TextRange[]): string => { + return ( + markers.map((marker, index) => text.slice(markers[index - 1]?.to ?? 0, marker.from)).join("") + + text.slice(markers[markers.length - 1]?.to ?? 0) + ); +}; + +/** How many marker characters come out before `position`. */ +const strippedBefore = (markers: readonly TextRange[], position: number): number => { + return markers + .filter((marker) => marker.from < position) + .reduce((removed, marker) => removed + (marker.to - marker.from), 0); +}; + +/** The text of the line holding `pos`, read without splitting the document. */ +const lineTextAt = (doc: string, pos: number): string => { + const end = doc.indexOf("\n", pos); + return doc.slice(doc.lastIndexOf("\n", pos - 1) + 1, end < 0 ? doc.length : end); +}; + +/** + * Whether removing the markers from a line changes whether it can take part in + * a table. This is the cheap gate: pure string work on the marker's own line, run + * over every comment on every document change. Nearly every note answers no here + * and nothing in the document is scanned or split. + */ +const markersDamageLine = (text: string): boolean => { + if (!ANCHOR_MARKER.test(text)) return false; + const bare = stripMarkers(text); + // A normal row must START with a pipe, a header must also END with one, a + // delimiter row must be nothing but dashes and colons, and the line above a + // table must be blank for it to render at all. + if ((bare.trim() === "") !== (text.trim() === "")) return true; + if (bare.startsWith("|") !== text.startsWith("|")) return true; + if (/\|\s*$/.test(bare) !== /\|\s*$/.test(text)) return true; + return isDelimiterLine(bare) !== isDelimiterLine(text); +}; + +/** The blank-line-delimited block holding `index`, or null for a line that + * doesn't exist. Tables never cross a blank line. */ +const blockAround = (lines: readonly SourceLine[], index: number): TextRange | null => { + if (!lines[index]) return null; + const blank = lines.map((line) => line.text.trim() === ""); + // A negative fromIndex counts back from the end, so the first line needs a guard. + const gapAbove = index > 0 ? blank.lastIndexOf(true, index - 1) : -1; + const gapBelow = blank.indexOf(true, index + 1); + const first = lines[gapAbove + 1]; + const last = lines[gapBelow < 0 ? lines.length - 1 : gapBelow - 1]; + return first && last ? { from: first.from, to: last.to } : null; +}; + +/** Group comment ids by the block their marker sits in. */ +const groupByBlock = ( + lines: readonly SourceLine[], + markers: ReadonlyArray<{ id: string; at: number }>, +): BlockAnchors[] => { + const groups = markers.reduce((byStart, { id, at }) => { + const block = blockAround(lines, lineIndexAt(lines, at)); + if (!block) return byStart; + const group = byStart.get(block.from) ?? { block, ids: new Set() }; + group.ids.add(id); + return byStart.set(block.from, group); + }, new Map()); + return Array.from(groups.values()); +}; + +/** + * Rewrite one block with `ids`' anchors moved inside their table cells, or null + * when it cannot be done safely. + * + * Every anchor in the block is reinserted, not just the repaired ones: the clamp + * has to run against a block with ALL markers stripped, because a table broken + * by two anchors only reappears once both are out of the way — and a healthy + * neighbour's markers must come back exactly where they were. + */ +const repairedBlock = (doc: string, block: TextRange, ids: ReadonlySet): string | null => { + const text = doc.slice(block.from, block.to); + const comments = parseComments(text); + const found = parsedMarkers(comments); + if (found.length === 0) return null; + const bare = withoutMarkers(text, found); + + // A comment with no markers in the block — an orphan whose anchored text was + // deleted, or a body whose anchor sits elsewhere — has nothing here to move. + const anchored = comments.filter((comment) => comment.open || comment.close); + const placements = anchored.map((comment): Placement | null => { + const range = anchorRange(comment); + // A lone marker has no range to restore, and stripping it would lose it. + if (!range) return null; + const from = range.from - strippedBefore(found, range.from); + const to = range.to - strippedBefore(found, range.to); + const target = ids.has(comment.id) ? clampToTableCells(bare, from, to) : { from, to }; + return target.to > target.from ? { id: comment.id, ...target } : null; + }); + const placed = placements.filter((placement): placement is Placement => placement !== null); + if (placed.length === 0 || placed.length < placements.length) return null; + + // Emit every marker as a point insertion in one forward pass. Splicing whole + // anchors one at a time looks equivalent but silently corrupts nested ranges: + // inserting an inner pair shifts the outer anchor's end, so its close marker + // lands inside the inner one and both comments are destroyed. A whole-table + // anchor around a cell comment — exactly what repair exists to undo — nests. + const markers = placed.flatMap(({ id, from, to }) => [ + { at: from, text: openMarker(id), closing: false, from, to }, + { at: to, text: closeMarker(id), closing: true, from, to }, + ]); + markers.sort((a, b) => { + if (a.at !== b.at) return a.at - b.at; + // At a shared boundary a close comes before an open, so neighbouring anchors + // read `……` rather than interleaving. + if (a.closing !== b.closing) return a.closing ? -1 : 1; + // Innermost closes first and outermost opens first, so nesting stays nested. + return a.closing ? b.from - a.from : b.to - a.to; + }); + + const rebuilt = + markers.map((marker, index) => bare.slice(markers[index - 1]?.at ?? 0, marker.at) + marker.text).join("") + + bare.slice(markers[markers.length - 1]?.at ?? 0); + return rebuilt === text ? null : rebuilt; +}; + +/** + * Ids whose anchor markers are breaking a table AND can be moved back inside it. + * + * Repairability is part of the test on purpose: a comment we cannot put right + * should never be offered a Repair action that does nothing. + */ +export const brokenTableAnchors = (doc: string, parsed?: readonly ParsedComment[]): Set => { + // A code comment's markers sit on lines of their own around a fence by design, + // so a line left blank by stripping them was never a gap a table relied on. + const damaging = (parsed ?? parseComments(doc)) + .filter((comment) => !isCodeComment(comment)) + .flatMap((comment) => + [comment.open, comment.close] + .filter((marker): marker is TextRange => marker !== null) + .filter((marker) => markersDamageLine(lineTextAt(doc, marker.from))) + .map((marker) => ({ id: comment.id, at: marker.from })), + ); + if (damaging.length === 0) return new Set(); + + // Blocks holding a marker that looks damaging, and the ids sitting in them. + const suspects = groupByBlock(sourceLines(doc), damaging); + + // Confirm against the block only — a line can look damaging in prose that was + // never a table. Scoped this way the check stays cheap even when it does run. + const broken = suspects + .filter(({ block }) => { + const text = doc.slice(block.from, block.to); + return tableCoverage(withoutMarkers(text, parsedMarkers(parseComments(text)))) > tableCoverage(text); + }) + .flatMap(({ block, ids }) => Array.from(ids).filter((id) => repairedBlock(doc, block, new Set([id])) !== null)); + return new Set(broken); +}; + +/** Rewrite every block whose table an anchor is breaking. `only` limits it to + * specific comments; without it, every repairable one in the document. */ +export const computeRepairTableAnchors = (doc: string, only?: ReadonlySet): Result => { + const targets = brokenTableAnchors(doc); + const ids = only ? new Set([...targets].filter((id) => only.has(id))) : targets; + if (ids.size === 0) return Result.ok([]); + + const blocks = groupByBlock( + sourceLines(doc), + parseComments(doc).flatMap((comment) => { + const marker = comment.open ?? comment.close; + return ids.has(comment.id) && marker ? [{ id: comment.id, at: marker.from }] : []; + }), + ); + + const changes = blocks.flatMap(({ block, ids: blockIds }) => { + const repaired = repairedBlock(doc, block, blockIds); + return repaired === null ? [] : [{ from: block.from, to: block.to, insert: repaired }]; + }); + return Result.ok(changes.sort((a, b) => a.from - b.from)); +}; diff --git a/src/format/table.ts b/src/format/table.ts new file mode 100644 index 0000000..227b6af --- /dev/null +++ b/src/format/table.ts @@ -0,0 +1,295 @@ +import { TextRange } from "./types"; + +export type SourceLine = { text: string; from: number; to: number }; +/** A table's line span (`start` inclusive, `end` exclusive) and offset span. */ +export type SourceTable = { start: number; end: number; from: number; to: number }; +type TableKind = "normal" | "simple"; + +// Obsidian decides where a table starts and stops with these line shapes, and +// the Live-Preview table widget spans exactly the run of lines they cover. Ours +// has to agree: a looser rule hands the renderer a row index its DOM doesn't +// have, and a stricter one drops a table the reader can plainly see. +// +// A "normal" table draws outer pipes and continues only while a line STARTS with +// one — which is why a row that doesn't (one whose leading pipe a comment marker +// displaced, say) truncates the table there. A "simple" table draws no outer +// pipes and continues while a line starts with a non-pipe and contains a pipe. +// +// These are Obsidian's own patterns, lifted from its bundled tokenizer, and they +// are deliberately stricter than GFM. Each of these GFM-valid shapes renders as +// plain text in Obsidian, with no table widget — checked in the app: +// +// indented by spaces or a tab · nested in a list item · inside a blockquote · +// a header with only one of its two outer pipes +// +// Don't widen the patterns to catch them. With no widget there is nothing to +// measure a row against, so the ordinary text path already highlights and aligns +// their comments, and a marker can't break a table that never rendered as one. +// test/table-anchors.test.ts pins each shape. +const NORMAL_HEADER = /^\|(?:[^|]+\|)+?\s*$/; +const SIMPLE_HEADER = /^\s*[^|].*?\|.*[^|]\s*$/; +const NORMAL_ROW = /^\|/; +const SIMPLE_ROW = /^\s*[^|].*\|/; +// One dash per column is enough here — Obsidian is looser than GFM's three. +const DELIMITER_CELL = /^\s*:?\s*-+\s*:?\s*$/; +// Setext underlines are let through with ATX headings. Treating one as a table's +// opener errs toward recognizing a table Obsidian doesn't draw, which is harmless; +// the other way round would strip a real table of its clamp and its alignment. +const HEADING = /^ {0,3}(?:#{1,6}(?:\s|$)|=+\s*$|-+\s*$)/; + +export const sourceLines = (doc: string): SourceLine[] => { + const lines: SourceLine[] = []; + let from = 0; + for (const text of doc.split("\n")) { + lines.push({ text, from, to: from + text.length }); + from += text.length + 1; + } + return lines; +}; + +export const sourceTables = (lines: readonly SourceLine[]): SourceTable[] => { + // Scanner that consumes a variable run of rows per table and advances `start` + // past it — a for loop is the natural fit, not an array method. + const tables: SourceTable[] = []; + for (let start = 0; start + 1 < lines.length; start++) { + const head = lines[start]; + const delimiter = lines[start + 1]; + if (!head || !delimiter || !opensTableBelow(lines[start - 1])) continue; + const kind = tableKind(head.text, delimiter.text); + if (!kind) continue; + const isRow = + kind === "normal" + ? (text: string) => NORMAL_ROW.test(text) + : (text: string) => SIMPLE_ROW.test(tokenizable(text)); + let end = start + 2; + let row = lines[end]; + while (row && isRow(row.text)) { + end++; + row = lines[end]; + } + const lastRow = lines[end - 1]; + if (!lastRow) continue; + tables.push({ start, end, from: head.from, to: lastRow.to }); + start = end - 1; + } + return tables; +}; + +/** + * The part of a line still tokenized as Markdown. Text after an HTML comment + * that does NOT close on the line sits inside a comment token running past the + * line end, so a pipe there never becomes a cell separator. + * + * That is what lets a comment's own `` block sit directly beneath a + * pipe-less table without being read as another row, even when its `quote:` + * carries a pipe — confirmed in the app, where the same table followed by a + * plain `ordinary line | with a pipe` DOES grow a row. A comment that opens and + * closes on the line (an anchor marker) leaves the rest of the line tokenizing + * normally, so a row starting with one is still a row. + */ +const tokenizable = (line: string): string => { + // A scan, not a transform: each step depends on where the previous comment + // closed, and it exits at whichever of two conditions comes first. + let cursor = 0; + for (;;) { + const open = line.indexOf("", open + 4); + if (close < 0) return line.slice(0, open); + cursor = close + 3; + } +}; + +/** + * Whether a table may start on the line after `above`. Obsidian's tokenizer only + * looks for a header below a blank line, a heading, or the top of the note + * (`prevLine.stream.string.trim() && !wasHeading`). Checked in the app: prose, a + * comment's own block, or a marker alone on the line above leaves the whole table + * as plain text. + */ +const opensTableBelow = (above: SourceLine | undefined): boolean => { + return !above || above.text.trim() === "" || HEADING.test(above.text); +}; + +/** Which flavor of table `head` opens, or null when it opens none. */ +const tableKind = (head: string, delimiter: string): TableKind | null => { + if (NORMAL_HEADER.test(head)) { + if (!NORMAL_HEADER.test(delimiter)) return null; + // The outer pipes aren't columns, so drop them before splitting. + return isDelimiterRow(delimiter.replace(/^\s*\|/, "").replace(/\|\s*$/, "")) ? "normal" : null; + } + if (SIMPLE_HEADER.test(head)) { + if (!SIMPLE_HEADER.test(delimiter)) return null; + return isDelimiterRow(delimiter) ? "simple" : null; + } + return null; +}; + +const isDelimiterRow = (line: string): boolean => { + return line.split("|").every((cell) => DELIMITER_CELL.test(cell)); +}; + +/** Whether a whole source line reads as a table's delimiter row, either flavor. */ +export const isDelimiterLine = (line: string): boolean => { + const inner = line.replace(/^\s*\|/, "").replace(/\|\s*$/, ""); + return inner.trim().length > 0 && isDelimiterRow(inner); +}; + +/** Total rows every table in `text` covers. Rises when a repair reveals more table. */ +export const tableCoverage = (text: string): number => { + return sourceTables(sourceLines(text)).reduce((rows, table) => rows + (table.end - table.start), 0); +}; + +/** Zero-based column of `offset` within a table row, ignoring the leading pipe. */ +export const tableColumnAt = (line: string, offset: number): number => { + const pipes = unescapedPipes(line); + const firstNonSpace = line.search(/\S/); + const leadingPipe = pipes[0] === firstNonSpace ? pipes[0] : null; + return pipes.filter((pipe) => pipe < offset && pipe !== leadingPipe).length; +}; + +const unescapedPipes = (line: string): number[] => { + const pipes: number[] = []; + for (let i = 0; i < line.length; i++) { + if (line[i] !== "|") continue; + let slashes = 0; + for (let j = i - 1; j >= 0 && line[j] === "\\"; j--) slashes++; + if (slashes % 2 === 0) pipes.push(i); + } + return pipes; +}; + +/** + * Pull a selection inside the outer pipes of any table row it touches, then trim + * the whitespace that padding leaves on the edges. + * + * A comment marker written outside those pipes breaks the row it lands on: + * Obsidian keeps an outer-pipe table going only while each line starts with a + * pipe, and reads a header only while it ends with one. So anchoring a whole row + * used to drop that row and everything after it out of the rendered table, and + * anchoring a whole table stopped it rendering as a table at all. Inside the + * pipes the markers are ordinary cell text and the table holds together. + * + * Returns an empty range when the selection held nothing but borders and + * padding, which the caller reports rather than anchoring. + */ +export const clampToTableCells = (doc: string, from: number, to: number): TextRange => { + if (to <= from) return { from, to }; + const lines = sourceLines(doc); + const tables = sourceTables(lines); + if (tables.length === 0) return { from, to }; + const touchesTable = !!tableAt(tables, lineIndexAt(lines, from)) || !!tableAt(tables, lineIndexAt(lines, to)); + + // Trim BEFORE deciding which ends sit in a table. Trimmed afterwards, an end + // left outside the table gets dragged across blank lines onto the table's own + // border, and a marker there stops it rendering. Trimmed first, a selection + // starting on the blank line above a table starts in the table and is clamped + // like one, while one starting in prose stays on that prose. + const selected = trimRange(doc, from, to); + const fromIndex = lineIndexAt(lines, selected.from); + const toIndex = lineIndexAt(lines, selected.to); + const head = tableAt(tables, fromIndex); + const tail = tableAt(tables, toIndex); + if (!head && !tail) { + // Only whitespace tied the selection to a table, or an end sits on the blank + // line a table needs above it — as when a triple-click takes a paragraph's + // newline. Either way the markers belong on the selected text. A selection + // that touches neither is left exactly as it was. + const fillsGap = [from, to].some((pos) => isGapAboveTable(lines, tables, lineIndexAt(lines, pos))); + return touchesTable || fillsGap ? selected : { from, to }; + } + + const start = head ? anchorablePosition(lines, head, fromIndex, selected.from, 1) : selected.from; + const end = tail ? anchorablePosition(lines, tail, toIndex, selected.to, -1) : selected.to; + if (start === null || end === null || end <= start) return { from, to: from }; + const trimmed = trimRange(doc, start, end); + if (trimmed.to <= trimmed.from) return { from, to: from }; + // A marker between a backslash and the pipe it escapes un-escapes that pipe, + // splitting the cell in two and showing the marker as text. Keep the pair whole. + const range = { + from: splitsEscapedPipe(doc, trimmed.from) ? trimmed.from - 1 : trimmed.from, + to: splitsEscapedPipe(doc, trimmed.to) ? trimmed.to + 1 : trimmed.to, + }; + // Clamping only moves the two ends, so a selection between two rows or around a + // separator still holds nothing but the pipes that divide cells. + return holdsCellText(doc, range) ? range : { from, to: from }; +}; + +/** Whether `pos` falls between a backslash and the pipe it escapes. */ +const splitsEscapedPipe = (doc: string, pos: number): boolean => { + if (doc[pos] !== "|") return false; + const before = doc.slice(doc.lastIndexOf("\n", pos - 1) + 1, pos); + const slashes = before.length - before.replace(/\\+$/, "").length; + return slashes % 2 === 1; +}; + +/** Whether `range` holds anything besides whitespace and unescaped cell pipes. */ +const holdsCellText = (doc: string, range: TextRange): boolean => { + // Scan from the line start: the backslash escaping a pipe can sit just before + // the range, and a pipe it escapes is cell text, not a border. + const lineStart = doc.lastIndexOf("\n", range.from - 1) + 1; + const pipes = new Set(unescapedPipes(doc.slice(lineStart, range.to)).map((pipe) => lineStart + pipe)); + return doc + .slice(range.from, range.to) + .split("") + .some((char, index) => !pipes.has(range.from + index) && char.trim() !== ""); +}; + +/** Whether a line is the blank one a table depends on to render at all. */ +const isGapAboveTable = (lines: readonly SourceLine[], tables: readonly SourceTable[], lineIndex: number): boolean => { + return lines[lineIndex]?.text.trim() === "" && tables.some((table) => table.start === lineIndex + 1); +}; + +const tableAt = (tables: readonly SourceTable[], lineIndex: number): SourceTable | undefined => { + return tables.find((table) => lineIndex >= table.start && lineIndex < table.end); +}; + +/** + * Where a marker may sit at or beyond `pos`, searching in `step`'s direction for + * the first row of `table` that can hold one. Null when the table has no such row. + */ +const anchorablePosition = ( + lines: readonly SourceLine[], + table: SourceTable, + lineIndex: number, + pos: number, + step: 1 | -1, +): number | null => { + // Walks in a caller-chosen direction and stops at the first usable row, which + // no single array method expresses. + for (let index = lineIndex; index >= table.start && index < table.end; index += step) { + // The delimiter row has to stay nothing but dashes and colons, so a marker + // dropped there stops the whole block reading as a table. Step over it. + if (index === table.start + 1) continue; + const line = lines[index]; + if (!line) continue; + const bounds = cellBounds(line); + if (bounds.to <= bounds.from) continue; // borders all the way across + return Math.min(Math.max(pos, bounds.from), bounds.to); + } + return null; +}; + +/** The offsets a table row's cell content may occupy, excluding its outer pipes. */ +const cellBounds = (line: SourceLine): TextRange => { + const pipes = unescapedPipes(line.text); + const first = pipes[0]; + const last = pipes[pipes.length - 1]; + const start = first !== undefined && first === line.text.search(/\S/) ? first + 1 : 0; + // Only a pipe with nothing but whitespace after it is a closing border; on a + // row without one the last pipe is a cell separator and content runs past it. + const end = last !== undefined && line.text.slice(last + 1).trim() === "" ? last : line.text.length; + return { from: line.from + start, to: line.from + Math.max(start, end) }; +}; + +export const lineIndexAt = (lines: readonly SourceLine[], pos: number): number => { + return lines.findIndex((line) => pos >= line.from && pos <= line.to); +}; + +const trimRange = (doc: string, from: number, to: number): TextRange => { + const text = doc.slice(from, to); + const leading = text.length - text.trimStart().length; + const trailing = text.length - text.trimEnd().length; + const start = from + leading; + return { from: start, to: Math.max(start, to - trailing) }; +}; diff --git a/src/main.ts b/src/main.ts index a727706..c78cc55 100644 --- a/src/main.ts +++ b/src/main.ts @@ -18,7 +18,7 @@ import { marginPlugin } from "./editor/margin"; import { commentConfig } from "./editor/config"; import { editorLayoutField } from "./editor/layout"; import { draftField, setDraft } from "./editor/draft"; -import { addComment, insertCommentInFile } from "./editor/commands"; +import { addComment, insertCommentInFile, repairTableAnchors } from "./editor/commands"; import { findHighlightAtSelection } from "./editor/edits"; import { findSectionRange, highlightPostProcessor, mapReadingSelection } from "./reading/highlight"; import { ReadingDeps, ReadingMarginManager } from "./reading/margin"; @@ -26,6 +26,7 @@ import { COMMENTS_VIEW_TYPE, CommentsSidebarView, SidebarDeps } from "./ui/sideb import { CommentModal } from "./ui/comment-modal"; import { DEFAULT_SETTINGS, DocCommentsSettings, DocCommentsSettingTab } from "./settings"; import { tableHighlightPlugin } from "./editor/table-highlights"; +import { brokenTableAnchors } from "./editor/table-repair"; import { authorColorCss, canonicalAuthorKey, @@ -191,6 +192,12 @@ export default class DocCommentsPlugin extends Plugin { editorCallback: (editor) => this.startAddComment(editor), }); + this.addCommand({ + id: "repair-table-comments", + name: "Repair table comments in this note", + editorCallback: (editor) => this.repairTableComments(editor), + }); + this.addCommand({ id: "toggle-comments", name: "Toggle comments", @@ -232,6 +239,28 @@ export default class DocCommentsPlugin extends Plugin { this.addSettingTab(this.settingsTab); } + /** Move every anchor that is breaking a table in this note back inside its + * cell. The card offers the same repair one comment at a time; this is the + * way out when several anchors broke the same table. */ + private repairTableComments(editor: Editor): void { + const view = editorView(editor); + if (!view) { + new Notice("Couldn't access the editor."); + return; + } + const broken = brokenTableAnchors(view.state.doc.toString()); + if (broken.size === 0) { + new Notice("No table comments need repairing."); + return; + } + const result = repairTableAnchors(view); + if (result.isErr()) { + new Notice(`Couldn't repair the table comments: ${result.error}`); + return; + } + new Notice(`Repaired ${broken.size} table ${broken.size === 1 ? "comment" : "comments"}.`); + } + private startAddComment(editor: Editor): void { const view = editorView(editor); if (!view) { diff --git a/src/ui/card.ts b/src/ui/card.ts index bdc64b6..a146abc 100644 --- a/src/ui/card.ts +++ b/src/ui/card.ts @@ -32,6 +32,9 @@ export type CardCallbacks = { /** Reveal this thread in the comments sidebar — the escape for a card too tall to * fit the margin even when expanded. Absent for cards already in the sidebar. */ openInSidebar?: (id: string) => void; + /** Move this comment's anchor back inside its table cell. Absent where repair + * isn't offered, in which case the card shows no repair notice at all. */ + repairTableAnchor?: (id: string) => void; }; /** Per-view context a card needs to render comment text as Markdown. */ @@ -63,6 +66,8 @@ export class Card { private overflows = false; private tooTall = false; private clipEl: HTMLElement | null = null; + /** This comment's anchor is breaking the table it sits in (see table-repair). */ + private tableAnchorBroken = false; private threadEl: HTMLElement | null = null; private footEl: HTMLElement | null = null; /** Owns the child components MarkdownRenderer attaches (link/embed handlers). */ @@ -100,6 +105,35 @@ export class Card { return this.comment.id; } + /** Show or hide the "this comment is breaking its table" notice. Cheap enough + * to call on every reconcile; only touches the DOM when the state flips. + * + * Deliberately does NOT call onResize: reconcile runs inside CodeMirror's + * update cycle, and repositioning reads layout, which throws there. The + * margin already schedules a measure pass after every reconcile, so the + * height change this causes is picked up then. */ + setTableAnchorBroken(broken: boolean): void { + if (this.tableAnchorBroken === broken) return; + this.tableAnchorBroken = broken; + this.renderRepairNotice(); + } + + /** A card whose anchor broke its table sits beside the damage, so this is where + * the reader is already looking when they wonder what went wrong. */ + private renderRepairNotice(): void { + this.el.querySelector(".dc-repair")?.remove(); + if (!this.tableAnchorBroken || !this.cb.repairTableAnchor) return; + const notice = createDiv("dc-repair"); + setIcon(notice.createSpan("dc-repair__icon"), "alert-triangle"); + notice.createSpan({ cls: "dc-repair__text", text: "This comment is breaking its table." }); + const button = notice.createEl("button", { cls: "dc-repair__action", text: "Repair" }); + button.addEventListener("click", (e) => { + e.stopPropagation(); + this.cb.repairTableAnchor?.(this.id); + }); + this.el.prepend(notice); + } + get signature(): string { return cardSignature(this.comment); } @@ -190,6 +224,7 @@ export class Card { // The thread lives in a clip wrapper that gets a max-height when a tall card is // collapsed; the footer (Show more / Open in sidebar) sits outside the clip. + this.renderRepairNotice(); const clip = this.el.createDiv("dc-card-clip"); this.clipEl = clip; const thread = clip.createDiv("dc-thread"); diff --git a/styles.css b/styles.css index 4b5ef25..d976879 100644 --- a/styles.css +++ b/styles.css @@ -623,3 +623,50 @@ body { min-height: 5em; resize: vertical; } + +/* ── Broken table anchor ────────────────────────────────────────────────── */ +/* A comment written before the anchor clamp can sit outside its row's outer + pipes and stop the block rendering as a table. The card is already anchored + beside the damage, so it is where the reader is looking when they wonder what + went wrong — hence a notice on the card rather than a command they'd have to + know to go find. */ +.dc-repair { + display: flex; + align-items: center; + gap: 8px; + margin: calc(-1 * var(--dc-card-pad-y, 10px)) -12px 8px -12px; + padding: 7px 10px; + border-bottom: 1px solid var(--background-modifier-border); + border-radius: var(--radius-m, 8px) var(--radius-m, 8px) 0 0; + background-color: color-mix(in srgb, var(--text-warning) 10%, transparent); + font-size: var(--font-ui-smaller); +} +.dc-repair__icon { + display: flex; + flex: 0 0 auto; + color: var(--text-warning); +} +.dc-repair__icon svg { + width: 14px; + height: 14px; +} +.dc-repair__text { + flex: 1 1 auto; + color: var(--text-muted); + line-height: 1.35; +} +.dc-repair__action { + flex: 0 0 auto; + padding: 2px 6px; + border: none; + border-radius: 4px; + background: none; + box-shadow: none; + color: var(--text-accent); + font-size: var(--font-ui-smaller); + font-weight: var(--font-medium); + cursor: pointer; +} +.dc-repair__action:hover { + background-color: var(--background-modifier-hover); +} diff --git a/test/card.test.ts b/test/card.test.ts index bd92b96..263389a 100644 --- a/test/card.test.ts +++ b/test/card.test.ts @@ -84,6 +84,48 @@ const callbacks = (): CardCallbacks => ({ toggleReaction: vi.fn(), }); +describe("broken table anchor notice", () => { + const view = { sourcePath: () => "note.md", colorForAuthor: () => null }; + + test("shows the notice and repair action only while the anchor is breaking a table", () => { + const repairTableAnchor = vi.fn(); + const card = new Card(commentWithText(), { ...callbacks(), repairTableAnchor }, view); + + expect(card.el.querySelector(".dc-repair")).toBeNull(); + + card.setTableAnchorBroken(true); + expect(card.el.querySelector(".dc-repair__text")?.textContent).toBe("This comment is breaking its table."); + card.el.querySelector(".dc-repair__action")?.click(); + expect(repairTableAnchor).toHaveBeenCalledWith(commentWithText().id); + + card.setTableAnchorBroken(false); + expect(card.el.querySelector(".dc-repair")).toBeNull(); + }); + + test("stays silent when no repair action is available, as in the sidebar", () => { + const card = new Card(commentWithText(), callbacks(), view); + + card.setTableAnchorBroken(true); + + expect(card.el.querySelector(".dc-repair")).toBeNull(); + }); + + test("never asks the margin to reposition", () => { + // The margin reconciles inside CodeMirror's update cycle, and repositioning + // reads layout — which throws there, taking the whole margin plugin down with + // it. The scheduled measure pass after each reconcile picks the height change + // up instead. + const cb = { ...callbacks(), repairTableAnchor: vi.fn() }; + const card = new Card(commentWithText(), cb, view); + (cb.onResize as ReturnType).mockClear(); + + card.setTableAnchorBroken(true); + card.setTableAnchorBroken(false); + + expect(cb.onResize).not.toHaveBeenCalled(); + }); +}); + describe("empty comment card", () => { test("colors every displayed author name with that author's assignment", () => { const comment = { diff --git a/test/table-anchors.test.ts b/test/table-anchors.test.ts new file mode 100644 index 0000000..2655dc1 --- /dev/null +++ b/test/table-anchors.test.ts @@ -0,0 +1,287 @@ +import { describe, expect, test } from "vitest"; +import { applyChanges, computeAddComment } from "../src/editor/edits"; +import { clampToTableCells, sourceLines, sourceTables } from "../src/format/table"; + +// Obsidian keeps an outer-pipe table going only while a line STARTS with a pipe, +// and a pipe-less one while a line starts with a non-pipe and holds a pipe. A +// header is read only while it also ENDS with a pipe, and a delimiter row only +// while every cell is dashes and colons. Mirroring those rules here is what makes +// "the table still renders" an assertion rather than a hope. +// +// One rule isn't a line shape. A pipe only separates cells where the tokenizer +// still sees Markdown, so text after an HTML comment that does NOT close on the +// line contributes nothing. All three cases were checked in the app: +// +// pipe-less table + `ordinary line | with a pipe` -> grows a row +// pipe-less table + `Monday | spec` -> still a row +// +// So a comment's own block can sit directly beneath a table, and a marker at the +// start of a pipe-less row is harmless. An outer-pipe row is different: its +// `^\|` test reads the raw line, so a marker before the leading pipe breaks it. +// +// Nor does a table start just anywhere: the tokenizer only looks for a header +// below a blank line, a heading, or the top of the note. Checked in the app, prose +// or a marker alone on the line above leaves the whole table as plain text. +const NORMAL_HEADER = /^\|(?:[^|]+\|)+?\s*$/; +const SIMPLE_HEADER = /^\s*[^|].*?\|.*[^|]\s*$/; +const DELIMITER_CELL = /^\s*:?\s*-+\s*:?\s*$/; +const HEADING = /^ {0,3}#{1,6}(?:\s|$)/; +/** The part of a line still tokenized as Markdown (see above). */ +const tokenizable = (line: string): string => { + // A scan, not a transform: each step depends on where the previous comment + // closed, and it exits at whichever of two conditions comes first. + let cursor = 0; + for (;;) { + const open = line.indexOf("", open + 4); + if (close < 0) return line.slice(0, open); + cursor = close + 3; + } +}; + +const renderedRows = (doc: string): number | null => { + const lines = doc.split("\n"); + // Finds the first table, then consumes its variable run of rows from there — + // two linked scans with an early exit, which no array method chain expresses. + for (let i = 0; i + 1 < lines.length; i++) { + const head = lines[i] ?? ""; + const delimiter = lines[i + 1] ?? ""; + const above = lines[i - 1]; + if (above !== undefined && above.trim() !== "" && !HEADING.test(above)) continue; + const normal = NORMAL_HEADER.test(head) && NORMAL_HEADER.test(delimiter); + const simple = !normal && SIMPLE_HEADER.test(head) && SIMPLE_HEADER.test(delimiter); + if (!normal && !simple) continue; + const cells = normal ? delimiter.replace(/^\s*\|/, "").replace(/\|\s*$/, "") : delimiter; + if (!cells.split("|").every((cell) => DELIMITER_CELL.test(cell))) continue; + const isRow = (text: string) => (normal ? text.startsWith("|") : /^\s*[^|].*\|/.test(tokenizable(text))); + let end = i + 2; + while (end < lines.length && isRow(lines[end] ?? "")) end++; + return end - i - 1; + } + return null; +}; + +const addComment = (doc: string, from: number, to: number): string | null => { + const result = computeAddComment(doc, from, to, { + id: "aa11", + createdAt: "2026-01-01T00:00:00.000Z", + author: "me", + text: "hi", + }); + return result.isErr() ? null : applyChanges(doc, result.value); +}; + +const normal = [ + "| Day | Task | Owner |", + "| --- | --- | --- |", + "| Monday | write the spec | ana |", + "| Tuesday | review the spec | ben |", +].join("\n"); +const simple = ["Day | Task", "--- | ---", "Monday | write the spec", "Tuesday | review the spec"].join("\n"); + +const span = (doc: string, text: string): [number, number] => { + const from = doc.indexOf(text); + expect(from, `missing ${text}`).toBeGreaterThanOrEqual(0); + return [from, from + text.length]; +}; + +/** The span of `part` where it first appears inside `context`. */ +const spanWithin = (doc: string, context: string, part: string): [number, number] => { + const [start] = span(doc, context); + const from = start + context.indexOf(part); + return [from, from + part.length]; +}; + +describe("anchoring inside a table", () => { + test.each([ + ["one cell", ...span(normal, "write the spec")], + ["a whole row", ...span(normal, "| Monday | write the spec | ana |")], + ["the header row", ...span(normal, "| Day | Task | Owner |")], + ["two cells of one row", ...span(normal, "Monday | write the spec")], + ["two rows", ...span(normal, "write the spec | ana |\n| Tuesday | review")], + ["the whole table", 0, normal.length], + ["the header and delimiter", 0, normal.indexOf("| Monday") - 1], + ])("keeps an outer-pipe table rendering when anchoring %s", (_label, from, to) => { + const out = addComment(normal, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(normal)); + }); + + // Only an end that lands in a table gets pulled inside its pipes. The other end + // stays where the selection put it, so it can't be dragged onto a border. + const between = ["Intro text", "", normal, "", "After."].join("\n"); + const headerOnly = ["Intro text", "", "| Day | Task |", "| --- | --- |", "", "After."].join("\n"); + test.each([ + ["starts at the end of the paragraph above", between, between.indexOf("\n\n"), between.indexOf("ana") + 3], + ["starts on the blank line above", between, between.indexOf("\n\n") + 1, between.indexOf("ana") + 3], + ["ends on the blank line above, as a triple-click does", between, 0, between.indexOf("\n\n") + 1], + [ + "ends on the blank line below a header-only table", + headerOnly, + headerOnly.indexOf("Day"), + headerOnly.lastIndexOf("\n\n") + 1, + ], + ["ends at the start of the paragraph below", between, between.indexOf("write"), between.indexOf("After.")], + [ + "starts after a table's last pipe and runs into the paragraph below", + headerOnly, + headerOnly.indexOf("| --- | --- |") + "| --- | --- |".length, + headerOnly.length, + ], + ])("keeps a table rendering when a selection %s", (_label, doc, from, to) => { + const out = addComment(doc, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(doc)); + }); + + test.each([ + ["one cell", ...span(simple, "write the spec")], + ["a whole row", ...span(simple, "Monday | write the spec")], + ["the whole table", 0, simple.length], + ])("keeps a pipe-less table rendering when anchoring %s", (_label, from, to) => { + const out = addComment(simple, from, to); + + expect(out).not.toBeNull(); + expect(renderedRows(out ?? "")).toBe(renderedRows(simple)); + }); + + test("anchors a whole row inside its outer pipes", () => { + const out = addComment(normal, ...span(normal, "| Monday | write the spec | ana |")); + + expect(out).toContain("| Monday | write the spec | ana |"); + }); + + test.each([ + ["where the header meets the delimiter", normal, ...span(normal, "|\n|")], + ["between two body rows", normal, ...spanWithin(normal, "ana |\n| Tuesday", "|\n|")], + ["around one cell separator", normal, ...spanWithin(normal, "Monday | write", " | ")], + ["around a pipe-less cell separator", simple, ...spanWithin(simple, "Monday | write", " | ")], + ])("refuses a selection of nothing but borders %s", (_label, doc, from, to) => { + const result = computeAddComment(doc, from, to, { + id: "aa11", + createdAt: "2026-01-01T00:00:00.000Z", + author: "me", + text: "hi", + }); + + expect(result.isErr() && result.error).toBe("Select the text inside a table cell, not its borders."); + }); + + // A marker between a backslash and the pipe it escapes un-escapes that pipe: in + // the app the cell splits in two and the marker itself shows as text. + test.each([ + ["the escaped pipe", "\\|", "\\|"], + ["just its pipe", "|", "\\|"], + ["text ending on its backslash", "this \\", "this \\|"], + ["text starting on its pipe", "| that", "\\| that"], + ])("keeps an escaped pipe whole when anchoring %s", (_label, part, anchored) => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\| that |"].join("\n"); + const [from, to] = spanWithin(doc, "this \\| that", part); + const [anchoredFrom, anchoredTo] = spanWithin(doc, "this \\| that", anchored); + + expect(clampToTableCells(doc, from, to)).toEqual({ from: anchoredFrom, to: anchoredTo }); + }); + + test("treats a pipe after an escaped backslash as the separator it is", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\\\| that |"].join("\n"); + const [from, to] = spanWithin(doc, "this \\\\| that", "this \\\\"); + + expect(clampToTableCells(doc, from, to)).toEqual({ from, to }); + }); + + test("writes the markers outside an escaped pipe, never between it and its backslash", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | this \\| that |"].join("\n"); + + expect(addComment(doc, ...spanWithin(doc, "this \\| that", "|"))).toContain( + "| Monday | this \\| that |", + ); + }); + + test("leaves a collapsed selection alone", () => { + const at = normal.indexOf("Monday"); + + expect(clampToTableCells(normal, at, at)).toEqual({ from: at, to: at }); + }); + + test("pulls a selection starting above a table into its first cell", () => { + const out = addComment(between, between.indexOf("\n\n"), between.indexOf("ana") + 3); + + expect(out).toContain("Intro text\n\n| Day | Task | Owner |"); + }); + + test("keeps a triple-clicked paragraph's markers off the blank line above a table", () => { + const out = addComment(between, 0, between.indexOf("\n\n") + 1); + + expect(out).toContain("Intro text\n"); + expect(out).toContain("-->\n\n| Day | Task | Owner |"); + }); + + test("leaves a prose selection and its trailing whitespace exactly as they were", () => { + // The blank line this one ends on sits above more prose, not a table. + const doc = ["Intro text", "", "More prose.", "", normal].join("\n"); + const to = doc.indexOf("\n\n") + 1; + + expect(clampToTableCells(doc, 0, to)).toEqual({ from: 0, to }); + }); + + test("leaves a selection outside any table alone", () => { + const doc = "Some prose with a | pipe in it."; + + expect(clampToTableCells(doc, 5, 10)).toEqual({ from: 5, to: 10 }); + }); + + test("trims the border and padding a clamped selection picks up", () => { + const [from, to] = span(normal, "| Monday "); + + expect(clampToTableCells(normal, from, to)).toEqual({ + from: normal.indexOf("Monday"), + to: normal.indexOf("Monday") + "Monday".length, + }); + }); + + test("keeps an interior separator, which is a real multi-cell selection", () => { + const [from, to] = span(normal, "Monday | write"); + + expect(clampToTableCells(normal, from, to)).toEqual({ from, to }); + }); +}); + +// Every shape below is valid GFM, and every one renders as plain text in Obsidian +// — checked in the app, where only the control mounted a `.cm-table-widget`. A +// scanner that matched them would hand the margin row indices no widget has. +describe("recognizing only the tables Obsidian renders", () => { + const tableCount = (doc: string) => sourceTables(sourceLines(doc)).length; + + test("finds a table with both outer pipes", () => { + expect(tableCount("| Day | Task |\n| --- | --- |\n| Monday | spec |")).toBe(1); + }); + + test.each([ + ["indented by spaces", " | Day | Task |\n | --- | --- |\n | Monday | spec |"], + ["indented by a tab", "\t| Day | Task |\n\t| --- | --- |\n\t| Monday | spec |"], + ["nested in a list item", "- plan\n | Day | Task |\n | --- | --- |\n | Monday | spec |"], + ["inside a blockquote", "> | Day | Task |\n> | --- | --- |\n> | Monday | spec |"], + ["headed with only a leading pipe", "| Day | Task\n| --- | ---\n| Monday | spec"], + ["headed with only a trailing pipe", "Day | Task |\n--- | --- |\nMonday | spec |"], + ["directly below a line of prose", "Intro text\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + [ + "directly below a marker alone on its line", + "\n| Day | Task |\n| --- | --- |\n| Monday | spec |", + ], + ["directly below a comment's own block", '\n| Day | Task |\n| --- | --- |'], + ])("finds no table %s", (_shape, doc) => { + expect(tableCount(doc)).toBe(0); + }); + + test.each([ + ["below a blank line", "Intro text\n\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ["below a line of only spaces", "Intro text\n \n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ["directly below a heading", "## Plan\n| Day | Task |\n| --- | --- |\n| Monday | spec |"], + ])("finds a table %s", (_shape, doc) => { + expect(tableCount(doc)).toBe(1); + }); +}); diff --git a/test/table-highlights.test.ts b/test/table-highlights.test.ts index dd72661..07ec7ea 100644 --- a/test/table-highlights.test.ts +++ b/test/table-highlights.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "vitest"; import { parseComments } from "../src/format/parse"; import { mapTableWidgets, + tableCellForRange, tableHighlightName, tableHighlightRule, tableHighlightTargets, @@ -22,8 +23,8 @@ describe("tableHighlightTargets", () => { ].join("\n"); expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ - { table: 0, row: 0, column: 0, quote: "Day", resolved: true, author: "me" }, - { table: 0, row: 1, column: 1, quote: "ship", resolved: false, author: "me" }, + { table: 0, row: 0, column: 0, id: "h1", quote: "Day", resolved: true, author: "me" }, + { table: 0, row: 1, column: 1, id: "t1", quote: "ship", resolved: false, author: "me" }, ]); }); @@ -45,11 +46,96 @@ describe("tableHighlightTargets", () => { ].join("\n"); expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ - { table: 0, row: 1, column: 1, quote: "two", resolved: false, author: "me" }, - { table: 1, row: 1, column: 0, quote: "three", resolved: false, author: "me" }, + { table: 0, row: 1, column: 1, id: "a1", quote: "two", resolved: false, author: "me" }, + { table: 1, row: 1, column: 0, id: "b1", quote: "three", resolved: false, author: "me" }, ]); }); + test("stops a pipe table where Obsidian stops it, not at the last line with a pipe", () => { + // Obsidian continues an outer-pipe table only while a line STARTS with a pipe. + // Treating any line containing one as a row used to claim the prose below as + // row 2 and hunt for a cell the rendered table doesn't have. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "Prose with a | pipe and a commented phrase here.", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test("stops at a row whose leading pipe a comment marker displaced", () => { + // Anchoring a whole row puts the opening marker before the leading pipe, which + // drops that row out of the table — so no cell exists to highlight. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test("accepts the single-dash delimiter row Obsidian allows", () => { + const doc = [ + "| Day | Task |", + "| - | :-: |", + "| Monday | spec |", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([ + { table: 0, row: 1, column: 1, id: "aa11", quote: "spec", resolved: false, author: "me" }, + ]); + }); + + test("ignores a pipe line that no delimiter row follows", () => { + const doc = [ + "Shopping | list", + "eggs | milk", + '", + ].join("\n"); + + expect(tableHighlightTargets(doc, parseComments(doc))).toEqual([]); + }); + + test.each([ + ["outside any table", "Some prose."], + ["on the delimiter row", "| --- | --- |"], + ])("finds no cell for a range starting %s", (_label, text) => { + const doc = ["Some prose.", "", "| Day | Task |", "| --- | --- |", "| Monday | spec |"].join("\n"); + const from = doc.indexOf(text) + 2; + + expect(tableCellForRange(doc, { from, to: from + 1 })).toBeNull(); + }); + + test("places an anchor spanning two rows on the row it starts in", () => { + // It can't be painted — no one cell holds it — but its card still belongs + // beside that row rather than at the top of the table. + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | spec |", "| Tuesday | review |"].join("\n"); + const from = doc.indexOf("spec"); + const range = { from, to: doc.indexOf("review") + "review".length }; + + expect(tableCellForRange(doc, range)).toEqual({ table: 0, row: 1, column: 1, whole: false }); + expect(tableCellForRange(doc, { from, to: from + 4 })).toEqual({ + table: 0, + row: 1, + column: 1, + whole: true, + }); + }); + test("uses separate stable registry names for each color and state", () => { expect(tableHighlightName("#0090ff", false)).toBe("document-comments-table-open-0090ff"); expect(tableHighlightName("#0090ff", true)).toBe("document-comments-table-resolved-0090ff"); @@ -57,6 +143,18 @@ describe("tableHighlightTargets", () => { expect(tableHighlightName(null, false)).toBe("document-comments-table-open-default"); }); + test("gives the hovered comment its own registry name and stronger rule", () => { + // A CSS Custom Highlight carries no element, so `is-active` has nowhere to + // live — the hovered range moves to a separate name with its own rule. + expect(tableHighlightName("#0090ff", false, true)).toBe("document-comments-table-open-active-0090ff"); + expect(tableHighlightName("#0090ff", true, true)).toBe("document-comments-table-resolved-active-0090ff"); + expect(tableHighlightName("#0090ff", false, true)).not.toBe(tableHighlightName("#0090ff", false)); + + // Matches the 18% / 38% pair the DOM highlight uses for prose. + expect(tableHighlightRule("#0090ff", false, true)).toContain("color-mix(in srgb, #0090ff 38%, transparent)"); + expect(tableHighlightRule("#0090ff", true, true)).toContain("color-mix(in srgb, #0090ff 38%, transparent)"); + }); + test("renders open and resolved table colors with distinct treatments", () => { expect(tableHighlightRule("#0090ff", false)).toContain("background-color: color-mix"); expect(tableHighlightRule("#0090ff", false)).toContain("text-decoration-style: solid"); diff --git a/test/table-repair.test.ts b/test/table-repair.test.ts new file mode 100644 index 0000000..0690f05 --- /dev/null +++ b/test/table-repair.test.ts @@ -0,0 +1,329 @@ +import { describe, expect, test } from "vitest"; +import { brokenTableAnchors, computeRepairTableAnchors } from "../src/editor/table-repair"; +import { applyChanges } from "../src/editor/edits"; +import { parseComments } from "../src/format/parse"; + +const body = (id: string, quote: string) => + [`"].join("\n"); + +const repair = (doc: string) => { + const result = computeRepairTableAnchors(doc); + if (result.isErr()) throw new Error(result.error); + return applyChanges(doc, result.value); +}; + +describe("brokenTableAnchors", () => { + test("finds an anchor that displaced a row's leading pipe", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("finds an anchor that displaced the header's trailing pipe", () => { + const doc = ["| Day | Task |", "| --- | --- |", "| Monday | spec |", body("aa11", "x")].join( + "\n", + ); + const withOpen = doc.replace("| Day", "| Day"); + + expect([...brokenTableAnchors(withOpen)]).toEqual(["aa11"]); + }); + + test("leaves a healthy in-cell anchor alone", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "spec"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("does not flag prose that merely starts with a pipe", () => { + // The line looks damaging, but no table appears when the markers come off, + // which is what the block-scoped confirmation is for. + const doc = ["Some prose.", "", "| not a table | at all", body("aa11", "x")].join( + "\n", + ); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("still finds a broken anchor beside an orphaned comment", () => { + // An orphan keeps its body after its anchored text is deleted, so it has no + // markers in the block at all. It has nothing to move and must not stop the + // anchors that do from being found. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "deleted text"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("offers no repair while a comment in the table has lost one marker", () => { + // Stripping a lone marker would delete it with nothing to put back. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "review"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("finds an anchor that filled the blank line above a table", () => { + // A triple-click selects a paragraph AND its newline, so the close marker + // used to land on the blank line a table needs above it. + const doc = [ + "Intro text", + "", + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "Intro text"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("does not flag a code comment's own-line markers", () => { + const doc = [ + "", + "```js", + "const a = 1;", + "```", + "", + "| Day | Task |", + "| --- | --- |", + '", + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual([]); + }); + + test("is empty for an empty document", () => { + expect([...brokenTableAnchors("")]).toEqual([]); + expect(computeRepairTableAnchors("").isOk()).toBe(true); + }); + + test("is empty for a document with no comments", () => { + expect([...brokenTableAnchors("| Day |\n| --- |\n| Monday |")]).toEqual([]); + }); +}); + +describe("computeRepairTableAnchors", () => { + test("moves the anchor inside the row and restores the table", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("repairs two broken anchors in the same table", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "y"), + ].join("\n"); + + const out = repair(doc); + + expect(brokenTableAnchors(out)).toEqual(new Set()); + expect(out).toContain("| Monday | spec |"); + expect(out).toContain("| Tuesday | review |"); + }); + + test("keeps a healthy neighbour's markers exactly where they were", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "x"), + body("bb22", "review"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Tuesday | review |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("keeps a nested anchor intact while repairing the one around it", () => { + // A legacy whole-table anchor around a healthy in-cell comment is exactly the + // shape repair exists to undo, and the two ranges nest. Reinserting anchor by + // anchor used to shift the outer end and split the inner close marker in half. + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "whole table"), + body("bb22", "spec"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(out).not.toContain("| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + "| Tuesday | review |", + body("aa11", "whole table"), + body("bb22", "spec"), + ].join("\n"); + + expect([...brokenTableAnchors(doc)]).toEqual(["aa11"]); + }); + + test("repairs past an orphaned comment and leaves its body alone", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + body("bb22", "deleted text"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Monday | spec |"); + expect(out).toContain(body("bb22", "deleted text")); + }); + + test("rewrites only the table's own block, not the paragraphs around it", () => { + const table = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + const doc = ["Before.", "", table, "", "After."].join("\n"); + const result = computeRepairTableAnchors(doc); + + expect(result.isOk() && result.value.map(({ from, to }) => [from, to])).toEqual([ + [doc.indexOf(table), doc.indexOf(table) + table.length], + ]); + }); + + test("moves a marker off the blank line above a table and restores the table", () => { + const doc = [ + "Intro text", + "", + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "Intro text"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("Intro text\n\n| Day | Task |"); + expect(brokenTableAnchors(out)).toEqual(new Set()); + }); + + test("keeps a literal marker inside a code span when repairing its table", () => { + // The parser skips code, so a marker written out in a code span is text, not + // an anchor. Repair has to leave it exactly as it found it. + const doc = [ + "| Syntax | Meaning |", + "| --- | --- |", + "| `` | opens a comment |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| `` | opens a comment |"); + expect(out).toContain("| Monday | spec |"); + }); + + test("keeps markers a stray backtick hides from the parser when repairing their table", () => { + const doc = [ + "| Day | Note | Code |", + "| --- | --- | --- |", + "| Monday | spec | x |", + "| Tuesday | it`s good | `code` |", + body("aa11", "x"), + body("bb22", "good"), + ].join("\n"); + + const out = repair(doc); + + expect(out).toContain("| Tuesday | it`s good | `code` |"); + expect(out).toContain("| Monday | spec | x |"); + }); + + test("leaves the body block and thread untouched", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + + expect(repair(doc)).toContain(body("aa11", "x")); + }); + + test("writes nothing when there is nothing to repair", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "spec"), + ].join("\n"); + const result = computeRepairTableAnchors(doc); + + expect(result.isOk() && result.value).toEqual([]); + }); + + test("honors an id filter", () => { + const doc = [ + "| Day | Task |", + "| --- | --- |", + "| Monday | spec |", + body("aa11", "x"), + ].join("\n"); + const result = computeRepairTableAnchors(doc, new Set(["other"])); + + expect(result.isOk() && result.value).toEqual([]); + }); +});