Skip to content

Commit

Permalink
Fix highlight sequencing when replacing text in code cells (#15803)
Browse files Browse the repository at this point in the history
* Fix typo in comment

* Fix currentIndex logic when replacing search match

* Additional test for selection in mid-cell

* Tests code cells specifically

* Lint fixes

* Isolates code cell mid-cell test

* Update packages/codemirror/src/searchprovider.ts

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

* Simplifies logic, deferring to caller to call highlightNext

---------

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
  • Loading branch information
JasonWeill and krassowski committed Feb 16, 2024
1 parent 1a98dea commit b689a68
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 6 deletions.
14 changes: 8 additions & 6 deletions packages/codemirror/src/searchprovider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,9 @@ export abstract class EditorSearchProvider<
*
* The caller of this method is expected to call `highlightNext` if after
* calling `replaceCurrentMatch()` attribute `this.currentIndex` is null.
* It is necesary to let the caller handle highlighting because this
* It is necessary to let the caller handle highlighting because this
* method is used in composition pattern (search engine of notebook cells)
* and highligthing on the composer (notebook) level needs to switch to next
* and highlighting on the composer (notebook) level needs to switch to next
* engine (cell) with matches.
*
* @param newText The replacement text.
Expand All @@ -343,15 +343,17 @@ export abstract class EditorSearchProvider<
this.currentIndex < this.cmHandler.matches.length
) {
const match = this.getCurrentMatch();
// If cursor there is no match selected, highlight the next match
if (!match) {
this.currentIndex = null;
} else {
this.cmHandler.matches.splice(this.currentIndex, 1);
const cmMatchesRemaining = this.cmHandler.matches.length;

// End at the end of the CodeMirror matches list; do not loop
// Let the caller call highlightNext if we've reached the end of the current code cell
this.currentIndex =
this.currentIndex < this.cmHandler.matches.length
? Math.max(this.currentIndex - 1, 0)
: null;
this.currentIndex < cmMatchesRemaining ? this.currentIndex : null;

const substitutedText = options?.regularExpression
? match!.text.replace(this.query!, newText)
: newText;
Expand Down
25 changes: 25 additions & 0 deletions packages/notebook/test/searchprovider.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,31 @@ describe('@jupyterlab/notebook', () => {
expect(provider.currentMatchIndex).toBe(0);
});

it('should start in the middle of a code cell, replace, and highlight next', async () => {
panel.model!.sharedModel.deleteCellRange(0, 2);
panel.model!.sharedModel.insertCells(0, [
{ cell_type: 'code', source: 'test1 test2 test3' }
]);
panel.content.activeCellIndex = 0;
await provider.cellChangeHandled;
panel.content.mode = 'edit';

// Pick the spot before the second element.
await panel.content.activeCell!.editor!.setCursorPosition({
line: 0,
column: 'test1 '.length
});

await provider.startQuery(/test\d/, { selection: true });

expect(provider.currentMatchIndex).toBe(1);
let replaced = await provider.replaceCurrentMatch('bar');
expect(replaced).toBe(true);
const source = panel.model!.cells.get(0).sharedModel.getSource();
expect(source).toBe('test1 bar test3');
expect(provider.currentMatchIndex).toBe(1);
});

it('should substitute groups in regular expressions', async () => {
await provider.startQuery(/test(\d)/, undefined);
expect(provider.currentMatchIndex).toBe(0);
Expand Down

0 comments on commit b689a68

Please sign in to comment.