Improve setup of mappings

mtth / cursorcross.vim

Sane cursorcolumn and cursorline handling

33 stars 1 forks source link

Improve setup of mappings #3

Closed blueyed closed 10 years ago

mtth commented 10 years ago

Thanks!

Could you do two quick changes before I merge it?

Cleaning up the lines with the empty return values in autoload/cursorcross.vim.
Removing the <unique> in the mappings inside the if g:cursorcross_mappings block. The error it'll throw isn't caught and in some cases you want to be able to override your mappings. Whereas disabling them is as easy as setting g:cursorcross_mappings to 0.

blueyed commented 10 years ago

Could you do two quick changes before I merge it? Sure.

The error it'll throw isn't caught and in some cases you want to be able to override your mappings The idea is that the user gets notified about their mapping getting overwritten.

Whereas disabling them is as easy as setting g:cursorcross_mappings to 0. Yes, but you need to be made aware of it.

The error it'll throw isn't caught That would be useful to point at g:cursorcross_mappings. I would have done it, but a try block around all of them would only catch the first error, and I also wanted to avoid having multiple catched errors with the same hint at g:cursorcross_mappings.

Just wanted to discuss this a bit - let me know if you still want it like outlined above.

Also, feel free to merge it and then change it like as you see fit, of course.

mtth commented 10 years ago

In my opinion, it's still not a worthy tradeoff to force these errors just to make sure the user is aware of the mappings getting overwritten.

If the user doesn't use the bindings, it doesn't matter anyway. But it is inconvenient to force them to change g:cursorcross_mappings just to suppress the errors.
If the user uses the bindings, it's very easy for them to figure out which plugin last mapped them, and from there disable them (it's really obvious from the code how). That is, if they haven't seen it from the documentation already. Also, not all plugins provide such a mapping disabling option, and in the odd chance these keys are mapped, this will lead to ugly hacks to keep the cursorcross ones (i.e. still disabling g:cursorcross_mappings but remapping them somewhere else).

I don't mind doing the changes myself if you haven't time. Probably sometime this weekend.

blueyed commented 10 years ago

@mtth Sorry, I had forgotten about this and just did your suggested edits.

Let me know, if I should squash/rebase/cleanup.

blueyed commented 10 years ago

btw: I am using the following to make it play nice together with delimitMate and endwise.

" Force delimitMate mapping (gets skipped if mapped already).
fun! My_CR_map()
  " "<CR>" via delimitMateCR
  return "\<Plug>delimitMateCR\<Plug>CursorCrossCR\<Plug>DiscretionaryEnd"
endfun
imap <expr> <CR> My_CR_map()

You might want to add this to the README.

blueyed commented 10 years ago

I've just reverted the "cleanup" - see the commit message.

mtth commented 10 years ago

Good to know about the empty string return value. Thanks for the edits!