sl0ppty: zero strength is not a pass

`shader_abi.h` has said `0 is identity` from the start, and the pass did not
honour it: it materialised a cell's default colours -- resolving "whatever the
terminal draws" into our `default_fg`/`default_bg` -- and only then called a
shader that was contractually going to change nothing.

So a chain that is currently *off* still repainted. `focused * 200` on an
unfocused pane, `(since < 500) * 255` half a second later, `(x < 10) * 200`
from column ten on: every one of those wrote our idea of the default over the
terminal's own. Visible whenever the two differ, which for a real terminal
theme is always, and invisible in the test suite because the tests compared
against the materialised value.

Found by measuring, not by reading: I was checking that each example I was
about to hand over actually changed the pane, and one that should have changed
nothing changed eight style runs.

The fix is an ordering: work out the strength, and if it is zero leave the cell
before touching it. A constant-zero shader now skips its whole rect before the
loops. Cheaper too -- a pass that is off costs no materialisation and no call --
though that is a side effect and not the reason.

Three test changes, all of them the same correction. `tests/shader_test.c`'s
traversal probe was built `{.kind, .fn}` with `amount` defaulting to 0, so it
was asking to be called for every cell at zero strength; it now asks for 255,
which is what those checks are about. A new block pins the rule: a zero-strength
pass visits nothing and leaves an unset cell unset. And four checks in
`tests/test_shader_expr.py` asserted `== "#ffffff"` for cells outside an
effect's band -- the materialised default. They now assert the cell has no
colour of its own at all, which is the stronger claim and the one that would
have caught this.

4 files changed, 65 insertions, 17 deletions

diff --git a/src/shader.c b/src/shader.c
index ba2888a..56a0a2b 100644
--- a/src/shader.c
+++ b/src/shader.c
@@ -367,6 +367,15 @@ void shade_apply(screen_t *s, const shader_t *shaders, size_t n, rect_t r,
      * once per pass rather than per cell, because copying a shader_t for every
      * cell would cost more than evaluating the expression. */
     shader_t local = *sh;
+    /* `0 is identity` (shader_abi.h), so a pass at zero strength is not a pass
+     * that runs and changes nothing -- it is one that does not run. The
+     * difference is visible: the loop below materialises a cell's default
+     * colours before calling a shader, so an identity pass would leave the
+     * terminal's own background written out as ours. A chain like
+     * `focused * 200` on an unfocused pane, or `(since < 500) * 255` half a
+     * second later, would faintly repaint the pane it is meant to be leaving
+     * alone. */
+    if (!sh->amount_expr && !sh->amount) continue;
     const uint8_t *map = NULL;
     expr_env_t env = {0};
     bool per_cell = false;
@@ -416,6 +425,19 @@ void shade_apply(screen_t *s, const shader_t *shaders, size_t n, rect_t r,
          * it would be computing a colour nothing can display. */
         if (!c->width) continue;
 
+        /* Strength first, because zero means this cell is not this pass's
+         * business and materialising it below would be the whole of the
+         * effect. */
+        ctx.x = x;
+        if (map) {
+          local.amount = map[(size_t)y * w + x];
+        } else if (per_cell) {
+          env.x = x;
+          env.y = y;
+          local.amount = (uint8_t)clamp255(expr_eval(sh->amount_expr, &env));
+        }
+        if (!local.amount) continue;
+
         /* What the cell was, for whichever colour this pass does not keep.
          * Taken before materialising, so a colour the terminal was drawing in
          * its own default goes back to *unset* rather than to our idea of it —
@@ -429,14 +451,6 @@ void shade_apply(screen_t *s, const shader_t *shaders, size_t n, rect_t r,
         if (!c->fg.set) c->fg = ctx.default_fg;
         if (!c->bg.set) c->bg = ctx.default_bg;
 
-        ctx.x = x;
-        if (map) {
-          local.amount = map[(size_t)y * w + x];
-        } else if (per_cell) {
-          env.x = x;
-          env.y = y;
-          local.amount = (uint8_t)clamp255(expr_eval(sh->amount_expr, &env));
-        }
         local.fn(&local, &ctx, c);
 
         if (!keep_fg) c->fg = was_fg;
diff --git a/src/shader_abi.h b/src/shader_abi.h
index dbd840d..96b7069 100644
--- a/src/shader_abi.h
+++ b/src/shader_abi.h
@@ -111,7 +111,11 @@ struct shader {
   shade_fn fn;
   expr_prog_t *amount_expr; /* NULL when `amount` is just a number */
   color_t color;  /* the target colour, for shaders that have one */
-  uint8_t amount; /* strength, 0..255; 0 is identity, 255 is fully applied */
+  /* Strength, 0..255; 0 is identity, 255 is fully applied. A cell whose
+   * strength works out to 0 is skipped by the pass rather than handed to the
+   * shader, so a plugin is never called to do nothing -- and a cell the
+   * terminal was drawing in its own default colour stays that way. */
+  uint8_t amount;
   /* One number whose meaning is the shader's own, because a second parameter
    * that is a column for one effect and a radius for another is not really
    * two things:
diff --git a/tests/shader_test.c b/tests/shader_test.c
index a8f68d3..872ff99 100644
--- a/tests/shader_test.c
+++ b/tests/shader_test.c
@@ -324,7 +324,9 @@ int main(void) {
       for (uint16_t x = 0; x < 8; x++)
         put(&s, x, y, rgb(0x11, 0x22, 0x33), rgb(0, 0, 0));
 
-    shader_t sh = {.kind = "probe", .fn = probe_fn};
+    /* Strength 255: the probe ignores it, but a pass at 0 is an identity and
+     * identities are skipped, so a traversal test has to ask for a real one. */
+    shader_t sh = {.kind = "probe", .fn = probe_fn, .amount = 255};
     shade_ctx_t base = base_ctx();
     base.now_ms = 1234;
     base.focused = true;
@@ -338,6 +340,7 @@ int main(void) {
        probe.x == 1 && probe.y == 1, "");
     ok("and the cell at that position is the right one",
        ceq(probe.saw_fg, 0x11, 0x22, 0x33), shown(probe.saw_fg));
+
     ok("time is passed through for animation", probe.now_ms == 1234, "");
     ok("so is focus", probe.focused, "");
 
@@ -349,13 +352,33 @@ int main(void) {
     screen_free(&s);
   }
 
+  /* ---- zero strength is not a pass at all ---- */
+  {
+    memset(&probe, 0, sizeof probe);
+    screen_init(&s, 4, 2); /* every colour unset: the terminal's own */
+    shader_t idle = {.kind = "probe", .fn = probe_fn}; /* amount 0 */
+    shade_ctx_t base = base_ctx();
+    shade_apply(&s, &idle, 1, (rect_t){0, 0, 4, 2}, NULL, &base);
+
+    ok("a pass at zero strength visits nothing", probe.calls == 0, "");
+    /* `0 is identity` in the ABI, and the pass used to materialise a cell's
+     * default colours before honouring that -- so a chain like `focused * 200`
+     * repainted the frame of the pane it was leaving alone. */
+    ok("and a default-coloured cell is left saying so",
+       screen_at(&s, 0, 0)->fg.set == false && screen_at(&s, 0, 0)->bg.set == false,
+       "an identity pass materialised the terminal's default into ours");
+    screen_free(&s);
+  }
+
   /* ---- the hole: a frame is a rect minus its contents ---- */
   {
     memset(&probe, 0, sizeof probe);
     screen_init(&s, 6, 5);
     fill(&s, rgb(0xff, 0xff, 0xff), rgb(0xff, 0xff, 0xff));
 
-    shader_t sh = {.kind = "probe", .fn = probe_fn};
+    /* Strength 255: the probe ignores it, but a pass at 0 is an identity and
+     * identities are skipped, so a traversal test has to ask for a real one. */
+    shader_t sh = {.kind = "probe", .fn = probe_fn, .amount = 255};
     shade_ctx_t base = base_ctx();
     rect_t hole = {1, 1, 4, 3};
     shade_apply(&s, &sh, 1, (rect_t){0, 0, 6, 5}, &hole, &base);
@@ -381,7 +404,9 @@ int main(void) {
     memset(&probe, 0, sizeof probe);
     screen_init(&s, 4, 2);
     fill(&s, rgb(0xff, 0xff, 0xff), rgb(0xff, 0xff, 0xff));
-    shader_t sh = {.kind = "probe", .fn = probe_fn};
+    /* Strength 255: the probe ignores it, but a pass at 0 is an identity and
+     * identities are skipped, so a traversal test has to ask for a real one. */
+    shader_t sh = {.kind = "probe", .fn = probe_fn, .amount = 255};
     shade_ctx_t base = base_ctx();
     rect_t empty = {1, 1, 0, 0};
     shade_apply(&s, &sh, 1, (rect_t){0, 0, 4, 2}, &empty, &base);
diff --git a/tests/test_shader_expr.py b/tests/test_shader_expr.py
index 136026d..834e5c1 100644
--- a/tests/test_shader_expr.py
+++ b/tests/test_shader_expr.py
@@ -24,6 +24,11 @@ def cfg(text):
     return f.name
 
 
+# `None` means the cell carries no colour of its own -- the terminal's default,
+# which is what a cell the pass never touched still looks like. A cell whose
+# strength worked out to zero reads as None rather than as our idea of the
+# default, because a pass at zero strength is skipped rather than run to no
+# effect: that is the difference between "left alone" and "repainted the same".
 def fg_row(snap, pane, row=0):
     y = pane["content_y"] + row
     return [(snap.style_at(pane["content_x"] + x, y) or {}).get("fg")
@@ -37,8 +42,8 @@ def test_an_expression_decides_the_amount_per_cell():
         row = fg_row(snap, s.pane())
         check("the first ten columns are dimmed",
               all(c == row[0] and c != "#ffffff" for c in row[:10]), str(row[:12]))
-        check("and the rest are not",
-              all(c == "#ffffff" for c in row[10:20]), str(row[10:20]))
+        check("and the rest are untouched, not repainted the same",
+              all(c is None for c in row[10:20]), str(row[10:20]))
 
 
 def test_a_constant_expression_is_just_a_number():
@@ -62,7 +67,7 @@ def test_the_rect_is_known_to_the_expression():
         pane = s.pane()
         row = fg_row(snap, pane)
         check("the last columns are dimmed, not the first",
-              row[0] == "#ffffff" and row[-1] != "#ffffff",
+              row[0] is None and row[-1] not in (None, "#ffffff"),
               f"{row[0]} .. {row[-1]}")
 
 
@@ -78,7 +83,7 @@ def test_it_follows_a_resize():
         # The cached map has to be rebuilt for the new rect, or the dimmed
         # band would still be sitting where the old right-hand edge was.
         check("the band moved with the edge",
-              row[-1] != "#ffffff" and row[len(row) - 8] == "#ffffff",
+              row[-1] not in (None, "#ffffff") and row[len(row) - 8] is None,
               str(row[-10:]))
 
 
@@ -106,7 +111,7 @@ def test_states_take_expressions_too():
         snap = s.snapshot()
         r0 = fg_row(snap, unfocused, 0)
         r1 = fg_row(snap, unfocused, 1)
-        check("even rows are left alone", r0[0] == "#ffffff", str(r0[:3]))
+        check("even rows are left alone", r0[0] is None, str(r0[:3]))
         check("odd rows are dimmed", r1[0] != "#ffffff" and r1[0] is not None,
               str(r1[:3]))