[vim/vim] vim.exe to 24bit color (#2060)
Nobuhiro Takasaki
Start with :set termguicolors.
Please build with Make_mvc.mak VCON=yes.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/2060
Commit Summary
- Added FEAT_VCON
File Changes
- M src/Make_mvc.mak (11)
- M src/feature.h (7)
- M src/misc1.c (12)
- M src/os_win32.c (388)
- M src/proto/os_win32.pro (1)
- M src/screen.c (22)
- M src/term.c (4)
- M src/version.c (5)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Nobuhiro Takasaki
before: (colorscheme desert)
after:
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -67,4 +67,5 @@ void used_file_arg(char *name, int literal, int full_path, int diff_mode); void set_alist_count(void); void fix_arg_enc(void); int mch_setenv(char *var, char *value, int x); +BOOL correct_vtp(void);
I think we are not using BOOL
Codecov
Codecov Report
Merging #2060 into master will increase coverage by
<.01%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #2060 +/- ## ========================================== + Coverage 73.73% 73.74% +<.01% ========================================== Files 90 90 Lines 129954 129954 Branches 28915 28915 ========================================== + Hits 95824 95829 +5 + Misses 34106 34105 -1 + Partials 24 20 -4
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/term.c | 50.41% <ø> (ø) |
⬆️ |
| src/version.c | 79.18% <ø> (-1.02%) |
⬇️ |
| src/misc1.c | 82.28% <ø> (ø) |
⬆️ |
| src/if_xcmdsrv.c | 84.25% <0%> (-2.04%) |
⬇️ |
| src/evalfunc.c | 82.69% <0%> (-0.02%) |
⬇️ |
| src/gui_gtk_x11.c | 47.24% <0%> (ø) |
⬆️ |
| src/window.c | 81% <0%> (+0.03%) |
⬆️ |
| src/channel.c | 82.33% <0%> (+0.08%) |
⬆️ |
| src/ex_cmds.c | 77.68% <0%> (+0.08%) |
⬆️ |
| ... and 3 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 6e32f61...debce99. Read the comment docs.
Nobuhiro Takasaki
@ntak commented on this pull request.
> @@ -67,4 +67,5 @@ void used_file_arg(char *name, int literal, int full_path, int diff_mode); void set_alist_count(void); void fix_arg_enc(void); int mch_setenv(char *var, char *value, int x); +BOOL correct_vtp(void);
I was careless...fixed.
K.Takata
This will fix #1270.
Nobuhiro Takasaki
I found it earlier.
Space key to the right, backspace key to the left.
notgc does not do this. Not even VCON=no does.
It resolves quickly.
Nobuhiro Takasaki
I made a mistake in handling the WriteConsole API.
Nobuhiro Takasaki
After scrolling, the right edge of the screen becomes colorless.
Scroll API ignores color information.
Wait for Microsoft's fix.
Or I might miss something...
Nobuhiro Takasaki
I forgot.
Start with
:set term=win32-24bit
:set termguicolors
Nobuhiro Takasaki
All problems have been resolved !
(Unexpectedly, the commit seems to have been aranged in order of date, Correctly Rewrote is later than Revert all.)
Nobuhiro Takasaki
I will check also in the GentooLinux and MinGW64 environment in the future.
Nobuhiro Takasaki
I learned.
I will make use of it next.
Nobuhiro Takasaki
I was careless again.
Now I am making changes to work only with on/off of tgc.
Nobuhiro Takasaki
K.Takata
I tried this on Windows 10 Fall Creators Update. It looks almost good, but 'tgc' is automatically set to on. It should be explicitly set by user.
Nobuhiro Takasaki
I did it like that.
I read it further and wrote it properly.
Nobuhiro Takasaki
Fix :colorscheme on notgc failed.
(Bug repro, :colorscheme blue | colorscheme default)
Nobuhiro Takasaki
The build was broken for a long time. Fixed. I wonder what I'm doing...
Bram Moolenaar
What is the status of this patch? Have enough people tried it out to know it's working well?
—
You are receiving this because you are subscribed to this thread.
K.Takata
@k-takata commented on this pull request.
@ntak Could you check my review comments?
I also wonder the prefix VCON_ is suitable for this.
Microsoft doesn't use the term "Virtual Console" in the reference: https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences
Used terms are "Virtual Terminal Sequences", "ENABLE_VIRTUAL_TERMINAL_PROCESSING", etc.
VTP_ might be prefer? What do you think?
In src/os_win32.c:
> +typedef struct _DYN_CONSOLE_SCREEN_BUFFER_INFOEX
+{
+ ULONG cbSize;
+ COORD dwSize;
+ COORD dwCursorPosition;
+ WORD wAttributes;
+ SMALL_RECT srWindow;
+ COORD dwMaximumWindowSize;
+ WORD wPopupAttributes;
+ BOOL bFullscreenSupported;
+ COLORREF ColorTable[16];
+} DYN_CONSOLE_SCREEN_BUFFER_INFOEX, *PDYN_CONSOLE_SCREEN_BUFFER_INFOEX;
+typedef BOOL (WINAPI *PfnDynGetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX);
+static PfnDynGetConsoleScreenBufferInfoEx pDynGetConsoleScreenBufferInfoEx;
+typedef BOOL (WINAPI *PfnDynSetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX);
+static PfnDynSetConsoleScreenBufferInfoEx pDynSetConsoleScreenBufferInfoEx;
The prefix Dyn is not needed for these function pointers, because they don't conflict with the actual function names. Can you change these 4 lines as follows?
typedef BOOL (WINAPI *PfnGetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); static PfnGetConsoleScreenBufferInfoEx pGetConsoleScreenBufferInfoEx; typedef BOOL (WINAPI *PfnSetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); static PfnSetConsoleScreenBufferInfoEx pSetConsoleScreenBufferInfoEx;
In src/os_win32.c:
> @@ -7232,3 +7417,521 @@ mch_setenv(char *var, char *value, int x)
return 0;
}
+
+#ifndef FEAT_GUI_W32
+
+# define VTP_FIRST_SUPPORT_BUILD MAKE_VER(10, 0, 10563)
I have some comments about this line.
- Can you add a comment to describe what this build number is?
E.g./* Windows 10 version 1507 (November Update) */ - You use the prefix
VTP_here, however you useVCON_orvcon_in other part.
Isn't it better to useVCON_here? (Or is it better to useVTP_(short for Virtual Terminal Processing) everywhere?) - Should we support 256 colors or 24-bit colors from 1507? I think they have been supported from 1703.
(Benefit for using VCON on 1507 might be support for underlines, but is it really needed?)
So, my suggestion is:
/* Support for 256 colors and 24-bit colors was added in Windows 10 * version 1703 (Creators Update). */ # define VCON_FIRST_SUPPORT_BUILD MAKE_VER(10, 0, 15063)
> @@ -67,4 +67,6 @@ void used_file_arg(char *name, int literal, int full_path, int diff_mode); void set_alist_count(void); void fix_arg_enc(void); int mch_setenv(char *var, char *value, int x); +void control_console_color_rgb(void); +int vcon_get_stat(void);
It seems that these lines conflict with the latest code.
Could you fix them?
In src/os_win32.c:
> +static void vcon_clear_to_end_of_display(void); +static void vcon_clear_to_end_of_line(void); +static void vcon_scroll(unsigned cLines); +static void vcon_standout(void); +static void vcon_standend(void); +static void hub_gotoxy(unsigned x, unsigned y); +static void vcon_gotoxy(unsigned x, unsigned y); +#endif +#ifndef FEAT_GUI_W32 +static void set_console_color_rgb(void); +static void reset_console_color_rgb(void); +#endif + +/* This flags newly created from Windows 10 */ +#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING +#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
s/#define/# define/
Please insert a space.
In src/os_win32.c:
> +static DWORD vcon_write_chars(char_u *pchBuf, DWORD cbToWrite); +static void vcon_clear_screen(void); +static void vcon_clear_to_end_of_display(void); +static void vcon_clear_to_end_of_line(void); +static void vcon_scroll(unsigned cLines); +static void vcon_standout(void); +static void vcon_standend(void); +static void hub_gotoxy(unsigned x, unsigned y); +static void vcon_gotoxy(unsigned x, unsigned y); +#endif +#ifndef FEAT_GUI_W32 +static void set_console_color_rgb(void); +static void reset_console_color_rgb(void); +#endif + +/* This flags newly created from Windows 10 */
s/This flags/This flag is/
I think this is a typo.
In src/os_win32.c:
> + WORD wAttributes; + SMALL_RECT srWindow; + COORD dwMaximumWindowSize; + WORD wPopupAttributes; + BOOL bFullscreenSupported; + COLORREF ColorTable[16]; +} DYN_CONSOLE_SCREEN_BUFFER_INFOEX, *PDYN_CONSOLE_SCREEN_BUFFER_INFOEX; +typedef BOOL (WINAPI *PfnDynGetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); +static PfnDynGetConsoleScreenBufferInfoEx pDynGetConsoleScreenBufferInfoEx; +typedef BOOL (WINAPI *PfnDynSetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); +static PfnDynSetConsoleScreenBufferInfoEx pDynSetConsoleScreenBufferInfoEx; +static BOOL has_csbiex = FALSE; +#endif + +/* This can be removed when used with FEAT_GUI. */ +#if !defined(FEAT_GUI)
I think that the lines L292-L295 can be removed.
In src/os_win32.c:
> + WORD wPopupAttributes; + BOOL bFullscreenSupported; + COLORREF ColorTable[16]; +} DYN_CONSOLE_SCREEN_BUFFER_INFOEX, *PDYN_CONSOLE_SCREEN_BUFFER_INFOEX; +typedef BOOL (WINAPI *PfnDynGetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); +static PfnDynGetConsoleScreenBufferInfoEx pDynGetConsoleScreenBufferInfoEx; +typedef BOOL (WINAPI *PfnDynSetConsoleScreenBufferInfoEx)(HANDLE, PDYN_CONSOLE_SCREEN_BUFFER_INFOEX); +static PfnDynSetConsoleScreenBufferInfoEx pDynSetConsoleScreenBufferInfoEx; +static BOOL has_csbiex = FALSE; +#endif + +/* This can be removed when used with FEAT_GUI. */ +#if !defined(FEAT_GUI) + +/* + * Get verion number including build number
s/verion/version/
Typo.
In src/os_win32.c:
> @@ -3812,7 +3927,12 @@ mch_settmode(int tmode) if (g_fMouseActive) cmodein |= ENABLE_MOUSE_INPUT; #endif - cmodeout &= ~(ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT); + cmodeout &= ~( +#if !defined(FEAT_GUI) && defined(FEAT_TERMGUICOLORS) + /* Do not turn off when using VCON */ + ((has_vcon) ? 0 : ENABLE_PROCESSED_OUTPUT) | +#endif
It looks the mask value is changed when FEAT_TERMGUICOLORS is not defined.
Additionally, I don't think !defined(FEAT_GUI) is needed, because this part is inside #ifndef FEAT_GUI_W32.
Shouldn't be as follows?
cmodeout &= ~( #ifdef FEAT_TERMGUICOLORS /* Do not turn off the ENABLE_PROCESSED_OUTPUT flag when using * VCON. */ ((has_vcon) ? 0 : ENABLE_PROCESSED_OUTPUT) | #else ENABLE_PROCESSED_OUTPUT | #endif
Nobuhiro Takasaki
I was doing other things. I will look it now.
Parker Snell
I've been using this patch for a while, overall it works great. The only issue I've found is the first time I press tab in the ex command line, the text flashes on the screen and then turns invisible. After that if I keep pressing tab, the rest of the completions are visible, it's only the first one that's a problem. This happens regardless of colorscheme and whether an actual completion exists - e.g. :asdfasdf<tab> is invisible, but :asdfasdf<tab><tab> shows up.
Parker Snell
Nobuhiro Takasaki
I redrawn the command line.
—
You are receiving this because you are subscribed to this thread.
Parker Snell
Awesome, that fixed it. Other than that I've been using this for a while with no issues 😃.
K.Takata
@ntak Could you include this patch? (Only cosmetic changes.)
https://bitbucket.org/k_takata/vim-ktakata-mq/src/fecb750f5391afa28527aee91efb914ba55d1a91/2060-3.diff?at=default&fileviewer=file-view-default
- Fix #ifdef indentation.
- Use
FEAT_GUI_W32instead ofFEAT_GUIin os_win32.c.FEAT_GUI_W32is used more thanFEAT_GUIin os_win32.c. - Don't break a line in each function declaration if the function doesn't have any parameters.
K.Takata
I also wondering what Vim should do if tgc is set on an older Windows which doesn't support virtual terminal processing.
I think that the display turns black and white in that case. Don't think most people want this. Should we avoid 'tgc' being turned on on such environment?
Nobuhiro Takasaki
It applied. Thank you very much.
—
You are receiving this because you are subscribed to this thread.
Nobuhiro Takasaki
If tgc is specified and there is no change, the user thinks it is a bug. maybe.
It is better to display black and white than to display status and errors. maybe again.
When using another terminal, is it different from .vimrc?
Nobuhiro Takasaki
Well, it seems to be a bug in black and white.
However, I want to select black and white, same as without this patch.
K.Takata
I agree that users might be confused when they set 'tgc' but the display isn't changed at all.
However I also think that the users don't want black and white display when virtual terminal processing is not supported.
Therefore my suggestion is that showing an error message and keeping 'tgc' unset, when users try to set 'tgc' and if VTP is not supported. This behavior is different from that on Unix, but I believe this is acceptable.
If users wanted to set 'tgc' only if it is available, they can write silent! set tgc in their .vimrc.
—
You are receiving this because you are subscribed to this thread.
Nobuhiro Takasaki
I am confused. I will fix it again. Please wait for a while.
—
You are receiving this because you are subscribed to this thread.
Nobuhiro Takasaki
I logically went round and returned. After all, there is no change. I will proceed as it is.
—
You are receiving this because you are subscribed to this thread.
K.Takata
@ntak Thank you for your great work. I think now it's almost okay. The remaining item is the document.
The error number should be added in it. My proposal is as follows:
--- a/runtime/doc/options.txt +++ b/runtime/doc/options.txt @@ -7832,7 +7832,7 @@ A jump table for the options with a shor :set encoding=utf-8 < You need to do this when your system has no locale support for UTF-8. - *'termguicolors'* *'tgc'* + *'termguicolors'* *'tgc'* *Exxx* 'termguicolors' 'tgc' boolean (default off) global {not in Vi} @@ -7840,7 +7840,8 @@ A jump table for the options with a shor |+termguicolors| feature} When on, uses |highlight-guifg| and |highlight-guibg| attributes in the terminal (thus using 24-bit color). Requires a ISO-8613-3 - compatible terminal. + compatible terminal. For Win32 console, Windows 10 version 1703 + (Creators Update) or later is required. If setting this option does not work (produces a colorless UI) reading |xterm-true-color| might help. Note that the "cterm" attributes are still used, not the "gui" ones.
—
You are receiving this because you are subscribed to this thread.
Nobuhiro Takasaki
Thank you very much for everything!
I used it without changing the proposal.
It was difficult with my ability.
I will fix bugs only. My greeting was delayed. I'm sorry. How about with this patch, Bram?
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Do you mean all known problems have been taken care of, ready to include the patch?
Nobuhiro Takasaki
That's right! I'm ready.
I made a lot of major changes, but I arrived.
Especially, there was help of @k-takata .
Please write my name with him.
Nobuhiro Takasaki
The problem is that API scrolling does not transfer color attributes on the right edge of the screen.
The solution is to write the right end again.
I'm sorry. This is the last fix.
—
You are receiving this because you are subscribed to this thread.
K.Takata
@ntak I found two additional problems.
- The background become gray when executed by
vim --clean --cmd "colorscheme desert", eventgcis not set. - AppVeyor fails because
:set tgccauses error on it (as we implemented so). I think we can simply skip this. (Coverage will decrease, though.)
I wrote patches for them in vim-jp. Please check them.
Nobuhiro Takasaki
Nobuhiro Takasaki
Thank you @k-takata ! Always sorry.
I feel that I am very light.
I will not say that it is the last.
—
You are receiving this because you are subscribed to this thread.
K.Takata
I think this is ready to be included.
K.Takata
Note: The error number E949 is now used in 8.0.1300. So the number used in this PR should be updated when it is merged.
@ntak Normally, Bram updates the error number by himself. So, contributors can use Exxx (or something) as a placeholder. (No need to update this only for changing the error number.)
Nobuhiro Takasaki
I am browsing this page every day...
What is going on...
What is not going on...
What should I do...