From ebd61ccd4fba738555f3e5d213f347d229896aa8 Mon Sep 17 00:00:00 2001 From: rebornix Date: Wed, 30 Aug 2017 15:43:30 -0700 Subject: [PATCH] Fix #33344. Keep Hue value even when Saturation is 0, otherwise converting color between RGBA/HSLA/HSVA will lead to the loss of Hue. Since we bring back Hue value properly, https://github.com/Microsoft/vscode/commit/23bd5fee0fcc3272b7c588d094987bd93702f6cf needs to be reverted, our old tests are good. --- src/vs/base/common/color.ts | 22 ++++++++++++++++--- src/vs/base/test/common/color.test.ts | 12 +++++++--- .../colorPicker/browser/colorPickerWidget.ts | 12 +++++++--- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/vs/base/common/color.ts b/src/vs/base/common/color.ts index 217dc4b426e..d650dc0d92d 100644 --- a/src/vs/base/common/color.ts +++ b/src/vs/base/common/color.ts @@ -260,8 +260,22 @@ export class Color { } readonly rgba: RGBA; - get hsla(): HSLA { return HSLA.fromRGBA(this.rgba); } - get hsva(): HSVA { return HSVA.fromRGBA(this.rgba); } + private _hsla: HSLA; + get hsla(): HSLA { + if (this._hsla) { + return this._hsla; + } else { + return HSLA.fromRGBA(this.rgba); + } + } + + private _hsva: HSVA; + get hsva(): HSVA { + if (this._hsva) { + return this._hsva; + } + return HSVA.fromRGBA(this.rgba); + } constructor(arg: RGBA | HSLA | HSVA) { if (!arg) { @@ -269,8 +283,10 @@ export class Color { } else if (arg instanceof RGBA) { this.rgba = arg; } else if (arg instanceof HSLA) { + this._hsla = arg; this.rgba = HSLA.toRGBA(arg); } else if (arg instanceof HSVA) { + this._hsva = arg; this.rgba = HSVA.toRGBA(arg); } else { throw new Error('Invalid color ctor argument'); @@ -278,7 +294,7 @@ export class Color { } equals(other: Color): boolean { - return !!other && RGBA.equals(this.rgba, other.rgba); + return !!other && RGBA.equals(this.rgba, other.rgba) && HSLA.equals(this.hsla, other.hsla) && HSVA.equals(this.hsva, other.hsva); } /** diff --git a/src/vs/base/test/common/color.test.ts b/src/vs/base/test/common/color.test.ts index a76c98a0333..7ccee7a4d51 100644 --- a/src/vs/base/test/common/color.test.ts +++ b/src/vs/base/test/common/color.test.ts @@ -23,9 +23,9 @@ suite('Color', () => { let color1 = new Color(new HSLA(60, 1, 0.5, 1)), color2 = new Color(new HSLA(0, 0, 0.753, 1)); assert.deepEqual(color1.hsla, Color.getLighterColor(color1, color2).hsla); - assert.deepEqual(new HSLA(0, 0, 0.918, 1), Color.getLighterColor(color2, color1).hsla); + assert.deepEqual(new HSLA(0, 0, 0.916, 1), Color.getLighterColor(color2, color1).hsla); assert.deepEqual(new HSLA(0, 0, 0.851, 1), Color.getLighterColor(color2, color1, 0.3).hsla); - assert.deepEqual(new HSLA(0, 0, 0.980, 1), Color.getLighterColor(color2, color1, 0.7).hsla); + assert.deepEqual(new HSLA(0, 0, 0.981, 1), Color.getLighterColor(color2, color1, 0.7).hsla); assert.deepEqual(new HSLA(0, 0, 1, 1), Color.getLighterColor(color2, color1, 1).hsla); }); @@ -47,7 +47,7 @@ suite('Color', () => { assert.deepEqual(new HSLA(60, 1, 0.284, 1), Color.getDarkerColor(color1, color2, 1).hsla); // Abyss theme - assert.deepEqual(new HSLA(355, 0.875, 0.157, 1), Color.getDarkerColor(Color.fromHex('#770811'), Color.fromHex('#000c18'), 0.4).hsla); + assert.deepEqual(new HSLA(355, 0.874, 0.157, 1), Color.getDarkerColor(Color.fromHex('#770811'), Color.fromHex('#000c18'), 0.4).hsla); }); test('luminance', function () { @@ -179,6 +179,12 @@ suite('Color', () => { assert.deepEqual(HSVA.fromRGBA(new RGBA(0, 128, 128, 1)), new HSVA(180, 1, 0.502, 1)); assert.deepEqual(HSVA.fromRGBA(new RGBA(0, 0, 128, 1)), new HSVA(240, 1, 0.502, 1)); }); + + test('Keep hue value when saturation is 0', () => { + assert.deepEqual(HSVA.toRGBA(new HSVA(10, 0, 0, 0)), HSVA.toRGBA(new HSVA(20, 0, 0, 0))); + assert.deepEqual(new Color(new HSVA(10, 0, 0, 0)).rgba, new Color(new HSVA(20, 0, 0, 0)).rgba); + assert.notDeepEqual(new Color(new HSVA(10, 0, 0, 0)).hsva, new Color(new HSVA(20, 0, 0, 0)).hsva); + }); }); suite('Format', () => { diff --git a/src/vs/editor/contrib/colorPicker/browser/colorPickerWidget.ts b/src/vs/editor/contrib/colorPicker/browser/colorPickerWidget.ts index 91eb124b905..9e416c5cac3 100644 --- a/src/vs/editor/contrib/colorPicker/browser/colorPickerWidget.ts +++ b/src/vs/editor/contrib/colorPicker/browser/colorPickerWidget.ts @@ -129,6 +129,7 @@ class SaturationBox extends Disposable { private width: number; private height: number; + private monitor: GlobalMouseMoveMonitor; private _onDidChange = new Emitter<{ s: number, v: number }>(); readonly onDidChange: Event<{ s: number, v: number }> = this._onDidChange.event; @@ -154,22 +155,24 @@ class SaturationBox extends Disposable { this._register(dom.addDisposableListener(this.domNode, dom.EventType.MOUSE_DOWN, e => this.onMouseDown(e))); this._register(this.model.onDidChangeColor(this.onDidChangeColor, this)); + this.monitor = null; } private onMouseDown(e: MouseEvent): void { - const monitor = this._register(new GlobalMouseMoveMonitor()); + this.monitor = this._register(new GlobalMouseMoveMonitor()); const origin = dom.getDomNodePagePosition(this.domNode); if (e.target !== this.selection) { this.onDidChangePosition(e.offsetX, e.offsetY); } - monitor.startMonitoring(standardMouseMoveMerger, event => this.onDidChangePosition(event.posx - origin.left, event.posy - origin.top), () => null); + this.monitor.startMonitoring(standardMouseMoveMerger, event => this.onDidChangePosition(event.posx - origin.left, event.posy - origin.top), () => null); const mouseUpListener = dom.addDisposableListener(document, dom.EventType.MOUSE_UP, () => { this._onColorFlushed.fire(); mouseUpListener.dispose(); - monitor.stopMonitoring(true); + this.monitor.stopMonitoring(true); + this.monitor = null; }, true); } @@ -221,6 +224,9 @@ class SaturationBox extends Disposable { } private onDidChangeColor(): void { + if (this.monitor && this.monitor.isMonitoring()) { + return; + } this.paint(); } }