jpalardy / vim-slime

A vim plugin to give you some slime. (Emacs)
http://technotales.wordpress.com/2007/10/03/like-slime-for-vim/
MIT License
1.83k stars 223 forks source link

Vim array #364

Closed jam1015 closed 1 year ago

jam1015 commented 1 year ago

This is my very first pull request so if I'm doing something wrong (too many changes, unclean git commit history etc) let me know and I can try to change it. This pull request improves support for neovim terminal in several ways.

Keeps track of the last open terminal in an array

and clears the terminal job id from that array when a terminal is closed. The most recently opened terminal is appended to the end of the array, which is the 'most recent' terminal. used by the config. This is implemented in the code:

if has('nvim') && get(g:, "slime_target", "") == "neovim"

    function SlimeAddChannel() "adds terminal job id to the g:slime_last_channel variable
        if !exists("g:slime_last_channel")
            let g:slime_last_channel = [&channel]
            echo g:slime_last_channel
        else
            call add(g:slime_last_channel, &channel)
            echo g:slime_last_channel
        endif
    endfunction

    function SlimeClearChannel() " checks if slime_last_channel exists and is nonempty; then fitlers slime_last_channel to only have existing channels
        if !exists("g:slime_last_channel")
        elseif len(g:slime_last_channel) == 0
            unlet g:slime_last_channel
        else
            let bufinfo = getbufinfo()
            call filter(bufinfo, {_, val -> has_key(val['variables'], "terminal_job_id") })
            call map(bufinfo, {_, val -> val["variables"]["terminal_job_id"] })
            call filter(g:slime_last_channel, {_, val -> index(bufinfo, val ) >= 0 })
        endif
    endfunction

        augroup nvim_slime
            autocmd!
            autocmd TermOpen * call SlimeAddChannel()
            autocmd TermClose * call SlimeClearChannel()
        augroup END
endif

stores config as integer taken from slime_last_channel array.

The NeovimConfig function is accordingly modified to get the last element of the g:slime_last_channel array. It is also modified to convert the input to be stored as an actual integer rather than a string of an integer when the user inputs into jobid. Done through wrapping the call to input in a call to str2nr.

splits SlimeGetConfig in two.

Well really appends to it. SlimeGetConfig really just calls two functions now. SlimeExistsConfig is just the old SlimeGetConfig. It checks if a config is set. The SlimeVerifyConfig function is a place to check if the existing config is valid; if it is not, it dispatches the Config function. Currently SlimeVerifyConfig only works for neovim, but anyone can make a PR to add config verification for other targets. This is done with an if-else block. It would have been cool to use your dispatch model for it but to do that it would seem I would have to add a verification function for each target. The call to index is the reason why I changed the jobid to be stored as an integer; without it we would be comparing strings to integers in that call to index. An alternative would be to convert to integer in the call to index rather than storing as an integer.

A future pull request would be to add a popup menu for neovim listing all running terminals to let the user choose between them. I wanted to expand the documentation, but the documentation file is not modifiable in the repo. Is the way to go about that just doing a chmod on it? Would explain the slime_get_jobid() function, and maybe add a short explainer of the features included in this pull request.

Finally,

jpalardy commented 1 year ago

Hi @jam1015

Thank you for the PR 👍

I had a few thoughts about the changes:

Right now, I've been leaning on different solutions for configuration — in particular, https://github.com/jpalardy/vim-slime-ext-plugins, which though incomplete was meant to give (near-) infinite flexibility.

jam1015 commented 1 year ago

Thanks for the feedback! How about this for course of action:

I will try to add more commits to get rid of the whitespace related changes. If that fails, I will re-fork the repository and add my commits sequentially to clearly reflect the changes I have made.

Then I will defer to your choice to either: 1) Decide to merge my changes or 2) Decide that you are not comfortable enough with Neovim to verify that my changes are good and merge them. Either is fine! I will clean up the changes either way, but let me know explicitly if you are leaning towards option 1 or 2.

In either case I will try to figure out how to make a plugin for slime-ext-plugins, although for my current use case integration into this version of the plugin is easier because I use both the neovim and tmux targets and may look into the X11 window target, but I may not have the expertise to write extensions for X11 windows and tmux besides pasting from this version of the plugin coupled with trial and error.

jpalardy commented 1 year ago

For now, I'm leaning towards 2 (not merging)

I know many people are following this repo and keeping an eye on things. Let's see if anyone else wants to chime in pro/con.

jam1015 commented 1 year ago

Fixed whitespace related differences between this branch and main.

jam1015 commented 1 year ago

Implemented in pull requests to the vim-slime-ext-plugins sub-plugin vim-slime-ext-neovim, with possible other pull requests happening in the future to add features.