From 0f9d98730eecfc5234d34de98bbc5d06127f372a Mon Sep 17 00:00:00 2001 From: Chris LaFreniere <40371649+chlafreniere@users.noreply.github.com> Date: Tue, 3 Mar 2020 16:56:11 -0800 Subject: [PATCH] Improve find by tracking actual rendered text (#9419) * Improve find by tracking actual rendered text * Fix tests, add notebook md render --- .../browser/cellViews/textCell.component.ts | 14 ++++++++++++ .../notebook/find/notebookFindModel.ts | 3 +-- .../notebookFindModel.test.ts | 22 ++++++++++++++++++- .../services/notebook/browser/models/cell.ts | 9 ++++++++ .../browser/models/modelInterfaces.ts | 1 + 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts index ba96a423d4..4a9a3fae94 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellViews/textCell.component.ts @@ -200,6 +200,7 @@ export class TextCellComponent extends CellView implements OnInit, OnChanges { this.setLoading(false); let outputElement = this.output.nativeElement; outputElement.innerHTML = this.markdownResult.element.innerHTML; + this.cellModel.renderedOutputTextContent = this.getRenderedTextOutput(); } } @@ -331,4 +332,17 @@ export class TextCellComponent extends CellView implements OnInit, OnChanges { } return children; } + + private getRenderedTextOutput(): string[] { + let textOutput: string[] = []; + let elements = this.getHtmlElements(); + elements.forEach(element => { + if (element && element.innerText) { + textOutput.push(element.innerText); + } else { + textOutput.push(''); + } + }); + return textOutput; + } } diff --git a/src/sql/workbench/contrib/notebook/find/notebookFindModel.ts b/src/sql/workbench/contrib/notebook/find/notebookFindModel.ts index d07d552f49..88d0021d46 100644 --- a/src/sql/workbench/contrib/notebook/find/notebookFindModel.ts +++ b/src/sql/workbench/contrib/notebook/find/notebookFindModel.ts @@ -527,9 +527,8 @@ export class NotebookFindModel extends Disposable implements INotebookFindModel private searchFn(cell: ICellModel, exp: string, matchCase: boolean = false, wholeWord: boolean = false, maxMatches?: number): NotebookRange[] { let findResults: NotebookRange[] = []; - let cellVal = cell.cellType === 'markdown' ? this.cleanUpCellSource(cell.source) : cell.source; + let cellVal = cell.cellType === 'markdown' ? cell.renderedOutputTextContent : cell.source; if (cellVal) { - if (typeof cellVal === 'string') { let findStartResults = this.search(cellVal, exp, matchCase, wholeWord, maxMatches); findStartResults.forEach(start => { diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts index 3917f57ee5..b406e86072 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/notebookFindModel.test.ts @@ -28,6 +28,7 @@ import { ClientSession } from 'sql/workbench/services/notebook/browser/models/cl import { TestStorageService } from 'vs/workbench/test/browser/workbenchTestServices'; import { NotebookEditorContentManager } from 'sql/workbench/contrib/notebook/browser/models/notebookInput'; import { NotebookRange } from 'sql/workbench/services/notebook/browser/notebookService'; +import { NotebookMarkdownRenderer } from 'sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown'; let expectedNotebookContent: nb.INotebookContents = { cells: [{ @@ -69,6 +70,7 @@ suite('Notebook Find Model', function (): void { let defaultModelOptions: INotebookModelOptions; const logService = new NullLogService(); let model: NotebookModel; + let markdownRenderer: NotebookMarkdownRenderer = new NotebookMarkdownRenderer(); setup(async () => { sessionReady = new Deferred(); @@ -108,12 +110,15 @@ suite('Notebook Find Model', function (): void { }); test('Should find results in the notebook', async function (): Promise { + // Need to set rendered text content for 2nd cell + setRenderedTextContent(1); + //initialize find let notebookFindModel = new NotebookFindModel(model); await notebookFindModel.find('markdown', false, false, max_find_count); assert(notebookFindModel.findMatches, 'Find in notebook failed.'); - assert.equal(notebookFindModel.findMatches.length, 2, 'Find couldnt find all occurances'); + assert.equal(notebookFindModel.findMatches.length, 2, 'Find couldnt find all occurrences'); }); test('Should not find results in the notebook', async function (): Promise { @@ -125,6 +130,9 @@ suite('Notebook Find Model', function (): void { }); test('Should match find result ranges', async function (): Promise { + // Need to set rendered text content for 2nd cell + setRenderedTextContent(1); + let notebookFindModel = new NotebookFindModel(model); await notebookFindModel.find('markdown', false, false, max_find_count); @@ -154,6 +162,9 @@ suite('Notebook Find Model', function (): void { }; await initNotebookModel(markdownContent); + // Need to set rendered text content for 1st cell + setRenderedTextContent(0); + let notebookFindModel = new NotebookFindModel(model); await notebookFindModel.find('best', false, false, max_find_count); @@ -190,6 +201,9 @@ suite('Notebook Find Model', function (): void { }); test('Should find results correctly with & without matching case selection', async function (): Promise { + // Need to set rendered text content for 2nd cell + setRenderedTextContent(1); + //initialize find let notebookFindModel = new NotebookFindModel(model); await notebookFindModel.find('insert', false, false, max_find_count); @@ -285,4 +299,10 @@ suite('Notebook Find Model', function (): void { await model.requestModelLoad(); } + function setRenderedTextContent(cellIndex: number): void { + model.cells[cellIndex].renderedOutputTextContent = [markdownRenderer.render({ + isTrusted: true, + value: model.cells[cellIndex].source[0] + }).element.innerText.toString()]; + } }); diff --git a/src/sql/workbench/services/notebook/browser/models/cell.ts b/src/sql/workbench/services/notebook/browser/models/cell.ts index 6dcf47173d..136d413781 100644 --- a/src/sql/workbench/services/notebook/browser/models/cell.ts +++ b/src/sql/workbench/services/notebook/browser/models/cell.ts @@ -37,6 +37,7 @@ export class CellModel implements ICellModel { private _cellGuid: string; private _future: FutureInternal; private _outputs: nb.ICellOutput[] = []; + private _renderedOutputTextContent: string[] = []; private _isEditMode: boolean; private _onOutputsChanged = new Emitter(); private _onCellModeChanged = new Emitter(); @@ -464,6 +465,14 @@ export class CellModel implements ICellModel { return this._outputs; } + public get renderedOutputTextContent(): string[] { + return this._renderedOutputTextContent; + } + + public set renderedOutputTextContent(content: string[]) { + this._renderedOutputTextContent = content; + } + private handleReply(msg: nb.IShellMessage): void { // TODO #931 we should process this. There can be a payload attached which should be added to outputs. // In all other cases, it is a no-op diff --git a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts index c81e9191ca..b4f42eb12c 100644 --- a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts +++ b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts @@ -468,6 +468,7 @@ export interface ICellModel { executionCount: number | undefined; readonly future: FutureInternal; readonly outputs: ReadonlyArray; + renderedOutputTextContent?: string[]; readonly onOutputsChanged: Event; readonly onExecutionStateChange: Event; readonly executionState: CellExecutionState;