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

fix user defined key map options #3351

Closed
wants to merge 1 commit into from

Conversation

elbeardmorez
Copy link

@elbeardmorez elbeardmorez commented Mar 22, 2019

commit message:

-don't use [nore]map mapping flavours otherwise any recursive mappings
employed by the user won't work

more detail

-this change is useful as it allows working key maps for anyone using
the (fairly fundamental) recursive mapping nature of vim
-with my (rather limited) understanding, i believe that plugin devs are
only supposed to 'protect' (via [nore]map) internal mappings. e.g.
when exposing a public command via <Plug>ycm..., 'noremap'ping of
that to an internal script function is all good. however, when taking a
user's key input, it's not really their place to provide 'protection' of that.
as with the above issue, it might be an intermediate map/name that
doesn't represent a valid/working key symbol

testing

-this is a trivial vimscript change, touching nothing of this plugin's core
functionality, thus i see it as an 'either author agrees with above point'
and it gets merged, else it gets rejected - the use of noremap might
very well be intentional and i've no interest in debating that 😄

Thank you for this plugin.

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • [ x ] I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • [ x ] I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

This change is Reviewable

-don't use [nore]map mapping flavours otherwise any recursive mappings
employed by the user won't work
@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #3351 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3351   +/-   ##
=======================================
  Coverage   94.51%   94.51%           
=======================================
  Files          20       20           
  Lines        2062     2062           
=======================================
  Hits         1949     1949           
  Misses        113      113

@bstaletic
Copy link
Collaborator

Thanks for the PR.

I've seen a lot of vim users mess up their vim configuration by using recursive maps, so I don't think using recursive mappings anywhere where it is not absolutely necessary (<Plug>) as a good thing. That said, YCM using nonrecursive mappings doesn't stop you from making YCM's functions an "itermediate" mapping.

Here's an example: Users often have some parenthesis auto closing plugin that maps <BS>, but YCM also maps <BS> by default. The solution is to do something like #2696 (comment) suggests.

If there's any use case that isn't addressed by the above, feel free to explain.

Copy link
Collaborator

@micbou micbou left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @elbeardmorez)


autoload/youcompleteme.vim, line 330 at r1 (raw file):

  for key in [ "<BS>", "<C-h>" ]
    silent! exe 'imap <unique> <expr> ' . key .
          \ ' <SID>OnDeleteChar( "\' . key . '" )'

This breaks completion when deleting characters as it will recurse on the <C-y> mapping in OnDeleteChar.

@elbeardmorez
Copy link
Author

@micbou: yep, nice spot. assuming <C-y> is mapped to anything.. indeed first thing I changed was the ycm_key_list_stop_completion var. Incidentally, aren't all commands with the hardcoded <C-y> (destined for feedkeys) effectively no-ops when that global (g:ycm_key_list_stop_completion) has had its default (<C-y>) removed?

@bstaletic: thanks for the link, I can work around anything yes 😄 . although I don't agree with the reasoning for protecting users from (some) vim functionality, I certainly understand your position. As such, please close this PR (if the button below fails to in any way!) - the above issue raised by @micbou is sufficient to let sleeping dogs lie

Thank you both for your time.

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

Successfully merging this pull request may close these issues.

None yet

3 participants