search

t9md / atom-quick-highlight

Highlight text quickly
https://atom.io/packages/quick-highlight
MIT License
32 stars 7 forks source link

Discussion: code change for minimap integration #12

Closed gouegd closed 7 years ago

gouegd commented 7 years ago

Hi ! Happy new year to all. I've had a look at how to build a minimap integration for this package, as @t9md noted elsewhere he is not a user of minimap himself.

I have something basic working, but I've had to make some modifications to quick-highlight as one of the current optimizations (highlighting only what's visible) means we would also show, in the minimap, those highlights only. In the context of minimap it kind of defeats its purpose, in my opinion. So, I'm now creating all highlights (even invisible ones), which could affect performance negatively, especially for large files. On the other side, there's currently two subscriptions to the editor scroll events that I can remove, so this will give a smoother experience during scroll, especially on large files.

What do you think @t9md ? Should such changes live in a fork, or could they be merged here ? I know some people do not use minimap, so if the change mentioned above sounds bad to you, I understand you'll be cautious about it.

t9md commented 7 years ago

Thanks, I think it's mergable. But if performance impact is big, I want make all-highlight-optional(auto detect minimap-quick-highlight and passing option to rendering method of quickhl?).

If you have working minimap package repo, I can update my quick-hl by myself.

gouegd commented 7 years ago

Here's the early working repo, not published on apm yet: https://github.com/gouegd/atom-minimap-quick-highlight

And here are the changes I did on a local version of quick-highlight: https://github.com/gouegd/atom-quick-highlight/commit/76c5049ef1ff22843815f1105daf96d6f78e0085

I believe any perf impact would be visible only on large files. Maybe the size of the file would be a good trigger to decide whether to rerender on scroll or to render everything once.

t9md commented 7 years ago

Thank, will check tomorrow.

gouegd commented 7 years ago

@t9md just updated, it's pretty cool now - it works for manual toggles too, and with the proper colors.

t9md commented 7 years ago

@gouegd Checked, cool!

I'm working on branch provide-service-for-minimap. for PR #13. Pls check comment in #13 for the required code change of your minimap-quick-highlight.

gouegd commented 7 years ago

@t9md ok, I just pushed this small change. Time to sleep here, I'll check back tomorrow 👍

t9md commented 7 years ago

Me too sleepy! On Tue, Jan 3, 2017 at 7:03 Greg D notifications@github.com wrote:

@t9md https://github.com/t9md ok, I just pushed this small change. Time to sleep here, I'll check back tomorrow 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/t9md/atom-quick-highlight/issues/12#issuecomment-270024942, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJeRTKZlDjl9xcInEfJqseZ_1-jpamwks5rOXQYgaJpZM4LYvnC .

t9md commented 7 years ago

Done in https://github.com/t9md/atom-quick-highlight/pull/17