windows colors improvements #645
Conversation
|
@mattn, any objection to these changes? |
|
Thanks for making this PR. Since I don't use Windows, I'd love to hear other Windows users chime in on this. Also, I'm finally adding clang-format tests to ag, so you'll have to rebase to get this mergeable again. |
|
Not sure I understand the rebase comment, these patches are on top of the current tip. Or did you mean you will add clang-format tests, and after that these patches will have to be rebased? As for the other changes, unused variables and no-op static declarations, assuming that a buffer is null terminated when it might not be, interpreting all color codes as bold (making it impossible to use non bold colors), not being able to see the color of Do you expect other windows users to apply these patches, compile Despite the big number of patches, the changes to the code are few. It just fixes what was incorrect, and adds the |
Thanks. Done. |
on environments which can build ag for windows with the default Makefile, such as MXE, print_w32.c was not built since it only appears at Makefile.w32, and so the build failed. include this file also at Makefile.am but put the whole of it in #idfef _WIN32. also add correct includes, removed some unused variables, and initialize others
this fixes undefined behavior when a single fprintf_w32 call ends up longer than BUFSIZ - which is only guarenteed to be "at least 256 bytes". this patch defines an arbitrary limit of 16k chars per call, and truncates correctly to this size if it's exceeded. it also falls back to passthrough if stdout is not a tty. this fixes pager (had bad output regardless of color) and pipe (never had color). now both of them handle colors with ag like on *nix systems.
on cases where the terminal or the pager have better ansi color support than the current code has (e.g. less for windows does better, or ConEmu supports 24 bit SGR ansi sequences which the existing code can't assume or handle, etc), allow the user to bail out of the native colors and let other applications handle the ansi sequences.
the code assumed that state doesn't carry between calls, and as such treated the screen color at the time of call as the "reset" color, and used this reset color when exiting the function. it does seem like it at least tried to keep the state (static attr_old) however it's never actually used as static and therefore overwritten on each call. (same goes for static stdo which is overwritten every time, but at least this one doesn't hurt us other than slighly slower code). this broke the matches colors since the color is printed in one fprintf call, and the text in another, and so the color was reset after the first call and didn't carry into the second, so it ended up without color. this patch initializes attr_old only once, removes the color reset at the end of each fprintf, and also doesn't reset it before any color change. also removes the static for stdo since it's never used as static.
- plain 30-37 and 40-47 should not set bold on, but they did - reset should (and now does) also reset all non color attributes - blink now used for bg intensity (same as bash/less/ansicon for windows) - uncommented and fixed fg/bg specific color resets - replaced a magic number 6 (max sequence length) with something more readable - removed 100 as reset (90-97 and 100-107 are aixterm codes for bright fg/bg) it's still not as robust as other ansi color interpreters for windows, especially when it comes to complex sequences, but now it does get the basics mostly right and similar to others for simple sequences also, fprintf_w32 tries to handle several other types of ansi sequences which ag never uses (cursor position, etc) except for K which is typically no-op anyway. I didn't look at those parts of the code, but they could probably be removed safely.
avih
force-pushed
the
win-colors-pr333-fixes
branch
from
8142609
to
4657dbb
Compare
Done. |
|
Where the |
|
Ah, I found it. |
|
Hmm.. it seems to me like the Travis CI failure is is unrelated to the patch. But if someone thinks otherwise, let me know. |
|
|
||
| const char *color_line_number = "\033[1;33m"; /* yellow with black background */ | ||
| const char *color_line_number = "\033[1;33m"; /* bold yellow */ |
There was a problem hiding this comment.
The color didn't change but the comment did?
There was a problem hiding this comment.
Yes I just fixed the comment. It's bold yellow, the same as 1;32 two lines below is bold green. Yellow on black background is 0;33, or rather 0;33;40 if you want to make sure the background is black in case the default background from reset (0) is not black.
There was a problem hiding this comment.
Ah, got it. That's probably been wrong for years. :)
There was a problem hiding this comment.
I'd never have noticed it myself, but I happened to spend way too much time recently studying the ansi color escape sequences and how they behave on different systems, wrote an implementation from scratch, and then fixed another one ;)
|
The Travis breakage was my fault. I've fixed it in master. I made a couple comments, but they're not show-stoppers. If you could rebase and move |
Makefile.w32BUFSIZ