Repository navigation
Conversation
proc.run() hands back a ValueArray whose index 0 is the PDB status, so reading from index 0 attached every label to its predecessor's value. On a uniform 300x200 #808080 canvas, get_histogram reported mean 3 (which is the GIMP_PDB_SUCCESS enum), std_dev 128, median 0, pixels 128 and count 60000. The correct values for that image are mean 128, std_dev 0, median 128 and pixels 60000 -- note that pixels was reported as 128 for a 60,000 pixel image, and std_dev as 128 for a perfectly uniform fill. Read indices 1 through 5 instead, and compare index 0 against SUCCESS so a failed call reports an error rather than silently returning zeros.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. Walkthrough
ChangesHistogram result handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized fix corrects histogram field mapping and reports failed procedure calls instead of silently returning fallback values; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Independently reproduced the same off-by-one on GIMP 3.2.6 — not by running this PR's code, but by exercising Test: a 256×4 8-bit grayscale horizontal ramp (column values 0–255, 1024 px total),
So index 0 is the status slot and the documented statistics start at index 1 — exactly the offset this PR corrects. The pre-fix code reads index 0, i.e. it returns the status enum where the mean is expected. The added |
Fixes #47.
Summary
get_histogramreturns plausible-looking numbers, which is why this hid for a while, but every field is offset by one.proc.run()returns aGimp.ValueArraywhose index 0 is the PDB status, so reading from index 0 attaches each label to its predecessor's value.Evidence
A uniform fill pins down every expected number in advance. On a 300x200 canvas (60,000 pixels) filled with
#808080, every pixel has value 128 and the standard deviation must be exactly 0:Three values in the "before" row are impossible on their face:
pixels: 128for a 60,000 pixel image,std_dev: 128for a uniform fill, andmean: 3when every pixel is 128. That3isGIMP_PDB_SUCCESS.Printing the array directly on GIMP 3.2.4 shows the layout:
Seven entries: one status followed by the six documented return values. This matches the official GIMP Python tutorial, which gates on
if result.index(0) is Gimp.PDBStatusType.SUCCESS:before reading results.The existing comment in the function already lists the six names in the right order, so only the leading status slot was overlooked.
Changes
SUCCESS, so a failed procedure call reports an error instead of silently returning zeros through the_safe()fallback. Happy to drop this guard if you would rather keep the diff to the index shift alone.Test plan
result.index(0)is aPDBStatusTypeand anintsubclass,!= Gimp.PDBStatusType.SUCCESSevaluatesFalseon a good call, and indices 1-5 return the correct statisticsruff checkclean at the version CI pins (0.15.11)py_compileclean under Python 3.10.21, matching the interpreter GIMP bundles to run plug-insrun_tests.pyagainst l 8000 ive GIMP: 59/59 PASSEDWorth noting for coverage:
run_tests.pyline 74 does callget_histogram, butt()only assertsstatus == 'success', so the wrong numbers passed. I did not add an assertion there to keep this focused, but it would be a reasonable follow-up.There is one more instance of the same misread at
gimp-mcp-plugin.py:1620, wherefiltr = result.index(0)takes the status instead of the filter object. It is wrapped in atry/exceptthat quietly falls back to the exec path, so it fails invisibly. I left it out deliberately, since that fallback deserves its own testing - noted in #47.Prepared with AI assistance; every result above was executed against a real GIMP 3.2.4 install rather than inferred.
Summary by CodeRabbit