jam1015 commented 5 months ago

Intro

I know that PRs are supposed to be monolithic. This PR is fairly monolithic as it has one major change to the actual plugin, and seveal minor changes.

They are (in rough order of importance)

Improving the tracking of previously opened terminals for the Neovim target
Adding more details on use of vim-style mappings to the README
Expanding the neovim target README , including changes to reflect the new features in point 1.
Replacing end with endif in appropriate places.

The Changes

Going point by point:

Improving Terminal Tracking

In the current release, the last open terminal is kept track of with

  augroup nvim_slime
    autocmd!
    autocmd TermOpen * let g:slime_last_channel = &channel
  augroup END

This works, but forgets about previously opened terminals if multiple are opened. This PR takes the approach:

  augroup nvim_slime
    autocmd!
    " keeping track of channels that are open
    autocmd TermOpen * call slime#targets#neovim#SlimeAddChannel()
    " keeping track when terminals are closed
    autocmd TermClose * call slime#targets#neovim#SlimeClearChannel()
  augroup END

Where SlimeAddChannel and SlimeClearChannel respectively add or remove channels from a variable stored as g:slime_last_channel

Here are the those functions:


function! slime#targets#neovim#SlimeAddChannel()
  if !exists("g:slime_last_channel")
    let g:slime_last_channel = [{'jobid': &channel, 'pid': b:terminal_job_pid}]
  else
    call add(g:slime_last_channel, {'jobid': &channel, 'pid': b:terminal_job_pid})
  endif
endfunction

function! slime#targets#neovim#SlimeClearChannel()
  let current_buffer_jobid = get(b:,"terminal_job_id",-1)

  if !exists("g:slime_last_channel")
    if exists("b:slime_config")
      unlet b:slime_config 
    endif
    return
  elseif len(g:slime_last_channel) == 1
    unlet g:slime_last_channel
    if exists("b:slime_config")
      unlet b:slime_config
    endif
  else
    let bufinfo = s:get_filter_bufinfo()

    " tests ifusing a version of Neovim that 
    " doesn't automatically close bufers when closed
    " or there is no autocommand that does that
    if len(bufinfo) == len(g:slime_last_channel)
      call filter(bufinfo, {_, val -> val != current_buffer_jobid})
    endif

    call filter(g:slime_last_channel, {_, val -> index(bufinfo, str2nr(val["jobid"])) >= 0})

  endif
endfunction

These functions use helper functions that should be self-explanatory. get-filter-bufinfo gets the info about all currently open buffers and filters to only include info on open terminal buffers.

I update the slime#targets#neovim#config() function:

function! slime#targets#neovim#config() abort

  " unlet current config if its jobid doesn't exist
  if exists("b:slime_config")
    let bufinfo = s:get_filter_bufinfo()
    let current_jobid = get(b:slime_config, "jobid", "-1")
    if index(bufinfo, current_jobid) == -1
      unlet b:slime_config
    endif
  endif

  if !exists("b:slime_config")
    let last_pid = get(get(g:slime_last_channel, -1, {}), 'pid', '')
    let last_job = get(get(g:slime_last_channel, -1, {}), 'jobid', '')
    let b:slime_config =  {"jobid":  last_job, "pid": last_pid }
  endif

  "include option to input pid
  if exists("g:slime_input_pid") && g:slime_input_pid
    let pid_in = input("pid: ", str2nr(jobpid(b:slime_config["jobid"])))
    let id_in = s:translate_pid_to_id(pid_in)
  else
    if exists("g:slime_get_jobid")
      let id_in = g:slime_get_jobid()
    else
      let id_in = input("jobid: ", str2nr(b:slime_config["jobid"]))
      let id_in = str2nr(id_in)
    endif
    let pid_in = s:translate_id_to_pid(id_in)
  endif

  let b:slime_config["jobid"] = id_in
  let b:slime_config["pid"] = pid_in
endfunction

Notable is that the user can now configure to input the terminal's PID rather than the Neovim internal job id. This change is documented in the Neovim target README.

Adding Details on vim-style Mappings

When I was learning vim I was looking through and exploring all the different send-to-repl plugins. This one seemed great, but I wasn't a fan of the CTRL-c key chords. I chose vim because I like the operator-motion style of doinag things. So I tried other send-to-repl plugins before reading the documentation of this one in more depth to see that the vim-style mappings do exist, they're just not advertised in the README.

So I made these additions:

Vim-like mappings

To use vim-style mappings:

"disables mappings from previous usage section
let g:slime_no_mappings = 1

"send visual selection
xmap <leader>s <Plug>SlimeRegionSend

"send based on motion or text object
nmap <leader>s <Plug>SlimeMotionSend

"send line
nmap <leader>ss <Plug>SlimeLineSend

Of course these mappings are just examples; you can set them according to your preference.

Hopefully this will let new users see these features from the moment they discover this plugin.

Expanding `neovim` target documentation

Adds documentation of option to use pid rather than neovim's internal jobid.

In addition to using &channel suggests that the user convigure their statusline to show the terminal pid and jobid.

Using `endif` instead of `end`

(i think) end works because it is a unique prefix of endif. endif is preferred and is what is showed in the documentation. There are almost certainly more places in this plugin I could make this change but only did it in places where I'm interested.

Closing Thoughts

I made these changes because I think that the vim and neovim targets for this plugin deserve special consideration because the user always has them available, no matter the environment they are using vim/neovim in.
I tested these changes on the current neovim release and the dev branch, v 10.0.0 using a minimal config. These tests are important because with Neovim 10.0.0 terminal buffers are automatically closed when the terminal closes
If you accept these changes, I have another PR ready to go that makes the user experience even better.

jpalardy commented 5 months ago

Hi @jam1015

Thanks for pitching in. I made a few recommendations.

I'm willing to merge this if you tell me that you're willing to support it (if-and-when bugs come in).

Let me know

jam1015 commented 5 months ago

Thank you so much for your feedback. I have made the changes you suggested. I also added some section breaks to the advanced configuration documentation. This made sense because I created the vim-style bindings section there to link to from the main README so it made sense to add other sections. I also added more clear language on the b:slime_cell_delimiter variable. I commit to maintaining the features I add here and to fix bugs that arise. As I mentioned in the intro, I have another branch ready to PR if you accept this one.

jpalardy commented 5 months ago

It's looking good ^

I'll dust off neovim and do a test run tonight — then I'll merge this in.

jpalardy commented 5 months ago

It works as far as I can tell — but I don't use neovim/terminal often myself.

But I'm a bit baffled by how it works: (as a non-user)

open vim, no terminal, hit the binding (a bunch of errors)
close a terminal, hit the binding (a bunch of errors)

I think that's how it worked before. Maybe terminal users don't run into this problem?

jpalardy commented 5 months ago

What about channels … how do they work? 🤔

jiz4oh commented 5 months ago

Hi @jam1015, thanks for your great works, there is a polite reminder, the b:terminal_job_pid is deprecated and will be removed in future. Should instead use jobpid() as follow the official suggestion

jpalardy / vim-slime

Track channels #411

Intro

The Changes

Improving Terminal Tracking

Adding Details on vim-style Mappings

Vim-like mappings

Expanding `neovim` target documentation

Using `endif` instead of `end`

Closing Thoughts

jpalardy / vim-slime

Track channels #411

Intro

The Changes

Improving Terminal Tracking

Adding Details on vim-style Mappings

Vim-like mappings

Expanding neovim target documentation

Using endif instead of end

Closing Thoughts

Expanding `neovim` target documentation

Using `endif` instead of `end`