Merge pull request #4195 from Tyriar/4190

Create onSpecificOptionChange and onMultipleOptionChange helpers

src/browser/Terminal.ts

@@ -264,50 +264,14 @@ export class Terminal extends CoreTerminal implements ITerminal {
     }
   }
 
-  protected _updateOptions(key: string): void {
-    super._updateOptions(key);
-
-    // TODO: These listeners should be owned by individual components
-    switch (key) {
-      case 'fontFamily':
-      case 'fontSize':
-        // When the font changes the size of the cells may change which requires a renderer clear
-        this._renderService?.clear();
-        this._charSizeService?.measure();
-        break;
-      case 'cursorBlink':
-      case 'cursorStyle':
-        // The DOM renderer needs a row refresh to update the cursor styles
-        this.refresh(this.buffer.y, this.buffer.y);
-        break;
-      case 'customGlyphs':
-      case 'drawBoldTextInBrightColors':
-      case 'letterSpacing':
-      case 'lineHeight':
-      case 'fontWeight':
-      case 'fontWeightBold':
-      case 'minimumContrastRatio':
-        // When the font changes the size of the cells may change which requires a renderer clear
-        if (this._renderService) {
-          this._renderService.clear();
-          this._renderService.handleResize(this.cols, this.rows);
-          this.refresh(0, this.rows - 1);
-        }
-        break;
-      case 'scrollback':
-        this.viewport?.syncScrollArea();
-        break;
-      case 'screenReaderMode':
-        if (this.optionsService.rawOptions.screenReaderMode) {
-          if (!this._accessibilityManager && this._renderService) {
-            this._accessibilityManager = new AccessibilityManager(this, this._renderService);
-          }
-        } else {
-          this._accessibilityManager?.dispose();
-          this._accessibilityManager = undefined;
-        }
-        break;
-      case 'tabStopWidth': this.buffers.setupTabStops(); break;
+  private _handleScreenReaderModeOptionChange(value: boolean): void {
+    if (value) {
+      if (!this._accessibilityManager && this._renderService) {
+        this._accessibilityManager = new AccessibilityManager(this, this._renderService);
+      }
+    } else {
+      this._accessibilityManager?.dispose();
+      this._accessibilityManager = undefined;
     }
   }
 
@@ -583,12 +547,13 @@ export class Terminal extends CoreTerminal implements ITerminal {
       // ensure the correct order of the dprchange event
       this._accessibilityManager = new AccessibilityManager(this, this._renderService);
     }
+    this.register(this.optionsService.onSpecificOptionChange('screenReaderMode', e => this._handleScreenReaderModeOptionChange(e)));
 
     if (this.options.overviewRulerWidth) {
       this._overviewRulerRenderer = this.register(this._instantiationService.createInstance(OverviewRulerRenderer, this._viewportElement, this.screenElement));
     }
-    this.optionsService.onOptionChange(() => {
-      if (!this._overviewRulerRenderer && this.options.overviewRulerWidth && this._viewportElement && this.screenElement) {
+    this.optionsService.onSpecificOptionChange('overviewRulerWidth', value => {
+      if (!this._overviewRulerRenderer && value && this._viewportElement && this.screenElement) {
         this._overviewRulerRenderer = this.register(this._instantiationService.createInstance(OverviewRulerRenderer, this._viewportElement, this.screenElement));
       }
     });

src/browser/Viewport.ts

@@ -75,6 +75,7 @@ export class Viewport extends Disposable implements IViewport {
 
     this._handleThemeChange(themeService.colors);
     this.register(themeService.onChangeColors(e => this._handleThemeChange(e)));
+    this.register(this._optionsService.onSpecificOptionChange('scrollback', () => this.syncScrollArea()));
 
     // Perform this async to ensure the ICharSizeService is ready.
     setTimeout(() => this.syncScrollArea(), 0);

src/browser/decorations/OverviewRulerRenderer.ts

@@ -110,15 +110,9 @@ export class OverviewRulerRenderer extends Disposable {
       }
     }));
     // overview ruler width changed
-    this.register(this._optionsService.onOptionChange(o => {
-      if (o === 'overviewRulerWidth') {
-        this._queueRefresh(true);
-      }
-    }));
+    this.register(this._optionsService.onSpecificOptionChange('overviewRulerWidth', () => this._queueRefresh(true)));
     // device pixel ratio changed
-    this.register(addDisposableDomListener(this._coreBrowseService.window, 'resize', () => {
-      this._queueRefresh(true);
-    }));
+    this.register(addDisposableDomListener(this._coreBrowseService.window, 'resize', () => this._queueRefresh(true)));
     // set the canvas dimensions
     this._queueRefresh(true);
   }

src/browser/renderer/dom/DomRenderer.ts

@@ -79,6 +79,7 @@ export class DomRenderer extends Disposable implements IRenderer {
       actualCellHeight: 0
     };
     this._updateDimensions();
+    this.register(this._optionsService.onOptionChange(() => this._handleOptionsChanged()));
 
     this.register(themeService.onChangeColors(e => this._injectCss(e)));
     this._injectCss(themeService.colors);
@@ -342,7 +343,7 @@ export class DomRenderer extends Disposable implements IRenderer {
     // No-op, the cursor is drawn when rows are drawn
   }
 
-  public handleOptionsChanged(): void {
+  private _handleOptionsChanged(): void {
     // Force a refresh
     this._updateDimensions();
   }

src/browser/renderer/shared/Types.d.ts

@@ -68,7 +68,6 @@ export interface IRenderer extends IDisposable {
   handleFocus(): void;
   handleSelectionChanged(start: [number, number] | undefined, end: [number, number] | undefined, columnSelectMode: boolean): void;
   handleCursorMove(): void;
-  handleOptionsChanged(): void;
   clear(): void;
   renderRows(start: number, end: number): void;
   clearTextureAtlas?(): void;

src/browser/services/CharSizeService.ts

@@ -7,6 +7,7 @@ import { IOptionsService } from 'common/services/Services';
 import { EventEmitter } from 'common/EventEmitter';
 import { ICharSizeService } from 'browser/services/Services';
 import { Disposable } from 'common/Lifecycle';
+import { ITerminalOptions } from 'common/Types';
 
 export class CharSizeService extends Disposable implements ICharSizeService {
   public serviceBrand: undefined;
@@ -27,6 +28,7 @@ export class CharSizeService extends Disposable implements ICharSizeService {
   ) {
     super();
     this._measureStrategy = new DomMeasureStrategy(document, parentElement, this._optionsService);
+    this.register(this._optionsService.onMultipleOptionChange(['fontFamily', 'fontSize'], () => this.measure()));
   }
 
   public measure(): void {

src/browser/services/RenderService.ts

@@ -84,8 +84,28 @@ export class RenderService extends Disposable implements IRenderService {
     this.register(decorationService.onDecorationRegistered(() => this._fullRefresh()));
     this.register(decorationService.onDecorationRemoved(() => this._fullRefresh()));
 
-    // No need to register this as renderer is explicitly disposed in RenderService.dispose
-    // this._renderer.onRequestRedraw(e => this.refreshRows(e.start, e.end, true));
+    // Clear the renderer when the a change that could affect glyphs occurs
+    this.register(optionsService.onMultipleOptionChange([
+      'customGlyphs',
+      'drawBoldTextInBrightColors',
+      'letterSpacing',
+      'lineHeight',
+      'fontFamily',
+      'fontSize',
+      'fontWeight',
+      'fontWeightBold',
+      'minimumContrastRatio'
+    ], () => {
+      this.clear();
+      this.handleResize(bufferService.cols, bufferService.rows);
+      this._fullRefresh();
+    }));
+
+    // Refresh the cursor line when the cursor changes
+    this.register(optionsService.onMultipleOptionChange([
+      'cursorBlink',
+      'cursorStyle'
+    ], () => this.refreshRows(bufferService.buffer.y, bufferService.buffer.y, true)));
 
     // dprchange should handle this case, we need this as well for browsers that don't support the
     // matchMedia query.
@@ -157,7 +177,6 @@ export class RenderService extends Disposable implements IRenderService {
     if (!this._renderer) {
       return;
     }
-    this._renderer.handleOptionsChanged();
     this.refreshRows(0, this._rowCount - 1);
     this._fireOnCanvasResize();
   }

src/browser/services/ThemeService.ts

@@ -111,16 +111,8 @@ export class ThemeService extends Disposable implements IThemeService {
     this._updateRestoreColors();
     this._setTheme(this._optionsService.rawOptions.theme);
 
-    this.register(this._optionsService.onOptionChange(key => {
-      switch (key) {
-        case 'minimumContrastRatio':
-          this._contrastCache.clear();
-          break;
-        case 'theme':
-          this._setTheme(this._optionsService.rawOptions.theme);
-          break;
-      }
-    }));
+    this.register(this._optionsService.onSpecificOptionChange('minimumContrastRatio', () => this._contrastCache.clear()));
+    this.register(this._optionsService.onSpecificOptionChange('theme', () => this._setTheme(this._optionsService.rawOptions.theme)));
   }
 
   /**

src/common/CoreTerminal.ts

@@ -130,7 +130,7 @@ export abstract class CoreTerminal extends Disposable implements ICoreTerminal {
     this.register(forwardEvent(this.coreService.onData, this._onData));
     this.register(forwardEvent(this.coreService.onBinary, this._onBinary));
     this.register(this.coreService.onUserInput(() =>  this._writeBuffer.handleUserInput()));
-    this.register(this.optionsService.onOptionChange(key => this._updateOptions(key)));
+    this.register(this.optionsService.onSpecificOptionChange('windowsMode', e => this._handleWindowsModeOptionChange(e)));
     this.register(this._bufferService.onScroll(event => {
       this._onScroll.fire({ position: this._bufferService.buffer.ydisp, source: ScrollSource.TERMINAL });
       this._inputHandler.markRangeDirty(this._bufferService.buffer.scrollTop, this._bufferService.buffer.scrollBottom);
@@ -261,20 +261,12 @@ export abstract class CoreTerminal extends Disposable implements ICoreTerminal {
     this.coreMouseService.reset();
   }
 
-  protected _updateOptions(key: string): void {
-    // TODO: These listeners should be owned by individual components
-    switch (key) {
-      case 'scrollback':
-        this.buffers.resize(this.cols, this.rows);
-        break;
-      case 'windowsMode':
-        if (this.optionsService.rawOptions.windowsMode) {
-          this._enableWindowsMode();
-        } else {
-          this._windowsMode?.dispose();
-          this._windowsMode = undefined;
-        }
-        break;
+  private _handleWindowsModeOptionChange(value: boolean): void {
+    if (value) {
+      this._enableWindowsMode();
+    } else {
+      this._windowsMode?.dispose();
+      this._windowsMode = undefined;
     }
   }
 

src/common/TestUtils.test.ts

@@ -9,7 +9,7 @@ import { clone } from 'common/Clone';
 import { DEFAULT_OPTIONS } from 'common/services/OptionsService';
 import { IBufferSet, IBuffer } from 'common/buffer/Types';
 import { BufferSet } from 'common/buffer/BufferSet';
-import { IDecPrivateModes, ICoreMouseEvent, CoreMouseEventType, ICharset, IModes, IAttributeData, IOscLinkData } from 'common/Types';
+import { IDecPrivateModes, ICoreMouseEvent, CoreMouseEventType, ICharset, IModes, IAttributeData, IOscLinkData, IDisposable } from 'common/Types';
 import { UnicodeV6 } from 'common/input/UnicodeV6';
 import { IDecorationOptions, IDecoration } from 'xterm';
 
@@ -113,7 +113,7 @@ export class MockOptionsService implements IOptionsService {
   public serviceBrand: any;
   public readonly rawOptions: Required<ITerminalOptions> = clone(DEFAULT_OPTIONS);
   public options: Required<ITerminalOptions> = this.rawOptions;
-  public onOptionChange: IEvent<string> = new EventEmitter<string>().event;
+  public onOptionChange: IEvent<keyof ITerminalOptions> = new EventEmitter<keyof ITerminalOptions>().event;
   constructor(testOptions?: Partial<ITerminalOptions>) {
     if (testOptions) {
       for (const key of Object.keys(testOptions)) {
@@ -121,6 +121,22 @@ export class MockOptionsService implements IOptionsService {
       }
     }
   }
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  public onSpecificOptionChange<T extends keyof ITerminalOptions>(key: T, listener: (arg1: ITerminalOptions[T]) => any): IDisposable {
+    return this.onOptionChange(eventKey => {
+      if (eventKey === key) {
+        listener(this.rawOptions[key]);
+      }
+    });
+  }
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  public onMultipleOptionChange(keys: (keyof ITerminalOptions)[], listener: () => any): IDisposable {
+    return this.onOptionChange(eventKey => {
+      if (keys.indexOf(eventKey) !== -1) {
+        listener();
+      }
+    });
+  }
   public setOptions(options: ITerminalOptions): void {
     for (const key of Object.keys(options)) {
       this.options[key] = options[key];

src/common/buffer/BufferSet.ts

@@ -32,6 +32,8 @@ export class BufferSet extends Disposable implements IBufferSet {
   ) {
     super();
     this.reset();
+    this.register(this._optionsService.onSpecificOptionChange('scrollback', () => this.resize(this._bufferService.cols, this._bufferService.rows)));
+    this.register(this._optionsService.onSpecificOptionChange('tabStopWidth', () => this.setupTabStops()));
   }
 
   public reset(): void {
@@ -119,6 +121,7 @@ export class BufferSet extends Disposable implements IBufferSet {
   public resize(newCols: number, newRows: number): void {
     this._normal.resize(newCols, newRows);
     this._alt.resize(newCols, newRows);
+    this.setupTabStops(newCols);
   }
 
   /**

src/common/services/BufferService.ts

@@ -43,7 +43,7 @@ export class BufferService extends Disposable implements IBufferService {
     this.cols = cols;
     this.rows = rows;
     this.buffers.resize(cols, rows);
-    this.buffers.setupTabStops(this.cols);
+    // TODO: This doesn't fire when scrollback changes - add a resize event to BufferSet and forward event
     this._onResize.fire({ cols, rows });
   }
 

src/common/services/LogService.ts

@@ -40,11 +40,7 @@ export class LogService extends Disposable implements ILogService {
   ) {
     super();
     this._updateLogLevel();
-    this.register(this._optionsService.onOptionChange(key => {
-      if (key === 'logLevel') {
-        this._updateLogLevel();
-      }
-    }));
+    this.register(this._optionsService.onSpecificOptionChange('logLevel', () => this._updateLogLevel()));
   }
 
   private _updateLogLevel(): void {

src/common/services/OptionsService.test.ts

@@ -5,6 +5,7 @@
 
 import { assert } from 'chai';
 import { OptionsService, DEFAULT_OPTIONS } from 'common/services/OptionsService';
+import { IDisposable } from 'common/Types';
 
 describe('OptionsService', () => {
   describe('constructor', () => {
@@ -71,4 +72,79 @@ describe('OptionsService', () => {
       assert.equal(service.options.fontWeight, DEFAULT_OPTIONS.fontWeight, 'Wrong string literals should be reset to default');
     });
   });
+  describe('onOptionChange', () => {
+    let service: OptionsService;
+    beforeEach(() => {
+      service = new OptionsService({});
+    });
+    it('should fire on any option change', async () => {
+      let disposable: IDisposable;
+      await new Promise<void>(r => {
+        disposable = service.onOptionChange(e => {
+          assert.strictEqual(e, 'cursorWidth');
+          r();
+        });
+        service.options.cursorWidth = 10;
+      });
+      disposable!.dispose();
+      await new Promise<void>(r => {
+        service.onOptionChange(e => {
+          assert.strictEqual(e, 'scrollback');
+          r();
+        });
+        service.options.scrollback = 20;
+      });
+    });
+  });
+  describe('onSpecificOptionChange', () => {
+    let service: OptionsService;
+    beforeEach(() => {
+      service = new OptionsService({});
+    });
+    it('should fire only on a specific option change', async () => {
+      await new Promise<void>(r => {
+        service.onSpecificOptionChange('scrollback', e => {
+          assert.strictEqual(e, 20);
+          r();
+        });
+        service.options.cursorWidth = 10;
+        service.options.scrollback = 20;
+      });
+    });
+  });
+  describe('onSpecificOptionChange', () => {
+    let service: OptionsService;
+    beforeEach(() => {
+      service = new OptionsService({});
+    });
+    it('should fire only on a specific option change', async () => {
+      await new Promise<void>(r => {
+        service.onSpecificOptionChange('scrollback', e => {
+          assert.strictEqual(e, 20);
+          r();
+        });
+        service.options.cursorWidth = 10;
+        service.options.scrollback = 20;
+      });
+    });
+  });
+  describe('onMultipleOptionChange', () => {
+    let service: OptionsService;
+    beforeEach(() => {
+      service = new OptionsService({});
+    });
+    it('should fire only for specific options', async () => {
+      await new Promise<void>(r => {
+        let called = false;
+        service.onMultipleOptionChange(['scrollback'], () => {
+          called = true;
+        });
+        service.options.cursorWidth = 10;
+        assert.notOk(called);
+        service.options.scrollback = 20;
+        assert.ok(called);
+        r();
+      });
+    });
+  });
 });

src/common/services/OptionsService.ts

@@ -6,7 +6,7 @@
 import { IOptionsService, ITerminalOptions, FontWeight } from 'common/services/Services';
 import { EventEmitter, IEvent } from 'common/EventEmitter';
 import { isMac } from 'common/Platform';
-import { CursorStyle } from 'common/Types';
+import { CursorStyle, IDisposable } from 'common/Types';
 import { Disposable } from 'common/Lifecycle';
 
 export const DEFAULT_OPTIONS: Readonly<Required<ITerminalOptions>> = {
@@ -58,7 +58,7 @@ export class OptionsService extends Disposable implements IOptionsService {
   public readonly rawOptions: Required<ITerminalOptions>;
   public options: Required<ITerminalOptions>;
 
-  private readonly _onOptionChange = this.register(new EventEmitter<string>());
+  private readonly _onOptionChange = this.register(new EventEmitter<keyof ITerminalOptions>());
   public readonly onOptionChange = this._onOptionChange.event;
 
   constructor(options: Partial<ITerminalOptions>) {
@@ -82,6 +82,24 @@ export class OptionsService extends Disposable implements IOptionsService {
     this._setupOptions();
   }
 
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  public onSpecificOptionChange<T extends keyof ITerminalOptions>(key: T, listener: (value: ITerminalOptions[T]) => any): IDisposable {
+    return this.onOptionChange(eventKey => {
+      if (eventKey === key) {
+        listener(this.rawOptions[key]);
+      }
+    });
+  }
+
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  public onMultipleOptionChange(keys: (keyof ITerminalOptions)[], listener: () => any): IDisposable {
+    return this.onOptionChange(eventKey => {
+      if (keys.indexOf(eventKey) !== -1) {
+        listener();
+      }
+    });
+  }
+
   private _setupOptions(): void {
     const getter = (propName: string): any => {
       if (!(propName in DEFAULT_OPTIONS)) {

src/common/services/Services.ts

@@ -182,9 +182,33 @@ export interface IOptionsService {
    * internally.
    */
   readonly rawOptions: Required<ITerminalOptions>;
+
+  /**
+   * Options as exposed through the public API, this property uses getters and setters with
+   * validation which makes it safer but slower. {@link rawOptions} should be used for pretty much
+   * all internal usage for performance reasons.
+   */
   readonly options: Required<ITerminalOptions>;
 
-  readonly onOptionChange: IEvent<string>;
+  /**
+   * Adds an event listener for when any option changes.
+   */
+  readonly onOptionChange: IEvent<keyof ITerminalOptions>;
+
+  /**
+   * Adds an event listener for when a specific option changes, this is a convenience method that is
+   * preferred over {@link onOptionChange} when only a single option is being listened to.
+   */
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  onSpecificOptionChange<T extends keyof ITerminalOptions>(key: T, listener: (arg1: Required<ITerminalOptions>[T]) => any): IDisposable;
+
+  /**
+   * Adds an event listener for when a set of specific options change, this is a convenience method
+   * that is preferred over {@link onOptionChange} when multiple options are being listened to and
+   * handled the same way.
+   */
+  // eslint-disable-next-line @typescript-eslint/naming-convention
+  onMultipleOptionChange(keys: (keyof ITerminalOptions)[], listener: () => any): IDisposable;
 }
 
 export type FontWeight = 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900' | number;

src/headless/Terminal.ts

@@ -78,15 +78,6 @@ export class Terminal extends CoreTerminal {
     return this.buffers.active;
   }
 
-  protected _updateOptions(key: string): void {
-    super._updateOptions(key);
-
-    // TODO: These listeners should be owned by individual components
-    switch (key) {
-      case 'tabStopWidth': this.buffers.setupTabStops(); break;
-    }
-  }
-
   // TODO: Support paste here?
 
   public get markers(): IMarker[] {