cursor blinking doesn't work for subsequently created terminals

[meganrogge]

because cursorBlinkManager is disposed of but not re-instantiated when a terminal is focused

microsoft/vscode#131446 (comment)

[meganrogge]

bug doesn't repro for dom/canvas

[meganrogge]

adding

if (this._optionsService.options.cursorBlink) {
      this._cursorBlinkStateManager = this._cursorBlinkStateManager || new CursorBlinkStateManager(this._coreBrowserService.isFocused, () => {
        this._render(true);
      });
      this._cursorBlinkStateManager.resume();
    } else {
      this._onRequestRedraw.fire({ start: this._bufferService.buffer.y, end: this._bufferService.buffer.y });
    }

to onFocus in the webgl CursorRenderLayer doesn't fix it - nor does the same code within onCursorMove.

[meganrogge]

the CursorRenderLayers look pretty much the same - an exception to that is the working one (non-webgl) has this function:

https://github.com/meganrogge/xterm.js/blob/8c94980315076e9c833b27fc3f8aff2bd7c32f67/src/browser/renderer/CursorRenderLayer.ts#L61-L67

[meganrogge]

adding that doesn't help

[jerch]

This looks like a renderer lifecycling issue to me: CursorBlinkStateManager gets only created in onOptionsChanged() in both renderer cases (canvas vs webgl), but other than the canvas renderer (which exists on the terminal from early bootstrapping), the webgl renderer gets attached late from the addon. By that time the options already got "changed", thus the webgl path never gets that initial "isBlinking" state via onOptionsChanged().

Hacky solution: Re-fire an options changed with isBlinking on a renderer change.
Prolly better solution: Init blinking state explicitly during ctor phase from optionsService, instead of relying on proper onOptionsChanged() bubbling.

Edit:
In the demo calling onOptionsChanged(this._terminal as any) late in the ctor fixes the behavior. (This might need further guards against a not readily DOM attached terminal instance, if not already done by the subsequent calls.)

[Tyriar]

Initializing it during renderer init seems like the right fix, I think we should keep onOptionsChanged to only get called when the options actually change so we're not doing extra work when creating the renderer.

[meganrogge]

Thanks for the info @jerch

I tried something very similar to that and it didn't work. will look at with @Tyriar