Fix add_text placeholder metadata on GIMP 3.2 (#15)

[vicArc]

Fixes #15.

Problem

add_text was returning placeholder metadata while reporting status=success:

{"status": "success",
 "results": {"layer_name": "unknown", "layer_id": -1,
             "text_width": 0, "text_height": 0, "position": [10, 10]}}

Root cause surfaced while probing a live GIMP 3.2.2 runtime:

1. _add_text was looking up gimp-text-fontname — a GIMP 2.x PDB procedure. On GIMP 3.2 pdb.lookup_procedure("gimp-text-fontname") returns None, so the whole branch was skipped and text_layer stayed None.
2. The response assembly then fell through to "unknown"/-1/0 defaults but still reported status=success, so callers had no signal that anything was wrong.
3. Independently, Gimp.Font.get_by_name("Sans") returns None on 3.2 (the matching font is "Sans-serif") — so even the native API path would have failed on the default font value.

Because the returned layer_id/layer_name were fake, any follow-up operation that tried to chain on the new text layer (move, rotate, reorder, recolor) would target the wrong layer or fail lookup entirely.

Fix

Rewrite _add_text for GIMP 3.2 (gimp-mcp-plugin.py):

• Font resolution with fallbacks. Try the exact name, then common aliases ("Sans" → "Sans-serif", "<name> Regular", hyphenated), then "Sans-serif"/"Serif"/"Monospace", and finally the first available font from Gimp.fonts_get_list(""). Stale font names no longer cause silent failure.
• Primary path: Gimp.TextLayer.new(image, text, font_obj, size, Gimp.Unit.pixel()) + image.insert_layer() + set_offsets() + gimp-text-layer-set-color PDB proc.
• Fallback path: gimp-text-font PDB procedure (the 3.x replacement for gimp-text-fontname; takes a GimpFont object rather than a string).
• Robust layer identification. Snapshot the layer-id set before creation; if neither path returned a handle directly, diff against the post-call layer list to find the newly inserted layer. This is resilient to PDB return-tuple shape differences across GIMP 3.x point releases.
• No more placeholder success. If no layer was actually created, return status=error with an explanatory message instead of status=success with fake values.

Test

Added tests/test_add_text_metadata.py. It opens tests/continuous_edit_test/files/winry_joy.png, calls add_text, and asserts:

• status == "success"
• layer_id is a positive int (not -1)
• layer_name is not "unknown" and not empty
• text_width > 0 and text_height > 0 (the layer actually rendered)
• list_layers sees the returned id/name — proving the handle is chainable (the core requirement of the issue)

Verification on GIMP 3.2.2

• tests/test_add_text_metadata.py — PASS (layer_id=3 name='Issue15' size=95x31, cross-checked by list_layers)
• run_tests.py — 56/56 PASSED, including Cat 7 add_text which now shows layer_name: 'Hello', layer_id: 13, text_width: 29, ...
• tests/continuous_edit_test/continuous_edit_test.py — SUCCESS (full BG removal → recolor → composite → export pipeline)

Quality gates

• ruff check . — clean
• python -m py_compile — clean

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Improved text insertion to prefer native GIMP 3 creation with robust fallbacks across versions.

• Bug Fixes

  • add_text returns accurate, non-placeholder text layer metadata: real layer name, ID, width, and height.
  • Better error reporting when text layer creation fails and more reliable detection of newly created layers.

• Tests

  • Added end-to-end regression test ensuring add_text returns real, resolvable layer metadata.

[coderabbitai]

Walkthrough

Resolve fonts to Gimp.Font and rework text insertion: attempt native Gimp.TextLayer creation with PDB fallback, detect newly created layer by snapshot diff, and return actual layer metadata or explicit error. Added an integration test that validates returned layer metadata and chainability.

Changes

Cohort / File(s)	Summary
Core text layer creation logic gimp-mcp-plugin.py	Rewrote _add_text to resolve an input font name to a Gimp.Font, prefer native Gimp.TextLayer.new + image.insert_layer (apply offsets/color), fallback to PDB gimp-text-font using the resolved Gimp.Font; snapshot pre/post layer IDs and locate the new layer by diff; return real layer_name, layer_id, text_width, text_height, and {status: "error"} when no new layer is found. Removed old gimp-text-fontname string-path handling.
Regression test for metadata accuracy tests/test_add_text_metadata.py	Added an integration test that connects to local MCP server, opens a fixture image, identifies image_index, calls add_text, asserts status == "success", verifies returned layer_id/layer_name/text_width/text_height are non-placeholder/positive, checks list_layers contains the returned layer, and best-effort closes the image in cleanup.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant MCP as MCP_Server
    participant Plugin as Plugin
    participant Native as GIMP_Native
    participant PDB as GIMP_PDB

    Client->>MCP: add_text request
    MCP->>Plugin: dispatch add_text
    Plugin->>Plugin: snapshot before_ids
    Plugin->>Native: attempt Gimp.TextLayer.new + image.insert_layer
    alt native success
        Native-->>Plugin: layer inserted
        Plugin->>Plugin: apply color/offset, snapshot after_ids
    else native fail
        Plugin->>PDB: call gimp-text-font with Gimp.Font
        PDB-->>Plugin: procedure return
        Plugin->>Plugin: snapshot after_ids
    end
    Plugin->>Plugin: _find_new_layer(diff before/after)
    Plugin-->>MCP: return status + layer metadata
    MCP-->>Client: respond (status, layer_id, layer_name, width, height)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• Fix GIMP 3.2 API compatibility + agent feedback tools #14 — Related changes to add_text/text-generation flow for GIMP 3.x compatibility; targets similar native/PDB handling and layer-introspection fixes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix add_text placeholder metadata on GIMP 3.2 (#15)' directly and concisely summarizes the main change: fixing placeholder metadata issues in add_text for GIMP 3.2, with clear reference to the linked issue.
Linked Issues check	✅ Passed	The PR fully addresses issue #15 objectives: robust font resolution with fallbacks, native GIMP 3 API path with PDB fallback, layer detection via before/after diffs, real metadata extraction, explicit error returns, and regression test validation.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing issue #15: font resolution logic, text layer creation methods, layer detection, metadata extraction, and regression testing. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gimp-mcp-plugin.py`:
- Around line 3048-3067: The code currently clears text_layer to None on any
exception after creating and inserting tl (via Gimp.TextLayer.new and
image.insert_layer), which can cause a duplicate layer when the PDB fallback
(pdb.lookup_procedure("gimp-text-font")) runs; change the flow so that you set
text_layer = tl immediately after a successful insert (image.insert_layer) and
do NOT reset it to None on exceptions from tl.set_offsets or the color call
(pdb.lookup_procedure("gimp-text-layer-set-color")); instead treat offset/color
operations as best-effort (catch their exceptions locally, log or ignore) so the
presence of an inserted layer prevents the PDB fallback from executing.

In `@tests/test_add_text_metadata.py`:
- Around line 72-88: The test currently picks an image by path suffix which can
select a stale instance; instead use the image_id returned from
cmd('open_image', {'file_path': TEST_IMAGE}) to find the exact image index in
the list returned by cmd('list_images', {}): read r.get('image_id') from the
open_image response and then iterate the images list to find the entry whose
'image_id' matches that value (use that index as target_index); keep the
fallback to path-suffix only if image_id is missing to preserve compatibility.
- Around line 111-129: The test currently allows layer_id == 0 and treats a
listed layer as a match if either id OR name matches; tighten it by requiring a
strictly positive integer id (change the check in the layer_id assertion to
require layer_id > 0) and change the search that builds match (the next(...)
over layers) to require both the id and name match the returned values (use
lyr.get('id') == layer_id AND lyr.get('name') == layer_name) so the same listed
layer must have both the returned ID and name when calling cmd('list_layers',
...).
- Around line 133-135: The test leaves a GIMP image open; ensure you track the
created image index (target_index) and close the image in a finally block to
avoid polluting the persistent GIMP session. Wrap the main test body so
target_index is set when the image is created (where you currently print PASS
add_text...) and add a try/finally that calls the GIMP close function (or
appropriate cleanup function used elsewhere in tests) on target_index in the
finally clause; ensure the close is conditional (only if target_index is not
None/valid) so tests that fail before creation still clean up.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0d57617b-7a02-4f65-a7be-909032524a7b

📥 Commits

Reviewing files that changed from the base of the PR and between 5c53da5 and 03cf0d5.

📒 Files selected for processing (2)

• gimp-mcp-plugin.py
• tests/test_add_text_metadata.py

[vicArc]

Hi @maorcc ,
Issue 15 fix completed, includes test_add_text_metadata.py.
Let me know if you need me adjusting somethig else. See you !! and thanks !!

[maorcc]

Comment verbosity vs. project conventions (CLAUDE.md)

Finding: The new helpers introduced in this PR carry significantly more documentation than the rest of the codebase, and that excess conflicts with the project's own stated guidelines.

What CLAUDE.md says

Default to writing no comments. Only add one when the WHY is non-obvious: a hidden constraint, a subtle invariant, a workaround for a specific bug, behavior that would surprise a reader. If removing the comment wouldn't confuse a future reader, don't write it.

Don't explain WHAT the code does, since well-named identifiers already do that.

Never write multi-paragraph docstrings or multi-line comment blocks — one short line max.

What the existing codebase does

Every other method in gimp-mcp-plugin.py has either no docstring or a single-line one, e.g.:

def _add_text(self, params):
    """Add a text layer."""

What this PR introduces

The four new helpers each carry multi-paragraph RST-style docstrings, and _add_text itself gains a 12-line docstring. On top of that, a 14-line ASCII flow-diagram comment block sits above all four helpers:

# The flow is:
#
#     resolve font  ──▶  try native API  ──▶  try PDB fallback
#                                │                    │
#                                └── layer committed ─┴──▶  find by diff
# ...

Most of the docstring content explains what the code does (restating the function name, parameters, and return value), not why — e.g. _find_new_layer's entire docstring is a restatement of what you can already read from the signature and body.

Genuinely useful commentary (worth keeping)

A few sentences do capture non-obvious WHYs that belong in the code:

• In _create_text_layer_native: the explanation that post-insert failures are swallowed intentionally to prevent the PDB fallback from inserting a second layer.
• In _resolve_font: the note that "Sans" no longer matches in GIMP 3.2 and the equivalent is "Sans-serif".
• In _add_text: the note that status=error is returned instead of placeholder success — the core guard for issue add_text returns placeholder layer metadata (layer_id: -1, layer_name: 'unknown') #15.

Those could be preserved as short inline comments; the rest could be trimmed to one-liners.

---

@maorcc — do you want these addressed before merge? Happy to submit a follow-up that trims the docstrings down to the project style while keeping the three genuinely informative notes above.

---

Generated by Claude Code

[coderabbitai]

🧹 Nitpick comments (1)

gimp-mcp-plugin.py (1)

3178-3184: Nit: broaden the error wording — the cause may not be PDB.

"no text layer was created (no PDB procedure succeeded)" is misleading when the actual failure mode is _resolve_font returning None (no fonts loaded at all) or the native Gimp.TextLayer.new path raising. Consider a more neutral message so the log points readers in the right direction when debugging.

✏️ Suggested wording

             if text_layer is None:
                 return {
                     "status": "error",
-                    "error":  "add_text: no text layer was created (no PDB procedure succeeded)",
+                    "error":  "add_text: no text layer was created (font resolution or text-layer creation failed)",
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@gimp-mcp-plugin.py` around lines 3178 - 3184, The error message returned when
no text layer is found is too specific about PDB procedures; update the message
in the add_text path that checks text_layer and _find_new_layer to use a neutral
wording (e.g., "no text layer was created; possible causes include missing fonts
or layer creation failure") so it covers failures from _resolve_font returning
None, exceptions from Gimp.TextLayer.new, or other creation issues; keep the
same return structure and only change the error string around the text_layer
check.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@gimp-mcp-plugin.py`:
- Around line 3178-3184: The error message returned when no text layer is found
is too specific about PDB procedures; update the message in the add_text path
that checks text_layer and _find_new_layer to use a neutral wording (e.g., "no
text layer was created; possible causes include missing fonts or layer creation
failure") so it covers failures from _resolve_font returning None, exceptions
from Gimp.TextLayer.new, or other creation issues; keep the same return
structure and only change the error string around the text_layer check.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6d2ecbd9-9888-43e3-8b67-8002f56eaee3

📥 Commits

Reviewing files that changed from the base of the PR and between 03cf0d5 and 757a8b3.

📒 Files selected for processing (2)

• gimp-mcp-plugin.py
• tests/test_add_text_metadata.py

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_add_text_metadata.py (1)

2-29: Trim docstrings to match project convention.

Per the discussion on this PR (maorcc) and the CLAUDE.md style note (prefer minimal comments explaining WHY, single-line docstrings), the module header and every helper here carry multi-paragraph docstrings that duplicate what the code already says. The genuinely useful "why" notes worth keeping as short inline comments / one-liners are:

• assert_real_metadata: rationale for layer_id > 0 (0 would let a default-int slip through like the old -1).
• assert_chainable: rationale for id and name (either-or would match an unrelated same-named layer).
• open_test_image: why image_id is preferred over path-suffix (stale duplicates in persistent session).
• close_image_if_open: why cleanup is best-effort (don't mask real test result).

Everything else (module history of the old sentinels, the "What this test proves" list, transport details in send) can be collapsed to a single line each or dropped.

Also applies to: 43-47, 86-94, 157-165, 183-194, 211-221

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_add_text_metadata.py` around lines 2 - 29, Trim multi-paragraph
module and function docstrings in tests/test_add_text_metadata.py to single-line
docstrings per project convention, preserving only concise one-line rationales
for assert_real_metadata (why layer_id > 0), assert_chainable (why require both
id and name), open_test_image (why prefer image_id over path-suffix), and
close_image_if_open (why cleanup is best-effort); collapse or remove other
explanatory blocks (module header history, "What this test proves" list,
transport/send details) into single-line docstrings or inline comments as
appropriate, and update the docstrings around the indicated ranges (including
the lines covering the module header and the helpers assert_real_metadata,
assert_chainable, open_test_image, close_image_if_open) so tests remain
functionally unchanged but comments match the CLAUDE.md style.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_add_text_metadata.py`:
- Around line 225-230: The cleanup log can raise a TypeError when r.get('error')
is None because the code slices it (None[:200]); update the cleanup block around
the cmd('close_image', ...) call to coerce the error to a string before
slicing—e.g., use str(r.get('error', '')) or (r.get('error') or '') and then
take the first 200 chars—so the print in the except/failure path always handles
None safely; change the reference in this block that currently calls
r.get('error', '') to the coerced-string form.

---

Nitpick comments:
In `@tests/test_add_text_metadata.py`:
- Around line 2-29: Trim multi-paragraph module and function docstrings in
tests/test_add_text_metadata.py to single-line docstrings per project
convention, preserving only concise one-line rationales for assert_real_metadata
(why layer_id > 0), assert_chainable (why require both id and name),
open_test_image (why prefer image_id over path-suffix), and close_image_if_open
(why cleanup is best-effort); collapse or remove other explanatory blocks
(module header history, "What this test proves" list, transport/send details)
into single-line docstrings or inline comments as appropriate, and update the
docstrings around the indicated ranges (including the lines covering the module
header and the helpers assert_real_metadata, assert_chainable, open_test_image,
close_image_if_open) so tests remain functionally unchanged but comments match
the CLAUDE.md style.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 749d928d-d039-4811-9b61-454f76bf1029

📥 Commits

Reviewing files that changed from the base of the PR and between 757a8b3 and ff1247c.

📒 Files selected for processing (2)

• gimp-mcp-plugin.py
• tests/test_add_text_metadata.py

🚧 Files skipped from review as they are similar to previous changes (1)

• gimp-mcp-plugin.py

[vicArc]

Hi @maorcc
I remove the docstrings and also fix the base of the code. pls let me know how looks like thank you !

[coderabbitai]

🧹 Nitpick comments (1)

gimp-mcp-plugin.py (1)

3087-3087: Consider defaulting font to "Sans-serif" instead of "Sans".

Per the PR description, "Sans" no longer resolves on GIMP 3.2 and "Sans-serif" is the native equivalent. _resolve_font handles this via the alias list, so this works today — but defaulting to the name that actually resolves on the target platform avoids an extra lookup on every call and makes the intent clearer to readers.

♻️ Proposed change

-            font        = params.get("font", "Sans")
+            font        = params.get("font", "Sans-serif")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@gimp-mcp-plugin.py` at line 3087, The default font parameter currently uses
params.get("font", "Sans") which no longer resolves on GIMP 3.2; change the
default to "Sans-serif" so the code (e.g., the font variable assignment and
subsequent _resolve_font usage) uses a name that resolves natively and avoids an
extra alias lookup on every call—update the params.get default and any related
documentation/comments to "Sans-serif".

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@gimp-mcp-plugin.py`:
- Line 3087: The default font parameter currently uses params.get("font",
"Sans") which no longer resolves on GIMP 3.2; change the default to "Sans-serif"
so the code (e.g., the font variable assignment and subsequent _resolve_font
usage) uses a name that resolves natively and avoids an extra alias lookup on
every call—update the params.get default and any related documentation/comments
to "Sans-serif".

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 80ae1246-a939-46ad-8c64-80187e2ff0cd

📥 Commits

Reviewing files that changed from the base of the PR and between ff1247c and 821761c.

📒 Files selected for processing (2)

• gimp-mcp-plugin.py
• tests/test_add_text_metadata.py

✅ Files skipped from review due to trivial changes (1)

• tests/test_add_text_metadata.py