New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate bright and bold #1391

Merged
merged 4 commits into from May 9, 2018

Conversation

6 participants
@LinusU
Contributor

LinusU commented Apr 17, 2018

Previously all bright colors were always rendered bold, and all bold text was always rendered using the bright colors. This patch separates that so that bold and bright can be controlled individually.

This is probably going to affect people who relied on this previously, so I'm working on an option that re-enables the old behavior. I will push that as a separate commit to this same branch.

Feedback appreciated! 馃檶

Fixes zeit/hyper#2836

@LinusU LinusU referenced this pull request Apr 17, 2018

Open

Bold text doesn't respect colors in config #2836

2 of 2 tasks complete
@Tyriar

This comment has been minimized.

Member

Tyriar commented Apr 17, 2018

Feature request for this is here: #1239, an option would be great :). How about drawBoldAsBright: boolean = true? Note that the default needs to be true, see the discussion in #1239.

@Tyriar Tyriar requested review from bgw and Tyriar Apr 17, 2018

@bgw

This actually also helps address a concern I had with my implementation of #1327, since cached and uncached codepaths currently appear to handle the bold-as-bright logic differently. #1327 (comment)

This is probably going to affect people who relied on this previously, so I'm working on an option that re-enables the old behavior. I will push that as a separate commit to this same branch.

As @Tyriar indicated, we'll probably want to hold off on merging this until that's in place. Let me know if you encounter any issues implementing it. Otherwise, looks good so far.

@LinusU

This comment has been minimized.

Contributor

LinusU commented Apr 18, 2018

Thanks for the feedback.

One question, if we have a drawBoldAsBright option, should that also draw bright text bold? That seems confusing but that's how it worked before, since bright and bold were essentially the same thing.

We could also make it two options: drawBoldAsBright and drawBrightAsBold, but then as might be confusing as it's rather both. Maybe drawBoldTextBright and drawBrightTextBold? Both defaulting to true since that was how it was before.

Personally leaning most towards drawBoldTextBright and drawBrightTextBold...

@mofux

This comment has been minimized.

Contributor

mofux commented Apr 18, 2018

I don't think that drawing bright text as bold would make sense. Most terminals offer two options:

  • enable / disable drawing bold text as bright (the text may still be bold)
  • enable / disable drawing bold text as bold (the text may still be bright)

So we might end up with something like drawBoldTextBright and drawBoldTextBold that can be combined to get the desired flavour 馃榿

@LinusU

This comment has been minimized.

Contributor

LinusU commented Apr 18, 2018

I don't think that drawing bright text as bold would make sense.

I agree, but as far as I know, that is the current behaviour of xterm.js. So if we want to keep backwards compatibility, I think we need an option for that 馃

@mofux

This comment has been minimized.

Contributor

mofux commented Apr 18, 2018

