search

AndrewRadev / vimrunner

Control a vim instance through ruby code
MIT License
238 stars 13 forks source link

Vimrunner.start_vim Does Not Fork GUI Vim Correctly #3

Closed mudge closed 12 years ago

mudge commented 12 years ago

One quick edge-case (that just bit me): if Vimrunner.start_vim starts a GUI vim (as it will on a Mac without symlinking vim to mvim), the @gui flag will not be set in the Server instance and so the process will not fork correctly (it will use PTY.spawn instead of Kernel.spawn).

I guess the reason this slipped through the cracks is that clientserver_enabled? is stubbed to true in the Server specs: if you have clientserver_enabled?("vim") return false then it should fallback to gvim or mvim which is when the @gui problem hits.

AndrewRadev commented 12 years ago

Should've known. I noticed the inconsistency myself today as I wrote some docstrings here and there. I thought I'd checked it and it worked either way, but, well, apparently I hadn't.

I've added the check for clientserver support in the instance, as opposed to doing it in the path method. That way, the @gui attribute can be set appropriately. Since I've managed to replicate the bug on linux by changing clientserver_enabled?, I assume I was able to fix it, but could you confirm?

mudge commented 12 years ago

Unfortunately not due to list requiring clientserver, see https://github.com/AndrewRadev/vimrunner/commit/d96a95ec8847a2bd5c1f61533a447a7579838548#commitcomment-1240723 for more details.

AndrewRadev commented 12 years ago

I've pushed a possible fix for the serverlist issue. As suggested, I simply moved it to the instance for now. I definitely need to re-think the whole thing, though. I compiled a "tiny" version of vim to test with and it seems to be alright with :vim_path => 'tinyvim', but I should probably look for some more edge cases, like a missing gvim for example. Not that anything interesting can be done there, but a better error message would be appropriate at least.

mudge commented 12 years ago

I can confirm that fixes the issue here.

As for the refactor: I think it might be a good idea to centralise the logic in a separate Vim class as you mentioned in your comment as something that both the Server and Client use to invoke commands.

AndrewRadev commented 12 years ago

Alright, I'll release a quick fix for now and work on the refactoring in the coming days.

By the way, how do you feel about becoming a contributor to the project? I don't have a mac and there might be some issues there yet. Stubbing and mocking can only get me so far with testing features on the different platforms. Plus, if you're using it for your own vim plugins, you'll probably find improvements/fixes to the client interface, like you did with your other pull requests.

In any case, thanks for looking out for me :). I'll try to figure out a better way to test with different Vims and other conditions.

mudge commented 12 years ago

Sure, I'd be happy to come on-board as a contributor to help test the Mac side of things more.