From 1108ebb5b6a90ef970b3b6b9fc2749f2082b7976 Mon Sep 17 00:00:00 2001 From: nick black Date: Fri, 15 Jan 2021 00:32:55 -0500 Subject: [PATCH] Fix up some subtle pile issues ncplane_destroy() needs to call ncplane_reparent_family(), not ncplane_reparent() as it was doing (closes #1291). ->absy and ->absx actually are absolute; return them directly for an O(1) ncplane_abs_yx() (down from O(N), huzzah). Add some unit tests related to destroying and reparenting (#1286). Add ncplane_abs_y() and ncplane_abs_x(), document them, etc. --- NEWS.md | 3 +-- USAGE.md | 6 ++++-- doc/man/man3/notcurses_plane.3.md | 4 ++++ include/notcurses/notcurses.h | 6 ++++-- src/lib/internal.h | 2 +- src/lib/notcurses.c | 34 +++++++++++++++---------------- tests/piles.cpp | 10 +++++---- tests/plane.cpp | 19 ++++++++++++----- 8 files changed, 51 insertions(+), 33 deletions(-) diff --git a/NEWS.md b/NEWS.md index 934db35f6..763768ac8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,8 +9,7 @@ rearrangements of Notcurses. `ncplane_putnstr_yx()` now return the number of columns output, as long documented (they were mistakenly returning the number of bytes). * `ncplane_abs_yx()` has been added, returning the absolute coordinates of - the plane's origin (i.e. coordinates relative to its pile). This call is - O(N) on the binding depth of the plane in question. + the plane's origin (i.e. coordinates relative to its pile). * 2.1.4 (2021-01-03): * Direct mode now supports `NCDIRECT_OPTION_NO_QUIT_SIGHANDLERS`, and by diff --git a/USAGE.md b/USAGE.md index 7dd5b955a..75699adf0 100644 --- a/USAGE.md +++ b/USAGE.md @@ -808,9 +808,11 @@ void ncplane_yx(const struct ncplane* n, int* restrict y, int* restrict x); int ncplane_y(const struct ncplane* n); int ncplane_x(const struct ncplane* n); -// Get the origin of plane 'n' relative to its pile. This is O(N) on the -// binding depth of 'n'. Either or both of 'x' and y' may be NULL. +// Get the origin of plane 'n' relative to its pile. Either or both of 'x' and +// 'y' may be NULL. void ncplane_abs_yx(const struct ncplane* n, int* y, int* x); +int ncplane_abs_y(const struct ncplane* n); +int ncplane_abs_x(const struct ncplane* n); // Return the dimensions of this ncplane. void ncplane_dim_yx(struct ncplane* n, int* restrict rows, int* restrict cols); diff --git a/doc/man/man3/notcurses_plane.3.md b/doc/man/man3/notcurses_plane.3.md index 5e3c6d289..7230702eb 100644 --- a/doc/man/man3/notcurses_plane.3.md +++ b/doc/man/man3/notcurses_plane.3.md @@ -63,6 +63,10 @@ typedef struct ncplane_options { **void ncplane_abs_yx(const struct ncplane* ***n***, int* ***y***, int* ***x***);** +**int ncplane_abs_y(const struct ncplane* ***n***);** + +**int ncplane_abs_x(const struct ncplane* ***n***);** + **struct ncplane* ncplane_parent(struct ncplane* ***n***);** **const struct ncplane* ncplane_parent_const(const struct ncplane* ***n***);** diff --git a/include/notcurses/notcurses.h b/include/notcurses/notcurses.h index d16dd7087..7bdaa8aca 100644 --- a/include/notcurses/notcurses.h +++ b/include/notcurses/notcurses.h @@ -1317,9 +1317,11 @@ API void ncplane_yx(const struct ncplane* n, int* RESTRICT y, int* RESTRICT x); API int ncplane_y(const struct ncplane* n); API int ncplane_x(const struct ncplane* n); -// Get the origin of plane 'n' relative to its pile. This is O(N) on the -// binding depth of 'n'. Either or both of 'x' and y' may be NULL. +// Get the origin of plane 'n' relative to its pile. Either or both of 'x' and +// 'y' may be NULL. API void ncplane_abs_yx(const struct ncplane* n, int* RESTRICT y, int* RESTRICT x); +API int ncplane_abs_y(const struct ncplane* n); +API int ncplane_abs_x(const struct ncplane* n); // Get the plane to which the plane 'n' is bound, if any. API struct ncplane* ncplane_parent(struct ncplane* n); diff --git a/src/lib/internal.h b/src/lib/internal.h index e2b6833e1..351c3cbfc 100644 --- a/src/lib/internal.h +++ b/src/lib/internal.h @@ -67,7 +67,7 @@ typedef struct ncplane { // ncplane_yx() etc. use coordinates relative to the plane to which this // plane is bound, but absx/absy are always relative to the terminal origin. // they must thus be translated by any function which moves a parent plane. - int absx, absy; // origin of the plane relative to the screen + int absx, absy; // origin of the plane relative to the pile's origin int lenx, leny; // size of the plane, [0..len{x,y}) is addressable egcpool pool; // attached storage pool for UTF-8 EGCs uint64_t channels; // works the same way as cells diff --git a/src/lib/notcurses.c b/src/lib/notcurses.c index ef8bc9416..b6483cd53 100644 --- a/src/lib/notcurses.c +++ b/src/lib/notcurses.c @@ -630,6 +630,7 @@ int ncplane_destroy(ncplane* ncp){ logerror(ncplane_notcurses(ncp), "Won't destroy standard plane\n"); return -1; } +//notcurses_debug(ncplane_notcurses(ncp), stderr); if(ncp->above){ ncp->above->below = ncp->below; }else{ @@ -649,7 +650,7 @@ int ncplane_destroy(ncplane* ncp){ struct ncplane* bound = ncp->blist; while(bound){ struct ncplane* tmp = bound->bnext; - if(ncplane_reparent(bound, ncp->boundto) == NULL){ + if(ncplane_reparent_family(bound, ncp->boundto) == NULL){ ret = -1; } bound = tmp; @@ -2147,17 +2148,21 @@ const notcurses* ncplane_notcurses_const(const ncplane* n){ return ncplane_pile_const(n)->nc; } -static void -ncplane_abs_yx_recurse(const ncplane* n, int* RESTRICT y, int* RESTRICT x){ - if(n->boundto != n){ - ncplane_translate(n, n->boundto, y, x); - ncplane_abs_yx_recurse(n->boundto, y, x); - } +int ncplane_abs_y(const ncplane* n){ + return n->absy; // FIXME adjust for margins? +} + +int ncplane_abs_x(const ncplane* n){ + return n->absx; // FIXME adjust for margins? } void ncplane_abs_yx(const ncplane* n, int* RESTRICT y, int* RESTRICT x){ - ncplane_yx(n, y, x); - ncplane_abs_yx_recurse(n->boundto, y, x); + if(y){ + *y = ncplane_abs_y(n); + } + if(x){ + *x = ncplane_abs_x(n); + } } ncplane* ncplane_parent(ncplane* n){ @@ -2218,6 +2223,7 @@ ncplane* ncplane_reparent(ncplane* n, ncplane* newparent){ if(n->boundto == newparent){ return n; } +//notcurses_debug(ncplane_notcurses(n), stderr); if(n->blist){ if(n->boundto == n){ // children become new root planes ncplane* lastlink; @@ -2244,7 +2250,7 @@ ncplane* ncplane_reparent(ncplane* n, ncplane* newparent){ } n->blist = NULL; } -//notcurses_debug(ncplane_notcurses(n), stderr); + // FIXME would be nice to skip ncplane_descendant_p() on this call...:/ return ncplane_reparent_family(n, newparent); } @@ -2292,14 +2298,10 @@ ncplane* ncplane_reparent_family(ncplane* n, ncplane* newparent){ if(ncplane_descendant_p(newparent, n)){ return NULL; } +//notcurses_debug(ncplane_notcurses(n), stderr); if(n->boundto == newparent){ // no-op return n; } - // if we are not a root plane, adjust our origin - if(n->boundto != n){ - n->absx -= n->boundto->absx; - n->absy -= n->boundto->absy; - } if(n->bprev){ // extract from sibling list if( (*n->bprev = n->bnext) ){ n->bnext->bprev = n->bprev; @@ -2321,8 +2323,6 @@ ncplane* ncplane_reparent_family(ncplane* n, ncplane* newparent){ pthread_mutex_unlock(&ncplane_notcurses(n)->pilelock); splice_zaxis_recursive(n); }else{ // establish ourselves as a sibling of new parent's children - n->absx += n->boundto->absx; - n->absy += n->boundto->absy; if( (n->bnext = newparent->blist) ){ n->bnext->bprev = &n->bnext; } diff --git a/tests/piles.cpp b/tests/piles.cpp index 349f160d0..97924d066 100644 --- a/tests/piles.cpp +++ b/tests/piles.cpp @@ -246,12 +246,14 @@ TEST_CASE("Piles") { CHECK(30 == x); CHECK(10 == ncplane_y(gen3)); CHECK(10 == ncplane_x(gen3)); +notcurses_debug(nc_, stderr); CHECK(nullptr != ncplane_reparent(gen1, gen2)); +notcurses_debug(nc_, stderr); ncplane_abs_yx(gen2, &y, &x); // gen2 is now the parent CHECK(20 == y); CHECK(20 == x); - CHECK(20 == ncplane_y(gen1)); - CHECK(20 == ncplane_x(gen1)); + CHECK(20 == ncplane_y(gen2)); + CHECK(20 == ncplane_x(gen2)); ncplane_abs_yx(gen1, &y, &x); // gen1 is below and to the left of gen2 CHECK(10 == y); CHECK(10 == x); @@ -260,8 +262,8 @@ TEST_CASE("Piles") { ncplane_abs_yx(gen3, &y, &x); CHECK(30 == y); // should stay the same CHECK(30 == x); - CHECK(20 == ncplane_y(gen3)); // should reflect new parentage - CHECK(20 == ncplane_x(gen3)); + CHECK(10 == ncplane_y(gen3)); // remain the same; still a child of gen2 + CHECK(10 == ncplane_x(gen3)); ncplane_destroy(gen1); ncplane_destroy(gen2); ncplane_destroy(gen3); diff --git a/tests/plane.cpp b/tests/plane.cpp index 1bc116b22..d410b8eef 100644 --- a/tests/plane.cpp +++ b/tests/plane.cpp @@ -913,17 +913,26 @@ TEST_CASE("Plane") { int absy, absx; CHECK(0 == notcurses_render(nc_)); ncplane_yx(nsub, &absy, &absx); - CHECK(1 == absy); // actually at 2, 2 + CHECK(1 == absy); // relatively at 1, 1 CHECK(1 == absx); + ncplane_abs_yx(nsub, &absy, &absx); + CHECK(2 == absy); // actually at 2, 2 + CHECK(2 == absx); ncplane_reparent(nsub, nsub); CHECK(ncplane_pile(nsub) != ncplane_pile(ndom)); + ncplane_abs_yx(nsub, &absy, &absx); + CHECK(2 == absy); // now relatively at 1, 1 + CHECK(2 == absx); ncplane_yx(nsub, &absy, &absx); - CHECK(1 == absy); // now truly at 1, 1 - CHECK(1 == absx); + CHECK(2 == absy); + CHECK(2 == absx); CHECK(0 == ncplane_move_yx(ndom, 0, 0)); + ncplane_abs_yx(nsub, &absy, &absx); + CHECK(2 == absy); // still at 1, 1 + CHECK(2 == absx); ncplane_yx(nsub, &absy, &absx); - CHECK(1 == absy); // still at 1, 1 - CHECK(1 == absx); + CHECK(2 == absy); + CHECK(2 == absx); } SUBCASE("NoReparentStdPlane") {