Fix select_by_color silently no-opping on GIMP 3.2 - #23
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesColor Selection API Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
The PDB procedure 'gimp-by-color-select' does not exist in GIMP 3.2 — it was renamed to 'gimp-image-select-color'. The previous code called pdb.lookup_procedure() which returned None, then guarded the run with `if proc:` and silently fell through to a successful return. No selection ever got created, so any subsequent operation that depends on the selection (notably fill_selection with fill_type='transparent', which calls Gimp.Drawable.edit_clear) acted on the entire layer. Switch to the GIMP 3.2 procedure name and raise if the lookup fails so the failure is surfaced instead of swallowed. The new procedure config exposes only image/drawable/color/operation; threshold, antialias, and feather settings are now read from the GIMP context, so set them via Gimp.context_* inside a context_push/pop pair. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The existing select_by_color check only asserted response status, so the pre-#23 bug (procedure lookup miss -> silent no-op returning success) went undetected. Fill a known color, clear any prior selection, then assert the selection is non-empty after select_by_color so a silent no-op fails. Co-authored-by: maor <maor@Maors-MacBook-Pro.local>
Summary
select_by_colorlooks up the PDB proceduregimp-by-color-select, which does not exist on GIMP 3.2 — it was renamed togimp-image-select-color. The current code guards the call withif proc:and silently falls through toreturn {"status": "success", ...}when the lookup returnsNone, so no selection is ever created, but the caller is told the operation succeeded.This is dangerous because downstream tools assume a selection exists. In particular,
fill_selectionwithfill_type='transparent'callsGimp.Drawable.edit_clear(drawable), which clears the entire layer when no selection is active. So a typical "remove black background" workflowsilently wipes the whole image instead of just the BG.
Fix
gimp-image-select-color.RuntimeErrorwhen the lookup returnsNoneinstead of silently no-opping, so future renames are caught immediately.image / drawable / color / operation; threshold, antialias, feather, and sample-merged are read from the GIMP context. Set them viaGimp.context_*inside acontext_push() / context_pop()pair so we don't leak settings into the rest of the session.How I hit this
Running an MCP-driven "remove black background, export transparent PNG for a black t-shirt print" pipeline on a JPEG. After
select_by_color('#000000', threshold=30)followed byfill_selection(fill_type='transparent'), every pixel ended up withalpha=0(RGB preserved). Confirmed withpdb.procedure_exists('gimp-by-color-select')→Falseandpdb.procedure_exists('gimp-image-select-color')→Trueon GIMP 3.2.2.Note on existing test coverage
run_tests.py:92exercisesselect_by_color, but only checks that the response status issuccess. Since the buggy path returns success without doing anything, the test passes against a broken plugin. Worth a follow-up to assert the selection is non-empty (e.g. viaGimp.Selection.is_empty()), but I left test changes out of this PR to keep the diff minimal.Test plan
select_by_color({'color': '#000000', 'threshold': 30})on a black-background image and verify the selection bounds are non-empty (Gimp.Selection.bounds(image)returnsnon_empty=True).fill_selection({'fill_type': 'transparent'})after the select and confirm only the BG pixels go toalpha=0; foreground pixels keepalpha=1.0.ruff check gimp-mcp-plugin.py— clean.🤖 Generated with Claude Code
Summary by CodeRabbit