Discussion: code change for minimap integration #12

Closed
gouegd opened this Issue Jan 1, 2017 · 8 comments

Projects

None yet

2 participants

@gouegd
gouegd commented Jan 1, 2017

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.

@gouegd gouegd referenced this issue in t9md/atom-vim-mode-plus Jan 1, 2017
Open

[META] List of Atom packages I'm maintaining #486

@t9md
Owner
t9md commented Jan 1, 2017

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
gouegd commented Jan 1, 2017

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:
gouegd@76c5049

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
Owner
t9md commented Jan 1, 2017

Thank, will check tomorrow.

@gouegd
gouegd commented Jan 2, 2017

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

@t9md
Owner
t9md commented Jan 2, 2017

@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
gouegd commented Jan 2, 2017

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

@t9md
Owner
t9md commented Jan 2, 2017
@t9md
Owner
t9md commented Jan 12, 2017

Done in #17

@t9md t9md closed this Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment