From a3ab942ea157c1981847f1467be7f5379f14e6c0 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Wed, 27 Aug 2025 20:45:52 -0700 Subject: [PATCH 1/7] Fix teleport issue when coming back from alt buffer --- src/common/TestUtils.test.ts | 3 +++ src/common/buffer/BufferSet.ts | 2 ++ src/common/services/BufferService.ts | 7 +++++++ src/common/services/Services.ts | 1 + 4 files changed, 13 insertions(+) diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index 127e1f24..ccdb54ae 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -46,6 +46,9 @@ export class MockBufferService implements IBufferService { public scrollLines(disp: number, suppressScrollEvent?: boolean): void { throw new Error('Method not implemented.'); } + public syncScrollPosition(): void { + // Mock implementation - no-op for tests + } public resize(cols: number, rows: number): void { this.cols = cols; this.rows = rows; diff --git a/src/common/buffer/BufferSet.ts b/src/common/buffer/BufferSet.ts index 8f3a6aec..54390f2a 100644 --- a/src/common/buffer/BufferSet.ts +++ b/src/common/buffer/BufferSet.ts @@ -91,6 +91,8 @@ export class BufferSet extends Disposable implements IBufferSet { activeBuffer: this._normal, inactiveBuffer: this._alt }); + // Prevent scrollbar "teleport" to top of the terminal, from previous alt buffer. + this._bufferService.syncScrollPosition(); } /** diff --git a/src/common/services/BufferService.ts b/src/common/services/BufferService.ts index c4698e68..85699f6f 100644 --- a/src/common/services/BufferService.ts +++ b/src/common/services/BufferService.ts @@ -148,4 +148,11 @@ export class BufferService extends Disposable implements IBufferService { this._onScroll.fire(buffer.ydisp); } } + + /** + * Synchronize the scroll position by firing a scroll event with the current buffer's ydisp. + */ + public syncScrollPosition(): void { + this._onScroll.fire(this.buffer.ydisp); + } } diff --git a/src/common/services/Services.ts b/src/common/services/Services.ts index 9c3aebf7..58ee113f 100644 --- a/src/common/services/Services.ts +++ b/src/common/services/Services.ts @@ -22,6 +22,7 @@ export interface IBufferService { onScroll: Event; scroll(eraseAttr: IAttributeData, isWrapped?: boolean): void; scrollLines(disp: number, suppressScrollEvent?: boolean): void; + syncScrollPosition(): void; resize(cols: number, rows: number): void; reset(): void; } From 86745e1320f95b0c4f71330acc2d6a56ed0344a4 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Wed, 27 Aug 2025 21:12:15 -0700 Subject: [PATCH 2/7] Add some tests from copilot --- src/common/buffer/BufferSet.test.ts | 67 +++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/common/buffer/BufferSet.test.ts b/src/common/buffer/BufferSet.test.ts index 944b8c15..4c937729 100644 --- a/src/common/buffer/BufferSet.test.ts +++ b/src/common/buffer/BufferSet.test.ts @@ -7,6 +7,7 @@ import { assert } from 'chai'; import { BufferSet } from 'common/buffer/BufferSet'; import { Buffer } from 'common/buffer/Buffer'; import { MockOptionsService, MockBufferService } from 'common/TestUtils.test'; +import { Emitter } from 'vs/base/common/event'; describe('BufferSet', () => { let bufferSet: BufferSet; @@ -81,4 +82,70 @@ describe('BufferSet', () => { assert.equal(bufferSet.alt.markers.length, 0); }); }); + + describe('scroll position synchronization', () => { + it('should sync scroll position when switching from alt back to normal buffer', () => { + bufferSet.activateNormalBuffer(); + + const originalYDisp = 50; + bufferSet.normal.ydisp = originalYDisp; + bufferSet.normal.ybase = 100; + + // Track onScroll events fired by the buffer service + let scrollEventFired = false; + let scrollEventPosition = -1; + + const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; + const originalOnScroll = mockBufferService.onScroll; + + // Mock the onScroll event to track when it's called + const scrollEmitter = new Emitter(); + mockBufferService.onScroll = scrollEmitter.event; + + // Override syncScrollPosition to actually fire the event + mockBufferService.syncScrollPosition = () => { + scrollEventFired = true; + scrollEventPosition = bufferSet.normal.ydisp; + scrollEmitter.fire(bufferSet.normal.ydisp); + }; + + + bufferSet.activateAltBuffer(); + assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be preserved'); + + // Reset scroll event tracking + scrollEventFired = false; + scrollEventPosition = -1; + + bufferSet.activateNormalBuffer(); + assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be restored'); + assert.equal(scrollEventFired, true, 'syncScrollPosition should have fired scroll event'); + assert.equal(scrollEventPosition, originalYDisp, 'Scroll event should contain correct position'); + assert.equal(bufferSet.active, bufferSet.normal, 'Normal buffer should be active'); + }); + + it('should preserve normal buffer scroll position even when alt buffer has different position', () => { + bufferSet.activateNormalBuffer(); + const normalScrollPos = 80; + bufferSet.normal.ydisp = normalScrollPos; + bufferSet.normal.ybase = 150; + + bufferSet.activateAltBuffer(); + bufferSet.alt.ydisp = 0; + bufferSet.alt.ybase = 0; + + const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; + let syncedPosition = -1; + + // Track the position that gets synced + mockBufferService.syncScrollPosition = () => { + syncedPosition = bufferSet.normal.ydisp; + }; + + bufferSet.activateNormalBuffer(); + assert.equal(bufferSet.normal.ydisp, normalScrollPos, 'Normal buffer should maintain its scroll position'); + assert.equal(syncedPosition, normalScrollPos, 'syncScrollPosition should sync with normal buffer position'); + assert.notEqual(syncedPosition, bufferSet.alt.ydisp, 'Sync position should not match alt buffer position'); + }); + }); }); From 675b2c3a70844d4a2a655a6e54a38125000498cc Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Wed, 27 Aug 2025 21:31:34 -0700 Subject: [PATCH 3/7] Improve test --- src/common/TestUtils.test.ts | 9 ++-- src/common/buffer/BufferSet.test.ts | 65 ++++++++++++++--------------- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index ccdb54ae..041481b5 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -18,8 +18,10 @@ export class MockBufferService implements IBufferService { public serviceBrand: any; public get buffer(): IBuffer { return this.buffers.active; } public buffers: IBufferSet = {} as any; - public onResize: Event<{ cols: number, rows: number }> = new Emitter<{ cols: number, rows: number }>().event; - public onScroll: Event = new Emitter().event; + private readonly _onResize = new Emitter<{ cols: number, rows: number }>(); + public readonly onResize: Event<{ cols: number, rows: number }> = this._onResize.event; + private readonly _onScroll = new Emitter(); + public readonly onScroll: Event = this._onScroll.event; public isUserScrolling: boolean = false; constructor( public cols: number, @@ -47,7 +49,8 @@ export class MockBufferService implements IBufferService { throw new Error('Method not implemented.'); } public syncScrollPosition(): void { - // Mock implementation - no-op for tests + // Fire scroll event with current buffer position + this._onScroll.fire(this.buffer.ydisp); } public resize(cols: number, rows: number): void { this.cols = cols; diff --git a/src/common/buffer/BufferSet.test.ts b/src/common/buffer/BufferSet.test.ts index 4c937729..449c40b8 100644 --- a/src/common/buffer/BufferSet.test.ts +++ b/src/common/buffer/BufferSet.test.ts @@ -84,47 +84,32 @@ describe('BufferSet', () => { }); describe('scroll position synchronization', () => { - it('should sync scroll position when switching from alt back to normal buffer', () => { + it('should call syncScrollPosition when switching from alt back to normal buffer', () => { bufferSet.activateNormalBuffer(); const originalYDisp = 50; bufferSet.normal.ydisp = originalYDisp; bufferSet.normal.ybase = 100; - - // Track onScroll events fired by the buffer service - let scrollEventFired = false; - let scrollEventPosition = -1; - const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; - const originalOnScroll = mockBufferService.onScroll; - // Mock the onScroll event to track when it's called - const scrollEmitter = new Emitter(); - mockBufferService.onScroll = scrollEmitter.event; - - // Override syncScrollPosition to actually fire the event + let syncScrollPositionCalled = false; + const originalSyncScrollPosition = mockBufferService.syncScrollPosition.bind(mockBufferService); mockBufferService.syncScrollPosition = () => { - scrollEventFired = true; - scrollEventPosition = bufferSet.normal.ydisp; - scrollEmitter.fire(bufferSet.normal.ydisp); + syncScrollPositionCalled = true; + originalSyncScrollPosition(); }; - bufferSet.activateAltBuffer(); assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be preserved'); - - // Reset scroll event tracking - scrollEventFired = false; - scrollEventPosition = -1; - + syncScrollPositionCalled = false; bufferSet.activateNormalBuffer(); + assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be restored'); - assert.equal(scrollEventFired, true, 'syncScrollPosition should have fired scroll event'); - assert.equal(scrollEventPosition, originalYDisp, 'Scroll event should contain correct position'); + assert.equal(syncScrollPositionCalled, true, 'activateNormalBuffer should call syncScrollPosition'); assert.equal(bufferSet.active, bufferSet.normal, 'Normal buffer should be active'); }); - it('should preserve normal buffer scroll position even when alt buffer has different position', () => { + it('should preserve normal buffer scroll position when switching back from alt buffer', () => { bufferSet.activateNormalBuffer(); const normalScrollPos = 80; bufferSet.normal.ydisp = normalScrollPos; @@ -133,19 +118,33 @@ describe('BufferSet', () => { bufferSet.activateAltBuffer(); bufferSet.alt.ydisp = 0; bufferSet.alt.ybase = 0; + bufferSet.activateNormalBuffer(); + assert.equal(bufferSet.normal.ydisp, normalScrollPos, 'Normal buffer should maintain its scroll position'); + assert.equal(bufferSet.active, bufferSet.normal, 'Normal buffer should be active'); + assert.notEqual(bufferSet.normal.ydisp, bufferSet.alt.ydisp, 'Normal and alt buffer should have different scroll positions'); + }); + + it('should fire scroll event with correct position when syncScrollPosition is called', () => { const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; - let syncedPosition = -1; - - // Track the position that gets synced - mockBufferService.syncScrollPosition = () => { - syncedPosition = bufferSet.normal.ydisp; - }; bufferSet.activateNormalBuffer(); - assert.equal(bufferSet.normal.ydisp, normalScrollPos, 'Normal buffer should maintain its scroll position'); - assert.equal(syncedPosition, normalScrollPos, 'syncScrollPosition should sync with normal buffer position'); - assert.notEqual(syncedPosition, bufferSet.alt.ydisp, 'Sync position should not match alt buffer position'); + const testScrollPosition = 42; + + mockBufferService.buffer.ydisp = testScrollPosition; + assert.equal(mockBufferService.buffer.ydisp, testScrollPosition, 'Active buffer ydisp should be set correctly'); + + let scrollEventFired = false; + let scrollEventPosition = -1; + + mockBufferService.onScroll((position: number) => { + scrollEventFired = true; + scrollEventPosition = position; + }); + + mockBufferService.syncScrollPosition(); + assert.equal(scrollEventFired, true, 'syncScrollPosition should fire scroll event'); + assert.equal(scrollEventPosition, testScrollPosition, `Scroll event should contain current buffer ydisp`); }); }); }); From 753ea81fc08661b4c661338461c81b3a06178d8c Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 28 Aug 2025 08:46:27 -0700 Subject: [PATCH 4/7] BufferService listen to onBufferActivate, remove tests --- src/common/TestUtils.test.ts | 8 ++-- src/common/buffer/BufferSet.test.ts | 64 ---------------------------- src/common/buffer/BufferSet.ts | 2 - src/common/services/BufferService.ts | 9 ++-- src/common/services/Services.ts | 1 - 5 files changed, 7 insertions(+), 77 deletions(-) diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index 041481b5..d9d3b24e 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -29,6 +29,10 @@ export class MockBufferService implements IBufferService { optionsService: IOptionsService = new MockOptionsService() ) { this.buffers = new BufferSet(optionsService, this); + // Listen to buffer activation events and automatically fire scroll events + this.buffers.onBufferActivate(e => { + this._onScroll.fire(e.activeBuffer.ydisp); + }); } public scrollPages(pageCount: number): void { throw new Error('Method not implemented.'); @@ -48,10 +52,6 @@ export class MockBufferService implements IBufferService { public scrollLines(disp: number, suppressScrollEvent?: boolean): void { throw new Error('Method not implemented.'); } - public syncScrollPosition(): void { - // Fire scroll event with current buffer position - this._onScroll.fire(this.buffer.ydisp); - } public resize(cols: number, rows: number): void { this.cols = cols; this.rows = rows; diff --git a/src/common/buffer/BufferSet.test.ts b/src/common/buffer/BufferSet.test.ts index 449c40b8..b6136094 100644 --- a/src/common/buffer/BufferSet.test.ts +++ b/src/common/buffer/BufferSet.test.ts @@ -83,68 +83,4 @@ describe('BufferSet', () => { }); }); - describe('scroll position synchronization', () => { - it('should call syncScrollPosition when switching from alt back to normal buffer', () => { - bufferSet.activateNormalBuffer(); - - const originalYDisp = 50; - bufferSet.normal.ydisp = originalYDisp; - bufferSet.normal.ybase = 100; - const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; - - let syncScrollPositionCalled = false; - const originalSyncScrollPosition = mockBufferService.syncScrollPosition.bind(mockBufferService); - mockBufferService.syncScrollPosition = () => { - syncScrollPositionCalled = true; - originalSyncScrollPosition(); - }; - - bufferSet.activateAltBuffer(); - assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be preserved'); - syncScrollPositionCalled = false; - bufferSet.activateNormalBuffer(); - - assert.equal(bufferSet.normal.ydisp, originalYDisp, 'Normal buffer ydisp should be restored'); - assert.equal(syncScrollPositionCalled, true, 'activateNormalBuffer should call syncScrollPosition'); - assert.equal(bufferSet.active, bufferSet.normal, 'Normal buffer should be active'); - }); - - it('should preserve normal buffer scroll position when switching back from alt buffer', () => { - bufferSet.activateNormalBuffer(); - const normalScrollPos = 80; - bufferSet.normal.ydisp = normalScrollPos; - bufferSet.normal.ybase = 150; - - bufferSet.activateAltBuffer(); - bufferSet.alt.ydisp = 0; - bufferSet.alt.ybase = 0; - bufferSet.activateNormalBuffer(); - - assert.equal(bufferSet.normal.ydisp, normalScrollPos, 'Normal buffer should maintain its scroll position'); - assert.equal(bufferSet.active, bufferSet.normal, 'Normal buffer should be active'); - assert.notEqual(bufferSet.normal.ydisp, bufferSet.alt.ydisp, 'Normal and alt buffer should have different scroll positions'); - }); - - it('should fire scroll event with correct position when syncScrollPosition is called', () => { - const mockBufferService = (bufferSet as any)._bufferService as MockBufferService; - - bufferSet.activateNormalBuffer(); - const testScrollPosition = 42; - - mockBufferService.buffer.ydisp = testScrollPosition; - assert.equal(mockBufferService.buffer.ydisp, testScrollPosition, 'Active buffer ydisp should be set correctly'); - - let scrollEventFired = false; - let scrollEventPosition = -1; - - mockBufferService.onScroll((position: number) => { - scrollEventFired = true; - scrollEventPosition = position; - }); - - mockBufferService.syncScrollPosition(); - assert.equal(scrollEventFired, true, 'syncScrollPosition should fire scroll event'); - assert.equal(scrollEventPosition, testScrollPosition, `Scroll event should contain current buffer ydisp`); - }); - }); }); diff --git a/src/common/buffer/BufferSet.ts b/src/common/buffer/BufferSet.ts index 54390f2a..8f3a6aec 100644 --- a/src/common/buffer/BufferSet.ts +++ b/src/common/buffer/BufferSet.ts @@ -91,8 +91,6 @@ export class BufferSet extends Disposable implements IBufferSet { activeBuffer: this._normal, inactiveBuffer: this._alt }); - // Prevent scrollbar "teleport" to top of the terminal, from previous alt buffer. - this._bufferService.syncScrollPosition(); } /** diff --git a/src/common/services/BufferService.ts b/src/common/services/BufferService.ts index 85699f6f..616e5619 100644 --- a/src/common/services/BufferService.ts +++ b/src/common/services/BufferService.ts @@ -37,6 +37,9 @@ export class BufferService extends Disposable implements IBufferService { this.cols = Math.max(optionsService.rawOptions.cols || 0, MINIMUM_COLS); this.rows = Math.max(optionsService.rawOptions.rows || 0, MINIMUM_ROWS); this.buffers = this._register(new BufferSet(optionsService, this)); + this._register(this.buffers.onBufferActivate(e => { + this._onScroll.fire(e.activeBuffer.ydisp); + })); } public resize(cols: number, rows: number): void { @@ -149,10 +152,4 @@ export class BufferService extends Disposable implements IBufferService { } } - /** - * Synchronize the scroll position by firing a scroll event with the current buffer's ydisp. - */ - public syncScrollPosition(): void { - this._onScroll.fire(this.buffer.ydisp); - } } diff --git a/src/common/services/Services.ts b/src/common/services/Services.ts index 58ee113f..9c3aebf7 100644 --- a/src/common/services/Services.ts +++ b/src/common/services/Services.ts @@ -22,7 +22,6 @@ export interface IBufferService { onScroll: Event; scroll(eraseAttr: IAttributeData, isWrapped?: boolean): void; scrollLines(disp: number, suppressScrollEvent?: boolean): void; - syncScrollPosition(): void; resize(cols: number, rows: number): void; reset(): void; } From 4d59eca5f9171e67a3e4d4daf28242746e6d7b28 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 28 Aug 2025 08:52:46 -0700 Subject: [PATCH 5/7] Clean up --- src/common/buffer/BufferSet.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/common/buffer/BufferSet.test.ts b/src/common/buffer/BufferSet.test.ts index b6136094..944b8c15 100644 --- a/src/common/buffer/BufferSet.test.ts +++ b/src/common/buffer/BufferSet.test.ts @@ -7,7 +7,6 @@ import { assert } from 'chai'; import { BufferSet } from 'common/buffer/BufferSet'; import { Buffer } from 'common/buffer/Buffer'; import { MockOptionsService, MockBufferService } from 'common/TestUtils.test'; -import { Emitter } from 'vs/base/common/event'; describe('BufferSet', () => { let bufferSet: BufferSet; @@ -82,5 +81,4 @@ describe('BufferSet', () => { assert.equal(bufferSet.alt.markers.length, 0); }); }); - }); From 287702610c81ab346202aa53494847ed2addbfdd Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 28 Aug 2025 08:54:01 -0700 Subject: [PATCH 6/7] More clean up after removing test --- src/common/TestUtils.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index d9d3b24e..f504cbe9 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -18,10 +18,9 @@ export class MockBufferService implements IBufferService { public serviceBrand: any; public get buffer(): IBuffer { return this.buffers.active; } public buffers: IBufferSet = {} as any; - private readonly _onResize = new Emitter<{ cols: number, rows: number }>(); - public readonly onResize: Event<{ cols: number, rows: number }> = this._onResize.event; + public onResize: Event<{ cols: number, rows: number }> = new Emitter<{ cols: number, rows: number }>().event; + public onScroll: Event = new Emitter().event; private readonly _onScroll = new Emitter(); - public readonly onScroll: Event = this._onScroll.event; public isUserScrolling: boolean = false; constructor( public cols: number, From a6fdf043cedc1a09fefd675cb7246314ff2c9138 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 28 Aug 2025 08:57:25 -0700 Subject: [PATCH 7/7] Respect original formatting --- src/common/services/BufferService.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/common/services/BufferService.ts b/src/common/services/BufferService.ts index 616e5619..d8d8d6b6 100644 --- a/src/common/services/BufferService.ts +++ b/src/common/services/BufferService.ts @@ -151,5 +151,4 @@ export class BufferService extends Disposable implements IBufferService { this._onScroll.fire(buffer.ydisp); } } - }