Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose specific option change listener on options service #4190

Closed
Tyriar opened this issue Oct 8, 2022 · 0 comments · Fixed by #4195
Closed

Expose specific option change listener on options service #4190

Tyriar opened this issue Oct 8, 2022 · 0 comments · Fixed by #4195
Assignees
Labels
type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Oct 8, 2022

Currently we only have the onOptionChange event:

this._onOptionChange.fire(propName);

Which leads to checking the options on the other side, this is especially ugly in the larger files:

// 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;
case 'theme':
this._setTheme(this.optionsService.rawOptions.theme);
break;
}
}
/**
* Binds the desired focus behavior on a given terminal object.

This sort of thing would encapsulate this repetitive pattern inside options service:

IOptionsService.onSpecificOptionChange(key: 'x' | 'y' | ..., listener: () => void
@Tyriar Tyriar added the type/debt Technical debt that could slow us down in the long run label Oct 8, 2022
@Tyriar Tyriar self-assigned this Oct 9, 2022
@Tyriar Tyriar added this to the 5.1.0 milestone Oct 9, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/debt Technical debt that could slow us down in the long run
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant