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: Do not render selected word markers for the same range multiple times #4727

Merged
merged 2 commits into from
May 30, 2022

Conversation

andrewnester
Copy link
Contributor

Issue #, if available:
#4687

Description of changes:
Avoid rendering multiple markers at the same range to avoid browser freeze when a lot of search values found in folded code.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@andrewnester andrewnester requested a review from nightwing May 30, 2022 12:07
@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #4727 (cd6ff4c) into master (f269889) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4727   +/-   ##
=======================================
  Coverage   71.15%   71.16%           
=======================================
  Files         554      554           
  Lines       55693    55698    +5     
  Branches    10421    10422    +1     
=======================================
+ Hits        39630    39635    +5     
  Misses      16063    16063           
Flag Coverage Δ
unittests 71.16% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/ace/search_highlight.js 97.22% <100.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f269889...cd6ff4c. Read the comment docs.

Copy link
Member

@nightwing nightwing left a comment

Choose a reason for hiding this comment

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

both calling toScreenRange, and searching for the word in a large number of lines are slow. It may be better to either use a loop skipping folded lines https://github.com/ajaxorg/ace/blob/master/lib/ace/layer/text.js#L292 or use lines data from text layer.

In the long run it may be worth to unify all these loops into something similar to getVisibleRanges of monaco.

@andrewnester
Copy link
Contributor Author

@nightwing I don't think this CR changes how often we call toScreenRange so from this perspective it should stay the same but I agree that generally we need to revisit this and just skip folded areas

@andrewnester andrewnester requested a review from nightwing May 30, 2022 14:02
@andrewnester andrewnester merged commit cd30f59 into ajaxorg:master May 30, 2022
@andrewnester andrewnester deleted the bug/4687 branch May 30, 2022 16:04
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.

2 participants