Skip to content

Conversation

@silamon
Copy link
Contributor

@silamon silamon commented Jul 30, 2022

This branch helps towards #3948. Keeping terminal.rows/terminal.cols and terminal.options.rows/terminal.options.cols in sync shouldn't be a problem then.

Also aligns xterm with xterm-headless after #3960.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also aligns xterm with xterm-headless after #3960.

Opps 🤦

Comment on lines 1403 to 1417
describe('options', () => {
beforeEach(async () => {
term = new TestTerminal({});
});
it('get options', () => {
assert.equal(term.options.cols, 80);
assert.equal(term.options.rows, 24);
});
it('set options', async () => {
term.options.cols = 40;
assert.equal(term.options.cols, 40);
term.options.rows = 20;
assert.equal(term.options.rows, 20);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll still want to keep these tests, just change to use different options

Comment on lines 39 to 40
this.cols = Math.max(80 || 0, MINIMUM_COLS);
this.rows = Math.max(24 || 0, MINIMUM_ROWS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the initial dimensions need to be passed into the ctor now?

@Tyriar Tyriar added this to the 5.0.0 milestone Jul 30, 2022
@silamon
Copy link
Contributor Author

silamon commented Jul 30, 2022

Removing cols/rows from ITerminalOptions isn't going to work. Closing this PR and we will have to keep rows and cols in the internal ITerminalOptions.

@silamon silamon closed this Jul 30, 2022
@Tyriar Tyriar removed this from the 5.0.0 milestone Sep 15, 2022
@silamon silamon deleted the rowscolsremoval branch December 25, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants