Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to search in notebook inputs (under experimental flag) #167952

Merged
merged 39 commits into from
Feb 13, 2023

Conversation

andreamah
Copy link
Contributor

@andreamah andreamah commented Dec 2, 2022

Affects #164926

@@ -666,7 +666,7 @@ export interface INotebookEditor {
getNextVisibleCellIndex(index: number): number | undefined;
getPreviousVisibleCellIndex(index: number): number | undefined;
find(query: string, options: INotebookSearchOptions, token: CancellationToken): Promise<CellFindMatchWithIndex[]>;
highlightFind(cell: ICellViewModel, matchIndex: number): Promise<number>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this argument wasn't be used by the implementation

@andreamah
Copy link
Contributor Author

notes about this PR:

  • this behavior is behind an experimental flag
  • tests will likely be added soon, but I just wanted reviewers to take a first look at the code, since it is such a large PR
  • there is some code that controls getting webview results from the find widget, but it's not enabled, even with the experimental flag on. I just left it in there because there's progress there that might be hard to separate from the Find Match code (and it might make some of the code make more sense)?

@andreamah andreamah marked this pull request as ready for review February 8, 2023 23:20
@vscodenpa vscodenpa added this to the February 2023 milestone Feb 8, 2023
Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make sure that extractSelectionLine doesn't run unless requested.

@@ -268,21 +312,6 @@ suite('SearchResult', () => {
assert.deepStrictEqual([{ elements: arrayToRemove, removed: true }], target.args[0]);
});

test('remove triggers change event', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a repeat test of a test with the same name above.

@@ -124,6 +124,8 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
readonly onDidChangeCellState = this._onDidChangeCellState.event;
private readonly _onDidChangeViewCells = this._register(new Emitter<INotebookViewCellsUpdateEvent>());
readonly onDidChangeViewCells: Event<INotebookViewCellsUpdateEvent> = this._onDidChangeViewCells.event;
private readonly _onWillChangeModel = this._register(new Emitter<NotebookTextModel | undefined>());
readonly onWillChangeModel: Event<NotebookTextModel | undefined> = this._onWillChangeModel.event;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing where this is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used here

src/vs/workbench/contrib/search/browser/replaceService.ts Outdated Show resolved Hide resolved
if (e.resource.scheme === network.Schemas.vscodeNotebookCell) {
const notebookResource = CellUri.parse(e.resource)?.notebook;
if (notebookResource) {
return this.editorService.save([...this.editorService.findEditors(notebookResource)]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should be doing this at the model level as well like for text files? I don't know which service would do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that saving the text models happened here

return this.textFileService.files.get(e.resource)?.save({ source: ReplaceService.REPLACE_SAVE_SOURCE });
(even if it's unsaved)

src/vs/workbench/contrib/search/browser/searchModel.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/search/browser/searchModel.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/search/browser/searchModel.ts Outdated Show resolved Hide resolved
rebornix
rebornix previously approved these changes Feb 10, 2023
roblourens
roblourens previously approved these changes Feb 13, 2023
@andreamah andreamah merged commit e4d5a17 into main Feb 13, 2023
@andreamah andreamah deleted the andreamah/issue148068 branch February 13, 2023 19:46
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants