bug: adding terminal mode navigation support breaks using apps using `FloatermNew

[svanharmelen]

Hi,

I use lazygit in a floating terminal and since merging #431 this now breaks when using <C-j/k> (which are shortcuts I use within lazygit). It now tries to execute bash - c TmuxNavigateXXX which lazygit of course doesn't understand 😏

  {
    'voldikss/vim-floaterm',
    keys = {
      { '<leader>gg', ':nohlsearch<Bar>FloatermNew --height=0.95 --width=0.7 lazygit<CR>', silent = true },
    },
    config = function()
      vim.g.floaterm_borderchars = '─│─│╭╮╯╰'
      vim.g.floaterm_title = ''
    end,
  },

Is there any way to fix/prevent this with the newly added code? Or was this an oversight and does the PR need a bugfix?

[francescoVaglienti]

You have overlapping/clashing mappings (which you already did have for C-j and C-k before the added functionality, just that in Term mode the functions would not do anything) my advice would be to make them context aware for your specific setup by setting tmux_navigator_no_mappings = 1, in your case if you just want to move back to the old functionality it would be adding the following to your .vimrc

let g:tmux_navigator_no_mappings = 1
let g:tmux_navigator_disable_netrw_workaround = 1
nnoremap <silent> <c-h> :<C-U>TmuxNavigateLeft<cr>
nnoremap <silent> <c-j> :<C-U>TmuxNavigateDown<cr>
nnoremap <silent> <c-k> :<C-U>TmuxNavigateUp<cr>
nnoremap <silent> <c-l> :<C-U>TmuxNavigateRight<cr>
nnoremap <silent> <c-\> :<C-U>TmuxNavigatePrevious<cr>

if !get(g:, 'tmux_navigator_disable_netrw_workaround', 0)
  if !exists('g:Netrw_UserMaps')
    let g:Netrw_UserMaps = [['<C-l>', '<C-U>TmuxNavigateRight<cr>']]
  else
    echohl ErrorMsg | echo 'vim-tmux-navigator conflicts with netrw <C-l> mapping. See https://github.com/christoomey/vim-tmux-navigator#netrw or add `let g:tmux_navigator_disable_netrw_workaround = 1` to suppress this warning.' | echohl None
  endif
endif

Because you can setup the keybindings to just about anything there is a technical infinite amount of interactions possible. For the standard case of FZF the exception was added as this was blocking to give users the excepted motion when doing c-hjkl when in a terminal, i.e. to move.

An extra default could be added that allows you to just turn off the terminal navigation (similar as tmux_navigator_no_mappings, turns off the default mappings), but in my opinion that is what tmux_navigator_no_mappings is for and by default it is strange that you get stuck using the default keybindings only when within a terminal within vim, as you do not get stuck in a terminal within tmux in the same manner, so why should a tmux terminal react differently to c-hjkl than a vim terminal.

[svanharmelen]

Thanks for your anwser and suggestion, unfortunately it doesn't seem to work. I updated my config to look like this:

  {
    'christoomey/vim-tmux-navigator',

    config = function()
      vim.g.tmux_navigator_no_mappings = 1
      vim.g.tmux_navigator_no_wrap = 1

      vim.keymap.set('n', '<C-h>', ':<C-U>TmuxNavigateLeft<CR>', { silent = true })
      vim.keymap.set('n', '<C-j>', ':<C-U>TmuxNavigateDown<CR>', { silent = true })
      vim.keymap.set('n', '<C-k>', ':<C-U>TmuxNavigateUp<CR>', { silent = true })
      vim.keymap.set('n', '<C-l>', ':<C-U>TmuxNavigateRight<CR>', { silent = true })
    end,
  },

But when I now remove the pinned commit and update the plugin, it still breaks with the same error. Any additional things I can/should look at?

And while I get your point of view, I would argue that intruducing a potentially breaking change like this on a plugin as old as this one should probably been done behind a feature flag to prevent issues for existing (long time) users 🤷‍♂

[svanharmelen]

Ok, updating my config to this "solved" the issue:

  {
    'christoomey/vim-tmux-navigator',

    init = function()
      vim.g.tmux_navigator_no_mappings = true
      vim.g.tmux_navigator_no_wrap = true
    end,

    config = function()
      vim.keymap.set('n', '<C-h>', ':<C-U>TmuxNavigateLeft<CR>', { silent = true })
      vim.keymap.set('n', '<C-j>', ':<C-U>TmuxNavigateDown<CR>', { silent = true })
      vim.keymap.set('n', '<C-k>', ':<C-U>TmuxNavigateUp<CR>', { silent = true })
      vim.keymap.set('n', '<C-l>', ':<C-U>TmuxNavigateRight<CR>', { silent = true })
    end,
  },

[francescoVaglienti]

Yeah that's because you're using nvim which doesn't use 1 or 0 but true and false. When replying I gave you a vim instead of a nvim config as I was on autopilot.

As to your added feature flag comment, your issue is because you have multiple bindings bound to the same key without context awareness (for an example on how to add context awareness for fzf in #433 line 26). What you're asking with a feature flag I would normally agree to, however this would mean locking all new interactions and patches behind flags as users might have already added their own scripts around them. With this feature the only pain comes from people who accept the default setup from tmux nav (i.e. c-hjkl) and the change is only kept within that default binding.

In other words, it would mean new users would not get the discovery versus advanced users not resolving clashes in their config. As for a new user the switch can already be overwhelming while an advanced users already knows how to modify their config files, thus I think the trade off is sound here. Again, remember, you're accepting the default of the navigator and it cannot be aware of every other plugin in existence (though a best effort is made as can be seen with fzf and netrw).

I still recommend that for your own plugin suite, as you have clashes, you add context awareness. With that you could enjoy the new functionality while still using the awesome powers of lazygit or fzf within your terminal within vim and enjoy the parity between your (n)vim terminal and tmux terminal that we have now. When you bind two commands to the same key, issues can appear.

[svanharmelen]

Thanks for taking the time to write such an elaborate reply @francescoVaglienti!

Yeah that's because you're using nvim which doesn't use 1 or 0 but true and false. When replying I gave you a vim instead of a nvim config as I was on autopilot.

Just for reference, the boolean notation wasn't the issue. This was a timing issue caused by setting the options in the config block rather then the init block (which runs much earlier).

In other words, it would mean new users would not get the discovery versus advanced users not resolving clashes in their config. As for a new user the switch can already be overwhelming while an advanced users already knows how to modify their config files, thus I think the trade off is sound here.

I agree that reasoning makes sense 👍🏻

I still recommend that for your own plugin suite, as you have clashes, you add context awareness. With that you could enjoy the new functionality while still using the awesome powers of lazygit or fzf within your terminal within vim and enjoy the parity between your (n)vim terminal and tmux terminal that we have now.

I could, but I don't use the terminal within nvim (as I use tmux for that 😉) so I'm good. Thanks!