From 06357b1184e1ae9e7a66c79f4ea542697be76342 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 14:08:46 -0600 Subject: [PATCH 01/10] get rid of some unnecessary code --- src/browser/services/DecorationService.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index f29ec7ea1..9dc4c4ac2 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -17,7 +17,6 @@ export class DecorationService extends Disposable implements IDecorationService private _renderService: IRenderService | undefined; constructor( - @IBufferService private readonly _bufferService: IBufferService, @IInstantiationService private readonly _instantiationService: IInstantiationService) { super(); } @@ -28,7 +27,6 @@ export class DecorationService extends Disposable implements IDecorationService this._container = document.createElement('div'); this._container.classList.add('xterm-decoration-container'); screenElement.appendChild(this._container); - this.refresh(); this.register(this._renderService.onRenderedBufferChange(() => this.refresh())); this.register(this._renderService.onDimensionsChange(() => this.refresh(true))); } @@ -44,7 +42,7 @@ export class DecorationService extends Disposable implements IDecorationService } public refresh(recreate?: boolean): void { - if (!this._bufferService || !this._renderService) { + if (!this._renderService) { return; } for (const decoration of this._decorations) { From f160436df8e4954a13319411ba389f99f60153e8 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 14:54:01 -0600 Subject: [PATCH 02/10] dispose of decoration on dispose of marker --- src/browser/services/DecorationService.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index 9dc4c4ac2..df21d076f 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -88,6 +88,7 @@ export class Decoration extends Disposable implements IDecoration { super(); this.x = options.x ?? 0; this._marker = options.marker; + this._marker.onDispose(() => this.dispose()); this.anchor = options.anchor || 'left'; this.width = options.width || 1; this.height = options.height || 1; From e44659e8bd3ef803b05571df141008db88257569 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 14:56:56 -0600 Subject: [PATCH 03/10] remove unused ID - can just use marker's --- src/browser/services/DecorationService.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index df21d076f..28f18a78e 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -54,16 +54,14 @@ export class DecorationService extends Disposable implements IDecorationService for (const decoration of this._decorations) { decoration.dispose(); } - if (this._container) { - this._screenElement?.removeChild(this._container); + if (this._screenElement && this._container) { + this._screenElement.removeChild(this._container); } } } export class Decoration extends Disposable implements IDecoration { - private static _nextId = 1; private readonly _marker: IMarker; private _element: HTMLElement | undefined; - private readonly _id: number = Decoration._nextId++; public isDisposed: boolean = false; public get element(): HTMLElement | undefined { return this._element; } From d8573e38a8570c047a329eaf90a8f7e90b92a2f3 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 15:01:07 -0600 Subject: [PATCH 04/10] cleanup dispose --- src/browser/services/DecorationService.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index 28f18a78e..c98756109 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -126,9 +126,13 @@ export class Decoration extends Disposable implements IDecoration { if (this.isDisposed) { return; } - this._container.removeChild(this._element!); + if (!this.marker.isDisposed) { + this.marker.dispose(); + } + if (this._element) { + this._container.removeChild(this._element); + } this.isDisposed = true; - this._marker.dispose(); // Emit before super.dispose such that dispose listeners get a change to react this._onDispose.fire(); super.dispose(); From de3a09e05f97d8b37e80b2af8d1cdba0e31e9dfe Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 15:09:40 -0600 Subject: [PATCH 05/10] fix #3633 --- src/common/InputHandler.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/InputHandler.ts b/src/common/InputHandler.ts index 886340486..7e6fbd3ae 100644 --- a/src/common/InputHandler.ts +++ b/src/common/InputHandler.ts @@ -1292,6 +1292,7 @@ export class InputHandler extends Disposable implements IInputHandler { this._resetBufferLine(j); } this._dirtyRowService.markDirty(0); + this._bufferService.buffer.clearMarkers(); break; case 3: // Clear scrollback (everything not in viewport) From 3f525fb960cca18b7c78d9744c7c3d1c1d68cedf Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 15:40:44 -0600 Subject: [PATCH 06/10] fix #3636 --- src/browser/services/DecorationService.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index c98756109..a775be3f4 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -15,9 +15,11 @@ export class DecorationService extends Disposable implements IDecorationService private _container: HTMLElement | undefined; private _screenElement: HTMLElement | undefined; private _renderService: IRenderService | undefined; + private _altBufferActive: boolean = false; constructor( - @IInstantiationService private readonly _instantiationService: IInstantiationService) { + @IInstantiationService private readonly _instantiationService: IInstantiationService, + @IBufferService private readonly _bufferService: IBufferService) { super(); } @@ -29,6 +31,7 @@ export class DecorationService extends Disposable implements IDecorationService screenElement.appendChild(this._container); this.register(this._renderService.onRenderedBufferChange(() => this.refresh())); this.register(this._renderService.onDimensionsChange(() => this.refresh(true))); + this.register(this._bufferService.buffers.onBufferActivate((event) => this._altBufferActive = event.activeBuffer === this._bufferService.buffers.alt)); } public registerDecoration(decorationOptions: IDecorationOptions): IDecoration | undefined { @@ -46,7 +49,7 @@ export class DecorationService extends Disposable implements IDecorationService return; } for (const decoration of this._decorations) { - decoration.render(this._renderService, recreate); + decoration.render(this._renderService, recreate, this._altBufferActive); } } @@ -92,14 +95,14 @@ export class Decoration extends Disposable implements IDecoration { this.height = options.height || 1; } - public render(renderService: IRenderService, recreate?: boolean): void { + public render(renderService: IRenderService, recreate?: boolean, altBufferActive?: boolean): void { if (!this._element || recreate) { this._createElement(renderService, recreate); } if (this._container && this._element && !this._container.contains(this._element)) { this._container.append(this._element); } - this._refreshStyle(renderService); + this._refreshStyle(renderService, altBufferActive); this._onRender.fire(this._element!); } @@ -140,14 +143,14 @@ export class Decoration extends Disposable implements IDecoration { }); } - private _refreshStyle(renderService: IRenderService): void { + private _refreshStyle(renderService: IRenderService, altBufferActive?: boolean): void { const line = this.marker.line - this._bufferService.buffers.active.ydisp; if (line < 0 || line > this._bufferService.rows) { // outside of viewport this._element!.style.display = 'none'; } else { this._element!.style.top = `${line * renderService.dimensions.scaledCellHeight}px`; - this._element!.style.display = 'block'; + this._element!.style.display = altBufferActive ? 'none' : 'block'; } } } From 7d357b941f9ac3342545b430d16d8ea9a4093f75 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 16:48:40 -0600 Subject: [PATCH 07/10] check if container has child before removing it --- src/browser/services/DecorationService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index a775be3f4..24ceebccc 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -57,7 +57,7 @@ export class DecorationService extends Disposable implements IDecorationService for (const decoration of this._decorations) { decoration.dispose(); } - if (this._screenElement && this._container) { + if (this._screenElement && this._container && this._screenElement.contains(this._container)) { this._screenElement.removeChild(this._container); } } From 40e6dba43c4686d32c042922e30db720eddf6cb0 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 17:00:24 -0600 Subject: [PATCH 08/10] cleaner --- src/browser/services/DecorationService.ts | 31 +++++++++++++---------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index 24ceebccc..d20961591 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -15,7 +15,6 @@ export class DecorationService extends Disposable implements IDecorationService private _container: HTMLElement | undefined; private _screenElement: HTMLElement | undefined; private _renderService: IRenderService | undefined; - private _altBufferActive: boolean = false; constructor( @IInstantiationService private readonly _instantiationService: IInstantiationService, @@ -31,7 +30,6 @@ export class DecorationService extends Disposable implements IDecorationService screenElement.appendChild(this._container); this.register(this._renderService.onRenderedBufferChange(() => this.refresh())); this.register(this._renderService.onDimensionsChange(() => this.refresh(true))); - this.register(this._bufferService.buffers.onBufferActivate((event) => this._altBufferActive = event.activeBuffer === this._bufferService.buffers.alt)); } public registerDecoration(decorationOptions: IDecorationOptions): IDecoration | undefined { @@ -44,12 +42,12 @@ export class DecorationService extends Disposable implements IDecorationService return decoration; } - public refresh(recreate?: boolean): void { + public refresh(shouldRecreate?: boolean): void { if (!this._renderService) { return; } for (const decoration of this._decorations) { - decoration.render(this._renderService, recreate, this._altBufferActive); + decoration.render(this._renderService, shouldRecreate); } } @@ -65,6 +63,8 @@ export class DecorationService extends Disposable implements IDecorationService export class Decoration extends Disposable implements IDecoration { private readonly _marker: IMarker; private _element: HTMLElement | undefined; + private _altBufferActive: boolean = false; + public isDisposed: boolean = false; public get element(): HTMLElement | undefined { return this._element; } @@ -93,21 +93,24 @@ export class Decoration extends Disposable implements IDecoration { this.anchor = options.anchor || 'left'; this.width = options.width || 1; this.height = options.height || 1; + this.register(this._bufferService.buffers.onBufferActivate((event) => this._altBufferActive = event.activeBuffer === this._bufferService.buffers.alt)); } - public render(renderService: IRenderService, recreate?: boolean, altBufferActive?: boolean): void { - if (!this._element || recreate) { - this._createElement(renderService, recreate); + public render(renderService: IRenderService, shouldRecreate?: boolean): void { + if (!this._element || shouldRecreate) { + this._createElement(renderService, shouldRecreate); } if (this._container && this._element && !this._container.contains(this._element)) { this._container.append(this._element); } - this._refreshStyle(renderService, altBufferActive); - this._onRender.fire(this._element!); + this._refreshStyle(renderService); + if (this._element) { + this._onRender.fire(this._element); + } } - private _createElement(renderService: IRenderService, recreate?: boolean): void { - if (recreate && this._element) { + private _createElement(renderService: IRenderService, shouldRecreate?: boolean): void { + if (shouldRecreate && this._element) { this._container.removeChild(this._element); } this._element = document.createElement('div'); @@ -132,7 +135,7 @@ export class Decoration extends Disposable implements IDecoration { if (!this.marker.isDisposed) { this.marker.dispose(); } - if (this._element) { + if (this._element && this._container.contains(this._element)) { this._container.removeChild(this._element); } this.isDisposed = true; @@ -143,14 +146,14 @@ export class Decoration extends Disposable implements IDecoration { }); } - private _refreshStyle(renderService: IRenderService, altBufferActive?: boolean): void { + private _refreshStyle(renderService: IRenderService): void { const line = this.marker.line - this._bufferService.buffers.active.ydisp; if (line < 0 || line > this._bufferService.rows) { // outside of viewport this._element!.style.display = 'none'; } else { this._element!.style.top = `${line * renderService.dimensions.scaledCellHeight}px`; - this._element!.style.display = altBufferActive ? 'none' : 'block'; + this._element!.style.display = this._altBufferActive ? 'none' : 'block'; } } } From e720643315a6e96d1c456156286e0adadef069e8 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 17:04:11 -0600 Subject: [PATCH 09/10] get rid of unused --- src/browser/services/DecorationService.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index d20961591..2ea164293 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -16,11 +16,7 @@ export class DecorationService extends Disposable implements IDecorationService private _screenElement: HTMLElement | undefined; private _renderService: IRenderService | undefined; - constructor( - @IInstantiationService private readonly _instantiationService: IInstantiationService, - @IBufferService private readonly _bufferService: IBufferService) { - super(); - } + constructor(@IInstantiationService private readonly _instantiationService: IInstantiationService) { super(); } public attachToDom(screenElement: HTMLElement, renderService: IRenderService): void { this._renderService = renderService; @@ -120,7 +116,8 @@ export class Decoration extends Disposable implements IDecoration { this._element.style.top = `${(this.marker.line - this._bufferService.buffers.active.ydisp) * renderService.dimensions.scaledCellHeight}px`; if (this.x && this.x > this._bufferService.cols) { - this._element!.style.display = 'none'; + // exceeded the container width, so hide + this._element.style.display = 'none'; } if (this.anchor === 'right') { this._element.style.right = this.x ? `${this.x * renderService.dimensions.scaledCellWidth}px` : ''; @@ -147,13 +144,16 @@ export class Decoration extends Disposable implements IDecoration { } private _refreshStyle(renderService: IRenderService): void { + if (!this._element) { + return; + } const line = this.marker.line - this._bufferService.buffers.active.ydisp; if (line < 0 || line > this._bufferService.rows) { // outside of viewport - this._element!.style.display = 'none'; + this._element.style.display = 'none'; } else { - this._element!.style.top = `${line * renderService.dimensions.scaledCellHeight}px`; - this._element!.style.display = this._altBufferActive ? 'none' : 'block'; + this._element.style.top = `${line * renderService.dimensions.scaledCellHeight}px`; + this._element.style.display = this._altBufferActive ? 'none' : 'block'; } } } From 8dfed8f979093b5c6a73e055324033214e07a387 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Fri, 11 Feb 2022 17:08:08 -0600 Subject: [PATCH 10/10] be safe and check if container contains element before removing --- src/browser/services/DecorationService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/browser/services/DecorationService.ts b/src/browser/services/DecorationService.ts index 2ea164293..287852247 100644 --- a/src/browser/services/DecorationService.ts +++ b/src/browser/services/DecorationService.ts @@ -106,7 +106,7 @@ export class Decoration extends Disposable implements IDecoration { } private _createElement(renderService: IRenderService, shouldRecreate?: boolean): void { - if (shouldRecreate && this._element) { + if (shouldRecreate && this._element && this._container.contains(this._element)) { this._container.removeChild(this._element); } this._element = document.createElement('div');