[vim/vim] Revert clipboard changes (PR #18633)
Foxe Chen
Reverts commits, addresses pointed out by @ychin:
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/18633
Commit Summary
- 1035e8d revert clipboard changes
File Changes
(28 files)
- M runtime/doc/builtin.txt (3)
- M runtime/doc/eval.txt (143)
- M runtime/doc/options.txt (29)
- M runtime/doc/tags (10)
- M runtime/doc/various.txt (14)
- M runtime/doc/version9.txt (20)
- M runtime/doc/wayland.txt (6)
- M runtime/syntax/vim.vim (4)
- M src/auto/configure (31)
- M src/clipboard.c (707)
- M src/config.h.in (3)
- M src/configure.ac (15)
- M src/evalfunc.c (32)
- M src/evalvars.c (3)
- M src/feature.h (13)
- M src/globals.h (16)
- M src/gui.c (2)
- M src/optiondefs.h (6)
- M src/optionstr.c (2)
- M src/proto/evalfunc.pro (1)
- M src/register.c (4)
- M src/testdir/Make_all.mak (2)
- D src/testdir/test_clipboard_provider.vim (172)
- M src/testdir/test_clipmethod.vim (35)
- M src/testdir/test_eval_stuff.vim (46)
- M src/testdir/test_options.vim (2)
- M src/version.c (21)
- M src/vim.h (6)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman pushed 1 commit.
- c89fdc1 revert clipboard changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#18633)I will create a new PR for the clipboard provider feature using a more sane approach later, see this comment.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman pushed 1 commit.
- a1aa228 revert clipboard changes
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman commented on this pull request.
> @@ -41902,29 +41900,13 @@ Options: ~ compositor Vim Variables: ~ -|v:clipmethod| The current 'clipmethod'. -|v:clipproviders| A dictionary containing clipboard providers - information. -|v:stacktrace| The most recent caught exception. -|v:t_enumvalue| Value of |enumvalue|.
Not sure why there are unrelated stuff here
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman pushed 1 commit.
- e24d5a0 revert clipboard changes
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman pushed 1 commit.
- 8bcf527 revert clipboard changes
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman pushed 1 commit.
- 36f564e revert clipboard changes
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18633)Just for context (since this PR description doesn't explicitly mention it), this is to fix #18600. The recent clipboard changes has completely broken clipboard support in different platforms, e.g. macOS.
I checked out this PR and it does fix the issue. I didn't realize there are so many commits that need to be reverted. In particular, 528196c (v9.1.1704) doesn't seem necessary for this (this is the change that introduced "gui" and "other" options for the clipmethod)? It's up to you guys whether you want to revert it but that commit is older than the others and also not the cause of this breakage. The main offender is v9.1.1857.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
> @@ -41902,29 +41900,13 @@ Options: ~ compositor Vim Variables: ~ -|v:clipmethod| The current 'clipmethod'. -|v:clipproviders| A dictionary containing clipboard providers - information. -|v:stacktrace| The most recent caught exception. -|v:t_enumvalue| Value of |enumvalue|.
Seems like maybe @chrisbra was using the opportunity to add some missing docs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#18633)Just for context (since this PR description doesn't explicitly mention it), this is to fix #18600. The recent clipboard changes has completely broken clipboard support in different platforms, e.g. macOS.
I checked out this PR and it does fix the issue. I didn't realize there are so many commits that need to be reverted. In particular, 528196c (v9.1.1704) doesn't seem necessary for this (this is the change that introduced "gui" and "other" options for the clipmethod)? It's up to you guys whether you want to revert it but that commit is older than the others and also not the cause of this breakage. The main offender is v9.1.1857.
The "gui" and "other" values were meant for the clipboard provider feature so they aren't needed anymore. Many of these PRs are just small fixes
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -41902,29 +41900,13 @@ Options: ~ compositor Vim Variables: ~ -|v:clipmethod| The current 'clipmethod'. -|v:clipproviders| A dictionary containing clipboard providers - information. -|v:stacktrace| The most recent caught exception. -|v:t_enumvalue| Value of |enumvalue|.
yes correct
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18633)Thanks for the fix. I can confirm that clipboard is once again working on macOS.
I was actually a little curious why the previous change even passed CI, as we do have test_quotestar which is supposed to test whether clipboard is working or not. The key issue here is that it does CheckFeature clipboard_working, but when clipboard is broken this feature flag would be off and cause the entire test to be skipped and so we have one of those silent failures.
I was trying to fix a similar issue in #16998, where we needed some ways to enforce that certain feature flags aren't just turned off unintentionally. In that PR I added the if_feat_check.vim for passing in a list of expected features that should be compiled in and enabled, e.g. python, sound, etc. It probably makes sense to add clipboard_working to that list. One caveat is that I didn't add this check to the Windows CI, so we still have a bit of a coverage gap there.
I also think it would make sense for the new PR to write new tests to make sure v:clipmethod to be set correctly for these other platforms, and if that was written, it would also have caught this issue.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#18633)I also think it would make sense for the new PR to write new tests to make sure v:clipmethod is set correctly for these other platforms (in this case it was supposed to be "other" rather than "none"), and if that was written, it would also have caught this issue.
The "other" and "gui" values for v:clipmethod have been removed, so no need for tests
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.