fix(terminal): support decimal font sizes #286212
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #286194 where terminal.integrated.fontSize truncates decimal values. The fix replaces parseInt() with parseFloat() for font size processing, making terminal font size behavior consistent with editor.fontSize.
Key Changes:
- Added
clampFloat()helper function to handle decimal font sizes - Updated font size processing to use
clampFloat()instead ofclampInt() - Added three unit tests verifying decimal font size preservation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/terminalConfigurationService.ts | Added clampFloat() function and updated font size clamping calls to preserve decimal values |
| src/vs/workbench/contrib/terminal/test/browser/terminalConfigurationService.test.ts | Added three tests verifying decimal font sizes work correctly, including Ubuntu-specific adjustment logic |
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 13.25, 'terminal.integrated.fontSize should preserve decimal values like 13.25'); | ||
| }); | ||
|
|
||
| test('fontSize decimal (Linux Ubuntu)', () => { |
There was a problem hiding this comment.
The test name 'fontSize decimal (Linux Ubuntu)' doesn't follow the established pattern of other tests. It should specify the actual font size value like 'fontSize 10.5 (Linux Ubuntu)' to match the convention used in tests like 'fontSize 0' and 'fontFamily (Linux Ubuntu)'.
| test('fontSize decimal (Linux Ubuntu)', () => { | |
| test('fontSize 10.5 (Linux Ubuntu)', () => { |
| test('fontSize 11.5 (decimal)', () => { | ||
| const terminalConfigurationService = createTerminalConfigationService({ | ||
| editor: { | ||
| fontFamily: 'foo', | ||
| fontSize: 9 | ||
| }, | ||
| terminal: { | ||
| integrated: { | ||
| fontFamily: 'bar', | ||
| fontSize: 11.5 | ||
| } | ||
| } | ||
| }); | ||
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 11.5, 'terminal.integrated.fontSize should preserve decimal values'); | ||
| }); | ||
|
|
||
| test('fontSize 13.25 (decimal)', () => { | ||
| const terminalConfigurationService = createTerminalConfigationService({ | ||
| editor: { | ||
| fontFamily: 'foo', | ||
| fontSize: 9 | ||
| }, | ||
| terminal: { | ||
| integrated: { | ||
| fontFamily: 'bar', | ||
| fontSize: 13.25 | ||
| } | ||
| } | ||
| }); | ||
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 13.25, 'terminal.integrated.fontSize should preserve decimal values like 13.25'); | ||
| }); | ||
|
|
||
| test('fontSize decimal (Linux Ubuntu)', () => { | ||
| const terminalConfigurationService = createTerminalConfigationService({ | ||
| editor: { | ||
| fontFamily: 'foo' | ||
| }, | ||
| terminal: { | ||
| integrated: { | ||
| fontFamily: null, | ||
| fontSize: 10.5 | ||
| } | ||
| } | ||
| }, LinuxDistro.Ubuntu); | ||
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 12.5, 'Ubuntu font size adjustment should preserve decimal values (10.5 + 2 = 12.5)'); | ||
| }); |
There was a problem hiding this comment.
These three test cases test essentially the same behavior with slightly different decimal values. Consider consolidating the first two tests (11.5 and 13.25) into a single test that validates decimal preservation with one representative value, keeping only the Ubuntu-specific test separate as it tests the adjustment logic. This would reduce redundancy while maintaining adequate coverage.
| test('fontSize 11.5 (decimal)', () => { | ||
| const terminalConfigurationService = createTerminalConfigationService({ | ||
| editor: { | ||
| fontFamily: 'foo', | ||
| fontSize: 9 | ||
| }, | ||
| terminal: { | ||
| integrated: { | ||
| fontFamily: 'bar', | ||
| fontSize: 11.5 | ||
| } | ||
| } | ||
| }); | ||
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 11.5, 'terminal.integrated.fontSize should preserve decimal values'); | ||
| }); | ||
|
|
||
| test('fontSize 13.25 (decimal)', () => { | ||
| const terminalConfigurationService = createTerminalConfigationService({ | ||
| editor: { | ||
| fontFamily: 'foo', | ||
| fontSize: 9 | ||
| }, | ||
| terminal: { | ||
| integrated: { | ||
| fontFamily: 'bar', | ||
| fontSize: 13.25 | ||
| } | ||
| } | ||
| }); | ||
| strictEqual(terminalConfigurationService.getFont(getActiveWindow()).fontSize, 13.25, 'terminal.integrated.fontSize should preserve decimal values like 13.25'); | ||
| }); |
There was a problem hiding this comment.
The test names should be more consistent with the existing pattern in the test suite. Tests like 'fontSize 10' don't include clarifying suffixes. Consider removing '(decimal)' from the first two test names since the value itself clearly indicates it's a decimal, leaving just 'fontSize 11.5' and 'fontSize 13.25' to match the existing convention.
3 participants
Fixes #286194
Problem
terminal.integrated.fontSizetruncates decimal values. Setting11.25results in11px, whileeditor.fontSizecorrectly preserves decimals.Solution
Replaced
parseInt()withparseFloat()in terminal font size processing by adding aclampFloat()helper function.Testing
vscodeterm.mp4