Open SimonGAndrews opened 1 year ago
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.
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.
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)
This sounds like a great idea to me.
However, I'm not entirely sure about: https://github.com/espruino/EspruinoTools/compare/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...
if (termControlChars[0]=='27' && termControlChars[1]=='91') { // Esc [
is quoted when it shouldn't be?/\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?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?
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.
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?’
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)
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(";") + ">"
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.
Looking something like this ...
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/
Just to confirm, it's these changes? https://github.com/espruino/EspruinoTools/compare/master...SimonGAndrews:EspruinoTools:Plan-B-(issue-%23154)
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?
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.
Excellent - thanks! When you're ready let me know here and I'll have a test and merge in :)
quick status - testing slowly but surely. My fork for webIDE now links to my modified fork of EspruinoTools core terminal.
Great! The code looks really nice to me on https://github.com/espruino/EspruinoTools/compare/master...SimonGAndrews:EspruinoTools:master - let me know when you feel it's tested enough and ready to merge :)
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.