scm: allow higher diff decoration gutter widths#297032
scm: allow higher diff decoration gutter widths#297032skdas20 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @lszomoruMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR expands the scm.diffDecorationsGutterWidth setting to support larger gutter decoration widths (up to 20px) and centralizes runtime normalization in the SCM quick diff implementation, with accompanying unit tests.
Changes:
- Updated the setting schema from an enum (1–5) to a numeric range (1–20).
- Introduced
normalizeDiffDecorationsGutterWidth()and routed the controller logic through it. - Added unit tests for normalization behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/scm/browser/scm.contribution.ts |
Broadens the configuration schema to allow larger gutter widths. |
src/vs/workbench/contrib/scm/browser/quickDiffDecorator.ts |
Adds a normalization helper and uses it when applying configuration changes. |
src/vs/workbench/contrib/scm/test/browser/quickDiffDecorator.test.ts |
Adds tests for the new normalization helper. |
| if (isNaN(width) || width <= 0 || width > MAX_DIFF_DECORATIONS_GUTTER_WIDTH) { | ||
| return DEFAULT_DIFF_DECORATIONS_GUTTER_WIDTH; | ||
| } | ||
|
|
There was a problem hiding this comment.
normalizeDiffDecorationsGutterWidth currently accepts non-integer values (eg 1.5) and will also pass through non-number values that coerce cleanly (eg '3'), despite the function’s return type being number. To keep runtime validation aligned with the schema, consider validating typeof width === 'number', Number.isFinite(width), and Number.isInteger(width), and treating values < 1 or > MAX_DIFF_DECORATIONS_GUTTER_WIDTH as invalid.
| if (isNaN(width) || width <= 0 || width > MAX_DIFF_DECORATIONS_GUTTER_WIDTH) { | |
| return DEFAULT_DIFF_DECORATIONS_GUTTER_WIDTH; | |
| } | |
| if (typeof width !== 'number' || !Number.isFinite(width) || !Number.isInteger(width)) { | |
| return DEFAULT_DIFF_DECORATIONS_GUTTER_WIDTH; | |
| } | |
| if (width < 1 || width > MAX_DIFF_DECORATIONS_GUTTER_WIDTH) { | |
| return DEFAULT_DIFF_DECORATIONS_GUTTER_WIDTH; | |
| } |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(0), 3); | ||
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(-1), 3); | ||
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(21), 3); | ||
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(Number.NaN), 3); |
There was a problem hiding this comment.
The new tests cover basic bounds and NaN, but they don’t exercise the runtime-validation cases that are most likely to regress (eg fractional values like 1.5, Infinity, or non-number values coming from a malformed settings JSON). Adding a couple of assertions for these cases would help ensure normalizeDiffDecorationsGutterWidth stays aligned with the intended schema constraints.
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(Number.NaN), 3); | |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(Number.NaN), 3); | |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(1.5), 3); | |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(Number.POSITIVE_INFINITY), 3); | |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth(Number.NEGATIVE_INFINITY), 3); | |
| assert.strictEqual(normalizeDiffDecorationsGutterWidth('3' as any), 3); |
| 'scm.diffDecorationsGutterWidth': { | ||
| type: 'number', | ||
| enum: [1, 2, 3, 4, 5], | ||
| minimum: 1, | ||
| maximum: 20, | ||
| default: 3, | ||
| description: localize('diffGutterWidth', "Controls the width(px) of diff decorations in gutter (added & modified).") | ||
| }, |
There was a problem hiding this comment.
The setting describes a pixel width, but the schema still uses type: 'number', which allows fractional values (eg 1.5). Consider switching this to type: 'integer' (or adding multipleOf: 1) so the Settings UI and JSON validation match the intended unit and avoid surprising fractional gutter widths.
There was a problem hiding this comment.
Great catch, thanks. Updated this to enforce integer-only widths:
- Changed
scm.diffDecorationsGutterWidthschema type tointeger(still bounded to 1..20). - Updated runtime normalization to reject non-integer values.
- Added a unit test to cover fractional input (
1.5) falling back to default.
This is now in commit 699c9ad.
3 participants
Fixes #278083
This updates
scm.diffDecorationsGutterWidthto allow larger values in settings while keeping runtime validation centralized.What changed
1..5) to a numeric range (minimum: 1,maximum: 20).normalizeDiffDecorationsGutterWidth()in quick diff logic.Files
src/vs/workbench/contrib/scm/browser/scm.contribution.tssrc/vs/workbench/contrib/scm/browser/quickDiffDecorator.tssrc/vs/workbench/contrib/scm/test/browser/quickDiffDecorator.test.ts