t9md / atom-quick-highlight

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

Improve highlighting feedback #4

Closed PaulPorfiroff closed 8 years ago

PaulPorfiroff commented 8 years ago

Tried to improve a bit highlight-on-fly feature:

t9md commented 8 years ago

Tried to improve a bit highlight-on-fly feature:

  • Omit highlighting selection if there are no other occurences of it in text buffer. Useful when buffer spans multiple pages. Made this available as highlightSelectionExcludeUnique config option.

I don't need this feature, what the purpose of this? This is the feature misleading user to think highlight not working? I don't want introduce complexity to codebase for this.

  • Constant delay of 100ms before highlighting after changing selection perceived really slow, as compared to default selection marker. I've just refactored that delay into highlightSelectionThrottle setting to allow modifying it with ease.

So you want make delay user-configurable? I don't thinks 100ms is not problematically slow for most of user. But I can add this config params, Is this really need?

t9md commented 8 years ago

For about your 1st suggestion, your concern is performance overhead for highlight, to avoid scan whole buffer, you have following configuration parameter to control this

And highlight itself won't scan whole buffer, only scan visible buffer ranges, so theoretically its performance impact is same regardless of number of line of buffer.

t9md commented 8 years ago

Sorry I wanted my commit to bind to issue #5.

t9md commented 8 years ago

Released v0.3.9. By the way your original PR use getCountForKeyword to check its occurrence, if you want mitigate performance impact, avoiding to call getCountForKeyword is important since its scan whole buffer. So for this purpose, use showCountOnStatusbar config param.

PaulPorfiroff commented 8 years ago

Sorry for way too late response.

For about your 1st suggestion ... I don't need this feature, what the purpose of this?

Just to clear things up, I was talking specifically about feature from #1 purposal, not about fixed markers.

This suggestion allows quickly reason about unused or not declared variables/functions/types in multipage buffers.

E.g. given this part of buffer visible, you can't tell whether forgottenInTheAnnalsOfHistory is some global variable or just an unused clutter by looking at first screen, while the second one clearly shows there are no other occurences found, so may remove this guy.

quick-highlight-pr4-1 quick-highlight-pr4-2

for this purpose, use showCountOnStatusbar config param

Using displayCountOnStatusBar config could be one option. But status bar count doesn't show up without dispatching quick-highlight:toggle command simply by modifying selection.

So I opted for another option: just to omit highlighting, as e.g. KDE Kate highlight selection extension does.

Perhaps you're right, that's a bit out of concept of status bar icon, but I liked it this way, so shared. Maybe, showing status bar icon always and falling back to this behaviour when icon is turned off in options, would be the way to go? Overwhelming, though.

Your concern is performance overhead for highlight ...

Highlight itself won't scan whole buffer, only scan visible buffer ranges

Yeah, that's why I wanted an explicit option turned off by default for new behaviour.

t9md commented 8 years ago

So you want to distinguish one occurrence or may occurrence of keyword for hightlightSelection. Understood.

Currently you can know number of occurrence by manually toggle and check statusbar icon as you described.

After I understand background of this PR but I still think this feature is misleading and dont want to add even if KDE support this.

I thinks updating occurrence count icon on highlightSelection solve your requirement. Im' also thinking about new feature to show count on hover indicator like I did in other my package.

Anyway I need some time.

e.g. incremental search on my vim-mode-plus package

gif

PaulPorfiroff commented 8 years ago

So I opted for another option: just to omit highlighting, as e.g. KDE Kate highlight selection extension does.

Please, excuse my stupid mistake and just ignore as e.g. KDE Kate highlight selection extension does. part. That's incorrect example.

Im' also thinking about new feature to show count on hover indicator like I did in other my package.

That feels too find-and-replaceish and heavy to me in current case (meaning on-the-fly selections).

Nevertheless, your package fits best at the moment, thanks.