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

ANSI colors regression compared to Notebook #3773

Closed
mgeier opened this issue Feb 1, 2018 · 9 comments
Closed

ANSI colors regression compared to Notebook #3773

mgeier opened this issue Feb 1, 2018 · 9 comments

Comments

@mgeier
Copy link
Contributor

@mgeier mgeier commented Feb 1, 2018

I've spent a considerable amount of work on the handling of ANSI colors in the Jupyter Notebook, mainly in https://github.com/jupyter/notebook/blob/master/notebook/static/base/js/utils.js.

It looks like when switching to JupyterLab, this was mostly for the bin.

At least my choice of colors has been taken in #2457, but some CSS names have been changed e.g. from ansi-yellow-intense-fg to ansi-bright-yellow-fg. This might affect people who want to use their Notebook styles for JupyterLab (I don't know if that's at all relevant, though).

More importantly, there seems to be no support for bold fonts. The ANSI escape sequences for "bold" only switch to the "bright" (or rather "intense") color (which is good), but don't switch to a bold font.

ANSI "underline" and "inverse" don't work either. Granted, support for those was only added to the Notebook quite recently.

The extended colors don't seem to work either (I tried it on a local installation and on binder), which is I guess due to a myriad of CSS definitions like ansi-palette-201-fg that don't seem to be available (and honestly, those hundreds of CSS classes seem a bit over-the-top to me).

Finally, 24-bit colors are not showing up. I don't know how this is supposed to work, the generated HTML looks like this:

<span class="ansi-truecolor-fg ansi-truecolor-bg" data-ansi-truecolor-fg="25, 0, 230" data-ansi-truecolor-bg="230, 0, 25">

All of the above works fine in the latest release of the Notebook.

So what about dumping ansi_up and just using the good old code from the Notebook instead?


Here are two test notebooks:

Note that the Notebook version currently used on binder doesn't support proper ANSI "inverse" yet (but the latest Notebook release does), nor does the latest nbconvert (but there is a hopefully soon-to-be-merged PR: jupyter/nbconvert#696). Also, the rendering on Github isn't quite ideal, either.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 2, 2018

@mgeier I don't think anyone working on lab has a strong usage of, nor opinions about ANSI colors. We would love to have your help (or at least advice) getting them in better shape in JupyterLab - especially as it looks like you have a lot of relevant experience. The regressions compared to the classic notebook in the ANSI colors are not deliberate - hopefully with a small bit of work, just temporary.

The test notebooks you have provided will be super helpful in bringing everything over.

I don't really have any opinion about ansi-up, versus the classic notebook's code.

Do you want to have a go at a PR, or do you think there are any decisions to make before you or someone dives into the code?

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Feb 2, 2018

Ooh I want to watch over this issue. :D

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Feb 2, 2018

do you think there are any decisions to make before you or someone dives into the code?

I don't know if I'm aware of all the options.

One option would be to try to fix ansi_up, but I think there is too much wrong with it for that to be feasible.

Another option would probably be to use xterm.js, since that's a dependency already.
Did you ever consider to use xterm.js for output cells that potentially contain ANSI escape sequences?
This would probably also handle carriage return and backspace characters automatically.
The support for ANSI colors seems to be quite good. There's just a minor problem with "inverse" (which is probably fixed in xtermjs/xterm.js#996) and 32-bit colors are just "emulated" with 256 colors (xtermjs/xterm.js#484), but that's IMHO not a big deal.
But probably using xterm.js many times per notebook is overkill?
And I don't know what the performance implications would be.

We could try to find yet another package that's more correct than ansi_up but probably less complex than xterm.js?

Or we could use the old Notebook code.
Since JupyterLab depends on the Notebook (it does, doesn't it?), is there a way to just call those JavaScript functions?
If not, where to put the code?
Would we have to port it to TypeScript?

Are there any other options that I missed?

Do you want to have a go at a PR

I don't have a lot of experience with JavaScript and I know nothing about TypeScript.
Nor do I know anything about the JupyterLab code. I couldn't even manage to make a development installation yet.
So it would probably be better if somebody else does it.

However, if nobody else wants to do it, I can try to tackle it, but it will probably take a long time and I'll need a lot of help ...

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 3, 2018

Hi @mgeier, thanks for engaging on this subject! It is a shame to hear that ansi_up isn't doing as much as we thought it did. Our preference has been to use existing npm packages where possible and port the classic notebook code where needed. I see two options moving forward:

  • Port the ansi handling code from the classic notebook
  • Use xterm 3.0 for console text iff it contains ansi escape sequences.

The reason I say xterm 3.0 is that they switched to canvas based drawing. I suspect that it would be overkill and take longer for the initial render when we don't need it, but it is worth trying as an experiment I think.

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Feb 6, 2018

Yes @blink1073, trying xterm.js sounds like an interesting (and possibly doomed) experiment. Sadly, I don't have the abilities to do that.

There is another concern: copying text from the xterm.js area could be problematic, at least it is currently in the JupyterLab Terminal.
Probably this was improved in xterm.js 3.0?

What about updating the JupyterLab Terminal to the latest xterm.js version?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 6, 2018

Yep, xterm.js 3.0 will be in the next release.

@jasongrout jasongrout removed this from the 1.0 milestone Sep 5, 2018
@blink1073 blink1073 added this to the Future milestone Sep 11, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 11, 2018

@mgeier, is this still an issue with the current JupyterLab 0.34?

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 15, 2018

@jasongrout

is this still an issue with the current JupyterLab 0.34?

AFAICT, "underline" and "inverse" don't work at all, and using "bold" doesn't switch to the "bright"/"intense" color (see #4230 (comment)).
The colors, both indexed and 24-bit RGB, seem to work fine now.

The current Classic Notebook and nbconvert work correctly, for example you can try this:

print('AB\x1b[43mCD\x1b[35mEF\x1b[1mGH\x1b[4mIJ\x1b[7m'
      'KL\x1b[49mMN\x1b[39mOP\x1b[22mQR\x1b[24mST\x1b[27mUV')

The result should look something like this:

grafik

Note that between F and G, when switching to "bold", the color also changes to the "intense" variant.
Between H and I, obviously the underlined section begins.
And between J and K, foreground and background are inverted.

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 18, 2018

I've created #5336 as proof of concept for what it would take to replace ansi_up with my code from the Classic Notebook.

This is how it looks:

grafik

Any opinions?

@blink1073 blink1073 removed this from the Future milestone Sep 19, 2018
@blink1073 blink1073 added this to the 1.0 milestone Sep 19, 2018
blink1073 added a commit to mgeier/jupyterlab that referenced this issue Oct 3, 2018
blink1073 added a commit to mgeier/jupyterlab that referenced this issue Oct 17, 2018
blink1073 added a commit that referenced this issue Oct 18, 2018
* WIP: Try to replace ansi_up with code from Classic Notebook

Fixes #3773.

* Add html escape and clean up lint

* TST: Update ANSI tests

* revert changes to staging

* Move code to Private namespace

* Rename main function: fixConsole() -> ansiSpan()

* Use ES6-style string formatting and arrow function

* Add return type annotations
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants