Skip to content

find returns only the first match, and every workaround loses normalisation or column correctness #264

Description

@vyncint

TodayScreen::find returns the first match and there is no way to ask for the others. Measured against 0.9.0 on a screen showing item on three rows:

s.find("item")        // Some((0, 0))  — and that is all there is

There is no find_all, no count, and no way to start a search from a position. The workarounds each give up something the crate is careful about:

  • s.text().matches("item").count() is byte-exact str matching, so it loses the NFC folding contains and find apply on both sides — a screen showing café and a needle typed as cafe\u{301} agree in find and disagree here.
  • Scanning row_text(row) per row loses the double-width column mapping that find does, so a match after a CJK character reports the wrong column.
  • Either way, a needle that spans rows — which find supports — cannot be handled at all.

Why it is worth fixing — two ordinary assertions cannot be written today. "This warning appears exactly once" is a real thing to assert about a TUI that deduplicates its messages, and the natural spelling counts matches. "Click the second item in the list" needs the coordinates of a later match, and clicking is a first-class part of this crate — find exists largely to feed a position to click.

Both are answerable with the machinery find already has; the search just stops after the first hit. Anyone reaching for the workarounds is quietly opting out of normalisation or column correctness, which is exactly the kind of silent downgrade the rest of the API is built to prevent.

Fix — one method beside find, sharing its implementation:

impl Screen {
    /// Every occurrence of `needle`, in reading order — the `(row, col)` of
    /// each match's first character. Matching is identical to
    /// [`find`](Self::find): NFC on both sides, trailing whitespace trimmed
    /// per row, real columns across double-width characters.
    pub fn find_all(&self, needle: &str) -> Vec<(u16, u16)>;
}

find then becomes the first element of the same scan, so the two cannot drift — which matters, because find's rustdoc promises it agrees with contains, and a second implementation would be a second thing to keep in agreement.

Three decisions to make in the PR and write into the rustdoc:

  • Overlapping matches. find_all("aa") on aaaa is either two matches or three. Non-overlapping (advance past each match) is what str::matches does and the least surprising; say so explicitly rather than leaving it to be discovered.
  • Multi-row needles. find supports a needle containing \n; find_all should behave the same way or refuse, and the docs must say which.
  • Vec or an iterator. A Vec is simpler and a screen holds at most a few thousand cells, so allocation is not the concern; an iterator borrows the Screen and composes better. Either is defensible — pick one and say why.

A count method is deliberately not proposed: find_all(x).len() reads fine and one method is easier to keep honest than two.

Done whenfind_all returns every match in reading order with the same normalisation, trimming and column handling as find; find is implemented in terms of the same scan; the overlap and multi-row behaviours are documented and tested; and a test covers matches on several rows, a match after a wide character, and a needle that appears nowhere.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions