Update highlight when replace string matches search expression

[JasonWeill]

References

Fixes #15572, #13968, and #15809.

Code changes

Updates search providers so that, if the replacement string also matches the search query, the highlight and selection move to the next search result.

User-facing changes

After replacing a string with a superset of the string, such as foo with food, the next search result highlights. The just-replaced string does not remain highlighted.

Backwards-incompatible changes

None.

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[JasonWeill]

The way that we keep track of matches needs to be rewritten as a revision to this pull request. Especially because both find and replace support regular expressions, the number of matches may go up or down by any number as a result of a single "replace" operation. The "replace current" function needs to:

1. Replace the old text with the new text
2. Perform a search for the old text starting at a position of the previous match, plus the length of the new text
3. If there is a match between the start point and the end of a cell, highlight it, and set this.currentIndex. Otherwise, make this.currentIndex be null.

For reference, see CodeMirror’s search code’s “replaceNext'” function: https://github.com/codemirror/search/blob/0d8af3e4cc6e9e9bc2350952698015ff639ae857/src/search.ts#L465-L490

For the sake of the search dialog's result count, we might need to stop the search and start another search after the replacement is done. @krassowski please stop me if this change is getting far beyond the scope you'd expect for a fix of this nature. 😄

[JasonWeill]

@krassowski This WIP change forcibly highlights the next match if the replacement text also matches the query. This is not ideal; replacing bar with barbell (one match in the replacement text) will move the highlight to the following bar, but replacing bar with barbar (two matches) will move the highlight to the second half of the replacement text. Not sure whether this is expected; what do you think?

Also, I ran into a problem where, in the first cell with matches, I'm looping through the matches after a single step with no matches at all. I think this is related to #15809, specifically a quirk with the logic of how CodeCellSearchProvider works. See #15809 (comment).

[krassowski]

For the sake of the search dialog's result count, we might need to stop the search and start another search after the replacement is done.

I agree that search results count may change during the replace operation (or even during notebook loading); this does not mean that we should resort to restarting the search. I see these as two different things:

• stopping/restarting search is allowed to loose current index
• updating number or matches should not change anything other than the counter and non-active highlights

Some editors/browsers do not update search results on modification at all, until user presses the "next" button.

replacing bar with barbar (two matches) will move the highlight to the second half of the replacement text. Not sure whether this is expected; what do you think?

Based on a quick check of the most popular editors, I believe users would be surprised by such behaviour and instead expect the highlight to move to the next original match; here are the editors I tried:

• VSCode: [bar] bar - (replace) → barbar [bar] - (replace) → [bar]bar barbar
• PyCharm: [bar] bar - (replace) → barbar [bar] - (replace) → barbar barbar (shows hint "press F3 to start from top")
• LibreOffice Writer: [bar] bar - (replace) → barbar [bar] - (replace) → [bar]bar barbar
• Google Docs: [bar] bar - (replace) → barbar [bar] - (replace) → [bar]bar barbar

I used [] to indicate active highlight.

[krassowski]

The way that we keep track of matches needs to be rewritten as a revision to this pull request

I am thinking if we could just cleverly adjust currentIndex (and matches) by the amount of matches inserted to the left.

For example, starting with:

# currentIndex = 1
bar [bar] bar

if we replace active bar with foo we get:

# currentIndex = 1
bar foo [bar]

but if we instead replace with barbarbar we get:

# currentIndex = 4 (this is 1 + 3)
bar barbarbar [bar]

It looks like some logic from onSharedModelChanged/updateCodeMirror could be resued to populate new matches, so the only part where a new logic is needed is the custom currentIndex handling. Does it make sense?

[JasonWeill]

@krassowski Thanks for your patience! I'm ready to take another look at this change.

I had looked into searching the replaced text for matches, but what if the replacement creates an additional match beyond the replacement text?

For example, start with barr bar, replace bar with barbarba.

# currentIndex = 0
[bar]r bar

Replace first instance, and we expect the status to be:

# currentIndex = 3
barbarbar [bar]

… but if we compare barbarba for matches of bar, the original search query, there are only 2 matches, not 3. The third match includes a trailing r.

[JasonWeill]

Updated the PR to correctly update the highlight to, after replacing a target string with a replacement that contains the target string, move forward to the next highlight after the replacement, if possible. Please try it out.

[krassowski]

I tested it locally. The replacement behaviour looks good but there are two edge cases which are regressions:

1. When you press replace on the last match you see -/x rather than get the highlight back at the first match (1/x); you need to press replace again for it to cycle back to the front in the same cell
2. When you do press replace again instead of moving to the matches in the next cell, it moves you to the first match in the current cell

The lint check failures are relevant:

/home/runner/work/jupyterlab/jupyterlab/packages/codemirror/src/searchprovider.ts
Error:   364:9   error  Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator  @typescript-eslint/no-floating-promises
Error:   379:13  error  Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator  @typescript-eslint/no-floating-promises

And js-notebook test failure looks relevant too:

  ● @jupyterlab/notebook › NotebookSearchProvider › #replaceCurrentMatch() › should replace with a longer text and highlight next

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: null

[krassowski]

For avoidance of doubt here is the expected behaviour for looping and transition between cells as captured in released versions/main:

[JasonWeill]

From a little debugging, it looks like the new code in codemirror's search provider is getting executed after the code in notebook's search provider checks whether the current match is null. This results in the "-/N" search result status.

[JasonWeill]

The most recent code uses a promise for the codemirror updates.

[krassowski]

Is still do not see a progression between cell boundaries with the latest commit:

Maybe this PR should include a playwright test for this?

[krassowski]

In the scenario when search string is not a substring of the replacement string this works well now though:

I am not sure what to make of this.

[krassowski]

Noting that this does not modify the public API and can be merged in RC and probably even backported to 4.1.x

[JasonWeill]

The most recent code changes should fix the highlight order when replacing source text with target text that contains the source text.

Screen.Recording.2024-04-23.at.2.22.02.PM.mov

[JasonWeill]

@krassowski If you have time, could you please look at this revised PR? Thanks!

[krassowski]

Thanks for the ping! Sure, I will take a look today; do you happen to have time to review #16241 and #16214 ?

[krassowski]

It still does not work in one tricky case; let's not block this PR on it - I opened #16242 to track it.

[krassowski]

Thank you @JasonWeill, let's merge and iterate!