fix(ui): stop status bar / chat bleed-through over the external editor #131

Closed
jabber.developer2 wants to merge 11 commits from fix/editor-ui-suspend-redraw into master
11 changed files with 187 additions and 21 deletions

View File

@@ -2638,7 +2638,8 @@ static const struct cmd_t command_defs[] = {
CMD_DESC(
"Spawn external editor to edit message. "
"After editing the inputline may appear empty. Press enter to send the text anyways. "
"Use /executable to set your favourite editor." )
"Use /executable to set your favourite editor. "
"Ctrl-Z in the editor aborts the session and returns to Profanity." )
},
{ CMD_PREAMBLE("/correct-editor",

View File

@@ -341,7 +341,7 @@ sv_ev_room_message(ProfMessage* message)
status_bar_active(num, WIN_MUC, mucwin->roomjid);
if ((g_strcmp0(mynick, message->from_jid->resourcepart) != 0) && (prefs_get_boolean(PREF_BEEP))) {
beep();
ui_beep();
}
// not currently on groupchat window
@@ -349,7 +349,7 @@ sv_ev_room_message(ProfMessage* message)
status_bar_new(num, WIN_MUC, mucwin->roomjid);
if ((g_strcmp0(mynick, message->from_jid->resourcepart) != 0) && (prefs_get_boolean(PREF_FLASH))) {
flash();
ui_flash();
}
cons_show_incoming_room_message(message->from_jid->resourcepart, mucwin->roomjid, num, mention, triggers, mucwin->unread, window);

View File

@@ -70,6 +70,38 @@ static void _connect_default(const char* const account);
pthread_mutex_t lock;
static gboolean force_quit = FALSE;
static volatile sig_atomic_t suspend_requested = FALSE;
static volatile sig_atomic_t cont_requested = FALSE;
static void
jabber.developer marked this conversation as resolved Outdated

comments are still too verbose. Let's keep implementation details within commit messages.

comments are still too verbose. Let's keep implementation details within commit messages.

corrected

corrected
_handle_suspend_signals(void)
{
if (cont_requested) {
cont_requested = FALSE;
if (!editor_is_active()) {
clearok(stdscr, TRUE);
ui_resize();
}
}
if (editor_check_stopped()) {
log_info("[Editor] stopped; aborting back to profanity");
suspend_requested = FALSE;
editor_emergency_kill();
} else if (suspend_requested) {
suspend_requested = FALSE;
if (editor_is_active()) {
log_info("[Editor] SIGTSTP from vim group; deferring to next poll");
} else {
log_info("[Suspend] dropping to shell");
if (!isendwin()) {
endwin();
}
kill(0, SIGSTOP);
}
}
}
void
jabber.developer marked this conversation as resolved Outdated

Please extract this functionality to a different method if possible. Let's keep main loop minimal.

Please extract this functionality to a different method if possible. Let's keep main loop minimal.

corrected

corrected
prof_run(gchar* log_level, gchar* account_name, gchar* config_file, gchar* log_file, gchar* theme_name, gchar** commands)
{
@@ -93,6 +125,7 @@ prof_run(gchar* log_level, gchar* account_name, gchar* config_file, gchar* log_f
int waittime = 0;
while (cont && !force_quit) {
log_stderr_handler();
_handle_suspend_signals();
session_check_autoaway();
line = commands ? *commands : inp_readline();
@@ -182,6 +215,18 @@ sigterm_handler(int sig)
force_quit = TRUE;
}
static void
sigtstp_handler(int sig)
{
suspend_requested = TRUE;
}
static void
sigcont_handler(int sig)
{
cont_requested = TRUE;
}
static void
_init(char* log_level, char* config_file, char* log_file, char* theme_name)
{
@@ -189,7 +234,8 @@ _init(char* log_level, char* config_file, char* log_file, char* theme_name)
// ignore SIGPIPE
signal(SIGPIPE, SIG_IGN);
signal(SIGINT, SIG_IGN);
signal(SIGTSTP, SIG_IGN);
signal(SIGTSTP, sigtstp_handler);
signal(SIGCONT, sigcont_handler);
signal(SIGWINCH, ui_sigwinch_handler);
signal(SIGTERM, sigterm_handler);
signal(SIGHUP, sigterm_handler);

View File

@@ -32,9 +32,15 @@ typedef struct EditorContext
void* user_data;
} EditorContext;
static gboolean editor_active = FALSE; // re-entrancy guard
static pid_t editor_pid = 0;
static void
_editor_exit_cb(GPid pid, gint status, gpointer data)
{
editor_active = FALSE;
jabber.developer marked this conversation as resolved Outdated

excessive comment length

excessive comment length
editor_pid = 0;
EditorContext* ctx = data;
gchar* contents = NULL;
GError* error = NULL;
@@ -52,8 +58,10 @@ _editor_exit_cb(GPid pid, gint status, gpointer data)
cons_show_error("Could not read edited content: %s", error->message);
g_error_free(error);
}
} else {
cons_show_error("Editor exited with error status %d", WEXITSTATUS(status));
} else if (WIFEXITED(status)) {
cons_show_error("Editor exited with status %d", WEXITSTATUS(status));
} else if (WIFSIGNALED(status)) {
cons_show("Editor session cancelled.");
}
if (remove(ctx->filename) != 0) {
@@ -70,6 +78,11 @@ _editor_exit_cb(GPid pid, gint status, gpointer data)
gboolean
launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* data), void* user_data)
{
if (editor_active) {
cons_show_error("An editor session is already active.");
return TRUE;
}
auto_gchar gchar* filename = NULL;
auto_gerror GError* glib_error = NULL;
const char* jid = connection_get_barejid();
@@ -118,6 +131,7 @@ launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* dat
ctx->callback = callback;
ctx->user_data = user_data;
editor_active = TRUE;
ui_suspend();
// Force GLib to install its SIGCHLD handler before forking. Creating a
@@ -129,6 +143,7 @@ launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* dat
pid_t pid = fork();
if (pid == -1) {
log_error("[Editor] Failed to fork: %s", strerror(errno));
editor_active = FALSE;
ui_resume();
ui_resize();
cons_show_error("Failed to start editor: %s", strerror(errno));
@@ -139,7 +154,8 @@ launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* dat
} else if (pid == 0) {
// Child process: Inherits TTY from parent
// Reset signal handlers that profanity sets so the editor doesn't inherit them
// SIGTSTP=SIG_DFL lets vim's :stop / Ctrl-Z work; profanity catches
// the STOPPED state via editor_check_stopped() and drops to the shell.
signal(SIGINT, SIG_DFL);
signal(SIGTSTP, SIG_DFL);
signal(SIGPIPE, SIG_DFL);
@@ -151,7 +167,39 @@ launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* dat
}
// Parent process: Watch the child asynchronously
editor_pid = pid;
g_child_watch_add((GPid)pid, _editor_exit_cb, ctx);
g_strfreev(editor_argv);
return FALSE;
}
void
editor_emergency_kill(void)
{
if (!editor_active || editor_pid <= 0) {
return;
}
log_warning("[Editor] aborting editor pid %d", editor_pid);
kill(editor_pid, SIGCONT); // wake if STOPPED; no-op otherwise
kill(editor_pid, SIGTERM);
// _editor_exit_cb will restore the UI when the child reaps.
}
gboolean
editor_is_active(void)
{
return editor_active;
}
gboolean
editor_check_stopped(void)
{
// Poll for STOPPED without reaping (g_child_watch handles exit via its
// own WNOHANG). Lets prof_run drop the group to the shell on Ctrl-Z.
if (!editor_active || editor_pid <= 0) {
return FALSE;
}
int status;
pid_t r = waitpid(editor_pid, &status, WNOHANG | WUNTRACED);
return (r == editor_pid && WIFSTOPPED(status));
}

View File

@@ -16,4 +16,13 @@
gboolean launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* data), void* user_data);
void editor_emergency_kill(void); // SIGCONT+SIGTERM the editor child
// TRUE if editor child is STOPPED (Ctrl-Z / :stop / gdb-attach). Does not
// reap; g_child_watch keeps handling exit.
gboolean editor_check_stopped(void);
// Editor-session flag accessor. Used by prof_run on SIGCONT resume.
gboolean editor_is_active(void);
#endif

