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

Terminal support for handling color text display with ANSI escape sequences #154

Open
SimonGAndrews opened this issue Oct 20, 2022 · 14 comments

Comments

@SimonGAndrews
Copy link

Formalising suggested enhancement to provide support for the display of coloured text in the terminal window of the WebIDE (and other usage of the core terminal module) as per forum discussion https://forum.espruino.com/conversations/380403/#comment16713624.
Suggestion is to allow console.log from Espruino to embed standard ANSI/VT100 set/reset display attribute sequences to enable changes to foreground and background colours of the text displayed in the terminal window.

@SimonGAndrews
Copy link
Author

SimonGAndrews commented Oct 20, 2022

Happy to be assigned the task

The ansi/VT100 standards allow for the the escape sequences 'esc [ nnn m; to be used to set/reset the foreground and background colour of text.
image

image

image

Investigations show the change can be effected in https://github.com/espruino/EspruinoTools/blob/master/core/terminal.js. In summary the new escape sequences need to be trapped and used to effect the creation of additional HTML to be sent to the terminal canvas in the browser plugin.
The existing 3rd party library https://github.com/osteele/terminal-codes-to-html under MIT license provides the code necessary to convert the ANSI control sequences into HTML spans with style=colour attributes.

In more detail terminal.js functions handleRecievedCharacter() and updateTerminal() will be modified and a new function terminalCodesToHtml() (a modified version of Osteele's library) will be added.

  • Function handleRecievedCharacter() will be restructured to allow longer and more varied escape sequences to be identified. The function will allow valid escape sequences which are to be converted to HTML to be written to termText[] for subsequent processing by terminalCodesToHtml().
  • The function addCharacters() is created from code in handleRecievedCharacter() for a reusable method to update termText[]
  • The function updateTerminal() will be changed to add a call to terminalCodesToHtml() passing the terminal line to be tested for the creation of colour styled spans from the colour control escape sequences. Not unlike how link URLs are currently converted.

Initial versions of these changes have been made and will be tested in the fork https://github.com/SimonGAndrews/EspruinoTools

Notes on testing to follow here (all comments welcome)

@SimonGAndrews SimonGAndrews changed the title Terminal support for handling color test display with ANSI escape sequences Terminal support for handling color text display with ANSI escape sequences Oct 20, 2022
@gfwilliams
Copy link
Member

This sounds like a great idea to me.

However, I'm not entirely sure about: master...SimonGAndrews:EspruinoTools:master

Specifically I'm very unsure about including escape characters in termText. I think this will break other things (clicking, deleting, and also stepping over coloured text with arrow keys) as we assume that termCursorX points to the column of chars, which is in termText[termCursorY][termCursorX].

As far as I can tell if you set the cursor color, right now it only sets the color for that line, not for subsequent lines?

Also...

  • It seems if (termControlChars[0]=='27' && termControlChars[1]=='91') { // Esc [ is quoted when it shouldn't be?
  • I'm not a great fan of the repeated /\x1B\[0m/.test(String.fromCharCode.apply(null, termControlChars)) - this seems pretty verbose and inefficient when just if (termControlChars[0]==.. && termControlChars[1]==.. ... ) would work (and is used in other places) - or even just a string compare. I know for things like /\x1B\[[3-4][0-7]m/ it makes a bit more sense, but I guess termControlChars could be made a string rather than an array and then the checks could on the whole just be string compares with the occasional regex?
  • Generally I'd prefer not to copy in code with different licences, especially when it's only a few lines worth. It's ok, but because of what I mentioned above with termText I wonder if we'd be better off with a different approach anyway?

I'm open to suggestions, but it feels like in handleReceivedCharacter we basically decode the escape codes anyway, so it might be better to figure out what they are meant to do in handleReceivedCharacter, and then store them separately.

For each terminal line we could have an array of {position,color} objects, and then when we're generating the HTML I guess we could fix up the output line as we go along?

@SimonGAndrews
Copy link
Author

ok, thanks Gordon,
will take another look with your feedback.
I do like the library , its a pretty elegant way to get the HTML created.
But yes it does only colour the current line , its not modal across all lines as a VT100 would behave. This was good enough for simple console.log. Is ther another use case to test ?

Ill get a test for the areas you suggest would break, as Ive not seen that in a node standalone build of the WebIDE.
(by the way the term text insert of escape sequences is as characters with cursor variables adjusted, but im probably missing something ).

What other builds would need testing ?

will have another go next week.

@SimonGAndrews
Copy link
Author

Ok slept on this and will abandon current approach.
Will try your idea:
‘For each terminal line we could have an array of {position,color} objects, and then when we're generating the HTML I guess we could fix up the output line as we go along?’

SimonGAndrews added a commit to SimonGAndrews/EspruinoTools that referenced this issue Nov 10, 2022
SimonGAndrews added a commit to SimonGAndrews/EspruinoTools that referenced this issue Nov 10, 2022
SimonGAndrews added a commit to SimonGAndrews/EspruinoTools that referenced this issue Nov 11, 2022
SimonGAndrews added a commit to SimonGAndrews/EspruinoTools that referenced this issue Nov 11, 2022
SimonGAndrews added a commit to SimonGAndrews/EspruinoTools that referenced this issue Dec 2, 2022
@SimonGAndrews
Copy link
Author

New version of terminal.js ready for my detailed testing shared in my branch (plan B) as above.
Takes on Board @gfwilliams comments and uses his suggested approach (leaves termText as before).
Solution now is fully modal in that a change to display attributes is maintained across multiple terminal lines until cleared.
Ive extended the solution to cover other ansi control sequences for text attributes: underline, bold, etc.
Supports delimited control sequences such as esc[0;31;41m
The two major areas of change areas of change are in funcions handleReceivedCharacter() and updateTerminal() (all in terminal.js)
Apologies Lots of moving about code to tidy up in my latest push make differences from original a little tricky to see)

@SimonGAndrews
Copy link
Author

handleReceivedCharacter() now uses a simple state machine approach to handling the control sequences. It makes adding new sequences pretty straight fprward, but seemed necessary to handle the more complicated delimited control sequences.

A new function buildAttribSpans() is used by updateTerminal() to construct HTML spans with stryles based upon the
termAttribute[] array which corresponds to termText[].
termAttribute[] is @gfwilliams suggested 'For each terminal line we could have an array of {position,color} objects'. Which is now extended to cover the other text attributes.

Theres a slightly complicated method to get from attributes to HTML styles which is based upon a lookup object in attributes[{}]. The new function setActiveStyles(obj) uses attributes[{}] to convert contents of termAttribute[] to the HTML styles currently active into activeStyles[]. An HTML style string containing all active styles is achieved with "<span style=" + activeStyles.join(";") + ">"

@SimonGAndrews
Copy link
Author

I will be testing in windows , building the nw version of the app. Ill post the tests im doing.
I assum other builds will need testing .. may need help with that.
As always ill take any advice.

@SimonGAndrews
Copy link
Author

Capture

Looking something like this ...

@gfwilliams
Copy link
Member

Great, thanks for this! I'll try and take a proper look at this next week.

Please could you try forking EspruinoWebIDE with your new EspruinoTools, then enable GitHub pages for it?

It should then be possible for anyone to just go to https://simongandrews.github.io/EspruinoWebIDE and try it out, a bit like you can for https://espruino.github.io/EspruinoWebIDE/

@gfwilliams
Copy link
Member

Just to confirm, it's these changes? master...SimonGAndrews:EspruinoTools:Plan-B-(issue-#154)

The state machine looks really tidy. About the only thing I'd say (and I'm not sure this really matters that much) is that right now you do:

ccStateMapCChars =[
    {state:'wait@ESC',inChrs:'[',nextState:'wait@CSI',action: '' },

    {state:'wait@CSI',inChrs:'A',nextState:'start',action: () => cursorUp() },
    {state:'wait@CSI',inChrs:'B',nextState:'start',action: () => cursorDown() },
    {state:'wait@CSI',inChrs:'C',nextState:'start',action: () => cursorRight() },
    {state:'wait@CSI',inChrs:'D',nextState:'start',action: () => cursorLeft() },
    {state:'wait@CSI',inChrs:'J',nextState:'start',action: () => delEndOfScreen() },
    {state:'wait@CSI',inChrs:'K',nextState:'start',action: () => delEndOfLine() },

// and
newState = ccStateMapCChars.find( 
        (states) =>
          states.state === currentState && ...

But if you did:

ccStateMapCChars = {
    'wait@ESC' : [ {inChrs:'[',nextState:'wait@CSI',action: '' }] ,
    'wait@CSI' : [
        {inChrs:'A',nextState:'start',action: () => cursorUp() },
        {inChrs:'B',nextState:'start',action: () => cursorDown() },
        {inChrs:'C',nextState:'start',action: () => cursorRight() },
        {inChrs:'D',nextState:'start',action: () => cursorLeft() },
        {inChrs:'J',nextState:'start',action: () => delEndOfScreen() },
        {inChrs:'K',nextState:'start',action: () => delEndOfLine() },
// and
newState = ccStateMapCChars[currentState] && ccStateMapCChars[currentState].find( 
        (states) => ...

I imagine it would be a lot faster processing characters as they came in... And I guess you could merge in ccStateMapStart and ccStateMapCChars - as do you currently do that at the moment to make sure things run quickly?

@SimonGAndrews
Copy link
Author

Thanks @gfwilliams for feedback. I'll sort a clone of espruinoWebIde using my changed Espruino tools fork as a submodule as suggested.
I'll also make the structure change you suggest to the state maps. Your right inthat I was trying to speed up by splitting the start states out, but also it confidently enabled the no match in the start state to be addcharacter(). But looks like a regex of [ -~] would match all printable characters and solve that.

Found a couple of bugs already with fixes WIP. Will be using my espruinoTools master branch now.

@gfwilliams
Copy link
Member

Excellent - thanks! When you're ready let me know here and I'll have a test and merge in :)

@SimonGAndrews
Copy link
Author

quick status - testing slowly but surely. My fork for webIDE now links to my modified fork of EspruinoTools core terminal.

@gfwilliams
Copy link
Member

Great! The code looks really nice to me on master...SimonGAndrews:EspruinoTools:master - let me know when you feel it's tested enough and ready to merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants