ternjs / tern_for_vim

Tern plugin for Vim
MIT License
1.83k stars 100 forks source link

Fix TernRename in gvim, where the wrong buffer was selected #36

Closed arian closed 11 years ago

arian commented 11 years ago

So the changes were made in the wrong buffer.

For me, the bufnr didn't correspond with the correct index in vim.buffers. This only happened in gvim, in vim in the terminal it worked correctly.

Perhaps it's because of some plugin, I didn't try it separately.

clausreinke commented 11 years ago

Don't have time to test this week, but didn't want to let this go unanswered: this is the first confirmed report we have of buffer indices not matching bufnr, thanks!

If I understand jparera's comment in https://github.com/marijnh/tern_for_vim/issues/32#issuecomment-22455081 correctly, that issue is gone in vim 7.4, but we should still do something like the patch suggested here (just check that it works in 7.3 and 7.4, please). Some people have vast numbers of buffers open, which is why I instinctively avoided looping, but slow is better than wrong (and looping over a hundred or so buffers shouldn't really be slow).

arian commented 11 years ago

Yes, seems I have the same problem as described in #32 and #24.

Perhaps you could have both bufnr and look that up in the vim.buffers, but if the buffer.name property does not equal, fall back to this loop to make it a bit faster in normal cases.

jparera commented 11 years ago

I agree with @clausreinke, correctness is more important than performance.

Another workaround would be to detect the Vim version and register the looping version to TernRename only for vim <7.4. This way the overhead is done once instead of calling the method every time. To enable the loop, a variable could be defined.

if version < 740
        let g:tern_rename_enable_loop = 'yes'
else 
        let g:tern_rename_enable_loop = 'no'
endif
clausreinke commented 11 years ago

@arian 's suggestion would cover both the 7.4 case and the common 7.3 case that index==bufnr (feature detection wins over version detection;-). Modulo error handling (if the buffer lists really are out of sync, there may not be a buffer at bufnr at all). Unless the python interface changes in 7.4 make a different code path necessary.

jparera commented 11 years ago

According to the Vim changelog ( https://gist.github.com/mjhea0/6200588 ), previous versions expected iteration of vim.buffers and bufnr was not directly related with the internal buffer list. In 7.4 vim.buffers is dictionary which key is bufnr.

I have done some testing:

Vim 7.3:

Vim 7.4:

If you use the modulo of bufnr you could get a KeyError in 7.4. I am not sure if you can do a feature detection mechanism and avoid error handling.

marijnh commented 11 years ago

I've merged this (somewhat late, holiday got in the way) -- a linear scan over a set of buffers doesn't seem like it's likely to get problematic. If somebody wants to further refine it, feel free.