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

implement :GoSameIds #936

Merged
merged 4 commits into from
Jul 14, 2016
Merged

implement :GoSameIds #936

merged 4 commits into from
Jul 14, 2016

Conversation

fatih
Copy link
Owner

@fatih fatih commented Jul 13, 2016

Carried #933 over to here with additional improvements and fixes.

@fatih fatih changed the title Bhcleek master implement :GoSameIds Jul 13, 2016
@fatih
Copy link
Owner Author

fatih commented Jul 13, 2016

@bhcleek I've carried your changes over here with the fixes and improvements I made. Thanks for all this again, I'm still testing it and think to merge it after the GopherCon workshop as I believe this should be thoroughly tested for edge cases.

>
set updatetime=100
<
By defult it's disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

defult -> default

@bhcleek
Copy link
Collaborator

bhcleek commented Jul 13, 2016

I don't think you need to use CursorHold. The docs for CursorMoved say "don't do anything that is slow", which is different than "it's an expensive operation". FWIW, I talked to Alan Donovan, and he confirmed that guru what is going to be very fast (a few milliseconds). Coupled with his input and actual use, I chose to use CursorMoved over CursorHold intentionally. In my testing it was definitely very performant; I didn't perceive any lag when navigating through some large files. And there are valid reasons that someone may not want vim-go to mess with their updatetime value.

@fatih
Copy link
Owner Author

fatih commented Jul 13, 2016

@bhcleek try to create a variable called foooooooooooo and then start from f till the end by moving your cursor. You'll see that there is lag and slowness with CursorMoved, whereas there is none with CursorHold as it doesn't count movements as you don't need to calculate the sameID query while you're moving.

I'm still using and testing it. Usually it takes time until I merge stuff like this. It's good because you'll find edge cases and fix them.

@bhcleek
Copy link
Collaborator

bhcleek commented Jul 13, 2016

Yeah, I do see some lag there. Here's another alternative solution that you may want to consider: http://vim.wikia.com/wiki/Prevent_frequent_commands_from_slowing_things_down.

I'm totally onboard with taking the time to make sure it's tested and the edge cases are properly worked out. Please let me know if you need me to do anything else; I'm happy to help polish this up.

@bhcleek
Copy link
Collaborator

bhcleek commented Jul 13, 2016

FWIW, the reason I'd like to find a solution that uses CursorMoved instead of CursorHold is because I really like the highlighting as I'm moving through the file instead of having to wait for a pause after movement.

@bhcleek
Copy link
Collaborator

bhcleek commented Jul 13, 2016

I've been playing with this some more, and I don't see any lag when using CursorMoved, even when editing a large file that has unsaved changes. Could the lag you're seeing be resolved by moving call go#guru#ClearSameIds to after call go#guru#What instead of before?

bhcleek and others added 4 commits July 14, 2016 12:46
Add :GoSameIds to highlight all identifiers in the current buffer that
are equivalent to the identifier under the cursor.
* Use `CursorHold` for performance
* Fix guru running for new buffers that are not saved yet
* Fix runtime except by returning dict error
* Fix showing not errors for auto_sameid
* Improve docs
* Add GoSameIdsClear and enable mapping
* Add explicit highlighting
@fatih
Copy link
Owner Author

fatih commented Jul 14, 2016

@bhcleek I've moved back to CursorMoved, but also added explicit highlighting. Thanks again for your work.

@bhcleek
Copy link
Collaborator

bhcleek commented Jul 14, 2016

Thanks @fatih, and thank you for helping to polish this up.

I enjoyed meeting you at GopherCon. Sorry I couldn't make your session on the hack day.

@fatih
Copy link
Owner Author

fatih commented Jul 14, 2016

You're welcome @bhcleek, me as well!

I've mentioned you in the session as well and gave you a shoot out to your name :) I hope people remember you. And thanks again for your work, much appreciated.

@fatih fatih merged commit 78a7667 into master Jul 14, 2016
@fatih fatih deleted the bhcleek-master branch July 14, 2016 18:54
@bhcleek
Copy link
Collaborator

bhcleek commented Jul 14, 2016

+1

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