marker fixes

[jerch]

Shall fix #3108.

Changes:

• calls super.dispose() in overloaded method
• onDispose added to public API

[jerch]

@Tyriar I think this should get a few unit test cases before it can be merged. Currently I am not sure, what action will trigger onDispose (only by scrolling out? line removal from buffer? before resize? during resize? after resize as batch?) Maybe some test cases will reveal the details and we might have to see whether it fits the bill.

[Tyriar]

@jerch sounds good, I think the only cases are:

• Scroll off top (trim)
• Delete line (in viewport via sequence)
• Embedder disposing

[Tyriar]

Good to merge now or when tests are added

[jerch]

@Tyriar Have implemented basic marker test cases for these actions:

• onDispose gets called - in Buffer.test.ts (working)
• dispose on trim off the buffer top in Terminal.test.js (working)
• dispose on DL Terminal.test.js (skipped, not working)
• dispose on IL Terminal.test.js (skipped, not working)

There are multiple issues with the tests - first I tried to get those test cases running on common/CoreTerminal, but this does not work for some reason. Seems there is still logic bound to browser/Terminal, which better would reside in CoreTerminal. In regards of buffer insert/update logic CoreTerminalis currently dysfunctional (does not do any circular buffer rotation).

DL and IL do not trigger any marker updates (no line updates, no dispose). This looks like a bug to me, as it does not replay the lines changes in markers, thus after doing those actions markers simply point to the wrong line. Guess markers need to be abit more clever than just listen to onTrim?

Imho we can still add the fixes here, and maybe address the issues above with later PRs?

[jerch]

@Tyriar Got DL and IL working by adding the insert and delete events in CircularList.splice. Plz have a look on those additions (hope they are free of any side effects).

[jerch]

@Tyriar Thinking about the interface - could we make onDispose to return the last active line before it turned to -1? This would help to track area updates (needed to know whether a line was removed from top or within the cursor active area).

Somthing like this:

interface IMarker {
  ...
  onDispose: IEvent<number>;
}
...
marker.onDispose(lastLine => {
  // lastLine holds idx of last line 
});

Would only need a tiny change here:

xterm.js/src/common/buffer/Marker.ts

Lines 32 to 34 in faa60be

	this.line = -1;
	// Emit before super.dispose such that dispose listeners get a change to react
	this._onDispose.fire();

Update: Not going to change that, seems it has only a small benefit of avoid scanning through the active cursor area, but that is rather small compared to the whole buffer.

[Tyriar]

Thanks for merging and sorry about the disappearance 🙂

[jerch]

@Tyriar Ah no worries, I think the added events in CircularList do the right thing (as the tests work now). Since vscode uses markers to some extend I just think you might want to check if their logic still sees the right line numbers after a DL/IL.