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

Use the terminal by default for nvim-0.2.1 or GVim (nightly) #1019

Merged
merged 6 commits into from
Sep 5, 2017

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Aug 17, 2017

Initial work to use the neovim terminal by default in Windows.
The double keypress remains for Windows 8+ because gdamore/tcell#159 is unresolved.

I prefer to keep this open/unmerged until the 0.2.1 release while I test this with my pull requests in fzf.vim.

@mikesmiffy128 I cherry-pick the commit as you suggested in #962 (comment)

@junegunn
Copy link
Owner

Thanks!

@janlazo
Copy link
Contributor Author

janlazo commented Aug 18, 2017

Using the same winpty.dll included in Neovim 0.2.1, I can use the :terminal in GVim as of patch-8.0.956 but it's not polished compared to Neovim's terminal. I have to manually close the terminal buffer.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 21, 2017

fzf#shellescape does not work with BCommits in s:execute_term.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 27, 2017

:terminal technically works in GVim on Windows but because it doesn't work on my vimrc with ConEmu and powershell, I will provide support for GVim only to resolve junegunn/fzf.vim#416 (comment).
TUI for regular Vim will be handled in a different PR and TUI for nvim will be considered when it is viable for daily use on cmd.exe.

@janlazo janlazo changed the title [neovim] Use the terminal in Windows if running nvim-0.2.1 Use the terminal in Windows for nvim-0.2.1 or GVim (nightly) Aug 27, 2017
@janlazo
Copy link
Contributor Author

janlazo commented Aug 27, 2017

Because +terminal is unstable in Windows, only nightly releases are supported for GVim.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 27, 2017

term_start seems incompatible with s:execute_term because the dict is restricted to the supported options.
Even if term_start throws an error in the try block, a new buffer will open anyway.

@janlazo janlazo changed the title Use the terminal in Windows for nvim-0.2.1 or GVim (nightly) Use the terminal by default for nvim-0.2.1 or GVim (nightly) Aug 27, 2017
Copy link
Contributor

@mikesmiffy128 mikesmiffy128 left a comment

Choose a reason for hiding this comment

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

Just a quick thought which popped into my head...
Rather than having my name in the commit message, I think it makes more sense to rewrite this commit so I'm the author and you're the committer. It doesn't matter that much but it will make the Git history a little more complete / correct. Also, if you can be bothered, you might want to also prefix this with [neovim] (I realise I should've done this before so I'm kind of passing the buck, sorry).

EDIT: I thought this was per-commit, but of course I'm talking about the commit I initially wrote.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 28, 2017

@mikesmiffy128 Is commit 7d91ecd how you want it? It's the first time I've amended the commit author.

@mikesmiffy128
Copy link
Contributor

mikesmiffy128 commented Aug 28, 2017

LGTM 👍
Thanks.

@junegunn junegunn mentioned this pull request Aug 30, 2017
12 tasks
@junegunn
Copy link
Owner

Wow this works pretty well with the latest macvim, thanks!

screen shot 2017-08-31 at 12 11 13 am

  1. brew edit macvim and add --enable-terminal to args
  2. brew install macvim --HEAD

@janlazo Do you think it's ready for merge?

@janlazo
Copy link
Contributor Author

janlazo commented Aug 30, 2017

Yes, for Neovim.
For Vim 8 terminal, I haven't considered additional cleanup (such as junegunn/fzf.vim#416 (comment)) for the channels.

No issues yet with fzf.vim commands in terminal for both on Windows.
I haven't tested on Linux yet.

About your comment for terminal Vim on Windows, s:execute_term works on the default terminals for cmd.exe and powershell.exe but I need a blacklist for environments for where it can't work.

@junegunn
Copy link
Owner

For Vim 8 terminal, I haven't considered additional cleanup

Related commit is mattn/vim-fz@ecf5fc3, but there's no explanation. I did some quick tests and it looks like we don't need it. The associated channel is actually open during exit_cb, but we can observe that it's finally closed when close_cb is run.

function! E(...)
  echom 'exit '.job_getchannel(term_getjob(g:buf))
endfunction

function! C(...)
  echom 'close '.job_getchannel(term_getjob(g:buf))
endfunction

let g:buf = term_start('fzf', {'exit_cb': function('E'), 'close_cb': function('C')})

Maybe it was to deal with some bugs of Vim terminal? I don't know. But let's not add it until we actually run into an issue.

By the way, Macvim I tested with did not have CTRL-J/CTRL-M issue (vim/vim#1998), but it may pop up in the future. If I recall correctly, terminal Vim did not have the problem when I first tried its terminal a few weeks ago.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 31, 2017

Related commit is mattn/vim-fz@ecf5fc3, but there's no explanation.

vim-fz's initial commit has ch_close but there's no explanation as well. The channel is closed on close_cb in GVim on Windows.

No issues with Ctrl mappings (ie. Ctrl-S, Ctrl-D, Ctrl-J, Ctrl-M) so far.

@junegunn
Copy link
Owner

junegunn commented Sep 3, 2017

I'm experiencing CTRL-J vs. CTRL-M problem on Macvim built from the latest source. Strangely, the problem does not always happen, and I don't know why. Sometimes enter key works well, sometimes not. We can merge this with --bind ctrl-j:accept for now, or we can wait until vim/vim#1998 is fixed.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 3, 2017

We can merge but I recommend not making a release yet so we can make adjustments to the patch requirements and fix other quirks.

Can you give me more time? I like to use s:execute_term in powershell for terminal Vim.

@junegunn
Copy link
Owner

junegunn commented Sep 3, 2017

Can you give me more time?

No problem! Just let me know when you think it's ready for merge.

I recommend not making a release yet

I was thinking about pushing a new tag like 0.17.0-1 that only updates auxiliary scripts because I made a stupid mistake before releasing 0.17.0. Thought it would be a good idea to include this, so that many users can test it and give us feedbacks, but no hurries. I'll proceed without this.

IMPORTANT:
cmd.exe and powershell are fine in default Windows terminal.
cmd.exe prompt is broken on ConEmu because it natively supports ucs-2 only.
utf-16 support is exclusive to .Net (ie. powershell).
utf-8 supports requires chcp, external program, but does not fix the cmd.exe prompt.
Use powershell on ConEmu to avoid corrupted text on display
@janlazo
Copy link
Contributor Author

janlazo commented Sep 3, 2017

I think we're good to go for user testing.
I added a note regarding ConEmu and cmd.exe. fzf itself has no issues inside terminal on terminal Vim in Windows but displaying unicode text (utf-8, utf-16) is a problem. We could use powershell to get around this but without proper support from Vim, Neovim for handling the shell related options automatically, it's best to wait for more Vim patches. Also, all of the shell hacks in Vim for Windows are for cmd.exe.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 4, 2017

+terminal is included in the huge build as of patch 8.0.1050
Waiting for 8.0.1052 release in https://github.com/vim/vim-win32-installer/releases/tag/v8.0.1052

@junegunn
Copy link
Owner

junegunn commented Sep 4, 2017

Yes, I noticed that too, thanks. The reason I'm hesitating to merge this is that I'm actually running into CTRL-J/CTRL-M issue on Macvim. Enter key doesn't work occasionally, and I have to terminate fzf with CTRL-C, or reach to trackpad to double-click on an item, which is really awkward and I figure many users don't even know that fzf supports mouse. I think this is not a trivial issue that we can just tell the users to deal with it for the moment. And I'm thinking if we should force-append --bind ctrl-j:accept when starting fzf with Vim terminal, and document that it's a current workaround for the open issue. It can be annoying for the users who primarily use CTRL-J to move cursor downward on fzf, but it should be much less annoying than enter key not working at all.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 4, 2017

" in fzf#run, before running s:execute_term
if use_height
  let height = s:calc_size(&lines, dict.down, dict)
  let optstr .= ' --height='.height
elseif use_term
  let optstr .= ' --no-height'
  if !has('nvim') && !s:is_win
    let optstr .= ' --bind ctrl-j:accept'
  endif
endif
let command = prefix.(use_tmux ? s:fzf_tmux(dict) : fzf_exec).' '.optstr.' > '.temps.result

@janlazo
Copy link
Contributor Author

janlazo commented Sep 5, 2017

Do we need to bind Ctrl-M too?

@junegunn
Copy link
Owner

junegunn commented Sep 5, 2017

Yes, I can confirm that this works. We should add a comment that it's a temporary workaround.

Do we need to bind Ctrl-M too?

No, it's already mapped to accept by default.

Temporary workaround for non-Windows environment

Reference:
vim/vim#1998
junegunn#1019 (comment)
@junegunn junegunn removed the pending label Sep 5, 2017
@junegunn junegunn merged commit 26d7896 into junegunn:master Sep 5, 2017
@junegunn
Copy link
Owner

junegunn commented Sep 5, 2017

Merged, thank you! Let's see how it turns out.

@gauthiermartin
Copy link

His this fix supposed to work in GVim ?

With default fzf.vim options i am still getting the external window ?
![selection_005]

(https://user-images.githubusercontent.com/7561038/30072423-3931d688-9238-11e7-8e66-8e4becd8f2ea.png)

@janlazo
Copy link
Contributor Author

janlazo commented Sep 5, 2017

Did you upgrade Vim (and GVim) to version 8.0.995 or greater?
Patch check in https://github.com/junegunn/fzf/pull/1019/files#diff-24d9d57e35df5afce7890f2400917919R392

@gauthiermartin
Copy link

The upgrade seems to have done the job ! Thank you

Is there a way to set the terminal color and font in this patch ?

@gauthiermartin gauthiermartin mentioned this pull request Sep 5, 2017
15 tasks
@janlazo
Copy link
Contributor Author

janlazo commented Sep 5, 2017

Not yet.
There is no hook to set these in the Vim plugin and I can't find anything about it in :h terminal.
There is :h gui-resources for platform-dependent solution.

@janlazo
Copy link
Contributor Author

janlazo commented Sep 5, 2017

It is possible to support font size if running the bang version like FZF! so fzf can set guifont without affecting other splits. This will be a GVim-only solution because there is no standard means to set the gui font in Neovim GUIs.

I'm not sure about the terminal color because fzf has --color.

@junegunn
Copy link
Owner

junegunn commented Sep 6, 2017

By setting g:fzf_colors, you can make fzf pick colors from your current color scheme.

let g:fzf_colors =
\ { 'fg':      ['fg', 'Normal'],
  \ 'bg':      ['bg', 'Normal'],
  \ 'hl':      ['fg', 'Comment'],
  \ 'fg+':     ['fg', 'CursorLine', 'CursorColumn', 'Normal'],
  \ 'bg+':     ['bg', 'CursorLine', 'CursorColumn'],
  \ 'hl+':     ['fg', 'Statement'],
  \ 'info':    ['fg', 'PreProc'],
  \ 'border':  ['fg', 'Ignore'],
  \ 'prompt':  ['fg', 'Conditional'],
  \ 'pointer': ['fg', 'Exception'],
  \ 'marker':  ['fg', 'Keyword'],
  \ 'spinner': ['fg', 'Label'],
  \ 'header':  ['fg', 'Comment'] }

@janlazo janlazo deleted the nvim-term branch September 7, 2017 17:57
@janlazo
Copy link
Contributor Author

janlazo commented Sep 16, 2017

@junegunn Does patch 8.0.1108 help for the Ctrl-J/M issue?

@junegunn
Copy link
Owner

@janlazo Oh it does. This looks pretty silly but effectively fixes the problem.

tnoremap <buffer> <c-m> <c-m>

We could add something like

  if !has('nvim') && has('patch-8.0.1108')
    tnoremap <buffer> <c-m> <c-m>
  endif

to the code, but let me report this workaround to Vim issue tracker first.

@junegunn
Copy link
Owner

junegunn commented Sep 17, 2017

@janlazo Hmm, looks like I was mistaken. The problem was resolved not by the use of the tnoremap, but by my action of exiting normal mode (CTRL-\ + CTRL-N) to set up the mapping and coming back to insert mode (i).

@janlazo
Copy link
Contributor Author

janlazo commented Sep 17, 2017

You mean exiting terminal mode (or normal mode of the terminal buffer) after term_start?

@junegunn
Copy link
Owner

Yes, it is reported in vim/vim#1998 (comment) that Vim correctly interprets keys only after some events, and exiting terminal mode seems to be one of them.

@lifepillar
Copy link

@junegunn I see that in plugin/fzf.vim you bind ctrl-j as a workaround for vim/vim#1998. A better workaround might be to call term_wait(term_id, 20) after calling term_start(). At least, it works for me, both in terminal and in MacVim. Btw, waiting for 10ms was not enough, but 20ms solved the issue.

@junegunn
Copy link
Owner

@lifepillar Hi, thanks for the comment. I can confirm that the workaround works for me too. But I'm not too confident that it's a reliable solution that will work for everybody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants