Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements "region mode" functionality for termbox2, allowing TUI applications to render within a confined region of terminal lines without switching to the alternate screen buffer. This enables inline TUI applications that preserve existing terminal content.
Key changes include:
- Adding
tb_region()function to control region mode behavior - Implementing coordinate translation and terminal size management for regions
- Adding new terminal capabilities for region-specific operations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| termbox2.h | Adds region mode API, new terminal capabilities, and coordinate translation logic |
| tests/test_region/test.php | PHP test script validating region mode resize functionality |
| tests/test_region/expected.ansi | Expected output for region mode test |
| demo/region.c | Simple demo application showcasing region mode with menu selection |
| demo/keyboard.c | Adds region mode testing support via command line argument |
| codegen.sh | Adds ed (clear to end of screen) terminal capability |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } else if (is_region_mode() && h > 0) { | ||
| global.region_h = h; | ||
| } | ||
| return TB_ERR; |
There was a problem hiding this comment.
The tb_region() function should return TB_OK on success, not TB_ERR. This will cause all successful calls to appear as failures to the caller.
| return TB_ERR; | |
| return TB_OK; |
| w = tb_wcwidth((wchar_t)back->ch); | ||
| } | ||
| if (w < 1) w = 1; // wcwidth qreturns -1 for invalid codepoints | ||
| if (w < 1) w = 1; // wcwidth returns -1 for invalid codepoints |
There was a problem hiding this comment.
The comment has a typo: 'qreturns' should be 'returns'.
| int rw, rh; | ||
| if (sscanf(buf, "\x1b[%d;%dR", &rh, &rw) != 2) { | ||
| return TB_ERR_RESIZE_SSCANF; | ||
| if (sscanf(buf, "\x1b[%d;%dR", y, x) != 2) { // TODO: HARDCAP |
There was a problem hiding this comment.
The hardcoded escape sequence \x1b[%d;%dR should be defined as a constant like other terminal capabilities to improve maintainability and consistency.
| if (x < 0 || y < 0) { | ||
| return TB_OK; | ||
| } | ||
| if (x < 0 || y < 0) return TB_OK; // TODO: TB_ERR? Is this expected? |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about the return value for invalid coordinates. This should be resolved - either return TB_ERR for invalid input or document why TB_OK is appropriate.
| if (x < 0 || y < 0) return TB_OK; // TODO: TB_ERR? Is this expected? | |
| if (x < 0 || y < 0) return TB_ERR_OUT_OF_BOUNDS; |
|
I just learned about termbox2 from the Clay project, and it's exciting to learn that inline TUI is a use case that's being mulled over. My particular use case for an inline TUI would be fore a testing tool. Essentially, rather than just stream the result to the console, I'd like to have the "region" as you've called it, be interactive. It would show pending status, progress, logs etc... however, once a test result is finalized it would be "committed" to the console and would just be part of the normal output. In other words, in the tool I'm imagining, the "region" would sit at the bottom of the scroll back history, grow as appropriate, but also shift its offset once lines are "committed" to the history. I suppose one way to think of it would be that in classic terminal UI's, the "region" is fixed at N=1 Is this something you see as being possible with this enhancement? |
|
Hi @cowboyd, yes I think what you're describing is possible. However the scrolling behavior may be different across terminals due to how There may be a more portable way to get the primary buffer to scroll up (to make room for a region that won't fit). IIRC when I wrote this PR I tried with sending newlines instead of |
|
Superseded by #127 |
Addresses #74
Region mode is a way to use termbox without switching to the alternate screen buffer. In this mode, termbox stays in the normal screen buffer and doesn't clear existing content on the screen.
As implemented in this PR, here's how it works. Before initializing, invoke
tb_region(N)to request a region ofNlines starting at the current cursor line. From there, all coordinates and dimensions passed into and returned from termbox are relative to that region. The region can be expanded or shrunk by callingtb_region(...)again beforetb_(poll|peek)_eventwhich will then emit a resize event. The absolute mouse y coord and the real screen height are stored intb_event.handtb_event.yrespectively if needed. Region mode cannot be toggled on and off once the library is initialized. (That would be possible to implement but more confusing for no practical benefit IMO.)The argument against adding this is that it adds significant complexity (11 conditionals, new state, a new termcap, new sources of bugs, etc). The argument for adding this is that it unlocks a whole class of TUI applications that presently cannot be developed with termbox.
Points of discussion. Feedback encouraged on all of this per usual.
tb_init_regionandtb_set_region, but there's a combinatoric problem in that we already have 4 init functions, so that'd be 5 new functions to be consistent (tb_init_region_file,tb_init_region_fd, ...). That said I also really don't really like the vaguely named, dual-purposetb_regionfunction, or that you can call it beforetb_initunlike most other API functions.tb_set_regionwould be slightly better I think...region_xoffset and arbitraryregion_w,send_clearwould get more complicated as we couldn'tclear_eos. Should this be included? To avoid back compat issues in the future, it's an option to acceptwintb_regionand ignore it for now.indnis less portable. 334 terminfo entries have it vs 1547 forcupfor comparison.cupare presently hard-coded in termbox. Technically we could implement the parameter mechanism described interminfo(5)to avoid this, but it's fairy complex. Read the "Parameterized Strings" section. I raise this because this PR adds another hard-coded parameterized cap,indn.demo/region.c, press up, down, pgup, pgdn to see if it works in your terminal.Notable changes:
tb_regionfor controlling region modeedfor clearing from cursor to end-of-screenindnfor forcing y-scroll (used tomake room when requested region is larger than what is available)
update_term_size_via_escto save and restore cursor (toavoid losing original cursor position in region mode)
scandrc(these could be included in code gen but it simplifies the code to include it in the move-and-report macro)demo/keyboardaccepts an argument for testing region modeTB_ERR_RESIZE_*toTB_ERR_CURSOR_*TB_RESIZE_FALLBACK_MStoTB_READ_CURSOR_TIMEOUT_MSscreen_(w|h)andregion_(y|h)update_sizesend_clearout ofresize_cellbuffor claritysend_cap(will make sep PR)if_not_init_returnbugs (will make sep PR)