Add better color support #224

sofia-m-a · 2022-04-25T10:20:13Z

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:

Added an AnsiColor type with three constructors: Color16, ColorPalette (maybe it should be ColorPaletted?) , and ColorRGB
To support ColorRGB, we need a dependency on the colour package, and we need to bump the version of ansi-terminal we depend on 0.4 → 0.9
I also added DoubleUnderlined to type Underlined and Faint to type Bold, 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 support Italicized, which is labelled the same
I added an extra style marker, 'Inverted', to support the SetSwapForegroundBackground of ansi-terminal, which is widely-supported. There's also SetBlinkSpeed and SetVisible, should we support those too?

Some observations on the library

Is there a reason we define the types Color and Intensity ourselves rather than using and reexporting the identical versions from System.Console.ANSI.Types?
- Similarly, we could re-use ConsoleIntensity and Underlining instead of Bold and Underlined, 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.Style
The prettyprinter-convert-ansi-wl-pprint converter has been updated, and it drops all of the new styles, like it did for italics
I couldn't get the doctests for prettyprinter-ansi-terminal to work: I get several errors of the form "Could not find module ‘System.Console.ANSI’" etc

sjakobi

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?

prettyprinter-ansi-terminal/prettyprinter-ansi-terminal.cabal

sjakobi · 2022-04-26T12:37:18Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

-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


Suggested change

-- 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

sjakobi · 2022-04-26T12:38:35Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

+-- 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)


The comments seem useful for users too, so how about turning them into Haddock comments?!

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.

Whoops, I meant for these to be Haddock comments :)

sjakobi · 2022-04-26T12:49:15Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

+  -- | 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


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.

Suggested change

-- | 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 (").

Color256 would be fine, too. I was just basing it off the naming in ansi-terminal (SetPaletteColor)

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.

sjakobi · 2022-04-26T12:55:42Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

+-- | Various kinds of colors that can be used in a terminal
+data AnsiColor


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?!

I used 'AnsiColor' by analogy with 'AnsiStyle', feel free to change :)

Let's use TerminalColor. My impression is that "ANSI color" usually refers to the 8 colors from the original ANSI standard.

sjakobi · 2022-04-26T12:58:12Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

+    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(..),


Are you planning to expose the additional exports from Prettyprinter.Render.Terminal too?

Oh, yes, sorry

sjakobi · 2022-04-26T13:39:11Z

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)

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 added an extra style marker, 'Inverted', to support the SetSwapForegroundBackground of ansi-terminal, which is widely-supported. There's also SetBlinkSpeed and SetVisible, should we support those too?

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.

Is there a reason we define the types Color and Intensity ourselves rather than using and reexporting the identical versions from System.Console.ANSI.Types?

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.

Similarly, we could re-use ConsoleIntensity and Underlining instead of Bold and Underlined, but that introduces an explicit 'None' value that might be confusing (#42). Or we could replace Maybe Bold → ConsoleIntensity in AnsiStyle. I took the first approach in Chapelure.Style

Not sure about these tradeoffs. Maybe @quchen has an opinion?!

I couldn't get the doctests for prettyprinter-ansi-terminal to work: I get several errors of the form "Could not find module ‘System.Console.ANSI’" etc

Yeah, doctests don't work well with cabal. stack handles them better. Feel free to record this on the issue tracker. Maybe we can fix these ergonomics issues somehow, e.g. by adopting cabal-docspec. I think there are other recent developments in this space, e.g. doctest-parallel.

quchen

LGTM with a few comments.

No new dependencies 👍
API remains similar 👍

I wouldn’t know what to do with a color inversion feature, but if ANSI terminals support it and the package claims to handle ANSI terminals, I guess that’s OK :-)

quchen · 2022-04-28T07:50:37Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

+-- 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)


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.

quchen · 2022-04-28T07:51:35Z

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs

@@ -242,6 +270,18 @@ panicStyleStackNotFullyConsumed len
           "end of rendering (there should be only 1). Please report" ++
           " this as a bug.")

+-- | 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


Does the -- | before the = render correctly? I always avoided this style for that reason, but maybe it changed?

quchen · 2022-04-28T07:55:22Z

prettyprinter-convert-ansi-wl-pprint/src/Prettyprinter/Convert/AnsiWlPprint.hs

-            Nothing -> id
-            Just (intensity, color) -> convertColor True intensity color
+            Just (NewTerm.Color16 intensity color) -> convertColor True intensity color
+            _ -> id


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.

Add better color support

35dd626

sjakobi mentioned this pull request Apr 25, 2022

Support for full color palette #65

Open

sjakobi reviewed Apr 26, 2022

View reviewed changes

quchen approved these changes Apr 28, 2022

View reviewed changes

sjakobi linked an issue Jun 11, 2022 that may be closed by this pull request

Support for full color palette #65

Open

sjakobi mentioned this pull request Aug 30, 2022

Use Color, Intensity etc. from ansi-terminal #233

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better color support #224

Add better color support #224

sofia-m-a commented Apr 25, 2022

sjakobi left a comment

sjakobi Apr 26, 2022

sjakobi Apr 26, 2022

quchen Apr 28, 2022 •

edited

Loading

sofia-m-a Apr 29, 2022

sjakobi Apr 26, 2022

sofia-m-a Apr 29, 2022

sjakobi May 2, 2022

sjakobi Apr 26, 2022

sofia-m-a Apr 29, 2022

sjakobi May 2, 2022

sjakobi Apr 26, 2022

sofia-m-a Apr 29, 2022

sjakobi commented Apr 26, 2022 •

edited

Loading

quchen left a comment •

edited

Loading

quchen Apr 28, 2022 •

edited

Loading

quchen Apr 28, 2022

quchen Apr 28, 2022

	-- 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

		-- \| Various kinds of colors that can be used in a terminal
		data AnsiColor

Add better color support #224

Are you sure you want to change the base?

Add better color support #224

Conversation

sofia-m-a commented Apr 25, 2022

Notes:

Some observations on the library

sjakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quchen Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi commented Apr 26, 2022 • edited Loading

quchen left a comment • edited Loading

Choose a reason for hiding this comment

quchen Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quchen Apr 28, 2022 •

edited

Loading

sjakobi commented Apr 26, 2022 •

edited

Loading

quchen left a comment •

edited

Loading

quchen Apr 28, 2022 •

edited

Loading