From 2b6fc32425d0c1a51b42e22ad91f3bb8b3d8e066 Mon Sep 17 00:00:00 2001
From: Hermes <hermes@hermes.local>
Date: Mon, 11 May 2026 14:08:51 +0000
Subject: [PATCH] review fixes: 3 blocking bugs + 2 improvements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

B1: modern-backend now inherits from backend (was standalone class)
B2: draw-rect y-position bug — loop now tracks row offset
B3: Layout module added to ASDF system definition
I1: 6 smoke tests replaced with behavioral tests (captured output)
I3: 6 edge case tests: empty, single-child, zero-size, deep
    nesting, large padding, negative grow

Also fixed:
- Added missing make-simple-backend constructor to simple.lisp
- Added in-package to classes.lisp and simple.lisp (SBCL's load
  restores *package* after each load, breaking batch-mode loading)
---
 backend/classes.lisp |   2 +
 backend/modern.lisp  |  18 ++++----
 backend/simple.lisp  |   7 +++
 backend/tests.lisp   | 106 +++++++++++++++++++++++++++++--------------
 cl-tui.asd           |  11 ++++-
 layout/tests.lisp    |  55 ++++++++++++++++++++++
 6 files changed, 153 insertions(+), 46 deletions(-)

diff --git a/backend/classes.lisp b/backend/classes.lisp
index 8214110..368f9d2 100644
--- a/backend/classes.lisp
+++ b/backend/classes.lisp
@@ -1,3 +1,5 @@
+(in-package :cl-tui.backend)
+
 (defclass backend () ())
 
 (defgeneric initialize-backend (backend)
diff --git a/backend/modern.lisp b/backend/modern.lisp
index 8842dcc..b83597a 100644
--- a/backend/modern.lisp
+++ b/backend/modern.lisp
@@ -111,7 +111,7 @@
                  (case pos (:horizontal "─") (:vertical "│"))
                  "+"))))
 
-(defclass modern-backend ()
+(defclass modern-backend (backend)
   ((output-stream :initform *standard-output*
                   :accessor backend-output-stream)
    (in-sync-p :initform nil :accessor in-sync-p)))
@@ -199,14 +199,14 @@
     (backend-write b bot)))
 
 (defmethod draw-rect ((b modern-backend) x y width height &key bg)
-  (let ((bg-esc (sgr-bg bg))
-        (reset (sgr-attr :reset))
-        (line (concatenate 'string
-                bg-esc
-                (make-string width :initial-element #\Space)
-                reset (string #\Newline))))
-    (loop repeat height do
-      (backend-write b (cursor-move-escape x y))
+  (let* ((bg-esc (sgr-bg bg))
+         (reset (sgr-attr :reset))
+         (line (concatenate 'string
+                 bg-esc
+                 (make-string width :initial-element #\Space)
+                 reset (string #\Newline))))
+    (loop :for row :from 0 :below height :do
+      (backend-write b (cursor-move-escape x (+ y row)))
       (backend-write b line))))
 
 (defmethod draw-link ((b modern-backend) x y string url
diff --git a/backend/simple.lisp b/backend/simple.lisp
index 5e482b8..ab82279 100644
--- a/backend/simple.lisp
+++ b/backend/simple.lisp
@@ -1,7 +1,14 @@
+(in-package :cl-tui.backend)
+
 (defclass simple-backend (backend)
   ((output-stream :initform *standard-output*
+                  :initarg :output-stream
                   :accessor backend-output-stream)))
 
+(defun make-simple-backend (&key output-stream)
+  (make-instance 'simple-backend
+    :output-stream (or output-stream *standard-output*)))
+
 (defmethod initialize-backend ((b simple-backend))
   b)
 
diff --git a/backend/tests.lisp b/backend/tests.lisp
index fd9b7f9..01d8359 100644
--- a/backend/tests.lisp
+++ b/backend/tests.lisp
@@ -6,6 +6,14 @@
 (def-suite backend-suite :description "Backend protocol tests")
 (in-suite backend-suite)
 
+;; ── Helpers ─────────────────────────────────────────────────────
+
+(defun make-capturing-backend ()
+  "Create a simple-backend that writes to a string stream."
+  (let* ((s (make-string-output-stream))
+         (b (make-simple-backend :output-stream s)))
+    (values b s)))
+
 ;; ── Simple Backend ──────────────────────────────────────────────
 
 (defun run-tests ()
@@ -19,32 +27,55 @@
   (let ((b (make-simple-backend)))
     (is (typep b 'simple-backend))
     (initialize-backend b)
-    (is (capable-p b :truecolor) nil "simple backend has no truecolor")
+    (is-false (capable-p b :truecolor) "simple backend has no truecolor")
     (shutdown-backend b)))
 
 (test simple-backend-draw-text
   "simple-backend renders text at position, ignoring style"
-  (let ((b (make-simple-backend)))
+  (multiple-value-bind (b s) (make-capturing-backend)
     (initialize-backend b)
-    (draw-text b 0 0 "hello" nil nil)
+    (draw-text b 0 0 "hello" :red nil :bold t :italic t)
     (shutdown-backend b)
-    (is-t t)))
+    (is (string= (get-output-stream-string s) "hello")
+        "draw-text should output the string ignoring style")))
 
-(test simple-backend-border-single
-  "simple-backend draws ASCII single border"
-  (let ((b (make-simple-backend)))
+(test simple-backend-draw-border
+  "simple-backend draws ASCII border with +-| characters"
+  (multiple-value-bind (b s) (make-capturing-backend)
     (initialize-backend b)
-    (draw-border b 0 0 10 5 :style :single)
+    (draw-border b 0 0 5 3 :style :single)
     (shutdown-backend b)
-    (is-t t)))
+    (let ((out (get-output-stream-string s)))
+      (is (search "-----" out) "top edge should have 5 dashes")
+      (is (search "|   |" out) "middle row should have pipe sides"))))
 
-(test simple-backend-border-rounded
-  "simple-backend falls back to straight edges for rounded"
-  (let ((b (make-simple-backend)))
+(test simple-backend-draw-rounded
+  "simple-backend falls back to straight edges for rounded style"
+  (multiple-value-bind (b s) (make-capturing-backend)
     (initialize-backend b)
-    (draw-border b 0 0 10 5 :style :rounded)
+    (draw-border b 0 0 5 3 :style :rounded)
     (shutdown-backend b)
-    (is-t t)))
+    (let ((out (get-output-stream-string s)))
+      ;; Rounded falls back to ASCII — identical output to single
+      (is (search "-----" out) "rounded style produces same dashes as single"))))
+
+(test simple-backend-draw-link
+  "simple-backend renders link as plain text"
+  (multiple-value-bind (b s) (make-capturing-backend)
+    (initialize-backend b)
+    (draw-link b 0 0 "click me" "http://example.com")
+    (shutdown-backend b)
+    (is (string= (get-output-stream-string s) "click me")
+        "simple-backend ignores URL, outputs text only")))
+
+(test simple-backend-draw-ellipsis
+  "simple-backend renders ... for ellipsis"
+  (multiple-value-bind (b s) (make-capturing-backend)
+    (initialize-backend b)
+    (draw-ellipsis b 0 0 5)
+    (shutdown-backend b)
+    (is (string= (get-output-stream-string s) "...")
+        "ellipsis should output 3 dots")))
 
 ;; ── Backend Capabilities ───────────────────────────────────────
 
@@ -54,8 +85,8 @@
     (initialize-backend b)
     (dolist (f '(:truecolor :osc8 :sync :mouse :bracketed-paste
                  :kitty-keyboard :sixel :cursor-style))
-      (is (capable-p b f) nil
-          (format nil "~s should not be supported on simple-backend" f)))
+      (is-false (capable-p b f)
+                (format nil "~s should not be supported on simple-backend" f)))
     (shutdown-backend b)))
 
 ;; ── Backend Size ───────────────────────────────────────────────
@@ -71,32 +102,37 @@
       (is (>= lines 3)))
     (shutdown-backend b)))
 
-;; ── Drawing Primitives ─────────────────────────────────────────
+;; ── Backend Protocol: Defaults and No-ops ──────────────────────
 
-(test draw-rect-fills-area
-  "draw-rect fills a rectangular area with background"
+(test default-methods-are-no-ops
+  "Default backend methods don't error"
   (let ((b (make-simple-backend)))
     (initialize-backend b)
-    (draw-rect b 0 0 5 3 :bg nil)
-    (shutdown-backend b)
-    (is-t t)))
-
-(test draw-text-multi-line
-  "draw-text handles strings with newlines"
-  (let ((b (make-simple-backend)))
-    (initialize-backend b)
-    (draw-text b 0 0 "line1~%line2" nil nil)
-    (shutdown-backend b)
-    (is-t t)))
-
-;; ── Synchronization ────────────────────────────────────────────
+    (is (null (multiple-value-list (cursor-hide b))))
+    (is (null (multiple-value-list (cursor-show b))))
+    (is (null (multiple-value-list (cursor-style b :block))))
+    (is (null (multiple-value-list (begin-sync b))))
+    (is (null (multiple-value-list (end-sync b))))
+    (shutdown-backend b)))
 
 (test sync-is-noop-on-simple
-  "begin-sync and end-sync are no-ops on simple-backend"
-  (let ((b (make-simple-backend)))
+  "begin-sync and end-sync produce no output on simple-backend"
+  (multiple-value-bind (b s) (make-capturing-backend)
     (initialize-backend b)
     (begin-sync b)
     (draw-text b 0 0 "in sync" nil nil)
     (end-sync b)
     (shutdown-backend b)
-    (is-t t)))
+    (is (string= (get-output-stream-string s) "in sync")
+        "no sync escape sequences should appear")))
+
+;; ── Draw-rect ──────────────────────────────────────────────────
+
+(test draw-rect-fills-area-correctly
+  "draw-rect with background writes nothing to output (simple-backend no-op)"
+  (multiple-value-bind (b s) (make-capturing-backend)
+    (initialize-backend b)
+    (draw-rect b 0 0 5 3 :bg :red)
+    (shutdown-backend b)
+    (is (string= (get-output-stream-string s) "")
+        "draw-rect is a no-op on simple-backend")))
diff --git a/cl-tui.asd b/cl-tui.asd
index 83034d1..d22a94a 100644
--- a/cl-tui.asd
+++ b/cl-tui.asd
@@ -2,7 +2,7 @@
 (asdf:defsystem :cl-tui
   :description "Reusable Common Lisp Terminal UI Framework"
   :author "Amr Gharbeia"
-  :version "0.0.1"
+  :version "0.0.3"
   :license "TBD"
   :depends-on (:fiveam)
   :components
@@ -10,7 +10,11 @@
     :components
     ((:file "package")
      (:file "classes" :depends-on ("package"))
-     (:file "simple" :depends-on ("package" "classes")))))
+     (:file "simple" :depends-on ("package" "classes"))
+     (:file "modern" :depends-on ("package" "classes"))))
+   (:module "layout"
+    :components
+    ((:file "layout"))))
   :in-order-to ((test-op (test-op :cl-tui-tests))))
 
 (asdf:defsystem :cl-tui-tests
@@ -18,6 +22,9 @@
   :depends-on (:cl-tui :fiveam)
   :components
   ((:module "backend"
+    :components
+    ((:file "tests")))
+   (:module "layout"
     :components
     ((:file "tests"))))
   :perform (test-op (o c)
diff --git a/layout/tests.lisp b/layout/tests.lisp
index 80b1065..5054bea 100644
--- a/layout/tests.lisp
+++ b/layout/tests.lisp
@@ -118,3 +118,58 @@
     (let ((sc (layout-node-children sidebar)))
       (is (= (layout-node-y (elt sc 0)) 0))
       (is (= (layout-node-y (elt sc 1)) 3)))))
+
+;; ── Edge Cases ────────────────────────────────────────────────
+
+(test empty-container-does-not-crash
+  "compute-layout on a node with no children should not error"
+  (let ((r (make-layout-node)))
+    (compute-layout r 20 20)
+    (is (integerp (layout-node-width r)))
+    (is (integerp (layout-node-height r)))))
+
+(test single-child-in-column
+  "A column with one child places it correctly"
+  (let* ((r (make-layout-node :direction :column :width 10 :height 20))
+         (c (make-layout-node :height 5)))
+    (layout-node-add-child r c)
+    (compute-layout r 10 20)
+    (is (= (layout-node-y c) 0))
+    (is (= (layout-node-height c) 5))))
+
+(test zero-size-container
+  "compute-layout with zero available space should not error"
+  (let* ((r (make-layout-node :direction :column))
+         (c (make-layout-node :height 5)))
+    (layout-node-add-child r c)
+    (compute-layout r 0 0)
+    (is (integerp (layout-node-x c)))
+    (is (integerp (layout-node-y c)))))
+
+(test deep-nesting-three-levels
+  "Three-level deep nesting produces correct leaf positions"
+  (let* ((out (vbox ()                                    ; outer box
+                (vbox (:grow 1)                           ; middle box
+                  (make-layout-node :height 2))))         ; leaf
+         (leaf (elt (layout-node-children
+                      (elt (layout-node-children out) 0)) 0)))
+    (compute-layout out 20 20)
+    (is (= (layout-node-y leaf) 0))))
+
+(test large-padding-leaves-room
+  "Large padding reduces content area but doesn't crash"
+  (let* ((r (make-layout-node :direction :column
+                              :padding '(:top 5 :left 5 :bottom 5 :right 5)))
+         (c (make-layout-node :height 3)))
+    (layout-node-add-child r c)
+    (compute-layout r 20 20)
+    (is (= (layout-node-x c) 5))
+    (is (= (layout-node-y c) 5))))
+
+(test negative-grow-is-clamped
+  "Grow values are adjusted but still compute"
+  (let* ((r (make-layout-node :direction :row :width 10))
+         (c (make-layout-node :width 5 :grow -1)))
+    (layout-node-add-child r c)
+    (compute-layout r 10 10)
+    (is (integerp (layout-node-width c)))))