From 5eb23cefb069ff4c7451deb735842514b9e9a6f7 Mon Sep 17 00:00:00 2001
From: nick black <dankamongmen@gmail.com>
Date: Tue, 7 Dec 2021 19:53:37 -0500
Subject: [PATCH] [ncplane_put] subtle cursor verification fix

We were checking for an off-plane cursor destination on
the X axis when it was provided explicitly, but not when
-1 was used to indicate the current position (contradicting
the comment immediately above the test). We also want to do
so when we're using the current position, though in this
case scrolling must be taken into account. Also, we were
placing the cursor on such a validation failure, despite not
intending to write anything.

Not testing for a current x (the -1 case) usually worked
because it was thrown out by ncplane_move_cursor_yx() below.
That fails, however, when we're working with an EGC that's
more than one column.

Delicate!
---
 src/lib/notcurses.c | 41 ++++++++++++++++++++++++++---------------
 src/tests/fade.cpp  |  1 -
 src/tests/wide.cpp  |  2 +-
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/src/lib/notcurses.c b/src/lib/notcurses.c
index 432f8dc47..c7158ed36 100644
--- a/src/lib/notcurses.c
+++ b/src/lib/notcurses.c
@@ -654,7 +654,7 @@ void ncplane_home(ncplane* n){
 int ncplane_cursor_move_yx(ncplane* n, int y, int x){
   if(x < 0){
     if(x < -1){
-      logerror("Negative target x %d\n", x);
+      logerror("negative target x %d\n", x);
       return -1;
     }
   }else if((unsigned)x >= n->lenx){
@@ -665,7 +665,7 @@ int ncplane_cursor_move_yx(ncplane* n, int y, int x){
   }
   if(y < 0){
     if(y < -1){
-      logerror("Negative target y %d\n", y);
+      logerror("negative target y %d\n", y);
       return -1;
     }
   }else if((unsigned)y >= n->leny){
@@ -1713,39 +1713,50 @@ static int
 ncplane_put(ncplane* n, int y, int x, const char* egc, int cols,
             uint16_t stylemask, uint64_t channels, int bytes){
   if(n->sprite){
-    logerror("Can't write [%s] to sprixelated plane\n", egc);
+    logerror("can't write [%s] to sprixelated plane\n", egc);
     return -1;
   }
   // reject any control character for output other than newline (and then only
   // on a scrolling plane).
   if(*egc == '\n'){
+    // if we're not scrolling, autogrow would be to the right (as opposed to
+    // down), and thus it still wouldn't apply to the case of a newline.
     if(!n->scrolling){
-      logerror("Rejecting newline on non-scrolling plane\n");
+      logerror("rejecting newline on non-scrolling plane\n");
       return -1;
     }
   }else if(is_control_egc((const unsigned char*)egc, bytes)){
-    logerror("Rejecting %dB control character\n", bytes);
+    logerror("rejecting %dB control character\n", bytes);
     return -1;
   }
   // check *before ncplane_cursor_move_yx()* whether we're past the end of the
   // line. if scrolling is enabled, move to the next line if so. if x or y are
-  // specified, we must always try to print at exactly that location.
-  if(x != -1){
-    if((unsigned)x + cols > n->lenx){
-      logerror("Target x %d + %d cols [%.*s] > length %d\n", x, cols, bytes, egc, n->lenx);
-      ncplane_cursor_move_yx(n, y, x); // update cursor, though
-      return -1;
+  // specified, we must always try to print at exactly that location, and
+  // there's no need to check the present location in that dimension.
+  //
+  // check x for all negatives; only -1 is valid, but our else clause is
+  // predicated on a non-negative x.
+  if(x < 0){
+    if(n->x + cols - 1 >= n->lenx){
+      if(!n->scrolling){
+        logerror("target x %d [%.*s] > length %d\n", n->x, bytes, egc, n->lenx);
+        return -1;
+      }
+      scroll_down(n);
     }
-  }else if(y == -1 && n->x + cols > n->lenx){
-    if(!n->scrolling){
-      logerror("No room to output [%.*s] %d/%d\n", bytes, egc, n->y, n->x);
+  }else{
+    if((unsigned)x + cols - 1 >= n->lenx){
+      logerror("no room for %d cols [%.*s] at length %d\n", cols, bytes, egc, n->lenx);
       return -1;
     }
-    scroll_down(n);
   }
+  // explicit targets outside the plane will be rejected here.
   if(ncplane_cursor_move_yx(n, y, x)){
     return -1;
   }
+  // FIXME scroll_down() always performs a virtual scroll of the plane, right?
+  // we don't need that unless we're at the bottom! is this right?!? FIXME
+  // FIXME note also the scroll_down() above--very fishy! FIXME
   if(*egc == '\n'){
     scroll_down(n);
   }
diff --git a/src/tests/fade.cpp b/src/tests/fade.cpp
index e11d00356..ddf458db7 100644
--- a/src/tests/fade.cpp
+++ b/src/tests/fade.cpp
@@ -41,7 +41,6 @@ TEST_CASE("Fade") {
   unsigned dimy, dimx;
   ncplane_dim_yx(n_, &dimy, &dimx);
   nccell c = NCCELL_CHAR_INITIALIZER('*');
-  nccell_set_fg_rgb8(&c, 0xff, 0xff, 0xff);
   unsigned rgb = 0xffffffu;
   CHECK(!ncplane_set_scrolling(n_, true));
   for(unsigned y = 0 ; y < dimy ; ++y){
diff --git a/src/tests/wide.cpp b/src/tests/wide.cpp
index 49c84295e..87df516c7 100644
--- a/src/tests/wide.cpp
+++ b/src/tests/wide.cpp
@@ -57,7 +57,7 @@ TEST_CASE("Wide") {
     unsigned y, x;
     ncplane_cursor_yx(n_, &y, &x);
     CHECK(0 == y);
-    CHECK(dimx - 1 == x);
+    CHECK(0 == x);
     CHECK(0 == notcurses_render(nc_));
   }