RCreateMaps: Create plug mappings unconditionally (fixes #470, fixes #494)

[BlueDrink9]

Previously, Plug mappings were only created if they were already in use - i.e., if someone had mapped them to something in their .vimrc.

The problem with that is if someone wants to create buffer-local maps to an nvim-R mapping, they cannot do so from .vimrc.
They must do so once the buffer is loaded - ie within vim, after loading .vimrc. This is usually done with an filetype autocmd.

This PR fixes that by making Nvim-R's <Plug>R* mappings always available, matching the behavior of most plugins that define maps.

[jalvesaq]

The option R_user_maps_only was added on 2014 (jcfaria/Vim-R-plugin@96fec1a).
We can either create a new option or add another possible value for R_user_maps_only (2, for example), but I don't want to change the default behavior of values 0 and 1.

[BlueDrink9]

It doesn't change the behavior of that option, unless I'm forgetting something. As before, the creation of mappings is independent of user_maps_only.
The diff is a little bit confusing. All I actually do is change the elseif g:R_user_maps_only to if g:R_user_maps_only. This option only affects the creation of default maps.

The only change that might affect users is that now, if a user has mapped to a mapping, the default mapping is still created. But that wasn't ever affected by user_maps_only anyway.

If you still think that change breaks compatibility, I think a new option would make more sense than re-using the other option. Something like R_create_plug_maps_on_startup?

Again I understand you are very busy, so happy to just implement the new option if you are unsure. Or, if there is another contributer you trust, tag them to review. The changes aren't urgent, but naturally I'd like them merged this week if possible.

I do want to emphasise that I think the current behavior is undocumented and inconsistent with the behavior of most other plugins.

[BlueDrink9]

@jalvesaq if you have time, could you please respond to the above and suggest how to move forward with this PR?

[jalvesaq]

This plugin is a file type plugin. Their maps must be available only for the file types that it supports. It doesn't make sense to have two maps for the same action.

[BlueDrink9]

<plug> maps don't really count as maps though. They are placeholders that don't interfere with the user experience until they decide to use them. Are you referring to <plug> maps?

Regarding your point about maps being only available for file types they support: That's what this PR allows. Under the current model, <plug> mappings cannot be filetype specific, because they cannot be set with a filetype autocommand. This PR fixes that. By your logic it should therefore be merged.

[BlueDrink9]

@jalvesaq are you able to respond to my previous comment, or would you like something else clarified or fixed?

[jalvesaq]

I don't remember who has proposed the use the <Plug> thing, but this happened many years ago and I would have to study again how file type plugins should create their maps, and I will not do this anytime soon. I'm much more busy in my job now than I was a few years ago. I believe that the current way of setting the key bindings is flexible enough. While you remain the only one complaining (in more than five years) I will keep assuming that Nvim-R does not have a bug on this issue.

[BlueDrink9]

Ok, I understand, thank you for the response. I will continue using my fork for now.

Would it help you (when you get time) for me to assemble examples of other plugins' use of <plug> mappings?

[jalvesaq]

Would it help you (when you get time) for me to assemble examples of other plugins' use of `<plug>` mappings?

Yes, please.

[BlueDrink9]

Here is my list, I hope it helps convince you that the way nvim-R does it is non-standard, and that this pull request does not ask for abnormal behavior.

First up is vimtex, a popular (2.5k stars) latex plugin.

• buffer-local <plug> imap defined without check for use
• the majority of its buffer-local <plug> maps defined without check for use
• Same for motion mappings
• They are only used if the user doesn't disable default mappings, but are declared regardless
• Note that this is how they are used even when mapped to local leader

Plugin author Luc Hermitte uses them here:

• LH-brackets (admittedly not an ftplugin)
• Here, Luc uses them in a similar way to Nvim-R, except the opposite way around. They are always produced, but there is only a default map to them if they aren't otherwise used.

And hopefully the nail in the coffin, the Vim sources themselves declare their <plug> mappings regardless of whether they are used.

• The default plugin MatchIt.vim (responsible for %)
• In fact, often the opposite behavior to Nvim-R

Also, adding <Plug> maps even if the user doesn't use them won't hurt anyone, because it doesn't pollute any namespaces or use up any key combinations. In conclusion, there should be nothing blocking this small PR that fixes inconsistent behaviour.

[jalvesaq]

I'm trying Nvim-R from your repository. Now, there a delay of about half a second to send lines to R. I have this in my init.vim:

nmap <Space> <Plug>RDSendLine

[BlueDrink9]

Can you also tell me the result of :verbose nmap <space>?

That sort of delay usually happens because you have something else mapped to <space>[x], and vim waits timeoutlen milliseconds to see if you are going to push the next key in the sequence or leave it at just <space>

[jalvesaq]

The output is:

n  <Space>       <Plug>RDSendLine                                                                                                                                             
        Last set from ~/.config/nvim/init.vim line 405

[BlueDrink9]

Ok, I'll have a look when I have more time and see if I can reproduce it, or see what might be causing it.

It definitely doesn't appear if you keep the same init.vim but with your nvim-R instead of mine?

[BlueDrink9]

Oh, is your localleader also <space>? It could be because previous behaviour was not to define the default mapping at all if a <plug> mapping is used. Currently, it will always define both unless g:R_user_maps_only != 0.

I can change that behaviour to still not define the default mapping if there is a <plug> mapping for that action, if you would prefer.

[jalvesaq]

My local leader is comma.

It definitely doesn't appear if you keep the same init.vim but with your nvim-R instead of mine?

That was what I did. I renamed my Nvim-R directory and cloned yours.

I can change that behaviour to still not define the default mapping if there is a mapping for that action, if you would prefer.

Yes.

[seb-mueller]

Just tried to understand this PR and it seems @BlueDrink9 has a point, according to :h usr_41.txt, hasmapto is exemplified with the reverse !hasmapto.

But what if the user wants to define his own key sequence? We can allow that
with this mechanism: >

21 if !hasmapto('TypecorrAdd')
22 map a TypecorrAdd
23 endif

This checks if a mapping to "TypecorrAdd" already exists, and only
defines the mapping from "a" if it doesn't. The user then has a
chance of putting this in his vimrc file: >

I'm gonna try out this PR to see if I can reproduce a delay.

[seb-mueller]

Giving the PR a go, everything seems to work fine, such as the default <localleader>l which is mapped to :call SendLineToR("stay")<CR> instantaneous sends the line to R.
However manually mapping nmap <Space> <Plug>RDSendLine does indeed result in a delay hitting space. Space is my leader which might wait for another input though, however mapping it to something else say <F6> instead of space still retains the delay. Changing back to before the PR, this delay seems indeed not to be there, so I'm a bit puzzled.

[BlueDrink9]

I can reproduce this using space with this minimal rc:

git clone https://github.com/BlueDrink9/Nvim-R.git /tmp/nvim-r
nvim -u NORC --cmd 'set rtp+=/tmp/nvim-r' -c 'nmap <space> <Plug>RDSendLine' -c 'call StartR("R")' test.r

Personally, baffled as to why.

[BlueDrink9]

I can change that behaviour to still not define the default mapping if there is a mapping for that action, if you would prefer.

@jalvesaq this is now done.

Figured out what's causing it, although I'm not sure why, since it seems like bizarre vim behaviour.

Since this fork now defines all plug mappings even if not in use, there are 3 plug mappings that start with the same thing:

call RCreateMaps('ni0', 'RDSendLine', 'd', ':call SendLineToR("down")')
call RCreateMaps('ni0', 'RDSendLineAndInsertOutput', 'o', ':call SendLineToRAndInsertOutput()')
call RCreateMaps('v',   'RDSendLineAndInsertOutput', 'o', ':call RWarningMsg("This command does not work over a selection of lines.")')

Deleting the second two of these solves the problem.

Lines are here

Haven't got more time to look into it now, but I'm pretty sure that this isn't the intended behavior with <plug> mappings, so might be to do with how nvim-r creates them.

[BlueDrink9]

maps to <plug>RSendLine don't have this issue either.

We need to check whether manually creating two <plug> maps with the same stem also cause this issue.

[BlueDrink9]

Sorry @jalvesaq for the long message. I hope my writing is clear and understandable.

I have confirmed that it is in fact standard vim behavior. 3 <plug> mappings start with the same stem, meaning vim waits to see if the rest of the fake map is typed.

The de-facto way to avoid this is to surround the mapping name in brackets.
(This seems to be convention in many plugins anyway.)

In this case, the fix would look like a single change in mapping:
call RCreateMaps('ni0', '(RDSendLine)', 'd', ':call SendLineToR("down")')
Or we could apply the change to the second two, since they are probably used less and it will affect fewer people:

call RCreateMaps('ni0', '(RDSendLineAndInsertOutput)', 'o', ':call SendLineToRAndInsertOutput()')
call RCreateMaps('v',   '(RDSendLineAndInsertOutput)', 'o', ':call RWarningMsg("This command does not work over a selection of lines.")')

That brings us to a cross-roads that requires @jalvesaq to choose, however. I will make an argument for why it should be changed, probably to the second solution above.
We have two people who really care about this (and given the non-standard behaviour of Nvim-R regarding declaration of <plug> mappings, I expect there are more that suffer silently).
However, to fix this and bring it up to convention, we need to make incompatible changes.
Given how stable this project has been, I can see how you might be reluctant to do that
I'm going to say first why changing it won't be that bad, then I will say why it really should be changed.

I don't think the impact would be too large, because if something breaks, a poweruser will come straight to the repo to look for the cause or complain. We could pin an issue highlighting the change and highlight it in the commit log and/or the readme, so that they see the change.
Also, it would only affect people that currently map to <plug>RDSendLineAndInsertOutput rather than using the default mappings. In exchange for making those people adjust their config slightly, we fix #494, and <plug> mappings become useful again.

This is why it needs to be changed.
I want to emphasise the problem underlying issue #494! Currently, a user cannot map a key to the <plug> mappings that they use for a different shortcut in non-R buffers. This is because they cannot set a buffer-local mapping in an autocmd.
For example, if they use <localleader>s to send to their python REPL, they cannot override it and use <localleader>s in an R buffer for nvim-R send. If they do, and switch back to a python buffer, their <localleader>s is broken.
(But even if they don't use <localleader>s for python, but do want to use it for nvim-R, that means that all non-r buffers will also have their <localleader>s key trying to send to nvim-R.)

The people that don't notice this problem, and use <plug> mappings anyway, either use obscure keys they don't use anywhere else (and so don't notice that the mapping is active in other buffers), OR they only ever use R buffers. I think this is likely to be a small group of people, if any, and it will only inconvenience them a little bit. In exchange, <plug> mappings become much more useful for everyone else.

I think the tradeoff is well-worth making some users update their config

[seb-mueller]

I'm much in favour of this PR since well justified by @BlueDrink9 . Also learned an awful lot along the way. I've just updated the this PR an haven't experienced any problems yet, but will keep testing.

[jalvesaq]

I've just tried Nvim-R from your repository again and still there is a delay when sending lines with my custom key binding (<Space>).

So, I'm thinking about doing the following:

1. Move the code that creates key bindings from R/common_global.vim to R/kb_default.vim.
2. Add a new option R_key_bindings = "default".
3. In R/common_global.vim do:

exe "runtime R/kb_" . g:R_key_bindings . ".vim"

In this way, I can maintain more than one style of defining key bindings (currently, I don't want to do this) and the user could install other scripts maintained by other people which would define the key bindings in a different way.

What do you think about this proposal?

[BlueDrink9]

It doesn't seem like the ideal solution, and like you say it introduces a lot of extra code. The suggestions I made would fix it and bring it in line with vimscript standards.

I hadn't changed my repository version, because I was waiting on your response to my last comment. I have changed it now, so please try again. The delay should be gone.

[jalvesaq]

Thanks! I did only a very quick test and there was no delay anymore. So, I merged it.