Fix add_text placeholder metadata on GIMP 3.2 (#15)

gimp-mcp-plugin.py

@@ -2993,6 +2993,89 @@ def _gradient_fill(self, params):
     # CATEGORY 7 — Text
     # =========================================================================
 
+    def _resolve_font(self, name):
+        """Resolve a font name to a Gimp.Font."""
+        if not (hasattr(Gimp, "Font") and hasattr(Gimp.Font, "get_by_name")):
+            return None
+
+        font_obj = Gimp.Font.get_by_name(name)
+        if font_obj is not None:
+            return font_obj
+
+        # GIMP 3.2 dropped GIMP 2.x aliases; e.g. "Sans" no longer resolves,
+        # the 3.2 equivalent is "Sans-serif".
+        for alias in (name + "-serif", name + " Regular",
+                      name.replace(" ", "-"),
+                      "Sans-serif", "Serif", "Monospace"):
+            font_obj = Gimp.Font.get_by_name(alias)
+            if font_obj is not None:
+                return font_obj
+
+        raw = Gimp.fonts_get_list("")
+        flist = list(raw[1]) if isinstance(raw, tuple) and len(raw) > 1 else list(raw)
+        return flist[0] if flist else None
+
+    def _create_text_layer_native(self, image, text, font_obj, size, x, y, color_str):
+        """Create a text layer via Gimp.TextLayer.new."""
+        from gi.repository import Gegl
+        if not hasattr(Gimp, "TextLayer"):
+            return None
+
+        try:
+            tl = Gimp.TextLayer.new(image, text, font_obj, float(size), Gimp.Unit.pixel())
+        except Exception:
+            return None
+        if tl is None:
+            return None
+
+        try:
+            image.insert_layer(tl, None, 0)
+        except Exception:
+            return None
+
+        # Layer is now in the image; swallow offset/color failures so the
+        # caller does not fall through to the PDB fallback and insert a
+        # second layer.
+        try:
+            tl.set_offsets(x, y)
+        except Exception:
+            pass
+        try:
+            pdb   = Gimp.get_pdb()
+            cproc = pdb.lookup_procedure("gimp-text-layer-set-color")
+            if cproc:
+                ccfg = cproc.create_config()
+                ccfg.set_property("layer", tl)
+                ccfg.set_property("color", Gegl.Color.new(color_str))
+                cproc.run(ccfg)
+        except Exception:
+            pass
+        return tl
+
+    def _create_text_layer_pdb(self, image, text, font_obj, size, x, y):
+        """Create a text layer via the gimp-text-font PDB procedure."""
+        proc = Gimp.get_pdb().lookup_procedure("gimp-text-font")
+        if proc is None:
+            return
+        cfg = proc.create_config()
+        cfg.set_property("image",     image)
+        cfg.set_property("drawable",  None)
+        cfg.set_property("x",         float(x))
+        cfg.set_property("y",         float(y))
+        cfg.set_property("text",      text)
+        cfg.set_property("border",    0)
+        cfg.set_property("antialias", True)
+        cfg.set_property("size",      float(size))
+        cfg.set_property("font",      font_obj)
+        proc.run(cfg)
+
+    def _find_new_layer(self, image, before_ids):
+        """Return the first layer whose id is not in before_ids."""
+        for lyr in image.get_layers():
+            if lyr.get_id() not in before_ids:
+                return lyr
+        return None
+
     def _add_text(self, params):
         """Add a text layer."""
         try:
@@ -3004,40 +3087,44 @@ def _add_text(self, params):
             font        = params.get("font", "Sans")
             size        = int(params.get("size", 24))
             color_str   = params.get("color", "black")
-            image = self._get_image(image_index)
+
+            image      = self._get_image(image_index)
+            before_ids = {lyr.get_id() for lyr in image.get_layers()}
+            text_layer = None
+
             image.undo_group_start()
             Gimp.context_push()
             try:
                 Gimp.context_set_foreground(Gegl.Color.new(color_str))
-                # Gimp.text_fontname() removed in GIMP 3 — use PDB procedure
-                pdb = Gimp.get_pdb()
-                proc = pdb.lookup_procedure("gimp-text-fontname")
-                text_layer = None
-                if proc:
-                    cfg = proc.create_config()
-                    cfg.set_property("image",     image)
-                    cfg.set_property("drawable",  None)
-                    cfg.set_property("x",         float(x))
-                    cfg.set_property("y",         float(y))
-                    cfg.set_property("text",      text_str)
-                    cfg.set_property("border",    0)
-                    cfg.set_property("antialias", True)
-                    cfg.set_property("size",      float(size))
-                    cfg.set_property("fontname",  font)
-                    result = proc.run(cfg)
-                    if result:
-                        text_layer = result.index(1)
+                font_obj = self._resolve_font(font)
+                if font_obj is not None:
+                    text_layer = self._create_text_layer_native(
+                        image, text_str, font_obj, size, x, y, color_str)
+                    if text_layer is None:
+                        self._create_text_layer_pdb(
+                            image, text_str, font_obj, size, x, y)
             finally:
                 Gimp.context_pop()
                 image.undo_group_end()
             Gimp.displays_flush()
+
+            if text_layer is None:
+                text_layer = self._find_new_layer(image, before_ids)
+            if text_layer is None:
+                # Issue #15: return an explicit error instead of a placeholder
+                # success so clients never chain ops on a fake handle.
+                return {
+                    "status": "error",
+                    "error":  "add_text: no text layer was created (no PDB procedure succeeded)",
+                }
+
             return {
                 "status": "success",
                 "results": {
-                    "layer_name":  text_layer.get_name() if text_layer else "unknown",
-                    "layer_id":    text_layer.get_id()   if text_layer else -1,
-                    "text_width":  text_layer.get_width()  if text_layer else 0,
-                    "text_height": text_layer.get_height() if text_layer else 0,
+                    "layer_name":  text_layer.get_name(),
+                    "layer_id":    text_layer.get_id(),
+                    "text_width":  text_layer.get_width(),
+                    "text_height": text_layer.get_height(),
                     "position":    [x, y],
                 }
             }

tests/test_add_text_metadata.py

@@ -0,0 +1,190 @@
+#!/usr/bin/env python3
+"""Regression test for issue #15 — add_text must return real layer metadata.
+
+Requires the GIMP MCP plugin running on localhost:9877.
+Exits 0 on pass, 1 on fail.
+"""
+import json
+import os
+import socket
+import sys
+
+HOST, PORT = '127.0.0.1', 9877
+HERE       = os.path.dirname(os.path.abspath(__file__))
+TEST_IMAGE = os.path.join(HERE, 'continuous_edit_test', 'files', 'winry_joy.png')
+
+
+# ── transport ─────────────────────────────────────────────────────────────
+
+def send(msg, timeout=30):
+    """Send one JSON message to the MCP socket and return the reply."""
+    s = socket.socket()
+    s.settimeout(timeout)
+    s.connect((HOST, PORT))
+    s.send(json.dumps(msg).encode() + b'\n')
+    buf = b''
+    while True:
+        try:
+            chunk = s.recv(65536)
+            if not chunk:
+                break
+            buf += chunk
+            try:
+                json.loads(buf.decode())
+                break
+            except json.JSONDecodeError:
+                continue
+        except socket.timeout:
+            break
+    s.close()
+    try:
+        return json.loads(buf.decode().strip())
+    except json.JSONDecodeError:
+        return {'status': 'error', 'error': 'parse: ' + buf.decode()[:200]}
+
+
+def cmd(t, params=None):
+    """Send a {type, params} command."""
+    return send({'type': t, 'params': params or {}})
+
+
+def fail(msg):
+    """Abort with a failure banner on stderr."""
+    print(f"FAIL: {msg}", file=sys.stderr)
+    sys.exit(1)
+
+
+# ── test steps ────────────────────────────────────────────────────────────
+
+def open_test_image():
+    """Open the test image and return (target_index, opened_id)."""
+    if not os.path.exists(TEST_IMAGE):
+        fail(f"test image missing: {TEST_IMAGE}")
+
+    print(f"Opening test image: {TEST_IMAGE}")
+    r = cmd('open_image', {'file_path': TEST_IMAGE})
+    if r.get('status') != 'success':
+        fail(f"open_image: {r.get('error', '')}")
+
+    opened_id = r.get('image_id') or (r.get('results') or {}).get('image_id')
+
+    li     = cmd('list_images', {})
+    images = (li.get('results') or {}).get('images') or []
+    if not images:
+        fail("no images after open_image")
+
+    # Match by image_id (authoritative) — path-suffix can hit a stale
+    # duplicate in a persistent GIMP session.
+    target_index = None
+    if opened_id is not None:
+        target_index = _find_index_by(images, 'image_id', opened_id)
+    if target_index is None:
+        target_index = _find_index_by_suffix(images, 'winry_joy.png')
+    if target_index is None:
+        target_index = 0
+
+    print(f"Using image_index={target_index} (image_id={opened_id})")
+    return target_index, opened_id
+
+
+def _find_index_by(images, key, value):
+    for i, info in enumerate(images):
+        if isinstance(info, dict) and info.get(key) == value:
+            return i
+    return None
+
+
+def _find_index_by_suffix(images, suffix):
+    for i, info in enumerate(images):
+        if isinstance(info, dict) and info.get('file_path', '').endswith(suffix):
+            return i
+    return None
+
+
+def call_add_text(target_index):
+    """Invoke add_text and return the response results dict."""
+    print("Calling add_text with real parameters...")
+    r = cmd('add_text', {
+        'image_index': target_index,
+        'text':        'Issue15',
+        'x':           10,
+        'y':           10,
+        'font':        'Sans',
+        'size':        24,
+        'color':       '#ff0000',
+    })
+    print(f"  response: {json.dumps(r)[:300]}")
+    if r.get('status') != 'success':
+        fail(f"add_text reported error: {r.get('error', '')}")
+    return r.get('results') or {}
+
+
+def assert_real_metadata(results):
+    """Verify add_text returned real values, not placeholders."""
+    layer_id   = results.get('layer_id')
+    layer_name = results.get('layer_name')
+    text_w     = results.get('text_width')
+    text_h     = results.get('text_height')
+
+    # layer_id must be strictly positive — a real Gimp.Drawable id is never 0,
+    # so accepting 0 would let a default-int slip through like the old -1 did.
+    if not isinstance(layer_id, int) or layer_id <= 0:
+        fail(f"layer_id is placeholder: {layer_id!r} (expected positive int)")
+    if not layer_name or layer_name == 'unknown':
+        fail(f"layer_name is placeholder: {layer_name!r}")
+    if not isinstance(text_w, int) or text_w <= 0:
+        fail(f"text_width is placeholder/zero: {text_w!r}")
+    if not isinstance(text_h, int) or text_h <= 0:
+        fail(f"text_height is placeholder/zero: {text_h!r}")
+    return layer_id, layer_name, text_w, text_h
+
+
+def assert_chainable(target_index, layer_id, layer_name):
+    """Verify the returned handle is visible to list_layers."""
+    ll = cmd('list_layers', {'image_index': target_index})
+    if ll.get('status') != 'success':
+        fail(f"list_layers failed: {ll.get('error', '')}")
+
+    # Require both id AND name to match — id-or-name could pass against an
+    # unrelated layer that happens to share a non-unique name.
+    layers = (ll.get('results') or {}).get('layers') or []
+    match  = next((lyr for lyr in layers
+                   if lyr.get('id') == layer_id and lyr.get('name') == layer_name),
+                  None)
+    if match is None:
+        fail(f"returned layer (id={layer_id}, name={layer_name!r}) not in list_layers: {layers}")
+    return match
+
+
+# ── entry point ───────────────────────────────────────────────────────────
+
+def close_image_if_open(target_index):
+    """Close the test image so a persistent GIMP session stays clean."""
+    if target_index is None:
+        return
+    try:
+        r = cmd('close_image', {'image_index': target_index, 'save_first': False})
+        if r.get('status') != 'success':
+            err = str(r.get('error') or '')[:200]
+            print(f"  (cleanup) close_image non-success: {err}", file=sys.stderr)
+    except Exception as e:
+        print(f"  (cleanup) close_image transport error: {e}", file=sys.stderr)
+
+
+def main():
+    target_index = None
+    try:
+        target_index, _opened_id = open_test_image()
+        results                  = call_add_text(target_index)
+        layer_id, layer_name, w, h = assert_real_metadata(results)
+        match                    = assert_chainable(target_index, layer_id, layer_name)
+
+        print(f"PASS add_text: layer_id={layer_id} name={layer_name!r} size={w}x{h}")
+        print(f"PASS list_layers confirms layer is chainable: {match}")
+        sys.exit(0)
+    finally:
+        close_image_if_open(target_index)
+
+
+if __name__ == '__main__':
+    main()