[vim/vim] fix popup opacity blend leaks white bg from textprop with undercurl-only highlight (PR #20095)
mattn
hl_blend_attr() only treated INVALCOLOR as "no underlying color", but the cterm.fg_rgb / cterm.bg_rgb fields can also hold the CTERMCOLOR sentinel (0x1FFFFFE) which means "use the cterm color". A textprop or sign highlight that only sets gui=undercurl/guisp leaves these fields as CTERMCOLOR, so blend_colors() blended that sentinel as a real near-white pixel and a faint white/gray bg appeared on textprop-covered cells under an opacity popup. Use COLOR_INVALID() instead so both INVALCOLOR and CTERMCOLOR are skipped.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20095
Commit Summary
- 0ef30c7 fix popup opacity blend leaks white bg from textprop with undercurl-only highlight
File Changes
(3 files)
- M src/highlight.c (8)
- A src/testdir/dumps/Test_popupwin_opacity_textprop_undercurl.dump (8)
- M src/testdir/test_popupwin.vim (33)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- f348e3b fix pum opacity blend leaks white bg from textprop with undercurl-only highlight
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 781c213 fix popup/pum opaque text leaks underlying decoration through blend
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- e634867 fix popup/pum opacity blend in 256-color terminals without termguicolors
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20095)The cterm blending added in e6348670d2 was originally a workaround: when an opacity popup is layered over a highlight that has only cterm colors (no gui definition), the blend cannot be computed in the truecolor path, so we had to handle it on the cterm side. As a side effect, opacity now produces a visually meaningful result in 256-color terminals as well — it's no longer effectively a no-op without termguicolors.
Given that we've ended up there anyway, would it make sense to officially open up opacity support to notermguicolors? Happy to update the docs accordingly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 4 commits.
- acb9d1c fix popup opacity blend leaks white bg from textprop with undercurl-only highlight
- f125679 fix pum opacity blend leaks white bg from textprop with undercurl-only highlight
- 6502d7c fix popup/pum opaque text leaks underlying decoration through blend
- 82b70a3 fix popup/pum opacity blend in 256-color terminals without termguicolors
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 2e5a457 fix tiny build of popup opacity blend (unused param, unguarded calls)
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- f485fe1 update pumopt opacity screendumps for cterm blend fix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 49708a3 update popupwin opacity screendumps for cterm blend fix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 2c2e126 update Test_popupwin_opacity_textprop_undercurl screendump
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20095)Test_popupwin_opacity_textprop_undercurl.dump updated from the FreeBSD CI log diff: locally my Vim's RunVimInTerminal does not emit the +8 undercurl attribute on the textprop-covered cells, so I reconstructed the expected dump from FreeBSD's actual output. Linux/macOS CI may fail this test until that platform difference is resolved.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 1f1bd26 revert Test_popupwin_opacity_textprop_undercurl screendump update
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 778e865 patch 9.2.0422: popup tests depend on terminal undercurl support
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- a4e5039 Make Test_visual_block_scroll less flaky
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- e012836 Update scrolloffpad fold screendumps
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- e3e0659 Stabilize Test_scrolloffpad_with_folds
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
It seems like this patch series mixes a few things:
- stabilizing screen dump tests
- add blending for cterm colors
Can you squash those 2 things into separate commits please? Also, please delete the then un-used dump files.
For the cterm-blending I am also wondering if we need to handle 16 and 88 color terminals? Or just document that this works on a 256 color terminal only?
In src/testdir/test_plugin_matchparen.vim:
> @@ -23,8 +23,13 @@ func Test_visual_block_scroll()
let buf = RunVimInTerminal('-S '.filename, #{rows: 7})
call term_sendkeys(buf, "V\<C-D>\<C-D>")
- call WaitForAssert({-> assert_match('VISUAL.*\d\+\s\+\d', term_getline(buf, 7))}, 1000)
- call VerifyScreenDump(buf, 'Test_display_visual_block_scroll', {})
I think we can drop that dump file then.
In src/testdir/test_scroll_opt.vim:
> call term_sendkeys(buf, "\<Esc>:\<C-U>normal! G\<CR>")
call term_sendkeys(buf, "\<C-L>")
- call TermWait(buf)
- call VerifyScreenDump(buf, 'Test_scrolloffpad_folds_1', {})
Same here, drop the unused dump file please
In src/testdir/test_scroll_opt.vim:
> call term_sendkeys(buf, "\<Esc>:\<C-U>normal! 60G\<CR>")
call term_sendkeys(buf, "\<C-L>")
- call TermWait(buf)
- call VerifyScreenDump(buf, 'Test_scrolloffpad_folds_2', {})
same here
In src/testdir/test_scroll_opt.vim:
> call term_sendkeys(buf, "\<Esc>:\<C-U>normal! zc\<CR>")
call term_sendkeys(buf, "\<Esc>:\<C-U>normal! G\<CR>")
call term_sendkeys(buf, "\<C-L>")
- call TermWait(buf)
- call VerifyScreenDump(buf, 'Test_scrolloffpad_folds_3', {})
and here
In src/highlight.c:
> +
+/*
+ * Convert an xterm 256-color index (0-255) to an approximate RGB triple.
+ * Uses the standard xterm palette: 0-15 ANSI, 16-231 6x6x6 cube,
+ * 232-255 grayscale ramp.
+ */
+ static void
+cterm_idx_to_rgb(int idx, int *r, int *g, int *b)
+{
+ static const int cube[] = { 0x00, 0x5F, 0x87, 0xAF, 0xD7, 0xFF };
+ static const int ansi[16][3] = {
+ { 0, 0, 0 }, { 224, 0, 0 }, { 0, 224, 0 }, { 224, 224, 0 },
+ { 0, 0, 224 }, { 224, 0, 224 }, { 0, 224, 224 }, { 224, 224, 224 },
+ { 128, 128, 128 }, { 255, 64, 64 }, { 64, 255, 64 }, { 255, 255, 64 },
+ { 64, 64, 255 }, { 255, 64, 255 }, { 64, 255, 255 }, { 255, 255, 255 },
+ };
So this is hard coding the first 16 cterm values. Not sure how common it is for users to re-configure this?
In src/highlight.c:
> + */
+ static int
+resolve_color_to_rgb(int cterm_c, guicolor_T rgb UNUSED, int *r, int *g, int *b)
+{
+#ifdef FEAT_TERMGUICOLORS
+ if (!COLOR_INVALID(rgb))
+ {
+ *r = (rgb >> 16) & 0xFF;
+ *g = (rgb >> 8) & 0xFF;
+ *b = rgb & 0xFF;
+ return TRUE;
+ }
+#endif
+ if (cterm_c > 0)
+ {
+ cterm_idx_to_rgb(cterm_c - 1, r, g, b);
This assumes 256 color terminal I guess? Should we guard this with t_colors >= 256?
In src/highlight.c:
> + if (d < best_d)
+ {
+ best_d = d;
+ best = i;
+ }
+ }
+ return best;
+}
+
+/*
+ * Resolve a color to an RGB triple. Prefer the gui RGB value (set by
+ * guifg/guibg) when valid, otherwise convert the cterm 256-color index.
+ * Returns TRUE on success; FALSE when no color is defined.
+ * cterm_c is 1-based (0 means "no color").
+ */
+ static int
bool?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 2 commits.
- 92394c6 stabilize popup/scroll/matchparen screendump tests
- 88f0cfd fix popup/pum opacity blend leaks white bg from textprop with undercurl-only highlight
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20095)Thanks for the review! Updated the branch:
- Squashed the series into two commits:
stabilize popup/scroll/matchparen screendump testsandfix popup/pum opacity blend leaks white bg from textprop with undercurl-only highlight. - Removed the unused dump files (
Test_display_visual_block_scroll,Test_scrolloffpad_folds_{1,2,3}). - Changed
resolve_color_to_rgbto returnbool. - Guarded the cterm blend on
t_colors >= 256; on terminals with fewer colors it falls back to the popup color so opacity becomes a no-op rather than producing an out-of-range index. The 6x6x6 cube and grayscale ramp baked intocterm_idx_to_rgbare the standard xterm-256 palette, so this scopes us cleanly to 256-color terminals; 16/88-color support would need a separate palette table and I'd rather defer that. - Documented the requirement (
popup-opacity,pumopt): GUI,'termguicolors', or a 256-color terminal.
PTAL.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn commented on this pull request.
In src/highlight.c:
> +
+/*
+ * Convert an xterm 256-color index (0-255) to an approximate RGB triple.
+ * Uses the standard xterm palette: 0-15 ANSI, 16-231 6x6x6 cube,
+ * 232-255 grayscale ramp.
+ */
+ static void
+cterm_idx_to_rgb(int idx, int *r, int *g, int *b)
+{
+ static const int cube[] = { 0x00, 0x5F, 0x87, 0xAF, 0xD7, 0xFF };
+ static const int ansi[16][3] = {
+ { 0, 0, 0 }, { 224, 0, 0 }, { 0, 224, 0 }, { 224, 224, 0 },
+ { 0, 0, 224 }, { 224, 0, 224 }, { 0, 224, 224 }, { 224, 224, 224 },
+ { 128, 128, 128 }, { 255, 64, 64 }, { 64, 255, 64 }, { 255, 255, 64 },
+ { 64, 64, 255 }, { 255, 64, 255 }, { 64, 255, 255 }, { 255, 255, 255 },
+ };
Oh, I found cterm_color_to_rgb
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
mattn
@mattn pushed 1 commit.
- 7f56031 fix popup/pum opacity blend leaks white bg from textprop with undercurl-only highlight
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20095)Re hardcoded ANSI 0-15: refactored to share the existing cterm_color_16[] table with cterm_color_to_rgb(), which Vim has been using for the same purpose. So we now have a single source of truth instead of two slightly different tables. (Whether those values match every emulator's actual palette is a pre-existing concern beyond this PR.)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20095)thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.