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

Fix search text in jars and add highlighting #918

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

Arthurm1
Copy link
Contributor

Quick fix for searching text in jars when using virtual documents.

Adds highlighting to search text. E.g. here is a search for Cipher...
image

@dos65 I felt guilty I broke your stuff

@dos65
Copy link
Member

dos65 commented Mar 16, 2022

@Arthurm1
No need to apologize!
I do break stuff too as others!

Tested it locally but it seems that there is some issues.
I test it using: *.java with query startsWith. In my case when I double-click on search results they aren't opening or opening with a significant delay.

@Arthurm1
Copy link
Contributor Author

@dos65 This should be better/faster - it uses the MetalsContentProvider directly.

The issue was that when you open a text document (even when it's not shown) then didOpen will fire causing a lot of processing in Metals. #815 would solve that but it would still be twice as slow as populating the text in Metals and passing that to VSCode. But that would involve a change to the API.

@dos65
Copy link
Member

dos65 commented Mar 17, 2022

@Arthurm1 oh, yep. I remembered that we already had this issue with didOpen. So, I think the best solution would be to extend the Location structure in response from the server.

@Arthurm1
Copy link
Contributor Author

@kpodsiad You're a typescript expert - how do I solve this issue with ESLint - it wants fileContent to be a const but that doesn't compile...

var fileContent;
if (uri.scheme == "jar") {
   fileContent = await metalsFileProvider.provideTextDocumentContent(uri);
} else {
   const readData = await workspace.fs.readFile(uri);
   fileContent = Buffer.from(readData).toString("utf8");
}

@kpodsiad
Copy link
Member

You're too kind @Arthurm1! :D
Rule of thumb in js/ts - never use var. Use const for immutable data and let for mutable. So in this case let should be enough.
You can also do some refactor to get rid of let like:

      const getFileContent = async () => {
        if (uri.scheme == "jar") {
          return await metalsFileProvider.provideTextDocumentContent(uri);
        } else {
          const readData = await workspace.fs.readFile(uri);
          return Buffer.from(readData).toString("utf8");
        }
      };
      const fileContent = await getFileContent();

@Arthurm1
Copy link
Contributor Author

@dos65 I agree - extending the Location structure with preview text would halve the work time and cut down on client code but I think that should be saved for another PR since it changes API. This should at least fix the issue - although it's slow.

I saw that there is a VSCode API proposal for find text in files API with what looks like a workable result class with preview but I've no idea how long these things stay as a proposal of if there is any timeline for them. The issue hasn't changed for a while.

This might also be solved by implementing the FileSystemProvider API for jars - which should put all the searching in VSCode's hands.

Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

@Arthurm1 tested the recent changes locally - works great!
I'm fine with the current solution as it works fast enough.
Thx for the fix!

I saw that there is a VSCode API proposal for find text in files API with what looks like a workable result class with preview but I've no idea how long these things stay as a proposal of if there is any timeline for them. The microsoft/vscode#59924 hasn't changed for a while.

Yep, I've seen it too. The only we can do is to wait and switch on it after it become stable.

This might also be solved by implementing the FileSystemProvider API for jars - which should put all the searching in VSCode's hands.

Some time ago I was checking vscode sources. From what I found, its native search doesn't work with custom FileSystems provided by extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants