Add better color support#224
Conversation
sjakobi
left a comment
There was a problem hiding this comment.
Thanks for this PR, @sofia-m-a!
Unfortunately I'm very unfamiliar with terminal colors and this part of the code, so if I seem to misunderstand something, please do correct me.
I hope that @quchen will soon provide a review too.
That said, what I've seen here looks pretty good to me so far.
One question regarding compatibility / versioning: Are there any other breaking changes here apart from the additional constructors in Bold and Underlined?
| data Bold = Bold deriving (Eq, Ord, Show) | ||
| data Underlined = Underlined deriving (Eq, Ord, Show) | ||
| data Italicized = Italicized deriving (Eq, Ord, Show) | ||
| -- FaintIntensity is not widely supported: sometimes treated as concealing text. Not supported natively on Windows 10 |
There was a problem hiding this comment.
| -- FaintIntensity is not widely supported: sometimes treated as concealing text. Not supported natively on Windows 10 | |
| -- Faint is not widely supported: sometimes treated as concealing text. Not supported natively on Windows 10 |
| -- FaintIntensity is not widely supported: sometimes treated as concealing text. Not supported natively on Windows 10 | ||
| data Bold = Bold | Faint deriving (Eq, Ord, Show) | ||
| -- DoubleUnderline is not widely supported. Not supported natively on Windows 10 | ||
| data Underlined = Underlined | DoubleUnderlined deriving (Eq, Ord, Show) | ||
| data Italicized = Italicized deriving (Eq, Ord, Show) | ||
| -- Swap the foreground and background colors. Supported natively on Windows 10 | ||
| data Inverted = Inverted deriving (Eq, Ord, Show) |
There was a problem hiding this comment.
The comments seem useful for users too, so how about turning them into Haddock comments?!
There was a problem hiding this comment.
Add Haddock comments to the types. Not really for the source code’s sake, but Haddock with un-haddocked definitions looks like a bit barren.
There was a problem hiding this comment.
Whoops, I meant for these to be Haddock comments :)
| -- | A color from a palette of 256 colors using a numerical index (0-based). | ||
| -- Supported natively on Windows 10 from the Creators Update (April 2017) but not on legacy Windows native terminals. | ||
| -- See xtermSystem, xterm6LevelRGB and xterm24LevelGray from 'System.Console.ANSI.Types' to construct indices based on xterm's standard protocol for a 256-color palette. | ||
| | ColorPalette Word8 |
There was a problem hiding this comment.
Is the name "palette" somehow established for this type of colours?
Alternatively, Color256 seems like a good name to me in analogy to Color16.
Regarding the references to the ansi-terminal docs, direct hyperlinks would be useful, e.g.
| -- | A color from a palette of 256 colors using a numerical index (0-based). | |
| -- Supported natively on Windows 10 from the Creators Update (April 2017) but not on legacy Windows native terminals. | |
| -- See xtermSystem, xterm6LevelRGB and xterm24LevelGray from 'System.Console.ANSI.Types' to construct indices based on xterm's standard protocol for a 256-color palette. | |
| | ColorPalette Word8 | |
| -- | A color from a palette of 256 colors using a numerical index (0-based). | |
| -- Supported natively on Windows 10 from the Creators Update (April 2017) but not on legacy Windows native terminals. | |
| -- See 'System.Console.ANSI.Types.xtermSystem', xterm6LevelRGB and xterm24LevelGray from "System.Console.ANSI.Types" to construct indices based on xterm's standard protocol for a 256-color palette. | |
| | ColorPalette Word8 |
And note that modules are hyperlinked when enclosed in double-quotes (").
There was a problem hiding this comment.
Color256 would be fine, too. I was just basing it off the naming in ansi-terminal (SetPaletteColor)
There was a problem hiding this comment.
I think Color256 would be a bit better and clearer: The haddocks for Color16 also refer to a "standard palette". Also "ColorPalette" might be misunderstood to refer to the palette itself instead of a color from the palette.
| -- | Various kinds of colors that can be used in a terminal | ||
| data AnsiColor |
There was a problem hiding this comment.
I think it's weird that the haddocks on Color say
The 8 ANSI terminal colors
but this type is now named AnsiColor. What's a good way to resolve this? Maybe name this type TerminalColor?!
There was a problem hiding this comment.
I used 'AnsiColor' by analogy with 'AnsiStyle', feel free to change :)
There was a problem hiding this comment.
Let's use TerminalColor. My impression is that "ANSI color" usually refers to the 8 colors from the original ANSI standard.
| AnsiColor(..), | ||
| Color(..), | ||
|
|
||
| -- ** Font color | ||
| color, colorDull, | ||
| color, colorDull, colorPaletted, colorRGB, | ||
|
|
||
| -- ** Background color | ||
| bgColor, bgColorDull, | ||
| bgColor, bgColorDull, bgColorPaletted, bgColorRGB, | ||
|
|
||
| -- ** Font style | ||
| bold, italicized, underlined, | ||
| bold, italicized, underlined, inverted, | ||
|
|
||
| -- ** Internal markers | ||
| Intensity(..), | ||
| Bold(..), | ||
| Underlined(..), | ||
| Italicized(..), | ||
| Inverted(..), |
There was a problem hiding this comment.
Are you planning to expose the additional exports from Prettyprinter.Render.Terminal too?
Note that this project is pretty conservative regarding dependencies. Any non-boot package addition to the dependency tree of the existing packages will be a pretty hard sale.
I don't really have an opinion on this. If you want to do this, I suggest leaving it for a follow-up PR, so this one stays at a more manageable size.
I'm not sure, but I suspect that the local types exist so the internals can be changed with less compatibility hassle. Re-exports also bring their own versioning problems.
Not sure about these tradeoffs. Maybe @quchen has an opinion?!
Yeah, doctests don't work well with |
| -- FaintIntensity is not widely supported: sometimes treated as concealing text. Not supported natively on Windows 10 | ||
| data Bold = Bold | Faint deriving (Eq, Ord, Show) | ||
| -- DoubleUnderline is not widely supported. Not supported natively on Windows 10 | ||
| data Underlined = Underlined | DoubleUnderlined deriving (Eq, Ord, Show) | ||
| data Italicized = Italicized deriving (Eq, Ord, Show) | ||
| -- Swap the foreground and background colors. Supported natively on Windows 10 | ||
| data Inverted = Inverted deriving (Eq, Ord, Show) |
There was a problem hiding this comment.
Add Haddock comments to the types. Not really for the source code’s sake, but Haddock with un-haddocked definitions looks like a bit barren.
|
|
||
| -- | Various kinds of colors that can be used in a terminal | ||
| data AnsiColor | ||
| -- | A color from the standard palette of 16 colors (8 colors by 2 color intensities). Many terminals allow the palette colors to be customised |
There was a problem hiding this comment.
Does the -- | before the = render correctly? I always avoided this style for that reason, but maybe it changed?
| Nothing -> id | ||
| Just (intensity, color) -> convertColor True intensity color | ||
| Just (NewTerm.Color16 intensity color) -> convertColor True intensity color | ||
| _ -> id |
There was a problem hiding this comment.
Is there a reason for _ vs. Nothing here? I don’t know NewTerm’s types, so making this explicit makes it clear which part you’re ignoring.
| bgColorRGB c = mempty { ansiBackground = Just (ColorRGB c) } | ||
|
|
||
| -- | Render in __bold__. | ||
| bold :: AnsiStyle |
There was a problem hiding this comment.
Should probably add a faint function here too, to match the API.
Followup to (#65)
For background, I hope to replace the current ad-hoc style module used by Chapelure (https://hackage.haskell.org/package/chapelure-0.0.1.0/docs/Chapelure-Style.html)
Notes:
AnsiColortype with three constructors:Color16,ColorPalette(maybe it should beColorPaletted?) , andColorRGBColorRGB, we need a dependency on thecolourpackage, and we need to bump the version ofansi-terminalwe depend on 0.4 → 0.9DoubleUnderlinedto typeUnderlinedandFaintto typeBold, for completeness to support these ansi-terminal features. Both are 'not widely supported', according to ansi-terminal, so whether to include them is debatable. But we do supportItalicized, which is labelled the sameSetSwapForegroundBackgroundofansi-terminal, which is widely-supported. There's alsoSetBlinkSpeedandSetVisible, should we support those too?Some observations on the library
ColorandIntensityourselves rather than using and reexporting the identical versions fromSystem.Console.ANSI.Types?ConsoleIntensityandUnderlininginstead ofBoldandUnderlined, but that introduces an explicit 'None' value that might be confusing (prettyprinter-ansi-terminal: There should be a way to reset a color/style to terminal default #42). Or we could replace Maybe Bold → ConsoleIntensity in AnsiStyle. I took the first approach in Chapelure.Styleprettyprinter-convert-ansi-wl-pprintconverter has been updated, and it drops all of the new styles, like it did for italicsprettyprinter-ansi-terminalto work: I get several errors of the form "Could not find module ‘System.Console.ANSI’" etc