colon notation for the parser #2241

jerch · 2019-06-17T18:54:27Z

PR to get support for colon parameter notation in the parser. Should fix #2200.

Several things todo:

new Params type to hold params and subparams in a speedy manner
apply ':' to the transition table and extend parse loop accordingly
apply new Params type to parser API and inputhandler methods
fix SGR handling for sub params
parser tests for new colon sequences
integration tests for stacked SGR params/subparams
add COLORTERM=truecolor to env

jerch · 2019-06-17T21:39:42Z

@Tyriar:
The last commit introduces the new params type to the rest of the codebase. Due to the addCsiHandler this type gets pulled right up to xterm.d.ts - we need to find a solution for that (just c&p'ed the interface for now to resolve the conflicts).

jerch · 2019-06-26T21:26:48Z

Some notes on the subparams implementation:

parser support
So far there is only one known CSI sequence with subparams - SGR. Due to syntactical limitations of sequences where the last byte decides which final action has to be triggered there is no elegant way to limit the subparams handling to SGR only at parser level. Thus the parser applies the subparams logic to all escape sequences with digit params (all CSI, all DCS).
new Params type
Any action handler that received params: number[] before now receives params: IParams. Compared to the old number array this new data type has several constraints:
- all params and subparams are borrowed and only valid for the current handler invocation (clone the data if you need a persistant version)
- params limits: int16_t, default: 0 (ZDM - zero default mode)
- subparams limits: int16_t, default: -1
- 32 slots for each params / subparams (configurable via ctor), >32 will be silently ignored
- params can be accessed by .params[idx]
- reading multiple params requires a .length check, reading beyond length - 1 might return arbitrary data
- existence of .params[0] is guaranteed, sequences that only support one param can omit the length check
- existence of subparams can be checked for every valid param index
SGR
This PR implements the subparams handling for foreground and background colors in SGR in an xterm compatible fashion (allowing some mixed semicolon/colon notations additionally to both standards). Support for underline color (SGR 58) and style (SGR 4) is not implemented yet.

Tyriar · 2019-06-28T17:24:05Z

src/InputHandler.ts

-      param = 1;
-    }
+  public cursorForward(params: IParams): void {
+    const param = params.params[0] || 1;


I think this will let negative numbers though now unless IParams handles this, is that a concern? Math.max(params.params[0], 1)?

Well, currently there is no way to set this to a negative value from a sequence. The parser currently applies zero default mode to params (as it always did), maybe just change the params typed array to uint16_t to be on the save side?

Thought again about this - imho it can keep it this way as long as we stick to ZDM. If we ever change this, we have to change default handling in all handlers anyway.

demo/server.js

Tyriar · 2019-06-29T20:58:42Z

src/Types.d.ts

-  /** CSI r */ setScrollRegion(params?: number[], collect?: string): void;
-  /** CSI s */ saveCursor(params?: number[]): void;
-  /** CSI u */ restoreCursor(params?: number[]): void;
+  /** CSI @ */ insertChars(params?: IParams): void;


Just checking if params still optional?

Eww good finding. Def. have to check all handler again, if they check for params before getting into business. Alternatively we could make params non optional for all handlers, but I am not sure if this would break on direct invocations here and there. So Id have to check this first.
Any preference whether to go with optional / non optional here?

Removed the question mark from params thus making it mandatory. They were not optional on Inputhandler anyway thus this chance is just getting the interface in line with the class.

Tyriar · 2019-06-29T21:04:43Z

src/common/parser/Params.ts

+   * Get a JS array representation of the current parameters and sub parameters.
+   * The array is structured as follows:
+   *    sequence: "1;2:3:4;5::6"
+   *    array   : [1, 2, [3, 4], 5, [-1, 6]]


:: => -1? Is that the default value for sub parameters?

Kinda.

In the past I had a longer discussion with @egmontkob about ZDM (zero default mode) for params (vte does not do this for params either). We do currently to be in line with the parser from vt100.net, and I would not change that for params unless we run into troubles for a certain sequence.
For subparams there is no ZDM thingy, which means it might be anything. I took -1 a placeholder for default since negative values cannot be reached by normal sequence means. This way the final action can place its real default value whenever it sees -1.

jerch · 2019-07-02T16:34:12Z

Last changes:

allow greater values (int32 now), no need to stick to smallish int16
clamp max value to 0x7FFFFFFF (fixes possible negative overflow)
reject values below -1

Tyriar · 2019-07-02T17:31:40Z

typings/xterm.d.ts

@@ -499,7 +499,7 @@ declare module 'xterm' {
     * The most recently-added handler is tried first.
     * @return An IDisposable you can call to remove this handler.
     */
-    addCsiHandler(flag: string, callback: (params: number[], collect: string) => boolean): IDisposable;
+    addCsiHandler(flag: string, callback: (params: IParams, collect: string) => boolean): IDisposable;


Maybe we should just use (number | number[])[] here to hide the complexity?

Fixed. Note that the translation to (number | number[])[] by calling Params.toArray() is ~5 times slower than directly using the Param type. Imho not a problem, since most handlers will be not invoked often.

src/InputHandler.ts

src/common/parser/EscapeSequenceParser.test.ts

src/common/parser/EscapeSequenceParser.ts

src/common/parser/Params.ts

jerch · 2019-07-04T11:57:58Z

@Tyriar Further documented ZDM. If you feel uneasy about it (we are not in line with the latest specs here), we could lift that restriction. For this to work we only have to fix all action handlers in Inputhandler to do a proper check against the param default of -1.

~~Up to you, I currently see no need to switch.~~

Edit: Looking at the tests we have for any method in InputHandler I suggest not to change this until we have better test coverage (including edge cases from the escape sequence test files) to avoid regressions going unnoticed.

Tyriar

LGTM, great job!

Params type implementation

383849b

jerch added the work-in-progress Do not merge label Jun 17, 2019

jerch self-assigned this Jun 17, 2019

jerch added 4 commits June 17, 2019 20:55

remove unused import

24c39f6

interfaces, add test for clone

cec1aa3

add ":" to param transitions, fix parse loop and tests

c5385a6

apply type over codebase

1dd63a6

jerch added 2 commits June 17, 2019 23:42

comment storable amount of params in parser

62007c3

apply colon handling to SGR

c558d48

jerch mentioned this pull request Jun 18, 2019

cleanup/enhancement of parser #1823

Closed

11 tasks

jerch added 5 commits June 18, 2019 14:40

SGR tests, fix bug in 38:5 handling

6f047aa

remove as number[] type conversions from tests

069e510

parser tests

f984204

add usage example for action handler

fc778c7

cleanup interface

c80ff1e

jerch removed the work-in-progress Do not merge label Jun 18, 2019

jerch added 2 commits June 18, 2019 18:55

add COLORTERM to env in demo

8ce67cb

Merge branch 'master' into parser_colon_notation

63a4e8f

Tyriar reviewed Jun 28, 2019

View reviewed changes

Merge branch 'master' into parser_colon_notation

b4ae5e1

Tyriar added this to the 4.0.0 milestone Jun 29, 2019

Tyriar reviewed Jun 29, 2019

View reviewed changes

jerch added 4 commits July 2, 2019 15:18

change to int32_t

5827b4b

make params mandatory

7dd5dfa

Merge branch 'master' into parser_colon_notation

a1acd4b

value range restrictions: clamp max, reject < -1

357faa7

Tyriar requested changes Jul 2, 2019

View reviewed changes

jerch and others added 4 commits July 2, 2019 20:41

remove IParams from public API

f276150

fix typos in docs

65b757b

js array integration test for addCsiHandler

17ca958

Merge branch 'master' into parser_colon_notation

69dcabb

Tyriar requested changes Jul 4, 2019

View reviewed changes

jerch added 6 commits July 4, 2019 12:12

ParamsArray to Types.ts

6bcd8e3

re-enable commented out tests

75d41ab

simply error parsing state

009b3c6

remove example snippet from docs

7bd0325

fix parser docs

6a691f2

cleanup Types.ts, docs for ZDM

5f8d07a

Merge branch 'master' into parser_colon_notation

be62267

Tyriar approved these changes Jul 6, 2019

View reviewed changes

jerch merged commit 68694da into xtermjs:master Jul 6, 2019

jerch mentioned this pull request Feb 21, 2020

some remarks regarding the parser migueldeicaza/XtermSharp#42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

colon notation for the parser #2241

colon notation for the parser #2241

jerch commented Jun 17, 2019 •

edited

jerch commented Jun 17, 2019 •

edited

jerch commented Jun 26, 2019 •

edited

Tyriar Jun 28, 2019

jerch Jun 29, 2019

jerch Jul 2, 2019 •

edited

Tyriar Jun 29, 2019

jerch Jun 29, 2019

jerch Jul 2, 2019

Tyriar Jun 29, 2019

jerch Jun 29, 2019

jerch commented Jul 2, 2019

Tyriar Jul 2, 2019

jerch Jul 2, 2019

jerch commented Jul 4, 2019 •

edited

Tyriar left a comment

colon notation for the parser #2241

colon notation for the parser #2241

Conversation

jerch commented Jun 17, 2019 • edited

jerch commented Jun 17, 2019 • edited

jerch commented Jun 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch Jul 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch commented Jul 4, 2019 • edited

Tyriar left a comment

Choose a reason for hiding this comment

jerch commented Jun 17, 2019 •

edited

jerch commented Jun 17, 2019 •

edited

jerch commented Jun 26, 2019 •

edited

jerch Jul 2, 2019 •

edited

jerch commented Jul 4, 2019 •

edited