[7576]: Support for `vscode.workspace.findTextInFiles` API #7868

nmorenor · 2020-05-22T22:39:36Z

What it does

fix Support for vscode.workspace.findTextInFiles API #7576: Provides ability to execute the vscode api vscode.workspace.findTextInFiles

How to test

This was tested with the Codetogether extension since it does use this api.

Install the extension
Start a session via command palette.
On the remote session on browser go to the search view.
Search for some text in the workspace you should see search text results.

Alternatively you can use a test extension (https://github.com/Genuitec/theiatestsearch):

Install the extension
Invoke the SearchTest: Search from command palette and let the input box know what you are searchig for.
On the Search test tree view contribution you should see the search results

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

nmorenor · 2020-05-22T22:40:19Z

This fixes #7576

vince-fugnitto

@nmorenor thank you for your contribution, I have some general comments:

can you please update the pull-request description to follow the template? I'm not sure why parts were removed/omitted
can you please provide more meaningful steps on 'how to test' so reviewers can more easily test the changes (this can be a minimal custom extension for test purposes)
I'm not sure about the changes as they are proposed. They will likely be updated until they are finalized and that means that potential major refactoring are necessary. My previous comment still holds true: Support for vscode.workspace.findTextInFiles API #7576 (comment)

packages/plugin/src/theia.d.ts

packages/search-in-workspace/src/browser/search-in-workspace-service.ts

akosyakov

that's important that for VS Code compatibility we don't invent anything, but port VS Code code

It would be helpful if you could point to implementation in VS Code. For such copied code we also need CQs usually.

packages/search-in-workspace/src/browser/search-in-workspace-service.ts

packages/search-in-workspace/src/common/search-in-workspace-interface.ts

nmorenor · 2020-05-25T17:43:12Z

I'm not sure what is a CQ but here are the orignal interfaces for the API from VSCode. https://github.com/microsoft/vscode/blob/f5d10326f5ef42eb7b6e3a3ce7a50baa3c7d7ed4/src/vs/vscode.proposed.d.ts these had to be the same so I bring those interfaces to this change.

packages/search-in-workspace/src/common/search-in-workspace-interface.ts

packages/search-in-workspace/src/browser/search-in-workspace-service.ts

packages/plugin-ext/src/main/browser/workspace-main.ts

akosyakov · 2020-05-28T08:08:27Z

@vince-fugnitto Could you try please?

vince-fugnitto · 2020-06-01T13:26:40Z

@vince-fugnitto Could you try please?

I tried using the second use-case (the .vsxi provided by the following extension) and it looked to work correctly. I was able to see results in the search-test widget when searching for terms.

kittaakos · 2020-06-25T14:31:45Z

Is this the reference? https://github.com/microsoft/vscode/blob/54d4edde505fb23e0094e56ba15a902764384690/src/vs/workbench/api/common/extHostWorkspace.ts#L447-L536

nmorenor · 2020-06-26T00:23:33Z

Right, that will be the implementation on VSCode

kittaakos · 2020-06-26T08:49:24Z

I am verifying it with https://github.com/Genuitec/theiatestsearch.

I installed the extension to both Theia and VS Code. I have both Theia and VS Code opened on your PR as a workspace.
When I execute the SearchTest: Search and search for index, I have 152 results with Theia. In VS Code, I see 1242 results.

kittaakos · 2020-06-26T08:51:21Z

I have 152 results with Theia.

I see. You are limiting the hit counts to 150. It's a bug then.

Update: ✅ verified, works now.

kittaakos · 2020-06-26T09:20:01Z

I found another regression, search for körte with the search-in-workspace (SIW) (Ctrl/⌘+Shift+F) you have to see one single hit if you opened Theia as a workspace. Use your test extension in VS Code, and search for körte again, and again. You will have one single result. 👍 Now, do the same in Theia:

SIW works in Theia:

kittaakos · 2020-06-26T09:07:13Z

packages/plugin-ext/src/main/browser/workspace-main.ts

+                    } else if (defaultMaxHits < matches) {
+                        resolve({ limitHit: true });
+                    }
+                    resolve({ limitHit: false });


Let's clean this up, and move the resolve({ limitHit: false }); into an else.

kittaakos · 2020-06-26T09:25:36Z

Hmm, the hit-count feels random. Search for "foo" with SIW, I see 542 results, do the same with your test extension, I see 160 results.

kittaakos · 2020-06-26T09:30:02Z

packages/plugin-ext/src/plugin/workspace.ts

+                uri: URI.parse(result.fileUri),
+                preview: {
+                    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+                    text: isObject(next.lineText) ? (<any>next.lineText).text : <string>next.lineText,


Is this supposed to be a type-guard for string | LinePreview? If so, let's simplify to text: typeof next.lineText === 'string' ? next.lineText : next.lineText.text, or use a proper type-guard and get rid of the any.

packages/plugin/src/theia-proposed.d.ts

kittaakos · 2020-06-26T09:32:17Z

packages/search-in-workspace/src/browser/search-in-workspace-service.ts

+        return this.doSearch(what, roots.map(r => r.uri), callbacks, opts);
+    }
+
+    private async doSearch(what: string, rootsUris: string[], callbacks: SearchInWorkspaceCallbacks, opts?: SearchInWorkspaceOptions): Promise<number> {


Should be protected.

kittaakos · 2020-06-26T09:48:09Z

I think I found the problem, the main issue here:

Search for something that does not hit the max count, it's important, your search won't be canceled,
Search for the same thing, you have duplicate results.

Please fix, and name me if you want a new review.

nmorenor · 2020-06-29T22:47:51Z

Just pushed the fixes for @kittaakos observations. Thanks for the review, I did not notice these issues, since I was just doing one search test.

kittaakos · 2020-06-30T06:42:33Z

I found a difference; search for const index = , see that leading whitespaces are ignored in Theia.

VS Code:

Theia:

kittaakos · 2020-06-30T06:55:27Z

👍 It works nicely; one remark on the behavior and some minor formatting issues.

packages/plugin/src/theia-proposed.d.ts

kittaakos · 2020-06-30T06:33:26Z

packages/plugin-ext/src/common/plugin-api-rpc-model.ts

@@ -539,7 +539,6 @@ export interface CallHierarchyOutgoingCall {
    to: CallHierarchyItem;
    fromRanges: Range[];
 }
-


Let's revert this change.

nmorenor · 2020-06-30T15:40:23Z

@kittaakos Formatting issues have been fixed also reverted the remove of line on plugin-api-rpc-model.ts

Now regarding the white spaces on the Theia, it looks like the search does return the line text with the white spaces there but somehow the UI is hiding the trailing white spaces. The extension does have a console.log for each returned result and in there I do see the trailing spaces but somehow they are not rendered on the UI. Perhaps that can be looked on a different PR? Here a Screen Shot of the console.log

I see this when I attach to the Host plugin while running on Debug.

packages/core/src/browser/style/sidepanel.css

nmorenor · 2020-07-02T16:11:08Z

Ok, now the tree also display the trailing whitespaces, I did add a new css class inside sidebar.css noWrapInfoTree I did not wanted to move the other one, because it could take effect on other parts of the UI.

Signed-off-by: Ignacio Moreno <ignacio@genuitec.com>

kittaakos · 2020-07-02T16:12:35Z

Ok, now the tree also display the trailing whitespaces, I did add a new css class inside sidebar.css noWrapInfoTree I did not wanted to move the other one, because it could take effect on other parts of the UI.

Wow, great 👍 I did not expect you to fix the tree too, but thank you! I'll take a look tomorrow, and will merge the changes.

kittaakos

This new feature works very well; I had the same behavior and UX as in VS Code with the sample extension. Thank you for your contribution, @nmorenor 👍

@akosyakov, can you please press the merge button if you are fine with the RPC API and model changes. Thanks!

vince-fugnitto reviewed May 23, 2020

View reviewed changes

packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved

packages/plugin/src/theia.d.ts Outdated Show resolved Hide resolved

packages/search-in-workspace/src/browser/search-in-workspace-service.ts Outdated Show resolved Hide resolved

vince-fugnitto added the vscode issues related to VSCode compatibility label May 23, 2020

akosyakov added the search in workspace issues related to the search-in-workspace label May 25, 2020

akosyakov reviewed May 25, 2020

View reviewed changes

packages/search-in-workspace/src/browser/search-in-workspace-service.ts Outdated Show resolved Hide resolved

packages/search-in-workspace/src/common/search-in-workspace-interface.ts Outdated Show resolved Hide resolved

nmorenor force-pushed the master-7576-working branch from 9389ff9 to bc8abdf Compare May 25, 2020 17:20

nmorenor requested review from akosyakov and vince-fugnitto May 25, 2020 17:53

akosyakov reviewed May 26, 2020

View reviewed changes

packages/search-in-workspace/src/common/search-in-workspace-interface.ts Outdated Show resolved Hide resolved

akosyakov reviewed May 26, 2020

View reviewed changes

packages/search-in-workspace/src/browser/search-in-workspace-service.ts Outdated Show resolved Hide resolved

nmorenor force-pushed the master-7576-working branch 4 times, most recently from 9dbf90f to 15d7cf9 Compare May 26, 2020 15:40

nmorenor requested a review from akosyakov May 26, 2020 15:46

akosyakov reviewed May 28, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/workspace-main.ts Outdated Show resolved Hide resolved

akosyakov requested a review from RomanNikitenko May 28, 2020 08:08

nmorenor force-pushed the master-7576-working branch from 15d7cf9 to 1c7114d Compare June 4, 2020 19:00

nmorenor requested a review from akosyakov June 17, 2020 21:33

akosyakov requested a review from kittaakos June 24, 2020 07:26

kittaakos reviewed Jun 26, 2020

View reviewed changes

packages/plugin/src/theia-proposed.d.ts Show resolved Hide resolved

kittaakos reviewed Jun 26, 2020

View reviewed changes

nmorenor force-pushed the master-7576-working branch from 1c7114d to 5646e5d Compare June 29, 2020 22:45

nmorenor requested a review from kittaakos June 29, 2020 22:48

kittaakos reviewed Jun 30, 2020

View reviewed changes

nmorenor force-pushed the master-7576-working branch from 5646e5d to bd53574 Compare June 30, 2020 15:29

nmorenor requested a review from kittaakos June 30, 2020 15:40

vince-fugnitto reviewed Jul 2, 2020

View reviewed changes

packages/core/src/browser/style/sidepanel.css Outdated Show resolved Hide resolved

nmorenor force-pushed the master-7576-working branch from 463b47e to 3ba0e2f Compare July 2, 2020 16:06

[7576]: Support for vscode.workspace.findTextInFiles API

4e2c39a

Signed-off-by: Ignacio Moreno <ignacio@genuitec.com>

nmorenor force-pushed the master-7576-working branch from 3ba0e2f to 4e2c39a Compare July 2, 2020 16:13

kittaakos approved these changes Jul 3, 2020

View reviewed changes

akosyakov merged commit 7a88d28 into eclipse-theia:master Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7576]: Support for `vscode.workspace.findTextInFiles` API #7868

[7576]: Support for `vscode.workspace.findTextInFiles` API #7868

nmorenor commented May 22, 2020 •

edited by akosyakov

Loading

nmorenor commented May 22, 2020

vince-fugnitto left a comment

akosyakov left a comment

nmorenor commented May 25, 2020

akosyakov commented May 28, 2020

vince-fugnitto commented Jun 1, 2020

kittaakos commented Jun 25, 2020

nmorenor commented Jun 26, 2020

kittaakos commented Jun 26, 2020

kittaakos commented Jun 26, 2020 •

edited

Loading

kittaakos commented Jun 26, 2020

kittaakos Jun 26, 2020

kittaakos commented Jun 26, 2020

kittaakos Jun 26, 2020

kittaakos Jun 26, 2020

kittaakos commented Jun 26, 2020

nmorenor commented Jun 29, 2020

kittaakos commented Jun 30, 2020

kittaakos commented Jun 30, 2020

kittaakos Jun 30, 2020

nmorenor commented Jun 30, 2020

nmorenor commented Jul 2, 2020

kittaakos commented Jul 2, 2020

kittaakos left a comment

[7576]: Support for vscode.workspace.findTextInFiles API #7868

[7576]: Support for vscode.workspace.findTextInFiles API #7868

Conversation

nmorenor commented May 22, 2020 • edited by akosyakov Loading

What it does

How to test

Review checklist

Reminder for reviewers

nmorenor commented May 22, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

nmorenor commented May 25, 2020

akosyakov commented May 28, 2020

vince-fugnitto commented Jun 1, 2020

kittaakos commented Jun 25, 2020

nmorenor commented Jun 26, 2020

kittaakos commented Jun 26, 2020

kittaakos commented Jun 26, 2020 • edited Loading

kittaakos commented Jun 26, 2020

kittaakos Jun 26, 2020

Choose a reason for hiding this comment

kittaakos commented Jun 26, 2020

kittaakos Jun 26, 2020

Choose a reason for hiding this comment

kittaakos Jun 26, 2020

Choose a reason for hiding this comment

kittaakos commented Jun 26, 2020

nmorenor commented Jun 29, 2020

kittaakos commented Jun 30, 2020

kittaakos commented Jun 30, 2020

kittaakos Jun 30, 2020

Choose a reason for hiding this comment

nmorenor commented Jun 30, 2020

nmorenor commented Jul 2, 2020

kittaakos commented Jul 2, 2020

kittaakos left a comment

Choose a reason for hiding this comment

[7576]: Support for `vscode.workspace.findTextInFiles` API #7868

[7576]: Support for `vscode.workspace.findTextInFiles` API #7868

nmorenor commented May 22, 2020 •

edited by akosyakov

Loading

kittaakos commented Jun 26, 2020 •

edited

Loading