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
10 changed files with 206 additions and 20 deletions
Showing only changes of commit e4422ac7a1 - Show all commits

View File

@@ -20,6 +20,7 @@
#include <string.h>
#include <glib.h>
#include <glib-unix.h>
#include "profanity.h"
#include "common.h"
@@ -70,6 +71,12 @@ static void _connect_default(const char* const account);
pthread_mutex_t lock;
static gboolean force_quit = FALSE;
// Signal-driven flags consumed by the main loop. SIGTSTP and a STOPPED editor
// child → drop the process group to the shell (mutt-style). SIGCONT →
// refresh curses on return.
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
static volatile sig_atomic_t suspend_requested = FALSE;
static volatile sig_atomic_t cont_requested = FALSE;
void
prof_run(gchar* log_level, gchar* account_name, gchar* config_file, gchar* log_file, gchar* theme_name, gchar** commands)
{
@@ -93,6 +100,36 @@ prof_run(gchar* log_level, gchar* account_name, gchar* config_file, gchar* log_f
int waittime = 0;
while (cont && !force_quit) {
log_stderr_handler();
// Resume from fg after SIGSTOP. Vim handles its own refresh.
if (cont_requested) {
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
cont_requested = FALSE;
if (!editor_is_active()) {
clearok(stdscr, TRUE);
ui_resize();
}
}
// Editor STOPPED → abort. Plain Ctrl-Z → drop to shell. While editor
// is active, swallow suspend_requested (race: vim's kill(0,SIGTSTP)
// beats waitpid seeing STOPPED) — next poll catches it.
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);
}
}
session_check_autoaway();
line = commands ? *commands : inp_readline();
@@ -182,6 +219,27 @@ sigterm_handler(int sig)
force_quit = TRUE;
}
// SIGUSR1 escape for an alive-but-stuck editor (WUNTRACED can't catch it).
// Dispatched in the main loop. Trigger: `pkill -USR1 profanity`.
static gboolean
_emergency_resume_handler(gpointer data)
{
editor_emergency_kill();
return G_SOURCE_CONTINUE;
}
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,10 +247,12 @@ _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);
g_unix_signal_add(SIGUSR1, _emergency_resume_handler, NULL);
if (pthread_mutex_init(&lock, NULL) != 0) {
log_error("Mutex init failed");
exit(1);

View File

@@ -32,19 +32,16 @@ typedef struct EditorContext
void* user_data;
} EditorContext;
// Set TRUE for the lifetime of an editor session (from launch_editor up to
// _editor_exit_cb / fork-error cleanup). The async editor model lost the old
// editor_task.active check from the pthread implementation; without this
// guard a second launch_editor (e.g. via a plugin dispatched through
// g_main_context_iteration) would call ui_suspend again and fork a second
// editor that fights the first one for the terminal, then prematurely
// ui_resume when either exits.
// Re-entrancy guard (lost in the async-editor rewrite). Prevents a plugin
// from spawning a second editor that would fight the first for the terminal.
static gboolean editor_active = FALSE;
static pid_t editor_pid = 0;
static void
_editor_exit_cb(GPid pid, gint status, gpointer data)
jabber.developer marked this conversation as resolved Outdated

excessive comment length

excessive comment length
{
editor_active = FALSE;
editor_pid = 0;
EditorContext* ctx = data;
gchar* contents = NULL;
@@ -63,8 +60,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) {
@@ -157,7 +156,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);
@@ -169,7 +169,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,15 @@
gboolean launch_editor(gchar* initial_content, void (*callback)(gchar* content, void* data), void* user_data);
// SIGUSR1 escape: SIGTERM the editor child. Use when WUNTRACED can't catch
// it (alive but stuck). Wired in profanity.c.
void editor_emergency_kill(void);
// 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

@@ -64,10 +64,8 @@ static Display* display;
static void _ui_draw_term_title(void);
// Skip drawing while an external editor is active (ui_suspended) or after the
// screen has been closed (endwin). The flag is authoritative and does not rely
// on isendwin() staying TRUE, which a stray doupdate() during the session could
// otherwise clear.
// 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)
{

View File

@@ -182,10 +182,8 @@ inp_readline(void)
{
// UI is suspended
if (is_suspended || isendwin()) {
// Mirror the normal-path unlock/sleep/lock so worker threads
// (http_upload / http_download / ai_client) can acquire `lock`
// during the editor session — otherwise the main thread holds
// it continuously and every in-flight transfer freezes.
// 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);
@@ -322,8 +320,8 @@ inp_get_line(void)
while (!line) {
line = inp_readline();
ui_update();
// Let GLib sources (notably the editor child watch) dispatch so
// ui_resume can run if the prompt was raised mid-editor-session.
// Let the editor child watch dispatch — otherwise a prompt raised
// mid-editor-session deadlocks here.
while (g_main_context_iteration(NULL, FALSE))
;
}
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.
@@ -354,8 +352,8 @@ inp_get_password(void)
while (!password) {
password = inp_readline();
ui_update();
// Let GLib sources (notably the editor child watch) dispatch so
// ui_resume can run if the prompt was raised mid-editor-session.
// Let the editor child watch dispatch — otherwise a prompt raised
// mid-editor-session deadlocks here.
while (g_main_context_iteration(NULL, FALSE))
;
}

View File

@@ -43,9 +43,8 @@ void ui_load_colours(void);
void ui_update(void);
void ui_redraw(void);
void ui_resize(void);
// Centralised physical-paint primitive: flushes the virtual screen unless the
// UI is suspended (external editor active) or the screen has been closed. Use
// this instead of raw doupdate() in any path reachable while suspended.
// 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);