Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add tb_get_cell #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions termbox2.h
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,21 @@ int tb_set_cell_ex(int x, int y, uint32_t *ch, size_t nch, uintattr_t fg,
uintattr_t bg);
int tb_extend_cell(int x, int y, uint32_t ch);

/* Get cell at specified position.
*
* If position is valid, function returns TB_OK and cell contents are copied to
* `cell`. Note if `nech>0`, then `ech` will be a pointer to memory which may
* be invalid or freed after subsequent library calls. Callers must copy this
* memory if they need to persist it for some reason. Modifying memory at `ech`
* results in undefined behavior.
*
* If `back` is non-zero, return cells from the internal back buffer. Otherwise,
* return cells from the front buffer. Note the front buffer is updated on each
* call to tb_present(), whereas the back buffer is updated immediately by
* tb_set_cell() and other functions that mutate cell contents.
*/
int tb_get_cell(int x, int y, int back, struct tb_cell *cell);

/* Sets the input mode. Termbox has two input modes:
*
* 1. TB_INPUT_ESC
Expand Down Expand Up @@ -1727,6 +1742,15 @@ int tb_set_cell_ex(int x, int y, uint32_t *ch, size_t nch, uintattr_t fg,
return TB_OK;
}

int tb_get_cell(int x, int y, int back, struct tb_cell *cell) {
if_not_init_return();
int rv;
struct tb_cell *cellp = NULL;
rv = cellbuf_get(back ? &global.back : &global.front, x, y, &cellp);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if x or y are out of bounds is it going to fail?
I think it needs check as in tb_set_cell -> if_err_return(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes cellbuf_get will return an error code if coords are out of bounds. There's a test assertion for this case here: https://github.com/termbox/termbox2/pull/65/files#diff-ff257682d1f1e118ed7186fee4abce09fca625c01728795e04d3c9a4fe95eadfR19

if (cellp) memcpy(cell, cellp, sizeof(*cell));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for better performance can we avoid memcpy and just return pointer to cell like in cellbuf_get function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I figure it's a trade-off between safety and perf. I was going for by-value semantics here (like struct tb_cell tb_get_cell(int x, int y) but reserving the return value for error codes per usual). Then the user can do whatever with their cell and it won't affect tb internals.

If the caller is concerned about an extra memcpy maybe we can assume they'd also want to avoid function overhead per-cell, so maybe tb_cell_buffer is for the perf critical use case? But tb_cell_buffer doesn't support returning the front-buffer currently. We could add another function for that (not my pref to bloat the API -- I already regret adding some like tb_set_func). Or we could change the signature of tb_cell_buffer to int tb_cell_buffer(int back, struct tb_cell **cellbuf) (also not my pref to break ABI).

Or like you suggest maybe tb_get_cell returns the pointer to our memory, and the caller could simply ask for 0, 0 if they want to index the cellbuf themselves for perf. We can warn users in documentation that mutating any memory results in undefined behavior. That seems like the best option to me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have point, I forgot about safety issues that could appear, but I will also prefer the latter. It is going to be more flexible code where one can get what one needs.

return rv;
}

int tb_extend_cell(int x, int y, uint32_t ch) {
if_not_init_return();
#ifdef TB_OPT_EGC
Expand Down
24 changes: 24 additions & 0 deletions tests/test_set_get/expected.ansi
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#5a
#5set=0
#5invalid_get=-9
#5back_get=0
#5back_ch=a
#5back_fg=3
#5back_bg=0
#5front1_get=0
#5front1_ch=
#5front1_fg=0
#5front1_bg=0
#5present=0
#5front2_get=0
#5front2_ch=a
#5front2_fg=3
#5front2_bg=0








43 changes: 43 additions & 0 deletions tests/test_set_get/test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
declare(strict_types=1);

$test->ffi->tb_init();

$w = $test->ffi->tb_width();
$h = $test->ffi->tb_height();
$green = $test->defines['TB_GREEN'];
$cell = $test->ffi->new('struct tb_cell');
$back = 1;
$front = 0;

$result = [];

$cell->ch = 42;

$result['set'] = $test->ffi->tb_set_cell(0, 0, ord('a'), $green, 0); // 0 (TB_OK)

$result['invalid_get'] = $test->ffi->tb_get_cell(-1, -1, $back, FFI::addr($cell)); // -9 (TB_ERR_OUT_OF_BOUNDS)

$result['back_get'] = $test->ffi->tb_get_cell(0, 0, $back, FFI::addr($cell)); // 0 (TB_OK)
$result['back_ch'] = chr($cell->ch); // 'a'
$result['back_fg'] = $cell->fg; // 3 (green)
$result['back_bg'] = $cell->bg; // 0

$result['front1_get'] = $test->ffi->tb_get_cell(0, 0, $front, FFI::addr($cell));
$result['front1_ch'] = chr($cell->ch); // <space> (front buffer empty)
$result['front1_fg'] = $cell->fg; // 0
$result['front1_bg'] = $cell->bg; // 0

$result['present'] = $test->ffi->tb_present(); // 0 (TB_OK) (front buffer now populated)

$result['front2_get'] = $test->ffi->tb_get_cell(0, 0, $front, FFI::addr($cell)); // 0 (TB_OK)
$result['front2_ch'] = chr($cell->ch); // 'a'
$result['front2_fg'] = $cell->fg; // 3 (green)
$result['front2_bg'] = $cell->bg; // 0

$y = 1;
foreach ($result as $k => $v) $test->ffi->tb_printf(0, $y++, 0, 0, '%s=%s', $k, "$v");

$test->ffi->tb_present();

$test->screencap();
Loading