hooks for custom control sequences

[PerBothner]

This fixes (at least partially) issue #1176
"Add a way to plugin a custom control sequence handler".

I have use of these new methods in an experimental option to build DomTerm using xterm.js: https://github.com/PerBothner/DomTerm/blob/master/README-xtermjs.md
Specifically, I use these hooks for escape sequences to create new sub-windows, and to set windows title, session name, and relayed. (The DomTerm-using-xterm.js option is rough, and missing many of the features of DomTerm, but the basic functionality works.)

[jerch]

@PerBothner Thx alot for this PR. I have severals remarks on this before we can go further down this rabbit hole:

1. perf impact
   The parser and the input handler are the result of carefully crafted stuff to keep it performant and therefore they are very sensitive to any code alterations/additions (they are "hot" during input). Even small changes might degrade throughput several magnitudes. Thats the reason why we did the close coupling with callbacks in the first place and not the more common event dispatch approach. So the callback invocations need to be tested for perf as well (You can test it with https://github.com/xtermjs/xterm-benchmark). I dont really see the need yet for multiple callbacks on one type of "event", maybe you can elaborate why you would need to "tee" here?
2. custom handler access to internals
   Imho a custom handler from outside will be of very limited use without access to Terminal internals (unless it replicates most things Terminal does). This is a major drawback, we really need to find a way to make the internals accessible. But this API-sation is a big problem of its own - as I wrote in another comment xterm.js comes from one big class object with all dependencies to DOM and such. We are still in the going process of slicing it into better separated lib chunks, so parts of it can be used more independently in different envs. Thus these internals still see lots of changes and a stable API is not yet possible. Once we decide to got with a "programmers API" the maintenace burden will go up, as any public API stuff might turn into a refactoring hindrance. Long story short - we are not yet there.
3. API in general
   As said under 2. we are not settled yet with the internal API. Furthermore we currently expose a very reduced set of capabilities as public API thats suitable for integrators. Therefore I think your changes to public/Terminal with the input handler getter defeats the purpose of this reduced API as it leaks unstable internals to the public. Imho it should never be seen from the integrators API.

@Tyriar, @mofux, @bgw: Should we refactor the public API to contain secondary entries from src/Types.ts marked as unstable/internal? I see the need for peeps that like to do more with the terminal stuff than simply integrate it within their app, I also knocked at limitations of the current exports in the benchmark tool and had to do quirky workarounds by using require. Still I think we should not do such a thing before we are done with the lib separations, esp. the fact that we still have no way to spawn a terminal without DOM dependency is a major drawback (more precise - this works currently for simple input due to lazy eval and is already used by some test cases, but this wonky state really needs a fix).

[Tyriar]

I dont really see the need yet for multiple callbacks on one type of "event", maybe you can elaborate why you would need to "tee" here?

I think multiple * are probably the way to go for most of the APIs, right now they're a bit of a mess as they were developed without much thinking about the future:

• attachCustomKeyEventHandler ("attach", there is also only one)
• registerLinkMatcher (returns an id that can be removed)
• deregisterLinkMatcher
• registerCharacterJoiner (returns an id that can be removed)
• deregisterCharacterJoiner
• addMarker (remove with IMarker.dispose())
• setOption (there is only one setting per Terminal)

We should probably come to a consensus on the ideal way to name things in the API. I would probably like the above to be close to what @PerBothner is proposing in order to align with the web platform (add/removeEventListener):

type ICustomKeyEventHandlerId = number;
addCustomKeyEventHandler(...): ICustomKeyEventHandlerId
removeCustomKeyEventHandler(id: ICustomKeyEventHandlerId): void;

type ILinkMatcherId = number;
addLinkMatcher(...): ILinkMatcherId
removeLinkMatcher(id: ILinkMatcherId): void;

type ICharacterJoinerId = number;
addCharacterJoiner(...): ICharacterJoinerId
removeCharacterJoiner(id: ICharacterJoinerId): void;

createMarker(cursorYOffset: number): IMarker;

// setOption unchanged

The rules being:

• Use add/remove when there are multiple, prefer multiple in general to improve the custom addon story Custom addons #1128
• Use set when there is only one
• Use create when you need to dispose the object to remove it

Related: on('key', ...) is going to be changed to onKey(...) to be more explicit and TS friendly in #1505

Therefore I think your changes to public/Terminal with the input handler getter defeats the purpose of this reduced API as it leaks unstable internals to the public.

This is correct, any API also needs to be includes in typings/xterm.d.ts which is self contained and we don't want to expose internals here. They also need some examples added to fixtures/typings-test/typings-test.ts which is meant to prevent us from changing the .d.ts in an unexpected way.

Should we refactor the public API to contain secondary entries from src/Types.ts marked as unstable/internal?

I'm not sure we want to introduce another API, we kind of already have this with things marked "experimental" either in the name or comments and I think it's a bit of a mess.

I'm fine with us committing to a way to set CSI/OSC handlers, provided we get the naming right and the API itself is future proof (I'm thinking mainly of @jerch's buffer/parser perf work here). The calls should be plumbed through the layers like this though to protect internal code and align with the future direction (#1507):

xterm.js/src/public/Terminal.ts

Lines 62 to 64 in b5f1c33

	public registerLinkMatcher(regex: RegExp, handler: (event: MouseEvent, uri: string) => void, options?: ILinkMatcherOptions): number {
	return this._core.registerLinkMatcher(regex, handler, options);
	}

[mofux]

The rules being:

• Use add/remove when there are multiple, prefer multiple in general to improve the custom addon story Custom addons #1128
• Use set when there is only one
• Use create when you need to dispose the object to remove it

I'd prefer to have most of these methods return a Disposable rather than a number, because it simplifies cleaning up resources and makes the API easier to use and also more consistent:

• no need to introduce and remember a method to remove a listener or registration, simply call disposable.dispose()
• you can collect disposables from different sources in a simple array and dispose them all at once

The only drawback I see with disposables is if you have to create lots (thousands) of them - in that case the creation of the disposable objects will put a penalty on the runtime.

[Tyriar]

Certainly slims things down:

addCustomKeyEventHandler(...): IDisposable;
addLinkMatcher(...): IDisposable
addCharacterJoiner(...): IDisposable;
addMarker(...): IMarker;

The scale problem doesn't apply to any of these APIs so far.

[jerch]

👍 on using Disposable where possible. Only thing I foresee here is that we have to comment/doc clearly about transferred ownership vs. borrowed things (means when to call dispose on a held reference vs. setting it to null).

If we really plan to solve this in a once and forever way I think we also have to discuss and decide about the general capabilities of an "event system" and/or/vs. a "callback invocation system".

1. We had EventEmitter for almost everything before, but started to replace it with a more direct callback approach wherever possible. Perfwise (yeah sorry I always look through the perf glasses) the latter clearly wins the game, but also has drawbacks in general usability (you cannot throw any handler from almost everywhere into the ring and be sure to get called once that event appears). Note that the EventEmitter is still synchronous as it does not use JS's event loop (thus its more a CallbackEmitter than Event...).

2. Order of execution for multiple callbacks
   This is an easy one imho, all systems I am aware of do a first registered first served approach.

3. Propagation rules of multiple callback handlers
   This is tricky one - most systems I know from other languages have some way to tell the event system, that a particular handler finally handled it and can stop propagation. For some reason this was never done in any of nodejs' event systems, not sure why (even browser JS events support this). Still I think a less clever system that does not support this will be enough. And I think we really should not go for a full event impl which spawns event objects and carries state around with them. Although it would be the most versatile system, it will be very expensive perfwise.

4. Capability of going through webworker barriers
   This is a thing that pops into my mind from Qt's signal-slot mechanism and their way of handling it over thread contexts. In an ideal world we would have some way to simplify callback registration even in other webworkers. I have no idea yet how this could be achieved transparently. Since we currently dont use webworkers its still more of an academical question, but we might want to use webworkers one day.

About the naming I am fine with whatever you all find appropiate. We should keep this simple, maybe use add* for anything that can have multiple things attached, set* for single thing and remove* for all of those?

[PerBothner]

Lots of good comments.
I don't really see the need yet for multiple callbacks on one type of "event", maybe you can elaborate why you would need to "tee" here?

Besides the vague goal of wanting addons to work as orthogonally as possible, I have some concrete examples in my DomTerm-using-xterm.js prototype: DomTerm has a number of custom escape sequences of the form CSI+params+"u"; xterm.js has a builtin handler (restoreCursor) for plain CSI+"u". So we need a handler that can "fall back" to the default. Another example is OSC+"0;"+title+"\a": This has a builtin handler (setTitle), but DomTerm also needs to intercept it for its own needs. In this case we want both the custom and builtin behaviors.

Order of execution for multiple callbacks
This is an easy one imho, all systems I am aware of do a first registered first served

That seems backwards. I think we want last added, first tried. How else could one override a builtin?

most systems I know from other languages have some way to tell the event system, that a particular handler finally handled it and can stop propagation

The pull request has the callback return a boolean (true means "stop propagation"). That seems simple and efficient. A function to stop propagation can also work. To avoid having to create event objects one could pass the IInputHandler as an extra parameter to the callback: You stop propagation by calling a method in the IInputHandler. I don't see that as having any advantage compared to returning a boolean - unless it's related to some more general event-handling convention.

_Returning an IDisposable. _
That seems like a good idea to me as well.

custom handler access to internals
Imho a custom handler from outside will be of very limited use without access to Terminal internals (unless it replicates most things Terminal does)

Perhaps - but let's fix one problem at a time. Earlier I gave examples where addon handlers are useful, even without expanded access to Terminal internals.

API in general
Therefore I think your changes to public/Terminal with the input handler getter defeats the purpose of this reduced API as it leaks unstable internals to the public

I could change Terminal.getInputHandler to return a more restricted interface. The set of methods that are part of IInputHandler seem sane and useful, but the specific API is awkward. For example cursorUp(params?: number[]): void is useful but clumsy. As a public API it would be better as cursorUp(count: number): void. Maybe it belongs in Terminal rather than InputHandler.

Performance
I haven't tried a benchmark yet. It should be possible to optimize for the case that we only have the builtin handler. I'd like to tentatively agree on a clean API before we try to benchmark or optimize it.

[PerBothner]

Here is an alternative implement of EscapeSequenceParser.addCsiHandler:

addCsiHandler(flag: string, callback: (params: number[], collect: string) => boolean): IDisposable {
  const index = flag.charCodeAt(0);
  const oldHead = this._csiHandlers[index];
  const newHead = Object.assign(
    (params: number[], collect: string): void => {
      if (callback(params, collect)) { }
      else if (newHead.nextHandler) {
         newHead.nextHandler(params, collect);
      } else { this._csiHandlerFb(collect, params, index); }
    },
    { nextHandler: oldHead,
      dispose(): void {
        let previous = null; let cur = this._csiHandlers[index];
        for (; cur && cur.nextHandler;
          previous = cur, cur = cur.nextHandler) {
          if (cur === newHead) {
            if (previous) { previous.nextHandler = cur.nextHandler; }
            else { this._csiHandlers[index] = cur.nextHandler; }
            break;
          }
        }
      }
    });
  this._csiHandlers[index] = newHead;
  return newHead;
}

The builtin bindings will continue to be set using setCsiHandler, which now just sets _csiHandlers[index] directly. The logic in parse is also unchanged. In other words there is no performance penalty unless addCsiHandler is used.

([This link]{https://spin.atomicobject.com/2017/04/11/typescript-represent-function-properties/} helped with the idea.)

(This has only been minimally tested, with no testing of dispose.)

[jerch]

@PerBothner I think we need to find a consensus how to do things here.

About my perf concerns:
The parser is kinda special and I am sure the perf considerations dont apply to most of the other terminal parts, at least not to that degree. During input the whole parser is "hot" and the JIT tries to heavily optimize it. Thats where we gain the input perf from. To give you an example - currently the CSI and ESC "events" do an intermediate string creation in the parser. Because of this string creation these events show only half of the throughtput of EXECUTE, and only a fifth of PRINT. (Did not yet optimize CSI and ESC further as they are still pretty rare even in curses apps, see #1399 (comment)). Worst is OSC which does a char by char string concat.
Now the question is - whats toxic for the JIT? This heavily depends on the engine (who would have guessed), since we do most testing on chromium we kinda aligned the code with v8 here. There are many posts all over the internet about this topic, to name a few things:

• inlining vs. function call - depends on function size
• out of bound accesses - never read/access a container behind the last element
• type switches - never monkey patch an object during runtime (ad/remove properties)
• array vs. object property access - avoid holey arrays at all costs
• array elements - never mix different element types
• loops and branches - bad, esp. if the tested vars change type and/or the result flips often (deopts the speculative execution)

Thats the reason why I raise an eyebrow on the idea to enhance the parser "events" with a more capable event handling system. Both additions - the capability to handle more handlers (looping) and the fact to rely on the return value for early stop propagation (flipping results) will lead to many more deopts. I am not against this, but it needs to be tested. I think a small penalty is acceptable, but we should not sacrifice input runtime just for "being able to do niftier things" with the parser events, and noone really uses this afterwards.

Again - I think this does not necessarily apply to any other terminal parts.

[PerBothner]

Worst is OSC which does a char by char string concat.

This seems a worthwhile optimization:

    case ParserAction.OSC_START:
      if (~print) {
        this._printHandler(data, print, i);
        print = -1;
      }
      for (let j = i + 1; ; j++) {
        if (j >= l
            || ((code = data.charCodeAt(j)) <= 0x9f
                && (table[ParserState.OSC_STRING << 8 | code] >> 4
                    !== ParserAction.OSC_PUT))) {
          osc = data.substring(i + 1, j);
          i = j - 1;
          break;
        }
      }
      break;

[Tyriar]

DomTerm has a number of custom escape sequences of the form CSI+params+"u"; xterm.js has a builtin handler (restoreCursor) for plain CSI+"u". So we need a handler that can "fall back" to the default.

This is not how I originally interpreted #1176, I believe the original request was all about filling in functionality not overriding. Was this the one you want to implement or something custom?

CSI Ps SP u
          Set margin-bell volume (DECSMBV), VT520.
            Ps = 1  -> off.
            Ps = 2 , 3  or 4  -> low.
            Ps = 0 , 5 , 6 , 7 , or 8  -> high.

[PerBothner]

The original #1176 request was partly about adding custom non-standard control sequences (see the 2nd paragraph). I'm not looking to override functionality, but I do want to to both add functionality to existing control sequences, and add extra behaviors to existing control sequences (such as those for setTitle). See also issue #576.

I'm not interested in "Set margin-bell volume" but I have a lot of other custom escape sequences.

[Tyriar]

@PerBothner oh I forgot to mention, all setTitle does is expose a hook for embedders to listening to the change:

xterm.js/typings/xterm.d.ts

Line 420 in 76e7057

on(type: 'title', listener: (title: string) => void): void;

[jerch]

This seems a worthwhile optimization:

Yes something like this should do the trick for OSC (did not yet optimize this either, we currently support only 2 OSC calls that occur once in a while). The inner loop though has to cover all OSC conditions, for example EXECUTES are a NOOP in this state (means neither executed nor added to the string, see https://vt100.net/emu/dec_ansi_parser). Well I think its abit offtopic here...

[PerBothner]

Yes something like this should do the trick for OSC

See pull request #1822.

The inner loop though has to cover all OSC conditions, for example EXECUTES are a NOOP in this state (means neither executed nor added to the string

The pull request is conservative - it stops as soon as its sees a character that is is not OSC_PUT.

[PerBothner]

I updated and cleaned my patch to have addCsiHandler/addOscHandler both return IDisposable. If these new functions are not used, there is no difference in the logic, so there should be no performance impact.

Awkwardly, the pull request shows 8 commits, but some of them a reverts and some touch on the OSC_STRING pull request. The actual cleaned up patch is just a simple commit: 8ceea11 . I don't know how to update the pull request to reflect this (except by starting over with a new pull request). (I can't say I've masted GitHub's "pull request" work flow, and I have very mixed feelings about it.)

I've run the xterm-benchmark. I don't see any consistent difference. Which is as expected, since there should be no change to execution flow as long as addCsiHandler/addOscHandler are not used.

These changes are used by the xdomterm prototype.

[Tyriar]

I don't know how to update the pull request to reflect this (except by starting over with a new pull request)

Generally on GitHub you should just keep committing and merging like you have done, the maintainers can squash at the time of merge if they want. If you squash yourself it makes it hard to review just the new changes.

[Tyriar]

Looks like this is getting close 😃

[Tyriar]

@jerch are the strings here future proof with your upcoming changes?

[jerch]

@Tyriar, @PerBothner: Not future proof, its still a subject to change, but imho good to go for now. I will address this with one of the typed array transitions to come.
Not sure about including this in public API yet, might be better to go unofficial until the transition is done (will not before 3.11 though due to the JS Array buffer).

[Tyriar]

Let's change this to expose the following interface (pending @jerch's comments on whether the types are future proof):

class Terminal {
  addCsiHandler(flag: string, callback: (params: number[], collect: string) => boolean): IDisposable;
  addOscHandler(ident: number, callback: (data: string) => boolean): IDisposable;
}

You'll then call through to _core, see addDisposableListener for what I mean here.

You should also merge IVtInputHandler back into IInputHandler after doing this.

[jerch]

Well, they will most likely change type to a borrowed typed array, but not until 3.11 since I have to wait for the JS Array buffer to be gone. So not sure if we should include it yet in the public API, well marking them "experimental" or "unstable" would work for me.

[PerBothner]

I simplified things as suggested (as I understand it).

[Tyriar]

If this becomes API we'll need an entry also in xterm.d.ts as well as a test in fixtures/typings-test/typings-test.ts. The .d.ts file is a TypeScript declaration file and that defines the entire API we want to expose, the test file just makes sure we don't regress the declaration file.

[PerBothner]

Done in commit 6b65ebd.

[jerch]

Made a few comments, overall it looks good 👍

[jerch]

Yupp this will work without penalty if no other handler is attached 👍
One thing though: can we make this management code more general usable, like in a private method called for CSI, OSC etc. when needed. This way the code is cleaner without duplication and feels less monkey patching, and can also be used for ESC.

[PerBothner]

See commit 8a5a032 "New method _linkHandler used by both addCsiHandler and addOscHandler." I had some problems with the type-checking, and the resulting code is a bit ugly, but not too bad, I think.

[jerch]

This needs to be documented: Inserting the additional handlers at top. (since it is somewhat surprising compared to many other event systems)

[PerBothner]

Done in commit 6b65ebd.
(I added detailed doc comments in xterm.d.ts, with just a "link" in Terminal.ts.)

[jerch]

Will this still work with cycles, like someone added a handler twice by accident? Also an cycle might prevent the autoclean up by the .dispose method, imho the dispose chain of additional handlers should be called there, too.
Ah well it creates always a new function object, so cycles should not be an issue (I think).

[PerBothner]

I believe it should be ok.

[jerch]

Yupp found no issues with it (handler is recreated anyway).

[jerch]

Much cleaner with this management method. 👍

[PerBothner]

There are still tests to be added (in EscapeSequenceParser.test.ts), but otherwise I hope the latest commit is good.

[PerBothner]

Any feedback on the latest version? I'm hoping to not drag this on too much longer.

[jerch]

@PerBothner Code looks good (just a few comments). Just one thing - please move the changes into its own branch so we can pull lastest master changes and merge later on.

[jerch]

Yupp found no issues with it (handler is recreated anyway).

[jerch]

Much cleaner with this management method. 👍

[jerch]

We have now a somewhat weird API here:

• set...: sets a handler, would remove any previously set/added handlers
• add...: sets (first) or adds a handler
• clear...: clears all handler(s)

Since set.../clear... are only internally used I dont care much, but once we want this go public we have to refactor it (or at least document the weird behavior).

[jerch]

Likely to change with the typed array in the parser (around v3.11): a single char string arg will turn into a number, the string arg is likely to be a typed array itself with start/end offsets (not clear yet whether UTF32 or UTF16 based).

[PerBothner]

"please move the changes into its own branch"
I checked my changes into a new control-seq-handler branch (of my fork). I'll to figure out how to update the pull request later.

EDIT: There doesn't seem to be a way to change the pull request to use the new branch. I can create a new pull request if you want.

[jerch]

@PerBothner Not sure if changing the PR branch is possible at all. Seems creating a new PR is the only option.

[PerBothner]

Created pull request #1853 .

[jerch]

@PerBothner Thank you for the branch split. Closing this PR.