Skip to content
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

Fix ANSI inverse #2967

Merged
merged 3 commits into from Nov 1, 2017

Conversation

Projects
None yet
3 participants
@mgeier
Copy link
Contributor

mgeier commented Oct 23, 2017

The "inverse" escape sequence was implemented in #2186, but not by actually inverting foreground and background.

@tonycpsu Since you created the original "inverse" implementation, can you please check if that's OK for you?

mgeier added a commit to mgeier/nbconvert that referenced this pull request Oct 23, 2017

mgeier added a commit to mgeier/nbconvert that referenced this pull request Oct 23, 2017

@tonycpsu

This comment has been minimized.

Copy link
Contributor

tonycpsu commented Oct 23, 2017

That looks like a very elegant way to "fake" inverse support with CSS!

However, one problem I'm seeing in my output is that it appears to be assuming that the background color (which becomes the foreground of the inverse) is white when it may not be. Example:

here

I suspect there's no reliable way to deal with that given the need to precompute styles ahead of time and not knowing what the background will be... But if there must be a default, I would think black foreground would be the most sensible, since the ANSI stuff tends to be used for terminal output, and terminals have traditionally used black as the default background.

@mgeier

This comment has been minimized.

Copy link
Contributor Author

mgeier commented Oct 23, 2017

@tonycpsu Thanks for checking this out!

It indeed assumes that the default text color is black and the background is white.
I chose this because that's how I thought it is typically used in the Jupyter notebook, which has a white background.
Where is your example screenshot from?

If I'm supposed to make this customizable, I could try to define two new CSS classes like ansi-default-fg and ansi-default-bg (or probably somebody can come up with better names?) and use those for "inverse" where foreground or background are not defined.
Those CSS classes could then be overwritten whenever white background is not appropriate.

@tonycpsu

This comment has been minimized.

Copy link
Contributor

tonycpsu commented Oct 23, 2017

Where is your example screenshot from?

It's from a notebook (console output from cdiff) but I use custom.css to make the output look more like an "old school" console.

Relevant custom.css in my case:

.output {
    font-family: Menlo,Consolas,Lucida Console,monospace;
    font-size: 9pt;
}

div.output_area pre {
    font-family: Menlo;
    background: #202020;
    color: #37f14a;
}

div.output_area pre::selection {
    color: #202020;
    background: #37f14a;
}
div.output_area pre::-moz-selection {
    color: #202020;
    background: #37f14a;
}

I like your idea of new classes for the default fg/bg that I could just override in custom.css.

@mgeier

This comment has been minimized.

Copy link
Contributor Author

mgeier commented Oct 25, 2017

@tonycpsu I've added a commit with 2 new overridable CSS classes: ab7b19d

By default this still assumes a white background, but you can override ansi-default-inverse-fg and ansi-default-inverse-bg in your custom.css.

Does this work for you?

@tonycpsu

This comment has been minimized.

Copy link
Contributor

tonycpsu commented Oct 25, 2017

Looks perfect! Thanks so much for doing this!

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 31, 2017

@mgeier Is this ready to merge?

mgeier added some commits Oct 23, 2017

Invert inverse ANSI colors
The "inverse" escape sequence was implemented in #2186, but not by
actually inverting foreground and background.

@mgeier mgeier force-pushed the mgeier:fix-ansi-inverse branch from ab7b19d to 2068de7 Nov 1, 2017

@mgeier

This comment has been minimized.

Copy link
Contributor Author

mgeier commented Nov 1, 2017

@gnestor Thanks for the review!

Yes, I think this is ready. I've rebased this PR in order to get rid of the AppVeyor error.

The only remaining question is if you are OK with adding two additional CSS classes and if the names ansi-default-inverse-fg and ansi-default-inverse-bg are OK.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Nov 1, 2017

Yes and yes. Thanks @mgeier!!

@gnestor gnestor merged commit 4918eb1 into jupyter:master Nov 1, 2017

4 checks passed

codecov/patch Coverage not affected when comparing e7f69cc...2068de7
Details
codecov/project 78.95% remains the same compared to e7f69cc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mgeier mgeier deleted the mgeier:fix-ansi-inverse branch Nov 2, 2017

mgeier added a commit to mgeier/nbconvert that referenced this pull request Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.