From a77774f4dc5c74c79db05dc8e3acb3499f22326b Mon Sep 17 00:00:00 2001 From: nick black Date: Fri, 27 Mar 2020 03:49:13 -0400 Subject: [PATCH] notcurses_at_yx(): value-result u32+u64, not cell Resolves #410. notcurses_at_yx() accepted a cell*, but the gcluster of this cell was always set to 0. The EGC is instead a heap-allocated copy, returned as the primary return value. This is due to the absence of an egcpool to bind against. Existing callers can be converted thus: * instead of passing cell 'c', pass &(c)->attrword, &(c)->channels * either initialize 'c' with CELL_TRIVIAL_INITIALIZER, or set its gcluster field to 0 following the call I've updated all calls from tests/demos, updated the docs, and updated the C++ and Python wrappers. --- CHANGELOG.md | 6 ++++++ README.md | 8 ++++---- doc/man/man3/notcurses_render.3.md | 8 +++++++- include/ncpp/NotCurses.hh | 4 ++-- include/notcurses/notcurses.h | 8 ++++---- python/src/notcurses/build_notcurses.py | 2 +- src/lib/render.c | 14 +++++++------- tests/ncplane.cpp | 24 ++++++++++++------------ tests/palette.cpp | 2 +- tests/render.cpp | 16 ++++++++-------- 10 files changed, 52 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6246a591b..434115b78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ rearrangements of Notcurse. ** `ncvisual_render()` now interprets length parameters of -1 to mean "to the end along this axis", and no longer interprets 0 to mean this. 0 now means "a length of 0", resulting in a zero-area rendering. +** `notcurses_at_yx()` no longer accepts a `cell*` as its last parameter. + Instead, it accepts a `uint32_t*` and a `uint64_t*`, and writes the + attribute and channels to these parameters. This was done because the + `gcluster` field of the `cell*` was always set to 0, which was surprising + and a source of blunders. The EGC is returned via the `char*` return + value. https://github.com/dankamongmen/notcurses/issues/410 * 1.2.4 2020-03-24 ** Add ncmultiselector diff --git a/README.md b/README.md index b63bfe054..8b16e484c 100644 --- a/README.md +++ b/README.md @@ -257,10 +257,10 @@ updated to reflect the changes: int notcurses_render(struct notcurses* nc); // Retrieve the contents of the specified cell as last rendered. The EGC is -// returned, or NULL on error. This EGC must be free()d by the caller. The cell -// 'c' is not bound to a plane, and thus its gcluster value must not be used-- -// use the return value only. -char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, cell* c); +// returned, or NULL on error. This EGC must be free()d by the caller. The +// attrword and channels are written to 'attrword' and 'channels', respectively. +char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, + uint32_t* attrword, uint64_t* channels); ``` One `ncplane` is guaranteed to exist: the "standard plane". The user cannot diff --git a/doc/man/man3/notcurses_render.3.md b/doc/man/man3/notcurses_render.3.md index 845586bd4..6239b0788 100644 --- a/doc/man/man3/notcurses_render.3.md +++ b/doc/man/man3/notcurses_render.3.md @@ -12,7 +12,7 @@ notcurses_render - sync the physical display to the virtual ncplanes **int notcurses_render(struct notcurses* nc);** -**char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, cell* c);** +**char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, uint32_t* attrword, uint64_t* channels);** # DESCRIPTION @@ -63,12 +63,18 @@ If the algorithm concludes without an EGC, the cell is rendered with no glyph and a default background. If the algorithm concludes without a color locked in, the color as computed thus far is used. +**notcurses_at_yx** retrieves a call *as rendered*. The EGC in that cell is +copied and returned; it must be **free(3)**d by the caller. + # RETURN VALUES On success, 0 is returned. On failure, a non-zero value is returned. A success will result in the **renders** stat being increased by 1. A failure will result in the **failed_renders** stat being increased by 1. +**notcurses_at_yx** returns a heap-allocated copy of the cell's EGC on success, +and **NULL** on failure. + # BUGS In addition to the RGB colors, it is possible to use the "default foreground color" diff --git a/include/ncpp/NotCurses.hh b/include/ncpp/NotCurses.hh index 3e7400ebd..e512b8403 100644 --- a/include/ncpp/NotCurses.hh +++ b/include/ncpp/NotCurses.hh @@ -203,9 +203,9 @@ namespace ncpp return notcurses_getc_nblock (nc, ni); } - char* get_at (int yoff, int xoff, Cell &c) const noexcept + char* get_at (int yoff, int xoff, uint32_t* attr, uint64_t* channels) const noexcept { - return notcurses_at_yx (nc, yoff, xoff, c); + return notcurses_at_yx (nc, yoff, xoff, attr, channels); } void drop_planes () const noexcept diff --git a/include/notcurses/notcurses.h b/include/notcurses/notcurses.h index 5e082ee89..418c04854 100644 --- a/include/notcurses/notcurses.h +++ b/include/notcurses/notcurses.h @@ -381,10 +381,10 @@ notcurses_term_dim_yx(struct notcurses* n, int* RESTRICT rows, int* RESTRICT col } // Retrieve the contents of the specified cell as last rendered. The EGC is -// returned, or NULL on error. This EGC must be free()d by the caller. The cell -// 'c' is not bound to a plane, and thus its gcluster value must not be used-- -// use the return value only. -API char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, cell* c); +// returned, or NULL on error. This EGC must be free()d by the caller. The +// attrword and channels are written to 'attrword' and 'channels', respectively. +API char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, + uint32_t* attrword, uint64_t* channels); // Alignment within the ncplane. Left/right-justified, or centered. typedef enum { diff --git a/python/src/notcurses/build_notcurses.py b/python/src/notcurses/build_notcurses.py index f734c4dee..aa0ea17fc 100644 --- a/python/src/notcurses/build_notcurses.py +++ b/python/src/notcurses/build_notcurses.py @@ -129,7 +129,7 @@ int ncplane_move_bottom(struct ncplane* n); int ncplane_move_below(struct ncplane* n, struct ncplane* below); int ncplane_move_above(struct ncplane* n, struct ncplane* above); struct ncplane* ncplane_below(struct ncplane* n); -char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, cell* c); +char* notcurses_at_yx(struct notcurses* nc, int yoff, int xoff, uint32_t* attrword, uint64_t* channels); int ncplane_at_cursor(struct ncplane* n, cell* c); int ncplane_at_yx(struct ncplane* n, int y, int x, cell* c); void* ncplane_set_userptr(struct ncplane* n, void* opaque); diff --git a/src/lib/render.c b/src/lib/render.c index 92dbd1ae9..c4f2eedb9 100644 --- a/src/lib/render.c +++ b/src/lib/render.c @@ -1040,16 +1040,16 @@ int notcurses_render(notcurses* nc){ return ret; } -char* notcurses_at_yx(notcurses* nc, int y, int x, cell* c){ +char* notcurses_at_yx(notcurses* nc, int yoff, int xoff, uint32_t* attrword, uint64_t* channels){ char* egc = NULL; if(nc->lastframe){ - if(y >= 0 && y < nc->lfdimy){ - if(x >= 0 || x < nc->lfdimx){ - const cell* srccell = &nc->lastframe[y * nc->lfdimx + x]; - memcpy(c, srccell, sizeof(*c)); // unsafe copy of gcluster -//fprintf(stderr, "COPYING: %d from %p\n", c->gcluster, &nc->pool); + if(yoff >= 0 && yoff < nc->lfdimy){ + if(xoff >= 0 || xoff < nc->lfdimx){ + const cell* srccell = &nc->lastframe[yoff * nc->lfdimx + xoff]; + *attrword = srccell->attrword; + *channels = srccell->channels; +//fprintf(stderr, "COPYING: %d from %p\n", srccell->gcluster, &nc->pool); egc = pool_egc_copy(&nc->pool, srccell); - c->gcluster = 0; // otherwise cell_release() will blow up } } } diff --git a/tests/ncplane.cpp b/tests/ncplane.cpp index f5161803f..636dd6817 100644 --- a/tests/ncplane.cpp +++ b/tests/ncplane.cpp @@ -922,14 +922,14 @@ TEST_CASE("NCPlane") { REQUIRE(0 < ncplane_at_yx(ncp, 1, 0, &c)); CHECK(!strcmp(cell_extended_gcluster(ncp, &c), "│")); cell_release(ncp, &c); - char* egc = notcurses_at_yx(nc_, 1, 0, &c); + char* egc = notcurses_at_yx(nc_, 1, 0, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp(egc, "│")); free(egc); REQUIRE(0 < ncplane_at_yx(ncp, 1, 3, &c)); CHECK(!strcmp(cell_extended_gcluster(ncp, &c), "│")); cell_release(ncp, &c); - egc = notcurses_at_yx(nc_, 1, 3, &c); + egc = notcurses_at_yx(nc_, 1, 3, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp(egc, "│")); free(egc); @@ -950,16 +950,16 @@ TEST_CASE("NCPlane") { ncplane_at_yx(n_, 0, 4, &c); CHECK(!cell_double_wide_p(&c)); REQUIRE(0 == notcurses_render(nc_)); - notcurses_at_yx(nc_, 0, 0, &c); - CHECK(cell_double_wide_p(&c)); - notcurses_at_yx(nc_, 0, 1, &c); - CHECK(cell_double_wide_p(&c)); - notcurses_at_yx(nc_, 0, 2, &c); - CHECK(cell_double_wide_p(&c)); - notcurses_at_yx(nc_, 0, 3, &c); - CHECK(cell_double_wide_p(&c)); - notcurses_at_yx(nc_, 0, 4, &c); - CHECK(!cell_double_wide_p(&c)); + notcurses_at_yx(nc_, 0, 0, &c.attrword, &c.channels); + CHECK(0 != (c.channels & 0x8000000080000000ull)); + notcurses_at_yx(nc_, 0, 1, &c.attrword, &c.channels); + CHECK(0 != (c.channels & 0x8000000080000000ull)); + notcurses_at_yx(nc_, 0, 2, &c.attrword, &c.channels); + CHECK(0 != (c.channels & 0x8000000080000000ull)); + notcurses_at_yx(nc_, 0, 3, &c.attrword, &c.channels); + CHECK(0 != (c.channels & 0x8000000080000000ull)); + notcurses_at_yx(nc_, 0, 4, &c.attrword, &c.channels); + CHECK(!(c.channels & 0x8000000080000000ull)); } SUBCASE("Perimeter") { diff --git a/tests/palette.cpp b/tests/palette.cpp index 4eb087cd5..0e6e253b0 100644 --- a/tests/palette.cpp +++ b/tests/palette.cpp @@ -102,7 +102,7 @@ TEST_CASE("Palette256") { cell_release(n_, &c); CHECK(0 == notcurses_render(nc_)); cell r = CELL_TRIVIAL_INITIALIZER; - CHECK(nullptr != notcurses_at_yx(nc_, 0, 0, &r)); + CHECK(nullptr != notcurses_at_yx(nc_, 0, 0, &r.attrword, &r.channels)); CHECK(cell_fg_palindex_p(&r)); CHECK(cell_bg_palindex_p(&r)); CHECK(CELL_ALPHA_OPAQUE == cell_fg_alpha(&r)); diff --git a/tests/render.cpp b/tests/render.cpp index be0f34bfb..c0e62bbd7 100644 --- a/tests/render.cpp +++ b/tests/render.cpp @@ -48,7 +48,7 @@ TEST_CASE("RenderTest") { CHECK(!strcmp("\xe5\x85\xa8", egc)); CHECK(cell_double_wide_p(&c)); free(egc); - egc = notcurses_at_yx(nc_, 0, 0, &c); + egc = notcurses_at_yx(nc_, 0, 0, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("\xe5\x85\xa8", egc)); CHECK(cell_double_wide_p(&c)); @@ -61,7 +61,7 @@ TEST_CASE("RenderTest") { CHECK(!strcmp("", egc)); CHECK(cell_double_wide_p(&c)); free(egc); - egc = notcurses_at_yx(nc_, 0, 1, &c); + egc = notcurses_at_yx(nc_, 0, 1, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("", egc)); CHECK(cell_double_wide_p(&c)); @@ -75,7 +75,7 @@ TEST_CASE("RenderTest") { CHECK(!strcmp("\xe5\xbd\xa2", egc)); CHECK(cell_double_wide_p(&c)); free(egc); - egc = notcurses_at_yx(nc_, 0, 2, &c); + egc = notcurses_at_yx(nc_, 0, 2, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("\xe5\xbd\xa2", egc)); CHECK(cell_double_wide_p(&c)); @@ -88,7 +88,7 @@ TEST_CASE("RenderTest") { CHECK(!strcmp("", egc)); CHECK(cell_double_wide_p(&c)); free(egc); - egc = notcurses_at_yx(nc_, 0, 3, &c); + egc = notcurses_at_yx(nc_, 0, 3, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("", egc)); CHECK(cell_double_wide_p(&c)); @@ -101,26 +101,26 @@ TEST_CASE("RenderTest") { CHECK(!notcurses_render(nc_)); // should be nothing, having been stomped - egc = notcurses_at_yx(nc_, 0, 0, &c); + egc = notcurses_at_yx(nc_, 0, 0, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp(" ", egc)); free(egc); cell_init(&c); // should be character from higher plane - egc = notcurses_at_yx(nc_, 0, 1, &c); + egc = notcurses_at_yx(nc_, 0, 1, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("A", egc)); free(egc); cell_init(&c); - egc = notcurses_at_yx(nc_, 0, 2, &c); + egc = notcurses_at_yx(nc_, 0, 2, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp("B", egc)); free(egc); cell_init(&c); // should be nothing, having been stomped - egc = notcurses_at_yx(nc_, 0, 3, &c); + egc = notcurses_at_yx(nc_, 0, 3, &c.attrword, &c.channels); REQUIRE(egc); CHECK(!strcmp(" ", egc)); free(egc);