From f16d3b21220e8945f235174b3c64412e5e51ac6f Mon Sep 17 00:00:00 2001 From: Florian Motlik Date: Fri, 25 Sep 2026 15:18:59 +0000 Subject: [PATCH] Review-Befunde: Warten, Zielbestimmung, Schutz, Export-DPI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Abarbeitung der Befunde aus dem externen Review (Claude Opus 5, GPT-5.6, Antigravity). Jeder Punkt ist vorher im Code nachgeprueft worden. Warten (alle drei Reviewer unabhaengig, zwei als kritisch): _waitUntilQuiet() zaehlte nur Objekte. Ein spaet eintreffendes Bild kann ein Objekt ERSETZEN, ohne die Anzahl zu aendern — addLogo() entfernt das alte Logo und fuegt das geflachte hinzu. Die Fassade bildet jetzt eine Signatur aus Typ, Position, Groesse, Winkel und Textinhalt aller Objekte und wartet auf deren Stabilitaet. insideMargin prueft jetzt alle vier Kanten statt nur den Ursprung. Ein dreifach zu breites Element sass am linken Rand auf der Schutzzone und wurde als "drin" gemeldet. place(), resize() und rotate() nehmen jetzt einen Index. Die llms.txt dokumentiert { target: index } laengst; vorher wurde die Zahl als Objekt verwendet und der Aufruf verpuffte. Logo, Hintergrundbild und Grundflaeche sind jetzt in JEDER aendernden Methode geschuetzt, nicht nur in remove(). objects() meldete sie als editable: false, und alles andere fasste sie trotzdem an — bringToFront(0) haette die gruene Grundflaeche ueber das ganze Bild gelegt. Die Fehlermeldungen sagen, was stattdessen zu tun ist. export() nimmt die DPI der aktuellen Vorlage statt fest 200. Der Downloadknopf tut das laengst; die Fassade lieferte fuer dieselbe Vorlage eine andere Aufloesung als fuer einen Menschen. bringLogoToFront() fehlte bei add-text, add-pink-circle und add-cross. Nachgezaehlt: drei Aufrufe im Code, jetzt sechs. Eine Schlagzeile am unteren Rand lag ueber dem Organisationslogo — und die llms.txt behauptete das Gegenteil. Dazu eine Regel, die der erste Testfehlschlag zutage gefoerdert hat und die jetzt dokumentiert ist: erst resize() und rotate(), dann place(). Fabric dreht per Vorgabe um den Mittelpunkt, wer vorher positioniert findet das Element hinterher woanders. VERSION auf 10. 132 Jest, 113 e2e gruen. Claude-Session: https://claude.ai/code/session_01Qtib9MPRZATQU3j9w7ymDk --- CLAUDE.md | 14 +++ e2e/review-findings.spec.js | 158 +++++++++++++++++++++++++++++++++ llms.txt | 17 +++- resources/js/api.js | 114 +++++++++++++++++++++--- resources/js/event-handlers.js | 11 ++- 5 files changed, 299 insertions(+), 15 deletions(-) create mode 100644 e2e/review-findings.spec.js diff --git a/CLAUDE.md b/CLAUDE.md index f84e1e7..3ae23e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,6 +160,20 @@ controls and handlers a person does; it is NOT a second rendering path. follows it, hits an error it cannot interpret, and improvises — the failure this interface exists to prevent. Keep examples complete and runnable, not fragments. +- Quiescence uses a canvas SIGNATURE (type, position, size, angle, text of + every object), not an object count. A late image load can replace an object + without changing the count — `addLogo()` removes the old logo and adds a + flattened one — so a count-based wait returns mid-change. Three independent + external reviews flagged the count version, two as critical. +- Everything `objects()` reports as `editable: false` — canvas surface, + background photo, organisation logo — is refused by every mutating method, + not just `remove()`. `bringToFront(0)` used to cover the whole image in flat + green. +- `export()` takes its default DPI from the current template, as the download + button does. Templates do not all use 200. +- Order of operations: `resize()` and `rotate()` BEFORE `place()`. Fabric + rotates around the centre by default, so rotating after placing moves the + element. - `lastAdded()` exists because `canvas.getObjects()` ends with the organisation LOGO, not with the element just added — `addLogo()` and the QR handler both call `bringLogoToFront()`. Anything that operates on "the last object" moves diff --git a/e2e/review-findings.spec.js b/e2e/review-findings.spec.js new file mode 100644 index 0000000..43ee698 --- /dev/null +++ b/e2e/review-findings.spec.js @@ -0,0 +1,158 @@ +import { test, expect } from '@playwright/test'; + +/** + * Regression tests for the findings of the external review (Claude Opus 5, + * GPT-5.6, Antigravity). Each test pins one defect they found, so it cannot + * come back quietly. + */ + +const TINY_PNG = + 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAYAAABytg0kAAAAFUlEQVR42mP8z8BQz0AEYBxVSF+FAP5FDvcfRYWgAAAAAElFTkSuQmCC'; + +test.describe('Review findings', () => { + test.beforeEach(async ({ page }) => { + await page.goto('/'); + await page.waitForFunction(() => typeof window.Bildgenerator !== 'undefined'); + page.on('dialog', (dialog) => dialog.accept()); + }); + + test('insideMargin checks all four edges, not just the origin', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('artikel_23'); + await B.setLogoEnabled(false); + await B.addText('Ein bewusst sehr langer Text der nicht hineinpasst'); + await B.resize(3); // far wider than the canvas + await B.place('top-left'); // origin is AT the margin + const wide = B.objects().find((o) => o.type === 'text'); + + await B.resize(0.3); + await B.place('center'); + const fitting = B.objects().find((o) => o.type === 'text'); + return { wide, fitting, canvasWidth: canvas.width }; + }); + + // Origin sits on the margin, so a left/top-only check called this "inside". + expect(result.wide.left).toBeLessThanOrEqual(result.wide.width); + expect(result.wide.width).toBeGreaterThan(result.canvasWidth); + expect(result.wide.insideMargin).toBe(false); + expect(result.fitting.insideMargin).toBe(true); + }); + + test('place, resize and rotate accept an index from objects()', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogoEnabled(false); + await B.addText('Erster'); + await B.addShape('cross'); + + // The first text is no longer the tracked element. + const text = B.objects().find((o) => o.type === 'text'); + // Position LAST: fabric rotates around the centre by default + // (centeredRotation), so rotating after placing moves the element. + await B.rotate(10, { target: text.index }); + await B.resize(0.5, { target: text.index }); + await B.place('top-left', { target: text.index }); + + const after = B.objects().find((o) => o.type === 'text'); + return { angle: after.angle, left: after.left, margin: B.protectiveMargin() }; + }); + + // llms.txt documents { target: index }; before the fix a number was used + // as the object itself and silently did nothing useful. + expect(result.angle).toBe(10); + expect(result.left).toBeCloseTo(result.margin, 0); + }); + + test('the logo, background and canvas surface cannot be moved or restyled', async ({ page }) => { + const errors = await page.evaluate(async (png) => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setBackground(png); + await B.setLogo('HERZOGENBURG'); + + const out = {}; + for (const role of ['logo', 'background', 'canvas']) { + const item = B.objects().find((o) => o.role === role); + out[role] = {}; + for (const method of ['place', 'resize', 'rotate', 'bringToFront', 'update']) { + try { + if (method === 'place') await B.place('top-left', { target: item.index }); + else if (method === 'resize') await B.resize(2, { target: item.index }); + else if (method === 'rotate') await B.rotate(45, { target: item.index }); + else if (method === 'bringToFront') await B.bringToFront(item.index); + else await B.update({ color: '#FFED00' }, { target: item.index }); + out[role][method] = null; + } catch (e) { + out[role][method] = e.message; + } + } + } + return out; + }, TINY_PNG); + + // objects() advertises these as editable:false; every mutating method + // must honour that, not just remove(). + for (const role of ['logo', 'background', 'canvas']) { + for (const method of ['place', 'resize', 'rotate', 'bringToFront', 'update']) { + expect(errors[role][method], `${method}() on ${role}`).toBeTruthy(); + } + } + }); + + test('export uses the DPI of the template, as the download button does', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + const out = {}; + for (const name of ['facebook_header', 'a6']) { + await B.setTemplate(name); + const exported = await B.export(); + const template = window.TemplateConstants.getTemplate(name); + out[name] = { got: exported.dpi, expected: template.dpi }; + } + return out; + }); + + // Templates do not all use 200; hardcoding it produced a different + // resolution than the same template gives a person. + expect(result.facebook_header.got).toBe(result.facebook_header.expected); + expect(result.a6.got).toBe(result.a6.expected); + }); + + test('text and shapes stay behind the organisation logo', async ({ page }) => { + const result = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogo('HERZOGENBURG'); + + const order = []; + await B.addText('Text'); + order.push(B.objects().map((o) => o.role).pop()); + await B.addShape('pinkCircle'); + order.push(B.objects().map((o) => o.role).pop()); + await B.addShape('cross'); + order.push(B.objects().map((o) => o.role).pop()); + return order; + }); + + // bringLogoToFront() was wired for images and QR codes but not for text, + // the Störer or the Wahlkreuz — those rendered over the logo. + expect(result).toEqual(['logo', 'logo', 'logo']); + }); + + test('quiescence notices a change that leaves the object count untouched', async ({ page }) => { + const signatures = await page.evaluate(async () => { + const B = window.Bildgenerator; + await B.setTemplate('feed_post_45'); + await B.setLogoEnabled(false); + await B.addShape('pinkCircle'); + const before = B._canvasSignature(); + await B.place('top-left'); + return { before, after: B._canvasSignature() }; + }); + + // Counting objects cannot see a move or a replacement; the signature can. + expect(signatures.after).not.toBe(signatures.before); + }); +}); diff --git a/llms.txt b/llms.txt index 900301b..25100da 100644 --- a/llms.txt +++ b/llms.txt @@ -49,7 +49,7 @@ weitere Freigabe. Seite laden, warten bis `window.Bildgenerator` existiert, dann aufrufen. Alle Methoden sind asynchron und liefern erst zurück, wenn das Canvas seine -Ruhelage erreicht hat. Aktuelle Schnittstellenversion: `Bildgenerator.VERSION === 9`. +Ruhelage erreicht hat. Aktuelle Schnittstellenversion: `Bildgenerator.VERSION === 10`. ```js const bild = await window.Bildgenerator.render({ @@ -393,6 +393,11 @@ Bewährte Verteilungen: prüft aber nicht, ob zwei Elemente einander überdecken. Setz sie auf gegenüberliegende Felder. +**Reihenfolge:** Erst `resize()` und `rotate()`, dann `place()`. Gedreht wird +um den Mittelpunkt, dadurch verschiebt sich die Position — wer vorher +positioniert, findet das Element hinterher woanders. `render()` hält diese +Reihenfolge von selbst ein. + ### Störer Der magentafarbene Kreis (`addShape('pinkCircle')`) ist der Störer aus dem CD. @@ -459,7 +464,15 @@ console.log(B.objects()); | `text` | der Inhalt, bei Textelementen | | `color`, `left`, `top`, `width`, `height`, `angle`, `scale` | Zustand | | `editable` | `false` bei Logo, Hintergrund und Grundfläche | -| `insideMargin` | ob das Element die Schutzzone einhält | +| `insideMargin` | ob das Element **vollständig** in der Schutzzone liegt, alle vier Kanten | + +Elemente mit `editable: false` lassen sich nicht verschieben, skalieren, +drehen, umfärben oder löschen — die Aufrufe werfen einen Fehler mit dem +Hinweis, was stattdessen zu tun ist. Für das Logo etwa `setLogoEnabled(false)` +oder `setLogo()`, für den Hintergrund ein neues `setBackground()`. + +Alle ändernden Aufrufe nehmen `{ target: index }` und wirken dann auf dieses +Element statt auf das zuletzt hinzugefügte. **Verlass dich nicht auf die Reihenfolge.** Das letzte Element in der Liste ist das Organisationslogo, nicht das, was du zuletzt hinzugefügt hast — der diff --git a/resources/js/api.js b/resources/js/api.js index fdb504d..8cf340c 100644 --- a/resources/js/api.js +++ b/resources/js/api.js @@ -31,7 +31,7 @@ * breakage surfaces in e2e/api.spec.js instead of in somebody else's script. */ const Bildgenerator = { - VERSION: 9, + VERSION: 10, /** The object the most recent add* call put on the canvas. */ _lastAdded: null, @@ -309,10 +309,13 @@ const Bildgenerator = { async place(position, options) { const opts = options || {}; const objects = canvas.getObjects(); - const target = opts.target || this._lastAdded; + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this._lastAdded; if (!target || !objects.includes(target)) { throw new Error("place() needs an element that was added first"); } + this._assertEditable(target, "place"); const margin = this.protectiveMargin(); const width = target.getScaledWidth(); @@ -415,8 +418,11 @@ const Bildgenerator = { */ async resize(factor, options) { const opts = options || {}; - const target = opts.target || this.lastAdded(); + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this.lastAdded(); if (!target) throw new Error("resize() needs an element that was added first"); + this._assertEditable(target, "resize"); if (typeof factor !== "number" || !(factor > 0)) { throw new Error("resize() needs a positive number"); } @@ -458,8 +464,11 @@ const Bildgenerator = { */ async rotate(degrees, options) { const opts = options || {}; - const target = opts.target || this.lastAdded(); + const target = opts.target !== undefined + ? this._resolveTarget(opts.target) + : this.lastAdded(); if (!target) throw new Error("rotate() needs an element that was added first"); + this._assertEditable(target, "rotate"); if (typeof degrees !== "number" || !isFinite(degrees)) { throw new Error("rotate() needs a number of degrees"); } @@ -546,7 +555,15 @@ const Bildgenerator = { height: Math.round(o.getScaledHeight()), angle: Math.round(o.angle || 0), scale: Math.round((o.scaleX || 1) * 1000) / 1000, - insideMargin: o.left >= margin - 0.5 && o.top >= margin - 0.5, + // All four edges, not just the origin. An element wider or + // taller than the usable area sits at the margin on the left + // and still runs off the right — reporting that as "inside" + // is worse than not reporting at all. + insideMargin: + o.left >= margin - 0.5 && + o.top >= margin - 0.5 && + o.left + o.getScaledWidth() <= canvas.width - margin + 0.5 && + o.top + o.getScaledHeight() <= canvas.height - margin + 0.5, editable: o !== logoObject && o !== contentImage && o.type !== "rect", }; }); @@ -581,6 +598,7 @@ const Bildgenerator = { const target = this._resolveTarget( opts.target !== undefined ? opts.target : this.lastAdded() ); + this._assertEditable(target, "update"); const spec = changes || {}; if (spec.text !== undefined) { @@ -632,15 +650,25 @@ const Bildgenerator = { if (target === contentImage) { throw new Error("The background image cannot be removed; set a different one"); } + if (target === contentRect) { + throw new Error("The canvas surface cannot be removed"); + } canvas.remove(target); if (this._lastAdded === target) this._lastAdded = null; canvas.renderAll(); return this; }, - /** Move an element to the front of the stack. */ + /** + * Move an element to the front of the stack. + * + * The UI's own handler guards contentImage and contentRect + * (event-handlers.js) — bringing the canvas surface to the front would + * cover the entire image in flat green. + */ async bringToFront(which) { const target = this._resolveTarget(which !== undefined ? which : this.lastAdded()); + this._assertEditable(target, "bringToFront"); canvas.bringToFront(target); // The organisation logo is meant to stay on top. CanvasUtils.bringLogoToFront(); @@ -648,6 +676,34 @@ const Bildgenerator = { return this; }, + /** + * Refuse to move, scale, rotate or restyle the things objects() reports as + * editable:false — the plain canvas rectangle, the background photo and + * the organisation logo. + * + * The UI protects these through its own handlers; the facade advertised + * them as off-limits in objects() but let every other method touch them. + * Dragging the logo out of its position or scaling the background breaks + * the brand, and the caller would have no way to put it back. + */ + _assertEditable(target, what) { + if (target === this._logoObject()) { + throw new Error( + `${what}() will not touch the organisation logo — ` + + `use setLogoEnabled(false) or setLogo() instead` + ); + } + if (target === contentImage) { + throw new Error( + `${what}() will not touch the background image — ` + + `use setBackground() to replace it` + ); + } + if (target === contentRect) { + throw new Error(`${what}() will not touch the canvas surface`); + } + }, + _resolveTarget(which) { if (which && typeof which === "object") return which; if (typeof which === "number") { @@ -676,10 +732,15 @@ const Bildgenerator = { */ async export(options) { const opts = options || {}; + // The download button reads the DPI from the template + // (event-handlers.js); a hardcoded 200 silently produced a different + // resolution than the same template yields for a person. + const template = TemplateConstants.getCurrentTemplate(); + const defaultDpi = (template && template.dpi) || 200; const result = await CanvasUtils.exportCanvas( opts.format || "png", typeof opts.quality === "number" ? opts.quality : 1, - typeof opts.dpi === "number" ? opts.dpi : 200 + typeof opts.dpi === "number" ? opts.dpi : defaultDpi ); const scale = result.actualDPI / 72; return { @@ -922,14 +983,43 @@ const Bildgenerator = { * canvas state rather than events — addLogo() alone is two async image * loads deep — so quiescence is the only honest completion signal. */ - async _waitUntilQuiet(settleMs = 400, timeout = 15000) { + /** + * A cheap fingerprint of everything the canvas currently shows. + * + * Counting objects is not enough. A late image load can replace an object + * or move one without changing the count — addLogo() removes the old logo + * and adds the flattened one, and replaceCanvas() re-adds a logo whose + * geometry only settles once both image decodes finish. The count is + * identical before and after; the picture is not. + */ + _canvasSignature() { + if (!canvas) return ""; + return canvas.getObjects().map(function (o) { + return [ + o.type, + Math.round(o.left), + Math.round(o.top), + Math.round(o.getScaledWidth()), + Math.round(o.getScaledHeight()), + Math.round(o.angle || 0), + o.type === "text" ? o.text : "", + ].join(","); + }).join("|"); + }, + + /** + * Wait until the canvas stops changing. The app offers no completion + * signal — addLogo() alone is two async image decodes deep — so stability + * of the rendered state is the only honest one available. + */ + async _waitUntilQuiet(settleMs = 500, timeout = 15000) { const started = Date.now(); - let last = -1; + let last = null; let stableSince = 0; while (Date.now() - started < timeout) { - const count = canvas ? canvas.getObjects().length : -1; - if (count !== last) { - last = count; + const signature = this._canvasSignature(); + if (signature !== last) { + last = signature; stableSince = Date.now(); } else if (Date.now() - stableSince >= settleMs) { return; diff --git a/resources/js/event-handlers.js b/resources/js/event-handlers.js index e50fd7c..b166c00 100644 --- a/resources/js/event-handlers.js +++ b/resources/js/event-handlers.js @@ -190,6 +190,10 @@ const EventHandlerUtils = { text.setCoords(); canvas.setActiveObject(text); updateScale(text); + // Keep the organisation logo on top. Without this a headline + // placed near the bottom covers it, which the other add handlers + // already guard against. + CanvasUtils.bringLogoToFront(); canvas.renderAll(); }); }, @@ -276,7 +280,9 @@ const EventHandlerUtils = { canvas.bringToFront(object); } }); - + + // ...and the organisation logo above those. + CanvasUtils.bringLogoToFront(); canvas.renderAll(); }); }, @@ -291,6 +297,9 @@ const EventHandlerUtils = { canvas.add(image); canvas.centerObject(image); canvas.bringToFront(image); + // The Wahlkreuz is brought to the very front above, which + // would otherwise put it over the organisation logo. + CanvasUtils.bringLogoToFront(); } ); });