Hmmm I haven't tested it, but I don't think that the current default behaviour is to draw bright text as bold (I've also not found any code that would point towards that). Imo we should not support an option that draws bright text as bold as it is confusing -- just as you noted. @xtermjs/core Any thoughts?

@parisk

Hi @LinusU, thanks for your contribution.

Let me try to help here to provide a path, so we can get this merged:

  1. Both default behavior of xterm.js for the end-user and the public API should stay the same, since this is not planned for a major release
  2. Implement an additional option drawBoldTextInBrightColors that will draw bold characters in bright colors and will default to false true

Afterwards we can plan an API breakage in 4.0 and drop the enableBlog flag in favor of the fontWeightBold superset, as @Tyriar proposed in #1239 (comment). This is also close to the proposal of @mofux in #1391 (comment).

Also, I agree with @mofux on not providing an option to draw bright colored text as bold:

we should not support an option that draws bright text as bold as it is confusing


I did a bit of research on how the xterm.js based Hyper draws bold characters and how Terminal.app, iTerm2 approach bold character drawing configuration.

In general it looks like drawing bold characters with bright colors is a universal option and that disabling bold character drawing at all is a thing as well.


Hyper / xterm.js drawing non-bright bold

image

Terminal.app Bold Option

image

iTerm2 Bold Options

image

@mofux

This comment has been minimized.

Contributor

mofux commented Apr 19, 2018

Implement an additional option drawBoldTextInBrightColors that will draw bold characters in bright colors and will default to false

@parisk The default should be true, as drawing bold text in bright colors is the default behaviour at the moment.

@parisk

This comment has been minimized.

Contributor

parisk commented Apr 19, 2018

Nah, you are right 馃槄. Fixed it.

@LinusU

This comment has been minimized.

Contributor

LinusU commented Apr 20, 2018

Hmmm I haven't tested it, but I don't think that the current default behaviour is to draw bright text as bold (I've also not found any code that would point towards that).

Hmmm, when I test it doesn't seem to happen that way, although I cannot understand why 馃 馃槀

To me, this bit of code that I removed:

-          // Convert the FG color to the bold variant
-          if (fg < 8) {
-            fg += 8;
-          }

Made it so that when the bold flag was set, we simply turned ordinary colors into their bright counterpart.

Before my patch, when drawing the char atlas, the bright colors would be drawn with the bold font:

-    // colors 8-15 are bold
-    if (colorIndex === 8) {
-      ctx.font = getFont(config.fontWeightBold, config);
-    }

The char atlas only held 2 + 16 colors before. Those 18 colors were:

  • 0 - default
  • 1 - default bold
  • 2 - Black
  • 3 - Red
  • 4 - Green
  • 5 - Yellow
  • 6 - Blue
  • 7 - Magenta
  • 8 - Cyan
  • 9 - White
  • 10 - Bright Black (drawn bold in the char atlas)
  • 11 - Bright Red (drawn bold in the char atlas)
  • 12 - Bright Green (drawn bold in the char atlas)
  • 13 - Bright Yellow (drawn bold in the char atlas)
  • 14 - Bright Blue (drawn bold in the char atlas)
  • 15 - Bright Magenta (drawn bold in the char atlas)
  • 16 - Bright Cyan (drawn bold in the char atlas)
  • 17 - Bright White (drawn bold in the char atlas)

Since the bright colors were drawn bold in the char atlas, I don't see how you would be able to print bright colors that aren't bold 馃

That being said, it seems that it's actually possible to print non-bold bright colors, since this is the output on Hyper 2.0.0:

screen shot 2018-04-20 at 09 54 00

Anyhow... this is good news 馃槃 I'll add an option to print bold colors bright, and then we should be good to go 馃憤

@LinusU

This comment has been minimized.

Contributor

LinusU commented Apr 20, 2018

Hahahhaaha 馃う鈥嶁檪锔 馃う鈥嶁檪锔 馃う鈥嶁檪锔

const isAscii = code < 256;
// A color is basic if it is one of the standard normal or bold weight
// colors of the characters held in the char atlas. Note that this excludes
// the normal weight _light_ color characters.
const isBasicColor = (colorIndex > 1 && fg < 16) && (fg < 8 || bold);
const isDefaultColor = fg >= 256;
const isDefaultBackground = bg >= 256;
if (this._charAtlas && isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground) {
    // draw from atlas
} else {
    // draw using ctx
}
@LinusU

This comment has been minimized.

Contributor

LinusU commented Apr 20, 2018

Rebased on master, and added two new commits.

The first one fixed so that I'm actually utilizing the char atlas for all 4-bit ANSI colors, bold and non-bold. The second one adds the drawBoldTextInBrightColors option which defaults to true.

2f1ec3b, and fd185a5 with drawBoldTextInBrightColors set to false:

screen shot 2018-04-20 at 10 31 58

fd185a5, which is same as current master:

screen shot 2018-04-20 at 10 33 44

From my side, this is ready for merging now 馃殌 let me know if anything else should be fixed 馃檶

@@ -54,6 +54,11 @@ declare module 'xterm' {
*/
disableStdin?: boolean;
/**
* Wheter to draw bold text in bright colors. The default is true.

This comment has been minimized.

@saul

saul Apr 23, 2018

s/Wheter/Whether/

This comment has been minimized.

@LinusU

LinusU Apr 23, 2018

Contributor

Fixed 馃憣

@Tyriar

This comment has been minimized.

Member

Tyriar commented Apr 23, 2018

Also, I agree with @mofux on not providing an option to draw bright colored text as bold:

we should not support an option that draws bright text as bold as it is confusing

I agree with @parisk agreeing with @mofux 馃槃. It was never the intent when I worked on this to support that.

LinusU added some commits Apr 17, 2018

@LinusU

This comment has been minimized.

Contributor

LinusU commented May 7, 2018

Rebased to avoid conflicts with #1422

ping @Tyriar @bgw @parisk, would love to get this merged 鉂わ笍

Add drawBoldTextInBrightColors to DEFAULT_OPTIONS
This lets term.setOption() work for drawBoldTextInBrightColors.
@bgw

This comment has been minimized.

Contributor

bgw commented May 9, 2018

I added this small commit to fix a minor bug: 9893338

Otherwise, LGTM. I manually tested it, and it seemed to work well for me. I'll merge once Travis finishes. Thanks for your contribution, sorry it took me so long to merge it.

@bgw

This comment has been minimized.

Contributor

bgw commented May 9, 2018

Travis is failing, but the issue is unrelated. It looks like a recent change to one of our dependencies broke some types. I was able to reproduce them locally in master by wiping and re-installing my node_modules.

@bgw bgw merged commit be32695 into xtermjs:master May 9, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@LinusU

This comment has been minimized.

Contributor

LinusU commented May 10, 2018

@bgw thank you 鉂わ笍

@LinusU LinusU deleted the LinusU:bright-vs-bold branch May 10, 2018

@Tyriar Tyriar added this to the 3.4.0 milestone May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment