Skip to content

Commit

Permalink
Fix search coming back in notebook and editor (jupyterlab#15443)
Browse files Browse the repository at this point in the history
* a test case to illustrate issue jupyterlab#14871

* cleanup unused code

* rename outline into highlight as it sounds more proper

* fix typo in variable name _unrenderedByHighligh → _unrenderedByHighlight

* Fix search coming back in notebook and editor

* Add missing return

* Rename snapshots and limit screenshot to notebook area

Remove no-op await locator line

* Update Playwright Snapshots

* Improve async test implementation to avoid early closing

* Update Playwright Snapshots

* Remove visual snapshots, use locator counts instead

and revert spurious snapshot updates

---------

Co-authored-by: Thierry Parmentelat <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
3 people authored Dec 13, 2023
1 parent 9844a6f commit 746ce2d
Show file tree
Hide file tree
Showing 9 changed files with 222 additions and 9 deletions.
58 changes: 58 additions & 0 deletions galata/test/jupyterlab/notebook-search-highlight.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright (c) Jupyter Development Team.
// Distributed under the terms of the Modified BSD License.
import { expect, galata, test } from '@jupyterlab/galata';
import * as path from 'path';

const TEST_FILENAME = 'search_highlight_notebook.ipynb';
const TEST_NEEDLE = 'come';

test.use({ tmpPath: 'test-search' });

test.beforeAll(async ({ request, tmpPath }) => {
const contents = galata.newContentsHelper(request);
await contents.uploadFile(
path.resolve(__dirname, `./notebooks/${TEST_FILENAME}`),
`${tmpPath}/${TEST_FILENAME}`
);
});

test.beforeEach(async ({ page, tmpPath }) => {
await page.notebook.openByPath(`${tmpPath}/${TEST_FILENAME}`);
await page.notebook.activate(TEST_FILENAME);
});

const HIGHLIGHTS_LOCATOR = '.cm-searching';

test('Open and close Search dialog, then add new code cell', async ({
page
}) => {
// search for our needle
await page.evaluate(async searchText => {
await window.jupyterapp.commands.execute('documentsearch:start', {
searchText
});
}, TEST_NEEDLE);

// wait for the search to complete
await page.waitForSelector('text=1/21');
expect(await page.locator(HIGHLIGHTS_LOCATOR).count()).toBeGreaterThanOrEqual(
4
);

// cancel search
await page.keyboard.press('Escape');

// expect the highlights to have gone
expect(await page.locator(HIGHLIGHTS_LOCATOR).count()).toEqual(0);

// insert a new code cell
await page.evaluate(async () =>
window.jupyterapp.commands.execute('notebook:insert-cell-below')
);

// wait an arbitrary amount of extra time
// and expect the highlights to be still gone
// regression-testing against #14871
await new Promise(resolve => setTimeout(resolve, 1000));
expect(await page.locator(HIGHLIGHTS_LOCATOR).count()).toEqual(0);
});
110 changes: 110 additions & 0 deletions galata/test/jupyterlab/notebooks/search_highlight_notebook.ipynb
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
{
"cells": [
{
"cell_type": "markdown",
"id": "e86bd77e-7e48-4941-8cb9-72410d8256aa",
"metadata": {},
"source": [
"# regular notebook (not myst)"
]
},
{
"cell_type": "markdown",
"id": "d06cb0c2-483b-4781-b283-8212ea667823",
"metadata": {},
"source": [
"## a subtitle\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "8541f40d-4f71-48fc-92d4-708063b476ec",
"metadata": {},
"source": [
"## another one\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "d896a643-8998-48cf-8a4f-ec2965656e73",
"metadata": {},
"source": [
"## yet another\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "a089ea8f-02b8-4b09-af17-aca38782b3c7",
"metadata": {},
"source": [
"## let's change\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "6f9ad439-3dc0-410c-9d98-2a9356f839a3",
"metadata": {},
"source": [
"## to a less predictible\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "a783fb23-b79c-40c7-b8fe-be597130a3ae",
"metadata": {},
"source": [
"## pattern in picking names\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
},
{
"cell_type": "markdown",
"id": "4e2ae1be-078e-4bf6-974b-6ea6750a74a9",
"metadata": {},
"source": [
"## the last one\n",
"\n",
"Where does it come from?\n",
"Contrary to popular belief, Lorem Ipsum is not simply random text. It has roots in a piece of classical Latin literature from 45 BC, making it over 2000 years old. Richard McClintock, a Latin professor at Hampden-Sydney College in Virginia, looked up one of the more obscure Latin words, consectetur, from a Lorem Ipsum passage, and going through the cites of the word in classical literature, discovered the undoubtable source. Lorem Ipsum comes from sections 1.10.32 and 1.10.33 of \"de Finibus Bonorum et Malorum\" (The Extremes of Good and Evil) by Cicero, written in 45 BC. This book is a treatise on the theory of ethics, very popular during the Renaissance. The first line of Lorem Ipsum, \"Lorem ipsum dolor sit amet..\", comes from a line in section 1.10.32.\n"
]
}
],
"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.11.5"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
10 changes: 5 additions & 5 deletions packages/cells/src/searchprovider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ class MarkdownCellSearchProvider extends CellSearchProvider {
const cell = this.cell as MarkdownCell;
if (cell.rendered && this.matchesCount > 0) {
// Unrender the cell
this._unrenderedByHighligh = true;
this._unrenderedByHighlight = true;
const waitForRendered = signalToPromise(cell.renderedChanged);
cell.rendered = false;
await waitForRendered;
Expand All @@ -347,7 +347,7 @@ class MarkdownCellSearchProvider extends CellSearchProvider {
const cell = this.cell as MarkdownCell;
if (cell.rendered && this.matchesCount > 0) {
// Unrender the cell if there are matches within the cell
this._unrenderedByHighligh = true;
this._unrenderedByHighlight = true;
const waitForRendered = signalToPromise(cell.renderedChanged);
cell.rendered = false;
await waitForRendered;
Expand Down Expand Up @@ -397,10 +397,10 @@ class MarkdownCellSearchProvider extends CellSearchProvider {
* @param rendered New rendered value
*/
protected onRenderedChanged(cell: MarkdownCell, rendered: boolean): void {
if (!this._unrenderedByHighligh) {
if (!this._unrenderedByHighlight) {
this.currentIndex = null;
}
this._unrenderedByHighligh = false;
this._unrenderedByHighlight = false;
if (this.isActive) {
if (rendered) {
void this.renderedProvider.startQuery(this.query);
Expand All @@ -413,7 +413,7 @@ class MarkdownCellSearchProvider extends CellSearchProvider {
}

protected renderedProvider: GenericSearchProvider;
private _unrenderedByHighligh = false;
private _unrenderedByHighlight = false;
}

/**
Expand Down
1 change: 0 additions & 1 deletion packages/documentsearch-extension/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,6 @@ const extension: JupyterFrontEndPlugin<ISearchProviderRegistry> = {
if (!currentWidget) {
return;
}

searchViews.get(currentWidget.id)?.close();
}
});
Expand Down
16 changes: 14 additions & 2 deletions packages/documentsearch/src/searchmodel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class SearchDocumentModel
}
}

searchProvider.stateChanged.connect(this.refresh, this);
searchProvider.stateChanged.connect(this._onProviderStateChanged, this);

this._searchDebouncer = new Debouncer(() => {
this._updateSearch().catch(reason => {
Expand Down Expand Up @@ -248,7 +248,10 @@ export class SearchDocumentModel
});
}

this.searchProvider.stateChanged.disconnect(this.refresh, this);
this.searchProvider.stateChanged.disconnect(
this._onProviderStateChanged,
this
);

this._searchDebouncer.dispose();
super.dispose();
Expand All @@ -258,6 +261,7 @@ export class SearchDocumentModel
* End the query.
*/
async endQuery(): Promise<void> {
this._searchActive = false;
await this.searchProvider.endQuery();
this.stateChanged.emit();
}
Expand Down Expand Up @@ -351,6 +355,7 @@ export class SearchDocumentModel
)
: null;
if (query) {
this._searchActive = true;
await this.searchProvider.startQuery(query, this._filters);
// Emit state change as the index needs to be updated
this.stateChanged.emit();
Expand All @@ -365,13 +370,20 @@ export class SearchDocumentModel
}
}

private _onProviderStateChanged() {
if (this._searchActive) {
this.refresh();
}
}

private _caseSensitive = false;
private _disposed = new Signal<this, void>(this);
private _parsingError = '';
private _preserveCase = false;
private _initialQuery = '';
private _filters: IFilters = {};
private _replaceText: string = '';
private _searchActive = false;
private _searchDebouncer: Debouncer;
private _searchExpression = '';
private _useRegex = false;
Expand Down
1 change: 1 addition & 0 deletions packages/fileeditor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"watch": "tsc -b --watch"
},
"dependencies": {
"@jupyter/ydoc": "^1.1.1",
"@jupyterlab/apputils": "^4.2.0-alpha.4",
"@jupyterlab/codeeditor": "^4.1.0-alpha.4",
"@jupyterlab/codemirror": "^4.1.0-alpha.4",
Expand Down
27 changes: 27 additions & 0 deletions packages/fileeditor/src/searchprovider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
import { ITranslator } from '@jupyterlab/translation';
import { Widget } from '@lumino/widgets';
import { FileEditor } from './widget';
import { ISharedText, SourceChange } from '@jupyter/ydoc';

/**
* Helper type
Expand Down Expand Up @@ -64,6 +65,7 @@ export class FileEditorSearchProvider
query: RegExp,
filters: IFilters | undefined
): Promise<void> {
this._searchActive = true;
await super.startQuery(query, filters);
await this.highlightNext(true, {
from: 'selection-start',
Expand All @@ -72,6 +74,29 @@ export class FileEditorSearchProvider
});
}

/**
* Stop the search and clean any UI elements.
*/
async endQuery(): Promise<void> {
this._searchActive = false;
await super.endQuery();
}

/**
* Callback on source change
*
* @param emitter Source of the change
* @param changes Source change
*/
protected async onSharedModelChanged(
emitter: ISharedText,
changes: SourceChange
): Promise<void> {
if (this._searchActive) {
return super.onSharedModelChanged(emitter, changes);
}
}

/**
* Instantiate a search provider for the widget.
*
Expand Down Expand Up @@ -116,4 +141,6 @@ export class FileEditorSearchProvider
);
return selection;
}

private _searchActive = false;
}
7 changes: 6 additions & 1 deletion packages/notebook/src/searchprovider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ export class NotebookSearchProvider extends SearchProvider<NotebookPanel> {
return;
}
await this.endQuery();
this._searchActive = true;
let cells = this.widget.content.widgets;

this._query = query;
Expand Down Expand Up @@ -426,6 +427,7 @@ export class NotebookSearchProvider extends SearchProvider<NotebookPanel> {
})
);

this._searchActive = false;
this._searchProviders.length = 0;
this._currentProviderIndex = null;
}
Expand Down Expand Up @@ -552,7 +554,9 @@ export class NotebookSearchProvider extends SearchProvider<NotebookPanel> {
this.widget.content.isSelectedOrActive(cell)
)
.then(() => {
void cellSearchProvider.startQuery(this._query, this._filters);
if (this._searchActive) {
void cellSearchProvider.startQuery(this._query, this._filters);
}
});
}

Expand Down Expand Up @@ -914,4 +918,5 @@ export class NotebookSearchProvider extends SearchProvider<NotebookPanel> {
> | null = null;
private _selectionSearchMode: 'cells' | 'text' = 'cells';
private _selectionLock: boolean = false;
private _searchActive: boolean = false;
}
1 change: 1 addition & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3466,6 +3466,7 @@ __metadata:
version: 0.0.0-use.local
resolution: "@jupyterlab/fileeditor@workspace:packages/fileeditor"
dependencies:
"@jupyter/ydoc": ^1.1.1
"@jupyterlab/apputils": ^4.2.0-alpha.4
"@jupyterlab/codeeditor": ^4.1.0-alpha.4
"@jupyterlab/codemirror": ^4.1.0-alpha.4
Expand Down

0 comments on commit 746ce2d

Please sign in to comment.