Repository navigation
Normalise colours to hex before Gegl instead of painting the failure - #54
floraschouw wants to merge 1 commit into
Conversation
Colour strings went straight into Gegl.Color.new(), which does not report a
parse failure. Probing it on GIMP 3.2.4 shows two distinct wrong results:
Gegl.Color.new("cornflowerblue").get_rgba() -> (0.0, 1.0, 1.0, 0.670)
Gegl.Color.new("orange").get_rgba() -> (0.0, 1.0, 1.0, 0.670)
Gegl.Color.new("not-a-color").get_rgba() -> (0.0, 1.0, 1.0, 0.670)
Gegl.Color.new("").get_rgba() -> (0.0, 1.0, 1.0, 0.670)
(0, 1, 1, 0.67) is Gegl's parse-failure sentinel, not a colour lookup. That
is the cyan reported in gimp-mcp#44: every unrecognised name lands on the same value,
so the failure is indistinguishable from a deliberate colour.
rgb() is worse, because it parses and then scales wrongly:
Gegl.Color.new("rgb(40,180,90)").get_rgba() -> (40.0, 180.0, 90.0, 1.0)
Those are 0..1 channels, so 40.0 is 40x full intensity; it clamps to white on
render. That is gimp-mcp#50 -- and on a white canvas it looks like the call did
nothing at all, while reporting status: success.
Basic names (white, black, red, green, blue) happen to work, which is what
made this hard to spot: the common cases are fine and the failure only shows
up on the extended palette.
So colours are now normalised to hex before Gegl sees them. parse_color()
accepts hex (#rgb, #rgba, #rrggbb, #rrggbbaa), all 148 CSS Level 4 names,
transparent, and rgb()/rgba() with numbers or percentages, and raises on
anything else. All 22 Gegl.Color.new() call sites go through it.
An unparseable colour is now an error rather than a wrong colour on the
canvas. That is the point: a batch job that reports success while painting
cyan is worse than one that stops.
Fixes gimp-mcp#44
Fixes gimp-mcp#50
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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 |
There was a problem hiding this comment.
Thanks for this, @floraschouw. The diagnosis is right and the fix works. I tested the PR on GIMP 3.2.6 (Linux) by installing this branch's plugin and painting through fill_rectangle, then reading the pixels back in sRGB:
| Color passed | main |
This PR | |
|---|---|---|---|
#FFA500 |
#ffa500 |
#ffa500 |
✅ |
orange |
#00ffff (cyan) |
#ffa500 |
✅ fixes #44 |
rgb(40,180,90) |
#ffffff (white) |
#28b45a |
✅ fixes #50 |
orang (typo) |
painted cyan, success |
error, nothing painted | ✅ |
rgb(1.0, 0.647, 0.0) |
#ffd200 |
#010100, success |
❌ |
I'd like two changes before merging.
1. rgb() with 0–1 values now gives near-black
_colour_channel always reads rgb() channels on a 0–255 scale. CLAUDE.md tells users to write colors as Gegl.Color.new("rgb(1.0, 0.647, 0.0)") with values from 0 to 1, and tests/continuous_edit_test/continuous_edit_test.py:766 uses rgb(0.96,0.80,0.30). With this PR those become #010100 and still report success. On a dark canvas the shape disappears entirely, which is the same silent wrong-color failure this PR is fixing.
On main this form isn't quite right either. Gegl reads the values as linear light, so orange comes out as #ffd200. Suggested fix: when every channel is ≤ 1 (and none uses %), treat the values as 0–1 sRGB and scale them by 255. That gives #ffa500 for the example above, fixing main's off color too. If you'd rather reject the 0–1 form, please update CLAUDE.md in the same PR.
2. Check the color before changing anything
In several tools parse_color() runs only after the image has been changed. A bad color then returns an error but leaves the image half-edited. I confirmed one case live: create_layer(name="bad-fill-test", fill="notacolor") returns unrecognised color 'notacolor', but list_layers shows the empty bad-fill-test layer was still added. The same pattern exists in:
_resize_canvas: the canvas is resized before the fill is checked_new_canvas: the image and layer are created first; on a bad fill an image with no display is left behind, which shiftsimage_index_apply_drop_shadow: the shadow layer is inserted first_edit_text: the text, font and size are changed before the color is checked_set_colors: the foreground is set before the background is checked
Suggested fix: parse every color at the top of each handler, before touching the image. A small helper such as make_color(s) -> Gegl.Color used once per color would do it. It would also replace the 22 repeated Gegl.Color.new(parse_color(x)) calls, the double parsing in _gradient_fill, and the redundant ternary in _fill_selection (both branches are color_str).
Optional
- Transparent fills:
new_canvas,create_layerandresize_canvascheck forfill.lower() == "transparent"exactly." transparent ",rgba(0,0,0,0)and#00000000skip that branch and go through a normal fill. On a layer without alpha that paints opaque black. Comparing againstparse_color(fill)(or the result's alpha) would catch all of them. - Rounding:
round()rounds halves to the nearest even number, sorgb(126.5, …)gives 126 rather than 127.int(v + 0.5)matches CSS. - Modern CSS syntax: the space-separated form
rgb(40 180 90 / 50%)is rejected even though the error message saysrgb()is supported. Accepting it or naming the supported form in the message would both be fine.
With 1 and 2 addressed, I'm happy to approve. This also seems like the best foundation for the color part of #41.
Fixes #44. Fixes #50.
Both issues have the same cause, so they are fixed together here.
What Gegl actually does
Colour strings went straight into
Gegl.Color.new(), which never reports a parse failure. Probed on GIMP 3.2.4:(0, 1, 1, 0.67)is a parse-failure sentinel, not a colour lookup. #44 reads as "CSS names resolve to cyan", but every unrecognised string lands on that same value — the cyan is the error, and it is indistinguishable from a deliberately requested colour.rgb()fails differently, and worse, because it parses and then scales wrongly:Those are 0..1 channels, so 40.0 is forty times full intensity. It clamps to white on render — #50. On the default white canvas that looks like the call did nothing, while still returning
status: success.Basic names (
white,black,red,green,blue) do work, which is what makes this easy to miss: the common cases are fine and only the extended palette breaks.Fix
parse_color()normalises to hex before Gegl sees anything. It accepts:#rgb,#rgba,#rrggbb,#rrggbbaatransparentrgb()/rgba(), numbers or percentages, clamped to rangeand raises
ValueErroron anything else (TypeErrorfor a non-string). All 22Gegl.Color.new()call sites go through it.The rejection is the substantive half. Painting a wrong colour while reporting success is worse than stopping: a batch job "succeeds" over hundreds of images and produces wrong output that nothing detects until someone looks.
Verification
Parser unit-tested standalone, 26 cases, all passing — including
rgb(300,-5,90)->#ff005a(clamping),rgb(50%,50%,50%)->#808080,#69e->#6699ee, case and whitespace handling, and every rejection path.End to end against a running GIMP 3.2.4 on Windows 10, drawing on a
#333333canvas and reading pixels back:#FF5733#ff1808#ff1808cornflowerblue#204cd7#204cd7rgb(40,180,90)#05741a#05741arebeccapurplergba(255,200,0,0.55)#0cfdefinitely-not-a-colorstatus: successValueError, nothing paintedRead-back values are linear-light rather than sRGB because of #43, which is unrelated and untouched here; the numbers above are the linear encodings of
#ff5733,#6495edand#28b45arespectively, so they confirm the correct colour reached the canvas.python -m py_compilepasses.ruff checkis unchanged at 183 findings — no new violations.The 148-entry table was generated from Pillow's
ImageColor.colormaprather than transcribed by hand.