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

Search in workspace improvements #6798

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Conversation

svenefftinge
Copy link
Contributor

What it does

Improves search in workspace performance by sending less messages and avoid sending long lines entirely.

How to test

Search for something including ignored files.

Review checklist

Reminder for reviewers

@elaihau
Copy link
Contributor

elaihau commented Dec 31, 2019

As far as I can tell from the code, search results should be collapsed if

  • "search.collapseResults" = "auto", and
  • there are more than 10 results from the same file

SIW seems not work this way with this change.

Screenshot from 2019-12-30 20-59-17

}

return a.length - b.length;
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do the "line" and "character" not matter any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a result contains all matches of a file

@akosyakov akosyakov added performance issues related to performance search in workspace issues related to the search-in-workspace labels Jan 1, 2020
@svenefftinge svenefftinge force-pushed the se/search-improvements branch from 5179060 to 9ee0c1f Compare January 2, 2020 09:24
@svenefftinge
Copy link
Contributor Author

Thanks for the feedback. I have fixed the auto collapse.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I believe the Cancel Search toolbar item should only be visible if there is an ongoing search (with a search term present) similarly to VS Code. At the moment, the following is displayed:

Screen Shot 2020-01-02 at 9 48 43 AM

Also, I believe the PR closes the following issue: #6461

- one message per file with multiple matches
- long lines are not send entirely to clients
- new option for max file size (defaults to 20M)

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
@svenefftinge svenefftinge force-pushed the se/search-improvements branch from 9ee0c1f to 6111061 Compare January 3, 2020 06:29
@svenefftinge
Copy link
Contributor Author

Good catch! I could only reproduce by first entering a search and then deleting it. I've fixed that.
Let me know if you saw that in another situation.

@akosyakov
Copy link
Member

@vince-fugnitto @elaihau please give your approve if you are fine with changes. Right now browser UI is hanging very often it would be good to have it in.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

It works great for me 👍

I noticed that:

  • the overall performance is much better and is less likely to go offline
  • my previous issue has been resolved (visibility of the cancel the toolbar item)
  • having the cancel toolbar item and command allows users to successfully terminate long searches
  • the search respects the preference collapseResults and lineNumbers

@svenefftinge svenefftinge merged commit ec2a866 into master Jan 3, 2020
@svenefftinge svenefftinge deleted the se/search-improvements branch January 3, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance issues related to performance search in workspace issues related to the search-in-workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants