Terminal color improvements #23
Conversation
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
I'd add the issues this resolves to the PR body.
I've tested it, and it seems to work nicely. A few comments for your consideration.
Signed-off-by: Egmont Koblinger <[email protected]>
…dling No longer ship two different 16-color palettes. Fix the exact color values of the 6x6x6 color cube. Fix the tooltip values of the "colors hint" section. Code refactored and simplified. Signed-off-by: Egmont Koblinger <[email protected]>
Signed-off-by: Egmont Koblinger <[email protected]>
egmontkob
force-pushed
the
terminal-color-improvements
branch
from
24661f2 to
2e3c6b9
Compare
|
I'm a little bit inconsistent with whitespaces. I hate the current style of I tried to follow the spaceless style when touching previous files, but went with the spaces in the new Do you have a preference? Maybe reformat the entire codebase to a nicer indentation? Follow mc's styleguide? |
I have a preference of either not caring about it at all, or of installing an auto-formatter and sticking with whatever it does. In terms of choosing the auto-formatter preset, I usually take the defaults as long as I can possibly tolerate them. The "default" formatter in the JavaScript world is Prettier. It can also do HTML, CSS, and JSON. Right now, working on this repository doesn't require npm, but we could add it: https://github.com/zyv/xyzzy.gmbh Not necessarily a good example, but to give you an idea of what this means (check I guess this might make sense, since I'm already doing "building" in CI for deployment. Or we can rely purely on stuff like autofix.ci at the moment - https://prettier.io/docs/ci . Or both. In any case, I would add this stuff separately and reformat the whole thing in one go (if we opt to do this). |
egmontkob
force-pushed
the
terminal-color-improvements
branch
from
2e3c6b9 to
4984c25
Compare
- Various palettes for the first 16 colors - Light vs. dark mode (swapping the default fg and bg colors) - Background image (stripes, implemented by applying gray with alpha) - Brightening the first 8 colors when bold (SGR 1 legacy confusion) Fix the handling of the 'default' color. Signed-off-by: Egmont Koblinger <[email protected]>
egmontkob
force-pushed
the
terminal-color-improvements
branch
from
4984c25 to
00ef5fe
Compare
If a character's definition was missing, empty, or multiple characters, it caused the screenshot to fall apart. Let's use a space in these cases. This is still not perfect, but I don't want to duplicate and later on maintain the list of defaults from mc. Signed-off-by: Egmont Koblinger <[email protected]>
|
Okay, I'm not touching the formatting. That's for a script to do at some time, not now. Could you please take another look? |
2 participants
This PR contains all the color handling related changes I wanted to land.
The way it's split to commits may not be ideal, but I don't want to waste too much time on clarifying that, so I hope you'll be a bit lenient :)