Conversation
huacnlee
left a comment
There was a problem hiding this comment.
Thanks for this. The need is real: selected_text() cannot tell which of two identical runs was selected, and a source byte range is the right shape for hosts that quote, annotate or edit the selection. It also fits the existing MarkdownNode::source_range() contract, and returning None instead of a guessed range is the right policy.
The implementation is not there yet, though. I reproduced each point below with temporary tests on this branch.
Wrong ranges (worse than None)
- Escaped backslash maps one byte early (
format/markdown.rs:196).source_segmentstries the literal-char branch before the backslash-escape branch, so\\is consumed as one byte and the next char absorbs the leftover. Sourcea\\brendersa\b; selecting the renderedb(2..3) returnsSome(2..4)(\b) instead of3..4. Checkremainder.strip_prefix('\\')beforeremainder.starts_with(rendered_char). - The
first.source.start -= 1adjustment fires on the previous node's escape (format/markdown.rs:217). It only checks that the byte before the span is\, so a preceding\\shifts the following inline math or hard break one byte early (a\\$x$, selecting$returns2..4instead of3..4) and can make a cross-node selection returnNonebecause the segments now overlap. Restrict it to Text nodes whose own first char is the escaped one. - Inline images never contribute their span (
node.rs:1279), so the result disagrees withselected_source(), which includeswhen the selection reaches the image edge. A host that deletes the returned range drops the image the clipboard copy kept. Either giveImageNodea span (markdown.rs:422discardsraw.position) and merge it under the same conditions, or returnUnmappedwhen the selection reaches an image.
Coverage (None for most real content)
- Inline code, footnote references, MDX expressions and
$$blocks can never map (format/markdown.rs:397,450,477,619). Their spans start with a delimiter thatsource_segmentsdoes not skip, so the segment list is always empty.`code` x: selectingcodereturnsNone, selectingxmaps.selected_source_range_rejects_unmapped_gap_in_merged_styled_nodepasses only because inline code is unmappable, not because a gap was detected.code_source_segmentsalready strips fences and could serve these nodes. - A soft break with a container prefix, continuation indentation or trailing spaces, or any HTML entity, makes the whole Text node unmappable (
format/markdown.rs:192).a\n b,> a\n> b,- a\n b,a \nball rendera band selecting0..1returnsNone(plaina\nbworks).Copyright © 2024: selectingCopyrightreturnsNone. Hard-wrapped paragraphs inside lists and blockquotes are the most common shape of LLM and README Markdown. Align line by line and emit a gap segment for the unmatched part instead of bailing on the node. - Multi-line indented code blocks and fences nested in a list or blockquote yield no segments (
format/markdown.rs:263). markdown-rs strips per-line indentation and container prefixes fromvalue, somatch_indices(code)on the raw span never matches; only single-line indented blocks and top-level fences work, which is exactly the tested set. Map line i ofvalueto the i-th raw line in the body instead of searching for the whole body.
Structure
- Doc comment displaced (
node.rs:1216).selected_source_rangewas inserted betweenselected_source's doc comment ("Reconstruct the Markdown source for the current selection…") and its body, so that comment now documents the new method andselected_sourcehas none. - One 32-byte
SourceSegmentper rendered character (node.rs:534), rewritten per emphasis/link nesting level and deep-copied with every streaming append. Storing equal-length runs and interpolating on lookup gives identical results with one segment per plain-text run or code block. Fine as a follow-up, but worth doing before this ships in streaming views.
Not blocking: the streaming tail-reparse leaves the last block unselected until the next paint, but that is pre-existing and affects selected_text() too.
Happy to re-review once the two wrong-range bugs and the doc comment are fixed and inline code plus list/blockquote paragraphs map.
|
Unfortunately the diff grew quite a bit, but I've addressed the bugs and missing coverage |
Description
This pull request adds a new TextViewState::selected_source_range() API. It returns the byte range in the original Markdown source corresponding to the text currently selected in a rendered TextView.
When the user selects text in rendered markdown, we are currently only exposed the text which the user selected, but not which range of bytes this actually is. This means if there are duplicate instances of the selected text in the markdown, we cannot reliably know which the user selected. This API fixes that.
Public API
List every public item this pull request adds, changes or removes, grouped by crate, with its signature and one line on what it is for. Include JavaScript methods and TypeScript declarations. If none, remove this section.
gpui_base::text::TextViewState::selected_source_range(&self) -> Option<Range<usize>>— Returns the original Markdown source byte range for the actual rendered selection.How to Test
Added tests for:
Checklist
cargo runfor story tests related to the changes.