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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add italics and dim modifier to set_color #3650

Merged
merged 5 commits into from Dec 30, 2016

Conversation

Projects
None yet
5 participants
@DivineGod
Contributor

DivineGod commented Dec 16, 2016

Description

I wanted to be able to use (set_color -i) and (set_color -d) to display text as italics and dimmed respectively. In terminals supporting these the effect will be that text is either italic or dimmed.

Make sure that the terminfo used contains the appropriate configuration. Here is an example terminfo file:

# A xterm-256color based TERMINFO that adds the escape sequences for italic.
xterm-256-italic|xterm with 256 colors and italic,
  sitm=\E[3m, ritm=\E[23m, dim=\E[2m,
  use=xterm-256color,

TODOs:

  • Changes to fish usage are reflected in user documenation/manpages.
  • Use heuristics to manually output escape code if we know that a terminal supports a certain mode (E.g. Terminal.app supports italics and dim, but does not reflect this in it's terminfo)

@faho faho requested a review from floam Dec 16, 2016

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 16, 2016

Member

oh wow, I had no idea terminals could do this.

Member

ridiculousfish commented Dec 16, 2016

oh wow, I had no idea terminals could do this.

@DivineGod

This comment has been minimized.

Show comment
Hide comment
@DivineGod

DivineGod Dec 17, 2016

Contributor

@ridiculousfish Me neither, I only just found out about this last week. I have no idea how many terminal emulators implement these features; only tested in iTerm2 on macOS Sierra.

Contributor

DivineGod commented Dec 17, 2016

@ridiculousfish Me neither, I only just found out about this last week. I have no idea how many terminal emulators implement these features; only tested in iTerm2 on macOS Sierra.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 17, 2016

Member

I've requested review from @floam, since he did some work in set_color. He also previously used the italic and dim sequences in our Makefile, so he should be the most familiar.

Please note that he's been a bit busy, so it might take a while before he gets to it.

Member

faho commented Dec 17, 2016

I've requested review from @floam, since he did some work in set_color. He also previously used the italic and dim sequences in our Makefile, so he should be the most familiar.

Please note that he's been a bit busy, so it might take a while before he gets to it.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 17, 2016

Member

So cool! This has been on my TODO for a while. As of Sierra, Terminal.app also does italics. It's done dim for a couple releases.

By using dim you can actually get 512 colors on Terminal.app.

Member

floam commented Dec 17, 2016

So cool! This has been on my TODO for a while. As of Sierra, Terminal.app also does italics. It's done dim for a couple releases.

By using dim you can actually get 512 colors on Terminal.app.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 17, 2016

Member

However - the xterm-256color terminfo shipped with MacOS does not include dim or italics (I've filed 27624224 radar with Apple). I'd like to see this PR somehow make it work for Terminal.app.

Member

floam commented Dec 17, 2016

However - the xterm-256color terminfo shipped with MacOS does not include dim or italics (I've filed 27624224 radar with Apple). I'd like to see this PR somehow make it work for Terminal.app.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 17, 2016

Member

I'd like to see this PR somehow make it work for Terminal.app.

This should be done by checking for $TERM_PROGRAM and $TERM_PROGRAM_VERSION and either overwriting the terminfo definitions or just outputting the escapes manually.

Member

floam commented Dec 17, 2016

I'd like to see this PR somehow make it work for Terminal.app.

This should be done by checking for $TERM_PROGRAM and $TERM_PROGRAM_VERSION and either overwriting the terminfo definitions or just outputting the escapes manually.

@Darkshadow2

This comment has been minimized.

Show comment
Hide comment
@Darkshadow2

Darkshadow2 Dec 17, 2016

If these are getting added, can we also add a reverse (set_color -r) mode as well? It would be done the same as these additions, using enter_reverse_mode.

In case it's not obvious, this is the same as tput rev or the line rev=\E[7m in the terminfo file. I think it's better to use this directly than standout mode (tput smso). While standout mode is usually the same as reverse, some terms could set it as underline or italics.

Also, the docs should probably mention that italics mode is not available on all terms / it will only work if your term supports it.

Darkshadow2 commented Dec 17, 2016

If these are getting added, can we also add a reverse (set_color -r) mode as well? It would be done the same as these additions, using enter_reverse_mode.

In case it's not obvious, this is the same as tput rev or the line rev=\E[7m in the terminfo file. I think it's better to use this directly than standout mode (tput smso). While standout mode is usually the same as reverse, some terms could set it as underline or italics.

Also, the docs should probably mention that italics mode is not available on all terms / it will only work if your term supports it.

@DivineGod

This comment has been minimized.

Show comment
Hide comment
@DivineGod

DivineGod Dec 18, 2016

Contributor

I've added reverse as well to set_color as --reverse or -r for short. I've also updated the documentation accordingly as well as adding a note about support.

I think adding a test for Terminal.app in set_color is a good temporary fix. Maybe we can also identify other misconfigured terms and maybe also add a note about updating terminfo?

Contributor

DivineGod commented Dec 18, 2016

I've added reverse as well to set_color as --reverse or -r for short. I've also updated the documentation accordingly as well as adding a note about support.

I think adding a test for Terminal.app in set_color is a good temporary fix. Maybe we can also identify other misconfigured terms and maybe also add a note about updating terminfo?

@DivineGod

This comment has been minimized.

Show comment
Hide comment
@DivineGod

DivineGod Dec 18, 2016

Contributor

During testing I just found out that Terminal.app does not do simulated italics, only if the typeface supports italics, will the mode work, which is why I thought it didn't work (I was using Fira Code)

Contributor

DivineGod commented Dec 18, 2016

During testing I just found out that Terminal.app does not do simulated italics, only if the typeface supports italics, will the mode work, which is why I thought it didn't work (I was using Fira Code)

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 18, 2016

Member

The reverse mode being added is nice. It's something we currently rely on tput for to do __fish_cancel_commandline -- the ^C thing -- although at present it seems @krader1961 chose to use smso/stand-out mode.

Member

floam commented Dec 18, 2016

The reverse mode being added is nice. It's something we currently rely on tput for to do __fish_cancel_commandline -- the ^C thing -- although at present it seems @krader1961 chose to use smso/stand-out mode.

@Darkshadow2

This comment has been minimized.

Show comment
Hide comment
@Darkshadow2

Darkshadow2 Dec 18, 2016

Hmm, I just wrote a script to go through the capabilities of all the terms in the terminfo database. It looks like the majority of terms have reverse and standout mode set the same, and of those that are different the standout mode is pretty evenly split to being reverse AND bold or reverse AND dim. There are a few where standout mode is just bold (no reverse).

BUT there are a not-insignificant number of terms that don't have a reverse set but DO have a standout mode set. There is only one (that I saw) that has a reverse set but no standout mode.

So a proposed change: if enter_reverse_mode isn't set, try and see if enter_standout_mode is, and use that instead. Like this:

    if (reverse && enter_reverse_mode) {
        writembs(enter_reverse_mode);
     } else if (reverse && enter_standout_mode) {
        writembs(enter_standout_mode);
    }

(Sorry to make a little more work for you @DivineGod)

Here's the script I made to check it out (also shows if terms have an italics or dim capability):

caps.txt

Darkshadow2 commented Dec 18, 2016

Hmm, I just wrote a script to go through the capabilities of all the terms in the terminfo database. It looks like the majority of terms have reverse and standout mode set the same, and of those that are different the standout mode is pretty evenly split to being reverse AND bold or reverse AND dim. There are a few where standout mode is just bold (no reverse).

BUT there are a not-insignificant number of terms that don't have a reverse set but DO have a standout mode set. There is only one (that I saw) that has a reverse set but no standout mode.

So a proposed change: if enter_reverse_mode isn't set, try and see if enter_standout_mode is, and use that instead. Like this:

    if (reverse && enter_reverse_mode) {
        writembs(enter_reverse_mode);
     } else if (reverse && enter_standout_mode) {
        writembs(enter_standout_mode);
    }

(Sorry to make a little more work for you @DivineGod)

Here's the script I made to check it out (also shows if terms have an italics or dim capability):

caps.txt

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 18, 2016

Member

FWIW, if you download the ncurses sources and compile it, there are some rather handy test utilities in the test dir. test_sgr is handy for this -- might find it as good or better than your script, though make sure you give it a good enough terminfo.

Member

floam commented Dec 18, 2016

FWIW, if you download the ncurses sources and compile it, there are some rather handy test utilities in the test dir. test_sgr is handy for this -- might find it as good or better than your script, though make sure you give it a good enough terminfo.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 18, 2016

Member

(Also, your proposition does seem like a good idea to me. Standout mode is a decent fallback, and we want to prefer true reverse video mode which has no side effects.)

Member

floam commented Dec 18, 2016

(Also, your proposition does seem like a good idea to me. Standout mode is a decent fallback, and we want to prefer true reverse video mode which has no side effects.)

@faho faho added the enhancement label Dec 20, 2016

@faho faho added this to the fish 2.5.0 milestone Dec 20, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 20, 2016

Member

I notice that these attributes don't seem to work when set on fish_color_* vars, while --boldfor example does work.

Member

floam commented Dec 20, 2016

I notice that these attributes don't seem to work when set on fish_color_* vars, while --boldfor example does work.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 20, 2016

Member

What @floam noticed here is that these options haven't been hooked up in the highlighter (highlight.cpp) yet.

That means they won't be usable as values for $fish_color_* variables that are directly used by it, e.g. $fish_color_escape.

Member

faho commented Dec 20, 2016

What @floam noticed here is that these options haven't been hooked up in the highlighter (highlight.cpp) yet.

That means they won't be usable as values for $fish_color_* variables that are directly used by it, e.g. $fish_color_escape.

@Darkshadow2

This comment has been minimized.

Show comment
Hide comment
@Darkshadow2

Darkshadow2 Dec 20, 2016

Ok, since I really would like this PR added, here's a patch for you @DivineGod. It patches color.h, highlight.cpp, and output.cpp. It should fix the issue with the highlighter. You should be able to download it and git apply to your repo.

Note for the devs: I added flags to rgb_color_t and had to change the bit size of the flags ivar (from 4 to 5). I only mention this because there's a comment that says the size of the class is tried to be kept within 4 bytes. I don't think my additions made it go over, but I honestly don't know how to check that one.

Here's the patch:
modes.txt

Darkshadow2 commented Dec 20, 2016

Ok, since I really would like this PR added, here's a patch for you @DivineGod. It patches color.h, highlight.cpp, and output.cpp. It should fix the issue with the highlighter. You should be able to download it and git apply to your repo.

Note for the devs: I added flags to rgb_color_t and had to change the bit size of the flags ivar (from 4 to 5). I only mention this because there's a comment that says the size of the class is tried to be kept within 4 bytes. I don't think my additions made it go over, but I honestly don't know how to check that one.

Here's the patch:
modes.txt

@DivineGod

This comment has been minimized.

Show comment
Hide comment
@DivineGod

DivineGod Dec 20, 2016

Contributor

@Darkshadow2 I've added your patch in and it works nicely.

Contributor

DivineGod commented Dec 20, 2016

@Darkshadow2 I've added your patch in and it works nicely.

@DivineGod

This comment has been minimized.

Show comment
Hide comment
@DivineGod

DivineGod Dec 20, 2016

Contributor

@faho I noticed the same. I found a previous issue sort of unrelated to this, but still related to colors #3412 where I noted that highlight.cpp and builtin_set_color.cpp do the same things but parallel implementations and with slightly different effects on how they're configured.

Contributor

DivineGod commented Dec 20, 2016

@faho I noticed the same. I found a previous issue sort of unrelated to this, but still related to colors #3412 where I noted that highlight.cpp and builtin_set_color.cpp do the same things but parallel implementations and with slightly different effects on how they're configured.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 20, 2016

Member

I'm OK with this now. Maybe @ridiculousfish should look it over, but I'm OK with merging it.

What's still left to be done is figuring out how to make it work for Apple_Terminal, or maybe other terminals out there with missing terminfo defintions, but I'm ok with deferring that to be a later PR.

Thanks both of you, @DivineGod and @Darkshadow2.

Member

floam commented Dec 20, 2016

I'm OK with this now. Maybe @ridiculousfish should look it over, but I'm OK with merging it.

What's still left to be done is figuring out how to make it work for Apple_Terminal, or maybe other terminals out there with missing terminfo defintions, but I'm ok with deferring that to be a later PR.

Thanks both of you, @DivineGod and @Darkshadow2.

@floam

floam approved these changes Dec 30, 2016

@floam floam merged commit b22842a into fish-shell:master Dec 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@floam floam added the release notes label Dec 30, 2016

@floam floam self-assigned this Dec 30, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 31, 2016

Member

So, it was merged yesterday, all rejoice! Thanks folks.

Member

floam commented Dec 31, 2016

So, it was merged yesterday, all rejoice! Thanks folks.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 29, 2017

Member

I'd like to see this PR somehow make it work for Terminal.app.

I finally did this: 635e714 (iTerm as well.)

Member

floam commented Sep 29, 2017

I'd like to see this PR somehow make it work for Terminal.app.

I finally did this: 635e714 (iTerm as well.)

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