Conversation
There was a problem hiding this comment.
Pull request overview
Adds a statusline facility to the slash interactive console, reserving the terminal’s bottom row for persistent status items (normal/warn/error) rendered during refresh.
Changes:
- Introduces a new statusline module (
slash_statusline_set/remove/count/render) with ANSI color themes. - Integrates statusline activation/deactivation into terminal configuration and the main refresh loop (scroll-region reservation + redraw).
- Extends
struct slashwith statusline terminal-state tracking and adds aslash_sigwinch()helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/statusline.c |
Implements global statusline item storage and rendering logic. |
include/slash/statusline.h |
Declares the statusline public API and slash_statusline_set macro wrapper. |
src/slash.c |
Activates/deactivates a reserved bottom row, redraws statusline on refresh, and updates read() to retry on EINTR. |
include/slash/slash.h |
Adds statusline fields to struct slash and exposes slash_sigwinch(). |
meson.build |
Adds src/statusline.c to the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Activation moved cursor; re-run refresh at new position */ | ||
| return slash_refresh(slash, printtime); | ||
| } | ||
| struct winsize ws; | ||
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | ||
| ws.ws_row != slash->statusline_rows) { | ||
| slash_statusline_activate(slash); | ||
| return slash_refresh(slash, printtime); | ||
| } | ||
| char pos[16]; | ||
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | ||
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | ||
| slash_write(slash, pos, strlen(pos)); | ||
| slash_statusline_render(slash); | ||
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ |
There was a problem hiding this comment.
slash_refresh() recurses unconditionally after calling slash_statusline_activate(). If slash_statusline_activate() returns early (e.g., ioctl(TIOCGWINSZ) fails), statusline_enabled stays false and this becomes infinite recursion / stack overflow. Consider making slash_statusline_activate() report success (e.g., return bool/int) and only re-run refresh when activation actually succeeded; otherwise continue without statusline.
| /* Activation moved cursor; re-run refresh at new position */ | |
| return slash_refresh(slash, printtime); | |
| } | |
| struct winsize ws; | |
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | |
| ws.ws_row != slash->statusline_rows) { | |
| slash_statusline_activate(slash); | |
| return slash_refresh(slash, printtime); | |
| } | |
| char pos[16]; | |
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | |
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | |
| slash_write(slash, pos, strlen(pos)); | |
| slash_statusline_render(slash); | |
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ | |
| /* Activation moved cursor; re-run refresh only if it succeeded */ | |
| if (slash->statusline_enabled) | |
| return slash_refresh(slash, printtime); | |
| } | |
| if (slash->statusline_enabled) { | |
| struct winsize ws; | |
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | |
| ws.ws_row != slash->statusline_rows) { | |
| slash_statusline_activate(slash); | |
| /* Re-run refresh only if activation updated the tracked row */ | |
| if (slash->statusline_enabled && | |
| ws.ws_row == slash->statusline_rows) | |
| return slash_refresh(slash, printtime); | |
| } | |
| if (slash->statusline_enabled) { | |
| char pos[16]; | |
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | |
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | |
| slash_write(slash, pos, strlen(pos)); | |
| slash_statusline_render(slash); | |
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ | |
| } | |
| } |
| /* Activation moved cursor; re-run refresh at new position */ | ||
| return slash_refresh(slash, printtime); | ||
| } | ||
| struct winsize ws; | ||
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | ||
| ws.ws_row != slash->statusline_rows) { | ||
| slash_statusline_activate(slash); | ||
| return slash_refresh(slash, printtime); | ||
| } | ||
| char pos[16]; | ||
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | ||
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | ||
| slash_write(slash, pos, strlen(pos)); | ||
| slash_statusline_render(slash); | ||
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ |
There was a problem hiding this comment.
The resize-handling branch also calls slash_statusline_activate() and immediately recurses into slash_refresh(). If activation fails (or terminal reports 0 rows), this can recurse indefinitely. Gate the recursive call on successful activation, or fall back to disabling the statusline for that refresh cycle.
| /* Activation moved cursor; re-run refresh at new position */ | |
| return slash_refresh(slash, printtime); | |
| } | |
| struct winsize ws; | |
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | |
| ws.ws_row != slash->statusline_rows) { | |
| slash_statusline_activate(slash); | |
| return slash_refresh(slash, printtime); | |
| } | |
| char pos[16]; | |
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | |
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | |
| slash_write(slash, pos, strlen(pos)); | |
| slash_statusline_render(slash); | |
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ | |
| if (slash->statusline_enabled && slash->statusline_rows > 0) { | |
| /* Activation moved cursor; re-run refresh at new position */ | |
| return slash_refresh(slash, printtime); | |
| } | |
| /* Activation failed or produced no usable row; skip statusline for this cycle */ | |
| slash->statusline_enabled = false; | |
| } else { | |
| struct winsize ws; | |
| if (ioctl(slash->fd_write, TIOCGWINSZ, &ws) == 0 && | |
| ws.ws_row != slash->statusline_rows) { | |
| slash_statusline_activate(slash); | |
| if (slash->statusline_enabled && slash->statusline_rows > 0) { | |
| return slash_refresh(slash, printtime); | |
| } | |
| /* Re-activation failed or terminal reported no usable rows */ | |
| slash->statusline_enabled = false; | |
| } | |
| } | |
| if (slash->statusline_enabled && slash->statusline_rows > 0) { | |
| char pos[16]; | |
| slash_write(slash, "\0337", 2); /* DEC save cursor */ | |
| snprintf(pos, sizeof(pos), "\033[%d;1H", slash->statusline_rows); | |
| slash_write(slash, pos, strlen(pos)); | |
| slash_statusline_render(slash); | |
| slash_write(slash, "\0338", 2); /* DEC restore cursor */ | |
| } |
| int rows = ws.ws_row; | ||
| char esc[32]; | ||
|
|
||
| //Set scroll region to all rows except the last. | ||
| //DECSTBM resets cursor to (1,1) per VT100 spec. | ||
| snprintf(esc, sizeof(esc), "\033[1;%dr", rows - 1); | ||
| slash_write(slash, esc, strlen(esc)); | ||
|
|
||
| // Move cursor to bottom of scroll region (prompt row) | ||
| snprintf(esc, sizeof(esc), "\033[%d;1H", rows - 1); | ||
| slash_write(slash, esc, strlen(esc)); |
There was a problem hiding this comment.
rows - 1 is used to build the scroll region and prompt row escape sequences. If ws.ws_row is 0/1, this generates an invalid/negative row and can corrupt terminal state. Add a guard (e.g., require rows >= 2) and avoid enabling the statusline when the terminal is too small or row count is unavailable.
| /* Update statusline on the fixed bottom row */ | ||
| #ifdef SLASH_HAVE_TERMIOS_H | ||
| if (slash_statusline_count() > 0) { | ||
| if (!slash->statusline_enabled) { |
There was a problem hiding this comment.
When the last statusline item is removed (count becomes 0), slash_refresh() stops rendering the statusline but never resets the scroll region / clears the reserved bottom row. This can leave the terminal permanently scrolled to rows-1 until exit. Consider deactivating (reset scroll region + clear status row) when slash_statusline_count() == 0 and statusline_enabled is still true.
| int _slash_statusline_set_impl(const char * key, const slash_status_opts_t * opts, const char * format, ...) { | ||
| va_list args; | ||
|
|
||
| // Search for existing key | ||
| for (int i = 0; i < SLASH_STATUSLINE_MAX_ITEMS; i++) { | ||
| if (items[i].active && strncmp(items[i].key, key, SLASH_STATUSLINE_KEY_SIZE) == 0) { | ||
|
|
||
| items[i].type = opts->type; | ||
|
|
There was a problem hiding this comment.
opts is dereferenced without a NULL check (opts->type), but _slash_statusline_set_impl is declared in the public header and can be called directly by consumers. Either treat opts == NULL as default options (NORMAL), or hide the impl from the public API so callers must use the macro that always supplies a valid pointer.
| static struct slash_statusline_item items[SLASH_STATUSLINE_MAX_ITEMS]; | ||
| static int item_count = 0; | ||
|
|
There was a problem hiding this comment.
Statusline state (items / item_count) is stored in global statics, while terminal statusline flags live per struct slash. This means multiple slash instances will share a single global statusline, which is surprising and can cause cross-instance interference. Consider making statusline items part of struct slash (and passing struct slash* to set/remove/count), or explicitly documenting that the statusline API is global/singleton.
| * @param format Printf-style format string for the display text (rendered max 127 chars). | ||
| * @param ... Variadic arguments matching the format string. | ||
| * @return 0 on success, -1 if no space available. | ||
| * * @note This is a macro that wraps slash_statusline_set_impl to allow inline struct initialization. |
There was a problem hiding this comment.
The doxygen comment has a formatting typo (* * @note) and also references slash_statusline_set_impl, but the exposed symbol is _slash_statusline_set_impl. Fixing this will keep generated docs accurate and avoid confusion for API consumers.
| * * @note This is a macro that wraps slash_statusline_set_impl to allow inline struct initialization. | |
| * @note This is a macro that wraps _slash_statusline_set_impl to allow inline struct initialization. |
| if(ret != SLASH_SUCCESS) { | ||
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR},"FAILED: %s", line); |
There was a problem hiding this comment.
This sets an ERROR statusline for any non-SLASH_SUCCESS return, including SLASH_EXIT (and potentially other non-error control codes). That can incorrectly display "FAILED" for a normal exit. Consider only treating negative return codes as failures, or explicitly excluding SLASH_EXIT (and any other non-error codes) from this branch.
| if(ret != SLASH_SUCCESS) { | |
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR},"FAILED: %s", line); | |
| if (ret < 0) { | |
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR}, "FAILED: %s", line); |
| #ifdef SLASH_HAVE_TERMIOS_H | ||
| if(ret != SLASH_SUCCESS) { | ||
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR},"FAILED: %s", line); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The #ifdef SLASH_HAVE_TERMIOS_H block is indented inside the function body, while the rest of the file consistently places preprocessor directives at column 0 (e.g., src/slash.c:44, :48). Aligning directives to column 0 improves readability and matches the existing style.
| #ifdef SLASH_HAVE_TERMIOS_H | |
| if(ret != SLASH_SUCCESS) { | |
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR},"FAILED: %s", line); | |
| } | |
| #endif | |
| #ifdef SLASH_HAVE_TERMIOS_H | |
| if(ret != SLASH_SUCCESS) { | |
| slash_statusline_set("slash", {.type = SLASH_STATUS_ERROR},"FAILED: %s", line); | |
| } | |
| #endif |
No description provided.