fix: correct hsl saturation and hue#94
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough此次变更在 FastColor 中引入独立的 HSL/HSV 饱和度缓存与懒加载计算（ Changes
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 需要重点检查的区域：
诗句
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @XGHeaven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the accuracy of HSL and HSV color model conversions by addressing discrepancies in saturation calculations. It introduces distinct methods for HSL and HSV saturation, deprecating a generic one, and updates all relevant conversion functions to use these new, precise calculations. Additionally, it includes minor fixes in the HSL parsing logic and a new, thorough test suite to ensure the correctness and robustness of these color transformations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
您好，感谢您对 hsl 色彩模型的修正。将饱和度计算拆分为 getHSLSaturation 和 getHSVSaturation 是一个很好的改进，因为它明确了 HSL 和 HSV 两种模型下饱和度的不同计算方式。同时，新增的 hsl.test.ts 测试文件也大大提高了代码的健壮性。
在 review 过程中，我发现了一些可以改进的地方，主要有两点：
- 关键问题：
getHSLSaturation和getHSVSaturation共用了同一个私有缓存属性_s，这会导致在交替调用这两个方法时返回错误的饱和度值。我已经在相关代码行提出了具体的修改建议，主要是将缓存属性拆分开。 fromHsl方法在处理负数色相（hue）值时存在一个 bug，会导致颜色转换不正确。我也提出了相应的修复建议。
另外，还有一个小发现：darken 和 lighten 方法内部调用了 getSaturation()，根据您这次的修改，它现在会返回 HSV 的饱和度。但 darken/lighten 是 HSL 模型下的操作，它们后续创建新颜色时需要的是 HSL 饱和度。这可能会导致这两个方法产生非预期的颜色结果。由于这部分代码不在本次 diff 范围内，仅作为提醒供您参考。
总的来说，这是一个很棒的修复，解决了重要的问题。期待您后续的更新！
| getHSLSaturation(): number { | ||
| if (typeof this._s === 'undefined') { | ||
| const delta = this.getMax() - this.getMin(); | ||
| if (delta === 0) { | ||
| this._s = 0; | ||
| } else { | ||
| const l = this.getLightness(); | ||
| this._s = (delta/255) / (1 - Math.abs(2 * l - 1)); | ||
| } | ||
| } | ||
| return this._s; | ||
| } |
There was a problem hiding this comment.
getHSLSaturation 和 getHSVSaturation 方法都使用 this._s 来缓存饱和度值。但 HSL 和 HSV 的饱和度计算方式不同，共用一个缓存属性会导致计算错误。
例如，当一个颜色先调用 getHSVSaturation()，this._s 会被设置为 HSV 的饱和度。之后再调用 getHSLSaturation()，它会直接返回缓存的 this._s 值，也就是错误的 HSV 饱和度值。
建议为 HSL 和 HSV 的饱和度使用不同的私有缓存属性，例如 _hsl_s 和 _hsv_s。
这需要修改以下几个地方：
- 在类属性定义中，将
_s拆分为_hsl_s和_hsv_s。 getHSLSaturation方法使用_hsl_s进行缓存。getHSVSaturation方法使用_hsv_s进行缓存。fromHsl方法设置_hsl_s。fromHsv方法设置_hsv_s。constructor中克隆FastColor实例时，需要同时复制_hsl_s和_hsv_s。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/FastColor.ts (2)
237-247: 关键问题：HSV 和 HSL 饱和度共享缓存变量。
getHSVSaturation()和getHSLSaturation()都使用相同的_s缓存变量（第 238 行和第 250 行），这会导致严重的缓存冲突。如果先调用getHSLSaturation()再调用getHSVSaturation()（或反之），第二次调用将返回错误的缓存值而不是重新计算。示例问题场景：
const color = new FastColor('#3d5eff'); const hslS = color.getHSLSaturation(); // 计算并缓存 HSL 饱和度到 _s const hsvS = color.getHSVSaturation(); // 错误地返回 HSL 饱和度，因为 _s 已定义建议为 HSV 和 HSL 饱和度使用独立的缓存变量：
// HSV privates private _h?: number; - private _s?: number; + private _hsv_s?: number; + private _hsl_s?: number; private _l?: number; private _v?: number;然后更新
getHSVSaturation()和getHSLSaturation()使用各自的缓存变量。
290-308: 关键问题：darken 和 lighten 方法使用了错误的饱和度类型。
darken()和lighten()方法在第 292 行和第 302 行调用getSaturation()（返回 HSV 饱和度），然后在第 297 行和第 307 行使用{ h, s, l, a }创建新颜色。构造函数会将此对象识别为 HSL 格式（第 156 行的matchFormat('hsl')检查），但s值实际上是 HSV 饱和度，这会导致颜色转换错误。应用此差异以使用正确的 HSL 饱和度：
darken(amount = 10) { const h = this.getHue(); - const s = this.getSaturation(); + const s = this.getHSLSaturation(); let l = this.getLightness() - amount / 100; if (l < 0) { l = 0; } return this._c({ h, s, l, a: this.a }); } lighten(amount = 10) { const h = this.getHue(); - const s = this.getSaturation(); + const s = this.getHSLSaturation(); let l = this.getLightness() + amount / 100; if (l > 1) { l = 1; } return this._c({ h, s, l, a: this.a }); }
♻️ Duplicate comments (2)
src/FastColor.ts (2)
249-260: HSL 饱和度公式正确，但存在缓存冲突。HSL 饱和度的计算公式
(delta/255) / (1 - Math.abs(2 * l - 1))正确实现了 issue #93 中描述的修复。然而，与getHSVSaturation()共享_s缓存变量会导致缓存冲突（已在前面的评论中标记）。
504-508: 相关：饱和度缓存冲突也影响 fromHsl 和 fromHsv。第 506 行缓存 HSL 饱和度到
_s，而fromHsv()（第 554 行）缓存 HSV 饱和度到同一个_s变量。这加剧了之前标记的缓存冲突问题。示例问题：
const color = new FastColor({ h: 230, s: 1, l: 0.5 }); // HSL - _s 缓存 HSL 饱和度 const hsvS = color.getHSVSaturation(); // 错误地返回 HSL 饱和度这与
getHSVSaturation()和getHSLSaturation()中已标记的缓存冲突是同一个问题。解决方案是为 HSV 和 HSL 饱和度使用独立的缓存变量。
🧹 Nitpick comments (1)
tests/hsl.test.ts (1)
39-45: 简化冗余的断言。第 41 行将
base.getLightness()与相同颜色的新实例进行比较，这个断言是冗余的。可以直接移除此行，因为第 44 行已经验证了setHue保持亮度不变的核心行为。应用此差异来简化测试：
it('setHue should not change lightness', () => { const base = new FastColor('#1677ff'); - expect(base.getLightness()).toBeCloseTo(new FastColor('#1677ff').getLightness(), 4); const turn = base.setHue(233); expect(turn.getLightness()).toBeCloseTo(base.getLightness(), 4); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/FastColor.ts(7 hunks)tests/hsl.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/hsl.test.ts (1)
src/FastColor.ts (1)
FastColor(61-636)
🔇 Additional comments (11)
tests/hsl.test.ts (6)
26-37: 测试逻辑正确。hex 到 HSL 的转换测试和往返测试覆盖全面，正确使用了
toBeCloseTo处理浮点数比较。
47-57: LGTM！HSLA 字符串解析和透明度处理的测试覆盖良好，正确验证了字符串标准化和透明度提取。
59-63: LGTM！色相 0 和 360 等价性测试很好地验证了色相环绕行为，这是 HSL 色彩模型的重要边界情况。
65-69: LGTM！饱和度为 0 时的灰度测试正确验证了当饱和度为零时色相被忽略的行为。
71-89: LGTM！往返字符串测试和 darken/lighten 测试覆盖全面，正确验证了亮度调整和边界裁剪行为（[0, 1] 范围）。
22-22: 移除重复的测试夹具。行 20 和行 22 包含相同的测试用例
{ hex: '#3d5dff', hsl: { h: 230, s: 1, l: 0.6196 } }。请删除其中一个重复项。Likely an incorrect or invalid review comment.
src/FastColor.ts (5)
230-235: 向后兼容的弃用处理正确。
getSaturation()正确地委托给getHSVSaturation()以保持向后兼容性，并通过 JSDoc 注释提示开发者使用更明确的方法。
404-411: LGTM！
toHsl()正确使用getHSLSaturation()返回 HSL 色彩空间的饱和度值，符合修复目标。
414-422: LGTM！
toHslString()正确使用getHSLSaturation()生成 HSL/HSLA 字符串，确保输出的饱和度百分比与 HSL 色彩模型一致。
425-432: LGTM！
toHsv()正确使用getHSVSaturation()返回 HSV 色彩空间的饱和度值，使色彩空间的区分更加明确。
504-516: LGTM！
fromHsl()的改进很好：
- 第 505 行：使用
h = h % 360标准化色相值，同时缓存到_h，这是一个优雅的优化。- 第 515 行：当饱和度 ≤ 0 时提前返回，避免后续不必要的计算，提高了灰度颜色的处理效率。
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FastColor.ts (1)
292-310: 严重：darken 和 lighten 使用了错误的饱和度类型！
darken()和lighten()方法调用getSaturation()（返回 HSV 饱和度），但随后用{ h, s, l, a }构造颜色对象，该对象会被路由到fromHsl，后者期望接收 HSL 饱和度。这导致饱和度类型不匹配，产生错误的颜色计算结果。虽然这是旧代码中已存在的问题（旧
getSaturation也使用 HSV 公式），但在修正饱和度计算的 PR 中应一并修复。🔎 应用此 diff 修复饱和度类型错误：
darken(amount = 10) { const h = this.getHue(); - const s = this.getSaturation(); + const s = this.getHSLSaturation(); let l = this.getLightness() - amount / 100; if (l < 0) { l = 0; } return this._c({ h, s, l, a: this.a }); } lighten(amount = 10) { const h = this.getHue(); - const s = this.getSaturation(); + const s = this.getHSLSaturation(); let l = this.getLightness() + amount / 100; if (l > 1) { l = 1; } return this._c({ h, s, l, a: this.a }); }
🧹 Nitpick comments (1)
src/FastColor.ts (1)
170-184: 建议：考虑改进缓存失效机制当前实现中，
setR/setG/setB方法通过_sc克隆对象并修改 RGB 值，但不会清空 HSL/HSV 相关的缓存字段（_h、_hsl_s、_hsv_s、_l、_v）。这可能导致 RGB 改变后，HSL/HSV 属性返回过期的缓存值。例如：
const color = new FastColor({ r: 255, g: 0, b: 0 }); // 红色 const hue = color.getHue(); // 缓存色相值 const newColor = color.setR(0); // RGB 变为 (0,0,0) - 黑色 const newHue = newColor.getHue(); // 仍返回红色的色相！这是一个已存在的架构问题，不是本次 PR 引入的。建议在后续优化中考虑在 RGB 变更时清空相关缓存，或使用更健壮的缓存失效策略。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/FastColor.ts(9 hunks)tests/hsl.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hsl.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/FastColor.ts (1)
src/types.ts (3)
OptionalA(22-22)HSL(8-13)HSV(15-20)
🔇 Additional comments (8)
src/FastColor.ts (8)
89-90: 完美解决了缓存共享问题！将
_s拆分为_hsl_s和_hsv_s正确地为 HSL 和 HSV 饱和度提供了独立缓存，避免了之前 Gemini 评审中指出的缓存冲突问题。
147-148: LGTM！克隆逻辑正确地复制了
_hsl_s和_hsv_s两个独立的饱和度缓存字段。
232-237: 良好的向后兼容策略！通过
@deprecated注解并委托给getHSVSaturation()，既保持了向后兼容性（旧实现使用的是 HSV 公式），又引导开发者使用更明确的新方法。
239-249: HSV 饱和度计算公式正确！使用标准 HSV 饱和度公式
S = delta / max，并正确处理了delta === 0的边界情况。缓存逻辑符合懒加载模式。
251-262: HSL 饱和度公式修正正确！实现了 Issue #93 中提出的正确 HSL 饱和度公式：
(delta/255) / (1 - Math.abs(2 * l - 1))。边界情况（delta === 0）得到妥善处理，避免了除零错误。
406-434: 转换方法正确使用了对应的饱和度计算！
toHsl()和toHslString()使用getHSLSaturation()toHsv()使用getHSVSaturation()确保了输出格式与所用饱和度公式的一致性。
506-519: 色相归一化问题已修复！Line 507 使用
((h % 360) + 360) % 360正确地将色相值归一化到[0, 360)区间，修复了之前 Gemini 评审中指出的负数色相导致计算错误的问题（例如h = -60现在正确映射为300）。同时正确设置了_hsl_s缓存。
555-560: HSV 转换同样修复了色相归一化！与
fromHsl一致，使用相同的色相归一化公式，并正确设置了_hsv_s缓存。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
===========================================
+ Coverage 79.43% 93.75% +14.31%
===========================================
Files 1 1
Lines 321 336 +15
Branches 95 93 -2
===========================================
+ Hits 255 315 +60
+ Misses 65 21 -44
+ Partials 1 0 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
close #93 and #90
顺手将这两个问题一起修复了，又调研了下，发现
hsl和hsv对 s 的计算公式是不同的，所以这里将getSaturation拆分成了getHSLSaturation和getHSVSaturation匹配不同的计算公式。原先这个我就废弃掉了Summary by CodeRabbit
新功能
改进
测试
✏️ Tip: You can customize this high-level summary in your review settings.