View File

@@ -327,7 +327,7 @@ chatwin_incoming_msg(ProfChatWin* chatwin, ProfMessage* message, gboolean win_cr
cons_show_incoming_message(display_name, num, chatwin->unread, window);
if (prefs_get_boolean(PREF_FLASH)) {
flash();
ui_flash();
}
chatwin->unread++;
@@ -364,7 +364,7 @@ chatwin_incoming_msg(ProfChatWin* chatwin, ProfMessage* message, gboolean win_cr
wins_add_quotes_ac(window, message->plain, FALSE);
if (prefs_get_boolean(PREF_BEEP)) {
beep();
ui_beep();
}
}

View File

@@ -54,6 +54,7 @@
static int inp_size;
static gboolean perform_resize = FALSE;
static gboolean ui_suspended = FALSE;
static GTimer* ui_idle_time;
static WINDOW* main_scr;
@@ -63,6 +64,14 @@ static Display* display;
static void _ui_draw_term_title(void);
// Fail-safe gate: authoritative flag (doesn't depend on isendwin staying
// TRUE), plus the regular endwin check for shutdown.
static gboolean
_ui_redraw_suspended(void)
{
return ui_suspended || isendwin();
}
static void
_ui_close(void)
{
@@ -120,8 +129,7 @@ ui_sigwinch_handler(int sig)
void
ui_update(void)
{
// UI is suspended
if (isendwin()) {
if (_ui_redraw_suspended()) {
return;
}
@@ -138,7 +146,7 @@ ui_update(void)
title_bar_update_virtual();
status_bar_draw();
inp_put_back();
doupdate();
prof_doupdate();
if (perform_resize) {
perform_resize = FALSE;
@@ -180,21 +188,49 @@ void
ui_suspend(void)
{
inp_suspend();
// Flush before flipping the flag (prof_doupdate skips while suspended).
prof_doupdate();
ui_suspended = TRUE;
endwin();
}
void
ui_resume(void)
{
ui_suspended = FALSE;
clearok(stdscr, TRUE); // ncurses can't see vim's screen clobber
refresh();
inp_resume();
}
void
ui_beep(void)
{
if (!isendwin()) {
beep();
}
}
void
ui_flash(void)
{
if (!isendwin()) {
flash();
}
}
void
prof_doupdate(void)
{
if (!_ui_redraw_suspended()) {
doupdate();
}
}
void
ui_resize(void)
{
// UI is suspended
if (isendwin()) {
if (_ui_redraw_suspended()) {
return;
}
@@ -216,8 +252,7 @@ ui_resize(void)
void
ui_redraw(void)
{
// UI is suspended
if (isendwin()) {
if (_ui_redraw_suspended()) {
return;
}

View File

@@ -182,7 +182,11 @@ inp_readline(void)
{
// UI is suspended
if (is_suspended || isendwin()) {
// Mirror the normal-path unlock so workers aren't starved
// for the whole editor session.
pthread_mutex_unlock(&lock);
g_usleep(100000); // 100ms
pthread_mutex_lock(&lock);
return NULL;
}
@@ -311,11 +315,13 @@ inp_get_line(void)
werase(inp_win);
wmove(inp_win, 0, 0);
_inp_win_update_virtual();
doupdate();
prof_doupdate();
char* line = NULL;
while (!line) {
line = inp_readline();
ui_update();
while (g_main_context_iteration(NULL, FALSE)) // non-blocking; bounded by ready sources
;
}
status_bar_clear_prompt();
return line;
jabber.developer marked this conversation as resolved Outdated

is it safe from infinite looping?

is it safe from infinite looping?

Yes. The inner g_main_context_iteration(NULL, FALSE) is non-blocking — returns FALSE as soon as no source is ready — and the only sources we have (g_child_watch for the editor, tray timer) dispatch once per state change, not re-arm in place. The outer while (!line) is the standard blocking-prompt pattern: it exits when readline accumulates a full line (user Enter). That same loop existed before; the only change is adding the GLib iteration so a prompt raised mid-editor-session can dispatch _editor_exit_cb instead of deadlocking until the editor exits. No new infinite-loop class.

Yes. The inner g_main_context_iteration(NULL, FALSE) is non-blocking — returns FALSE as soon as no source is ready — and the only sources we have (g_child_watch for the editor, tray timer) dispatch once per state change, not re-arm in place. The outer while (!line) is the standard blocking-prompt pattern: it exits when readline accumulates a full line (user Enter). That same loop existed before; the only change is adding the GLib iteration so a prompt raised mid-editor-session can dispatch _editor_exit_cb instead of deadlocking until the editor exits. No new infinite-loop class.
@@ -338,12 +344,14 @@ inp_get_password(void)
werase(inp_win);
wmove(inp_win, 0, 0);
_inp_win_update_virtual();
doupdate();
prof_doupdate();
char* password = NULL;
get_password = TRUE;
while (!password) {
password = inp_readline();
ui_update();
while (g_main_context_iteration(NULL, FALSE)) // non-blocking; bounded by ready sources
;
}
get_password = FALSE;
status_bar_clear_prompt();
@@ -459,7 +467,7 @@ _inp_write(char* line, int offset)
_inp_win_handle_scroll();
_inp_win_update_virtual();
doupdate();
prof_doupdate();
}
static int

View File

@@ -57,7 +57,7 @@ privwin_incoming_msg(ProfPrivateWin* privatewin, ProfMessage* message)
privatewin->unread++;
if (prefs_get_boolean(PREF_FLASH)) {
flash();
ui_flash();
}
}
@@ -65,7 +65,7 @@ privwin_incoming_msg(ProfPrivateWin* privatewin, ProfMessage* message)
wins_add_quotes_ac(window, message->plain, TRUE);
if (prefs_get_boolean(PREF_BEEP)) {
beep();
ui_beep();
}
if (notify) {

View File

@@ -43,6 +43,9 @@ void ui_load_colours(void);
void ui_update(void);
void ui_redraw(void);
void ui_resize(void);
// doupdate() wrapper that skips when the UI is suspended. Use this instead
// of raw doupdate() to avoid the per-site whitelist class of bug.
void prof_doupdate(void);
void ui_focus_win(ProfWin* window);
void ui_sigwinch_handler(int sig);
void ui_handle_otr_error(const char* const barejid, const char* const message);
@@ -84,6 +87,8 @@ void ui_handle_recipient_error(const char* const recipient, const char* const er
void ui_handle_error(const char* const err_msg);
void ui_clear_win_title(void);
void ui_goodbye_title(void);
void ui_beep(void);
void ui_flash(void);
void ui_handle_room_configuration_form_error(const char* const roomjid, const char* const message);
void ui_handle_room_config_submit_result(const char* const roomjid);
void ui_handle_room_config_submit_result_error(const char* const roomjid, const char* const message);

View File

@@ -85,6 +85,10 @@ void
ui_resize(void)
{
}
void
prof_doupdate(void)
{
}
void
ui_focus_win(ProfWin* win)
@@ -1475,3 +1479,13 @@ ui_is_suspended(void)
{
return FALSE;
}
void
ui_beep(void)
{
}
void
ui_flash(void)
{
}