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

Increased the allowed regex search matches to max number value to acc… #501

Closed
wants to merge 2 commits into from

Conversation

aasierra
Copy link

…omodate high match volumes

This PR was made to fix issue 496 where there is a limit of 999 match values to a file.
Link to issue #496

@msftclas
Copy link

Hi @aasierra, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@aasierra
Copy link
Author

@alexandrudima Looks like omnisharp:fetch failed?

@alexdima
Copy link
Member

@aasierra Yes, the build failure is unrelated to your change.

I see the need to highlight more than the first 1k results, but I feel reluctant to remove the limit altogether. It is not so much a problem to find all these matches, but afterwards they are converted into model decorations (tracked ranges) and the view model converts them again for fast painting times (in case of wrapping).

I'm thinking about 20MB files where as you begin typing in the find widget there could be >1M find matches. From my playing with it, I am certain the model/view model are not in such a good shape to deal with these many decorations at this time.

I will do some additional testing to see if there is something low hanging, otherwise an alternative solution (for unlimited find results highlighting) would be to make the entire find widget not rely on decorations and bake the concept into the view -- to get O(viewport) complexity.

@joaomoreno joaomoreno closed this Nov 25, 2015
@joaomoreno joaomoreno reopened this Nov 25, 2015
@msftclas
Copy link

Hi @aasierra, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@joaomoreno
Copy link
Member

Closed and opened PR to get a later merge commit for Travis to build on and get this green.

@joaomoreno joaomoreno closed this Nov 25, 2015
@joaomoreno joaomoreno reopened this Nov 25, 2015
@msftclas
Copy link

Hi @aasierra, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@aasierra
Copy link
Author

@alexandrudima I totally understand, would a better solution (and this is one I would be willing to look into) be to have highlighting and other actions update as you scroll the document?

@aasierra
Copy link
Author

@joaomoreno I am going to close this for now however @alexandrudima What do you think of these changes as a starting point for how to search within the current view context. This code is a little buggy and does not update on scroll yet but poses the idea I was talking about earlier. The idea is that viewImpl.getView is used and altered to return the search range that should be matched against. Thoughts?

@aasierra aasierra closed this Nov 29, 2015
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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