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

[RFC] provider/clipboard: allow configuration #6030

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@nhooyr
Copy link
Contributor

nhooyr commented Jan 30, 2017

Closes #6029
Closes #6658

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 30, 2017

Tested on my server, looks good.

@tweekmonster

This comment has been minimized.

Copy link
Member

tweekmonster commented Jan 30, 2017

The script still looks a little rigid for something that's supposed to be configurable. Couldn't each clipboard executable be in a list?

  let s:defaults = [
        \   {
        \     'cmd': ['pbcopy', 'pbpaste'],
        \   },
        \   {
        \     'cmd': 'xsel',
        \     'check': function('s:check_xsel'),
        \     '+': ['--nodetach -i -b', '-o -b'],
        \     '*': ['--nodetach -i -p', '-o -p'],
        \   },
        \   ...
        \ ]

Then iterate over the list to find the first available cmd. If the user has g:clipboard_providers (also a list), it's checked first before falling back to the default list. This way, the user could just use a memcached server or something (nc in your case) without needing to fool the provider script.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 30, 2017

Ah, good point, I'll do that tomorrow.

@nhooyr nhooyr changed the title provider/clipboard.vim: allow configuration of clipboard executable [WIP] provider/clipboard.vim: allow configuration of clipboard executable Jan 31, 2017

@marvim marvim added the WIP label Jan 31, 2017

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch 2 times, most recently from a06eb6d to 9c6daa8 Jan 31, 2017

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch from 9c6daa8 to 5faa005 Jan 31, 2017

@nhooyr nhooyr changed the title [WIP] provider/clipboard.vim: allow configuration of clipboard executable [RFC] provider/clipboard.vim: allow configuration of clipboard executable Jan 31, 2017

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 31, 2017

Ok, done! 🎉

edit: jk

\ '*': 'pbpaste',
\ },
\ 'cache_enabled': 0,
\ 'check': "executable('pbcopy')",

This comment has been minimized.

@nhooyr

nhooyr Jan 31, 2017

Author Contributor

I intentionally removed the has('mac') check from here because now that it is configurable, we don't need to check it. If someone has pbcopy on a non-mac os, and it does not do clipboard pasting/copying properly, they should just configure whatever they need themselves.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 31, 2017

Alright, done for now.

@marvim marvim added RFC and removed WIP labels Jan 31, 2017

@tweekmonster
Copy link
Member

tweekmonster left a comment

Looks good to me.

I think it's a little weird that there's so much repetition with the command names. pbcopy + pbpaste seem to be the only one that require this. I wouldn't block this PR over it, though.

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch from 640926d to 2cdb45b Jan 31, 2017

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jan 31, 2017

Alright cool, I just noticed a small mistake; I accidentally deleted a line. Added it back.

@nhooyr nhooyr changed the title [RFC] provider/clipboard.vim: allow configuration of clipboard executable [RFC] provider/clipboard: allow configuration of clipboard executable Jan 31, 2017

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch 3 times, most recently from 21aac9f to 5342642 Jan 31, 2017

@nhooyr nhooyr changed the title [RFC] provider/clipboard: allow configuration of clipboard executable [RFC] provider/clipboard: allow configuration of clipboard provider Jan 31, 2017

@nhooyr nhooyr changed the title [RFC] provider/clipboard: allow configuration of clipboard provider [RFC] provider/clipboard: allow configuration Jan 31, 2017

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch from 5342642 to 548b657 Jan 31, 2017

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 23, 2017

What is the status?

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 23, 2017

Nothing so far. I have an exam today and then I am free for the holidays. I'll fix it up then.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 26, 2017

Updated :)

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 26, 2017

would prefer to avoid the churn. Is it really needed to move most of the code?

let s:paste = a:p.paste
let s:cache_enabled = a:p.cache_enabled
return a:p.name
endfunction

This comment has been minimized.

@justinmk

justinmk Jun 26, 2017

Member

this isn't actually saving lines since the dictionary definition ends up being 50% bigger. So the churn isn't worth it.

let s:copy['*'] = s:copy['+']
let s:paste['*'] = s:paste['+']
return 'tmux'
if exists('g:clipboard_provider')

This comment has been minimized.

@justinmk

justinmk Jun 26, 2017

Member

just check this before the above lines.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 27, 2017

I didn't indent to save lines. I wanted a homogenous way of handling both external and internal clipboard providers. In addition, I think it's a lot easier to read the list of dictionaries than the chain of if else statements.

Since you want to avoid the churn though, I'll update this PR in a few hours and remove it.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 27, 2017

Done.

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch from 4fdce40 to 9262c1d Jun 27, 2017


function! s:set_provider(p) abort
function! provider#clipboard#Executable() abort
if exists('g:clipboard_provider')
let s:copy = a:p.copy
let s:paste = a:p.paste
let s:cache_enabled = a:p.cache_enabled
return a:p.name

This comment has been minimized.

@chrisbra

This comment has been minimized.

@nhooyr

nhooyr Jun 27, 2017

Author Contributor

argh, copy paste error

@nhooyr nhooyr force-pushed the nhooyr:clipboard branch from 9262c1d to 8c801ea Jun 27, 2017

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 27, 2017

Thanks @nhooyr . One last thing, let's rename it to g:clipboard_prog in analogy with g:python_host_prog, 'equalprg', etc. "Provider" is more the name for the abstraction: changing the provider would entail changing autoload/provider/clipboard.vim (which is something we support).

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 27, 2017

Ok done. Do you want me to clean up the commit history by turning it into a single commit or can you do that and merge?

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 27, 2017

I'll squash it. Actually the name could be g:clipboard since it's a dictionary of options, there won't be any other g:clipboard_foo things. Though that's confusing since there is also a 'clipboard' option. Ideally we would turn 'clipboard' option into a dictionary option, but that's out of scope...

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Jun 27, 2017

Ok, updated.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 28, 2017

Merged. Thanks @nhooyr

@justinmk justinmk closed this Jun 28, 2017

@justinmk justinmk removed the RFC label Jun 28, 2017

justinmk added a commit that referenced this pull request Jun 28, 2017

@Shougo Shougo referenced this pull request Jun 28, 2017

Open

clipboard support #46

DanThomson pushed a commit to DanThomson/neovim that referenced this pull request Sep 8, 2017

Dan Thomson
Merge remote-tracking branch 'upstream/master'
* upstream/master: (95 commits)
  provider/clipboard.vim: Handle missing g:clipboard keys
  provider/clipboard.vim: allow configuration neovim#6030
  ex_getln: Lint command_line_handle_key readability/fn_size
  vim-patch:7.4.2320
  vim-patch:7.4.2318
  vim-patch:7.4.2268
  functests/legacy: Add lua version of test_search.vim
  socket.c: Disable Nagle's algorithm on TCP sockets (neovim#6915)
  vim-patch:7.4.2259
  scripts/pvscheck.sh: fix function rename
  scripts/pvscheck.sh: HACK: de-parallelize on CI
  scripts/pvscheck.sh: HACK: de-parallelize on CI
  bufhl: fix move
  bufhl: some style cleanup
  kbtree: make warning free and delete deprecated macros
  kbtree: eliminate unneccesary heap allocation
  kbtree: use proper structs
  kbtree: allow iterators to start at arbitrary position
  bufhl: use kbtree for bufhl
  kbtree.h
  ...

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.