devs/cproof
1
2
Fork 1
Code Issues 43 Pull Requests 6 Actions Packages Projects 1 Releases Wiki Activity

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
pull from: fix/editor-ui-suspend-redraw
merge into: devs:master
Conversation 2 Commits 11 Files Changed 11 +187 -21
jabber.developer2 commented 2026-05-27 13:23:47 +00:00
Collaborator
Copy Link

Problem

When composing in an external editor (/editor or Alt+C, e.g. nano), Profanity's UI bleeds through onto the editor screen. Most visible artifact: after a short pause, the status-bar clock's updated minute digit (a single character) appears on top of the editor; a partial status bar and the previous input line can show up too.

Root cause

The upstream-merge editor rewrite (blocking pthread + background_mode → async g_child_watch) dropped the background_mode guard that paused the main loop while the editor runs. ui_update/ui_resize/ui_redraw now self-guard on isendwin(), which is not a reliable "editor active" signal — any real doupdate() during the session (e.g. the readline redisplay right after the Alt+C keypress) clears it. Once cleared, the ~10 Hz main-loop ui_update() keeps repainting the status bar, current chat window and input pad, flushing them over the editor with doupdate(). ncurses emits only changed cells, so in a quiet session the sole frame-to-frame change is the clock digit — hence the "single character" bleed-through tied to minute ticks.

Fix

Builds on the upstream fix 5e4b5d313 ("guard terminal writes while suspended", upstream PR 2167) which is cherry-picked here, and layers seven additional commits across three concerns: closing the suspended-write path upstream left open, fixing pre-existing hang classes around the async editor model (including the Ctrl-Z / hung-editor case), and centralising the no-stray-write invariant into a primitive that can't be silently skipped. Eight commits:

  1. fix(ui): guard terminal writes while suspended — upstream (Michael Vetter), cherry-picked. Routes beep()/flash() through ui_beep()/ui_flash() guarded by !isendwin(), flushes pending output with doupdate() in ui_suspend() before endwin(), and guards the _inp_write() redisplay doupdate() with !isendwin(). Keeps isendwin() a valid "suspended" signal so the existing redraw-path checks hold.
  2. fix(ui): guard password/TLS prompt redraw while suspended — ours. Guards the doupdate() in inp_get_line()/inp_get_password() with is_suspended || isendwin() (matching inp_readline()'s guard), closing the one suspended-write path upstream left open.
  3. fix(ui): add authoritative suspend flag gating redraws over the editor — ours. Adds a ui_suspended flag set in ui_suspend()/ui_resume() and gates ui_update/ui_resize/ui_redraw on ui_suspended || isendwin() via _ui_redraw_suspended(). Fail-safe against any future raw doupdate() clearing isendwin().
  4. fix(ui): release global lock around suspend-branch sleep in inp_readline — ours. Mirrors 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 held it continuously and every in-flight transfer froze for the whole suspend window.
  5. fix(ui): iterate main context inside synchronous input loops — ours. Adds while (g_main_context_iteration(NULL, FALSE)) ; to inp_get_line() / inp_get_password() loops so the editor child watch can dispatch, breaking the deadlock that occurred when a TLS-cert (sv_ev_certfail) or PGP passphrase (ui_ask_pgp_passphrase) prompt was raised from a network handler mid-session.
  6. fix(editor): guard against re-entrant launch_editor — ours. Restores the editor_task.active check the pre-merge pthread implementation had; bails with a console error if a plugin (via g_main_context_iteration dispatch) tries to launch a second editor while one is in flight.
  7. refactor(ui): centralize physical paint through prof_doupdate() — ours. Introduces a single prof_doupdate() primitive in src/ui/core.c that internally checks the suspend flag; replaces every direct doupdate() in src/ui/inputwin.c and the trailing one in ui_update() with the wrapper, collapsing the inline guards. Turns "remember to guard each doupdate site" into "use prof_doupdate() instead of doupdate()" — a single discipline check (e.g. a grep CI rule) rather than per-site review.
  8. fix(editor): avoid Ctrl-Z hang on the editor child, add SIGUSR1 escape — ours. Drops the SIGTSTP reset in the editor child: SIGTSTP stays inherited as SIG_IGN from profanity's _init, so Ctrl-Z and vim's :stop become no-ops inside /editor. This avoids the hang where g_child_watch (waiting with WNOHANG without WUNTRACED) never wakes for a STOPPED child. SIGINT and SIGPIPE are still reset to SIG_DFL so Neovim's libuv still detects an interactive terminal. As a universal escape for any other hung-editor case (looping $EDITOR, corrupted tty), installs a SIGUSR1 handler via g_unix_signal_add that sends SIGTERM to the editor child through editor_emergency_kill(); the existing child watch then runs _editor_exit_cb → ui_resume. Recovery from another terminal: pkill -USR1 profanity.

Why three layers

Upstream's commit 1 keeps isendwin() as the single "suspended" signal and guards each direct terminal write. That is correct but fail-open: any future unguarded doupdate() / refresh() reachable during a session clears isendwin(), and the periodic ui_update() repaints over the editor — exactly how the original bug slipped in.

Commit 7's prof_doupdate() wrapper generalises the upstream approach by centralising the primitive. New code that paints through prof_doupdate() is automatically guarded; the no-stray-write invariant collapses from a per-site whitelist to a single discipline check ("use the wrapper, not raw doupdate").

Commit 3's ui_suspended flag is the fail-safe orthogonal to both. It gates ui_update / ui_resize / ui_redraw at the consumer side, so even if some path flips isendwin() mid-session, the periodic full-screen repaint that produces the visible bleed cannot run. The flag does not cover direct writes (which is why the upstream guards and the wrapper are both still needed), but it makes the dominant damage path declaratively safe.

Together: upstream guards plug the known direct-write sites; the wrapper makes future ones automatic; the flag keeps the worst-case repaint impossible regardless of either.

Verify

Alt+C (or /editor), type a longish line, stay idle ≥ 1 minute. Before: the minute digit / status bar bled onto the editor. After: nothing repaints over the editor.

Behavior verification

Traced through the relevant scenarios against the actual code on the branch.

Scenario Expected Why it holds
Alt+C, idle ≥ 1 minute Status-bar clock digit does not bleed onto the editor ui_update returns at _ui_redraw_suspended() before status_bar_draw / win_update_virtual / doupdate.
A future stray doupdate() clears isendwin() mid-session Flag still prevents ui_update from repainting _ui_redraw_suspended() ORs the flag with isendwin(); the flag held alone keeps the gate closed (fail-safe).
Stray readline redisplay right after Alt+C The one-shot redisplay does not flush over the editor Upstream's guard in _inp_write skips doupdate() while isendwin().
Incoming message → ui_beep / ui_flash during session No bell / flash over the editor Upstream !isendwin() guard; isendwin() stays TRUE because all doupdate sites are guarded.
TLS-cert / PGP passphrase prompt during session Prompt does not bleed onto the editor, and resolves after the editor closes Commits 2 / 7 (prof_doupdate checks is_suspended || isendwin()) skip the entry doupdate(). Commit 5's g_main_context_iteration inside the loop lets the editor child watch dispatch, so ui_resume runs as soon as the editor closes and the prompt resumes accepting input — no external kill -9 required.
ui_suspend ordering inp_suspend deprep, flush pending output, set the flag, then endwin() Sequence in src/ui/core.c: inp_suspend(), prof_doupdate() (flag still FALSE so it flushes), set ui_suspended, endwin(). Flushing before flipping the flag preserves upstream's pre-endwin flush.
ui_resume ordering Flag cleared, then refresh(), then inp_resume() Between flag-clear and refresh(), isendwin() is still TRUE, so the gate still holds. After refresh() both are clear and normal operation resumes.
Editor exit child watch → _editor_exit_cb → ui_resume() → ui_resize() → callback In src/tools/editor.c, ui_resize runs after ui_resume, so its _ui_redraw_suspended() check is FALSE and the resize proceeds.

Build: clean under -Werror. The flag and _ui_redraw_suspended helper are file-static in src/ui/core.c; commit 7 adds a prof_doupdate(void) declaration in src/ui/ui.h and a corresponding stub in tests/unittests/ui/stub_ui.c (next to the ui_beep / ui_flash stubs from upstream's commit 1).

Pre-existing hang classes closed by this PR

Four hang classes that pre-dated this branch (and are not addressed upstream either) are closed by commits 4 / 5 / 6 / 8:

  • Lock starvation during the editor session. The single global pthread_mutex_t lock (src/profanity.h) is acquired once by the main thread in prof_run's _init and held for the whole program lifetime, released only around the select() inside inp_readline (src/ui/inputwin.c). The suspend branch used to skip that unlock — every worker thread (src/tools/http_upload.c, src/tools/http_download.c, src/ai/ai_client.c) blocked for the whole session, long enough to time out in-flight transfers on the server side. Commit 4 mirrors the normal-path unlock/sleep/lock so the window exists during the editor session too.
  • Deadlock from a synchronous prompt during the session. inp_get_line() / inp_get_password() loop with no g_main_context_iteration; if a network handler raised a TLS-cert prompt (sv_ev_certfail → ui_get_line() in src/event/server_events.c) or PGP passphrase prompt (ui_ask_pgp_passphrase() in src/pgp/gpg.c) mid-session, the main thread parked in that loop, the editor child watch never dispatched, and kill -9 was the only recovery. Commit 5 runs g_main_context_iteration inside the loop so the watch can fire and ui_resume runs as soon as the editor closes.
  • Re-entrant launch_editor. The async editor lost the editor_task.active check the old pthread implementation had; a plugin firing launch_editor from a g_main_context_iteration dispatch could spawn a second editor that fought the first for the terminal and triggered premature ui_resume. Commit 6 restores an explicit editor_active static, bailing with a console error on re-entry.
  • Stuck or stopped editor. g_child_watch waits with WNOHANG without WUNTRACED, so a STOPPED editor (Ctrl-Z, vim's :stop) never woke the watch and the app froze until killed externally. Same for any hung $EDITOR (looping script, corrupted tty). Other TUI chat clients carry this open for years (e.g. weechat/weechat#985); shells solve it with full job control (setpgid + tcsetpgrp + WUNTRACED), heavy for our use case. Commit 8 closes Ctrl-Z by inheriting SIGTSTP as SIG_IGN (the editor cannot be stopped; vim's :stop becomes a no-op — acceptable trade for not freezing the whole app), and adds pkill -USR1 profanity as a universal escape hatch for the remaining hung-editor cases via editor_emergency_kill(). ("Broken $EDITOR" was never a trigger — execvp failure makes the child _exit(EXIT_FAILURE).)

Out of scope — architectural limits inherent to the external-editor model

Independent of which approach is in use, the "spawn external editor with terminal handover" model has a few intrinsic ceilings:

  • While the editor owns the TTY, profanity cannot show anything on screen, accept any input, or signal the user except via channels outside the terminal (libnotify, desktop tray).
  • The user cannot switch windows / read another chat without closing the editor first.
  • The single global lock in src/profanity.h couples worker threads to the main loop through one writer; commit 4 opens a window during suspend, but the coarse-grained locking remains.

These would require a different model (embedded editor, tmux-style pane) to close — not "fixes".

## Problem When composing in an external editor (`/editor` or `Alt+C`, e.g. nano), Profanity's UI bleeds through onto the editor screen. Most visible artifact: after a short pause, the status-bar clock's updated minute digit (a single character) appears on top of the editor; a partial status bar and the previous input line can show up too. ## Root cause The upstream-merge editor rewrite (blocking pthread + `background_mode` → async `g_child_watch`) dropped the `background_mode` guard that paused the main loop while the editor runs. `ui_update`/`ui_resize`/`ui_redraw` now self-guard on `isendwin()`, which is not a reliable "editor active" signal — any real `doupdate()` during the session (e.g. the readline redisplay right after the `Alt+C` keypress) clears it. Once cleared, the ~10 Hz main-loop `ui_update()` keeps repainting the status bar, current chat window and input pad, flushing them over the editor with `doupdate()`. ncurses emits only changed cells, so in a quiet session the sole frame-to-frame change is the clock digit — hence the "single character" bleed-through tied to minute ticks. ## Fix Builds on the upstream fix `5e4b5d313` ("guard terminal writes while suspended", [upstream PR 2167](https://github.com/profanity-im/profanity/pull/2167)) which is cherry-picked here, and layers seven additional commits across three concerns: closing the suspended-write path upstream left open, fixing pre-existing hang classes around the async editor model (including the Ctrl-Z / hung-editor case), and centralising the no-stray-write invariant into a primitive that can't be silently skipped. Eight commits: 1. **`fix(ui): guard terminal writes while suspended`** — *upstream (Michael Vetter), cherry-picked.* Routes `beep()`/`flash()` through `ui_beep()`/`ui_flash()` guarded by `!isendwin()`, flushes pending output with `doupdate()` in `ui_suspend()` before `endwin()`, and guards the `_inp_write()` redisplay `doupdate()` with `!isendwin()`. Keeps `isendwin()` a valid "suspended" signal so the existing redraw-path checks hold. 2. **`fix(ui): guard password/TLS prompt redraw while suspended`** — *ours.* Guards the `doupdate()` in `inp_get_line()`/`inp_get_password()` with `is_suspended || isendwin()` (matching `inp_readline()`'s guard), closing the one suspended-write path upstream left open. 3. **`fix(ui): add authoritative suspend flag gating redraws over the editor`** — *ours.* Adds a `ui_suspended` flag set in `ui_suspend()`/`ui_resume()` and gates `ui_update`/`ui_resize`/`ui_redraw` on `ui_suspended || isendwin()` via `_ui_redraw_suspended()`. Fail-safe against any future raw `doupdate()` clearing `isendwin()`. 4. **`fix(ui): release global lock around suspend-branch sleep in inp_readline`** — *ours.* Mirrors 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 held it continuously and every in-flight transfer froze for the whole suspend window. 5. **`fix(ui): iterate main context inside synchronous input loops`** — *ours.* Adds `while (g_main_context_iteration(NULL, FALSE)) ;` to `inp_get_line()` / `inp_get_password()` loops so the editor child watch can dispatch, breaking the deadlock that occurred when a TLS-cert (`sv_ev_certfail`) or PGP passphrase (`ui_ask_pgp_passphrase`) prompt was raised from a network handler mid-session. 6. **`fix(editor): guard against re-entrant launch_editor`** — *ours.* Restores the `editor_task.active` check the pre-merge pthread implementation had; bails with a console error if a plugin (via `g_main_context_iteration` dispatch) tries to launch a second editor while one is in flight. 7. **`refactor(ui): centralize physical paint through prof_doupdate()`** — *ours.* Introduces a single `prof_doupdate()` primitive in `src/ui/core.c` that internally checks the suspend flag; replaces every direct `doupdate()` in `src/ui/inputwin.c` and the trailing one in `ui_update()` with the wrapper, collapsing the inline guards. Turns "remember to guard each `doupdate` site" into "use `prof_doupdate()` instead of `doupdate()`" — a single discipline check (e.g. a grep CI rule) rather than per-site review. 8. **`fix(editor): avoid Ctrl-Z hang on the editor child, add SIGUSR1 escape`** — *ours.* Drops the `SIGTSTP` reset in the editor child: `SIGTSTP` stays inherited as `SIG_IGN` from profanity's `_init`, so Ctrl-Z and vim's `:stop` become no-ops inside `/editor`. This avoids the hang where `g_child_watch` (waiting with `WNOHANG` without `WUNTRACED`) never wakes for a STOPPED child. `SIGINT` and `SIGPIPE` are still reset to `SIG_DFL` so Neovim's libuv still detects an interactive terminal. As a universal escape for any other hung-editor case (looping `$EDITOR`, corrupted tty), installs a `SIGUSR1` handler via `g_unix_signal_add` that sends `SIGTERM` to the editor child through `editor_emergency_kill()`; the existing child watch then runs `_editor_exit_cb` → `ui_resume`. Recovery from another terminal: `pkill -USR1 profanity`. ### Why three layers Upstream's commit 1 keeps `isendwin()` as the single "suspended" signal and guards each direct terminal write. That is correct but **fail-open**: any future unguarded `doupdate()` / `refresh()` reachable during a session clears `isendwin()`, and the periodic `ui_update()` repaints over the editor — exactly how the original bug slipped in. Commit 7's `prof_doupdate()` wrapper generalises the upstream approach by **centralising the primitive**. New code that paints through `prof_doupdate()` is automatically guarded; the no-stray-write invariant collapses from a per-site whitelist to a single discipline check ("use the wrapper, not raw `doupdate`"). Commit 3's `ui_suspended` flag is the **fail-safe** orthogonal to both. It gates `ui_update` / `ui_resize` / `ui_redraw` at the consumer side, so even if some path flips `isendwin()` mid-session, the periodic full-screen repaint that produces the visible bleed cannot run. The flag does not cover direct writes (which is why the upstream guards and the wrapper are both still needed), but it makes the dominant damage path declaratively safe. Together: upstream guards plug the known direct-write sites; the wrapper makes future ones automatic; the flag keeps the worst-case repaint impossible regardless of either. ## Verify `Alt+C` (or `/editor`), type a longish line, stay idle ≥ 1 minute. Before: the minute digit / status bar bled onto the editor. After: nothing repaints over the editor. ## Behavior verification Traced through the relevant scenarios against the actual code on the branch. | Scenario | Expected | Why it holds | |---|---|---| | `Alt+C`, idle ≥ 1 minute | Status-bar clock digit does not bleed onto the editor | `ui_update` returns at `_ui_redraw_suspended()` before `status_bar_draw` / `win_update_virtual` / `doupdate`. | | A future stray `doupdate()` clears `isendwin()` mid-session | Flag still prevents `ui_update` from repainting | `_ui_redraw_suspended()` ORs the flag with `isendwin()`; the flag held alone keeps the gate closed (fail-safe). | | Stray readline redisplay right after `Alt+C` | The one-shot redisplay does not flush over the editor | Upstream's guard in `_inp_write` skips `doupdate()` while `isendwin()`. | | Incoming message → `ui_beep` / `ui_flash` during session | No bell / flash over the editor | Upstream `!isendwin()` guard; `isendwin()` stays `TRUE` because all `doupdate` sites are guarded. | | TLS-cert / PGP passphrase prompt during session | Prompt does not bleed onto the editor, and resolves after the editor closes | Commits 2 / 7 (`prof_doupdate` checks `is_suspended \|\| isendwin()`) skip the entry `doupdate()`. Commit 5's `g_main_context_iteration` inside the loop lets the editor child watch dispatch, so `ui_resume` runs as soon as the editor closes and the prompt resumes accepting input — no external `kill -9` required. | | `ui_suspend` ordering | `inp_suspend` deprep, flush pending output, set the flag, then `endwin()` | Sequence in `src/ui/core.c`: `inp_suspend()`, `prof_doupdate()` (flag still `FALSE` so it flushes), set `ui_suspended`, `endwin()`. Flushing before flipping the flag preserves upstream's pre-`endwin` flush. | | `ui_resume` ordering | Flag cleared, then `refresh()`, then `inp_resume()` | Between flag-clear and `refresh()`, `isendwin()` is still `TRUE`, so the gate still holds. After `refresh()` both are clear and normal operation resumes. | | Editor exit | child watch → `_editor_exit_cb` → `ui_resume()` → `ui_resize()` → callback | In `src/tools/editor.c`, `ui_resize` runs after `ui_resume`, so its `_ui_redraw_suspended()` check is `FALSE` and the resize proceeds. | Build: clean under `-Werror`. The flag and `_ui_redraw_suspended` helper are file-static in `src/ui/core.c`; commit 7 adds a `prof_doupdate(void)` declaration in `src/ui/ui.h` and a corresponding stub in `tests/unittests/ui/stub_ui.c` (next to the `ui_beep` / `ui_flash` stubs from upstream's commit 1). ## Pre-existing hang classes closed by this PR Four hang classes that pre-dated this branch (and are not addressed upstream either) are closed by commits 4 / 5 / 6 / 8: - **Lock starvation during the editor session.** The single global `pthread_mutex_t lock` (`src/profanity.h`) is acquired once by the main thread in `prof_run`'s `_init` and held for the whole program lifetime, released only around the `select()` inside `inp_readline` (`src/ui/inputwin.c`). The suspend branch used to skip that unlock — every worker thread (`src/tools/http_upload.c`, `src/tools/http_download.c`, `src/ai/ai_client.c`) blocked for the whole session, long enough to time out in-flight transfers on the server side. **Commit 4** mirrors the normal-path unlock/sleep/lock so the window exists during the editor session too. - **Deadlock from a synchronous prompt during the session.** `inp_get_line()` / `inp_get_password()` loop with no `g_main_context_iteration`; if a network handler raised a TLS-cert prompt (`sv_ev_certfail` → `ui_get_line()` in `src/event/server_events.c`) or PGP passphrase prompt (`ui_ask_pgp_passphrase()` in `src/pgp/gpg.c`) mid-session, the main thread parked in that loop, the editor child watch never dispatched, and `kill -9` was the only recovery. **Commit 5** runs `g_main_context_iteration` inside the loop so the watch can fire and `ui_resume` runs as soon as the editor closes. - **Re-entrant `launch_editor`.** The async editor lost the `editor_task.active` check the old pthread implementation had; a plugin firing `launch_editor` from a `g_main_context_iteration` dispatch could spawn a second editor that fought the first for the terminal and triggered premature `ui_resume`. **Commit 6** restores an explicit `editor_active` static, bailing with a console error on re-entry. - **Stuck or stopped editor.** `g_child_watch` waits with `WNOHANG` without `WUNTRACED`, so a STOPPED editor (Ctrl-Z, vim's `:stop`) never woke the watch and the app froze until killed externally. Same for any hung `$EDITOR` (looping script, corrupted tty). Other TUI chat clients carry this open for years (e.g. [weechat/weechat#985](https://github.com/weechat/weechat/issues/985)); shells solve it with full job control (`setpgid` + `tcsetpgrp` + `WUNTRACED`), heavy for our use case. **Commit 8** closes Ctrl-Z by inheriting `SIGTSTP` as `SIG_IGN` (the editor cannot be stopped; vim's `:stop` becomes a no-op — acceptable trade for not freezing the whole app), and adds `pkill -USR1 profanity` as a universal escape hatch for the remaining hung-editor cases via `editor_emergency_kill()`. ("Broken `$EDITOR`" was never a trigger — `execvp` failure makes the child `_exit(EXIT_FAILURE)`.) ## Out of scope — architectural limits inherent to the external-editor model Independent of which approach is in use, the "spawn external editor with terminal handover" model has a few intrinsic ceilings: - While the editor owns the TTY, profanity cannot show anything on screen, accept any input, or signal the user except via channels outside the terminal (`libnotify`, desktop tray). - The user cannot switch windows / read another chat without closing the editor first. - The single global `lock` in `src/profanity.h` couples worker threads to the main loop through one writer; commit 4 opens a window during suspend, but the coarse-grained locking remains. These would require a different model (embedded editor, tmux-style pane) to close — not "fixes".
jabber.developer2 added 1 commit 2026-05-27 13:23:48 +00:00
fix(ui): gate redraw on suspend flag to stop bleed-through over editor
All checks were successful
CI Code / Check spelling (pull_request) Successful in 18s
Details
CI Code / Check coding style (pull_request) Successful in 32s
Details
CI Code / Code Coverage (pull_request) Successful in 2m38s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m37s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m46s
Details
CI Code / Linux (arch) (pull_request) Successful in 10m12s
Details
f5fdc85324 
The editor rewrite (async g_child_watch) dropped the background_mode
guard that paused the main loop while an external editor (e.g. nano)
runs. ui_update/ui_resize/ui_redraw instead self-guarded on isendwin(),
which is unreliable: any doupdate() during the editor session (e.g. the
readline redisplay after the alt+c keypress) clears it, after which the
~10Hz main-loop ui_update() repaints the status bar, chat window and
input pad over the editor. The most visible artifact is the per-minute
status-bar clock digit bleeding through onto the editor screen.

Track suspension explicitly with a ui_suspended flag set in
ui_suspend()/ui_resume() and gate ui_update/ui_resize/ui_redraw on it
(kept alongside isendwin() for the shutdown/closed-screen case). The
flag stays TRUE for the whole editor session regardless of isendwin
flapping, so nothing repaints over the editor.
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from f5fdc85324 to 1bc5c51bdb 2026-05-27 13:40:21 +00:00 Compare
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from 1bc5c51bdb to 56e0305017 2026-05-27 19:05:45 +00:00 Compare
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from 56e0305017 to 394df6d391 2026-05-27 19:17:09 +00:00 Compare
jabber.developer2 added 4 commits 2026-05-29 06:03:50 +00:00
fix(ui): release global lock around suspend-branch sleep in inp_readline 2da2449777 
The normal path of inp_readline unlocks the global `lock` around
select(), giving worker threads (http_upload / http_download / ai_client
in src/tools and src/ai) a window to acquire it and update the UI.
The suspend branch (entered while an external editor is active) used to
g_usleep(100000) and return NULL while holding the lock, so the main
thread monopolised it for the whole editor session and every in-flight
transfer froze; on a long composition (the customer's main /editor use
case) this could be long enough for server-side timeouts to fire.

Mirror the normal-path unlock / sleep / lock pattern so the same window
exists during the editor session. Workers in the suspended state print
through wnoutrefresh paths only (no direct doupdate / refresh / wrefresh
in src outside core.c / inputwin.c, all of which are already guarded
against suspended writes), so opening the window introduces no new race.
fix(ui): iterate main context inside synchronous input loops b1ffc750a2 
inp_get_line() / inp_get_password() loop `while (!x) { inp_readline();
ui_update(); }` with no g_main_context_iteration call. If such a prompt
is raised from a network handler during an editor session — realistic
triggers are TLS-cert verification on a reconnect (sv_ev_certfail ->
ui_get_line() in src/event/server_events.c) and PGP passphrase on an
incoming encrypted message (ui_ask_pgp_passphrase() in src/pgp/gpg.c) —
the main thread is parked in this loop. inp_readline returns NULL
(suspended), ui_update returns (ui_suspended), and the editor child
watch never dispatches because the main loop never reaches its own
g_main_context_iteration. is_suspended stays TRUE forever, the loop
never receives input, and the app deadlocks until killed externally.

Run the GLib iteration inside the loop body so the child watch can
dispatch, _editor_exit_cb fires, ui_resume clears suspension, and the
next inp_readline reads from the terminal — letting the user answer the
deferred prompt once the editor is closed.
fix(editor): guard against re-entrant launch_editor a51afa53f6 
The async editor rewrite (g_child_watch + ui_suspend / ui_resume) lost
the editor_task.active check the old pthread implementation had at the
top of get_message_from_editor_async. With the guard gone, a second
launch_editor call while the first session is in flight would call
ui_suspend a second time, fork another editor, and register a second
child watch; whichever child exits first runs ui_resume and clears
suspension while the other still owns the terminal. Triggering this
needs a code path other than the main-loop alt+c / /editor (those can't
fire while the main loop is parked in the suspended iteration), so the
realistic source is a plugin firing launch_editor from a
g_main_context_iteration dispatch.

Restore an explicit editor_active static in src/tools/editor.c: check it
at the start of launch_editor (bail with a console error), set it just
before ui_suspend, and clear it on the fork-error path and at the start
of _editor_exit_cb. Keeps the same simple semantics the pre-merge code
had, without exposing a new ui.h API.
refactor(ui): centralize physical paint through prof_doupdate()
All checks were successful
CI Code / Check spelling (pull_request) Successful in 18s
Details
CI Code / Check coding style (pull_request) Successful in 32s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m57s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 5m0s
Details
CI Code / Linux (arch) (pull_request) Successful in 5m46s
Details
CI Code / Code Coverage (pull_request) Successful in 7m3s
Details
bb0af8b7c8 
Upstream's "guard terminal writes while suspended" fix and commit 2 of
this branch both work by sprinkling `if (!isendwin())` (or
`!is_suspended && !isendwin()`) guards around every doupdate() reachable
during a suspended session. That keeps isendwin() a valid signal but
turns the no-stray-write invariant into a whitelist: every new
doupdate / refresh / wrefresh added later in src/ has to remember the
guard, and the original bug class came from exactly such an oversight
in _inp_write.

Introduce a single primitive, prof_doupdate(), that flushes only when
not suspended (via the existing _ui_redraw_suspended() helper). Replace
every direct doupdate() in src/ui/inputwin.c (inp_get_line,
inp_get_password, _inp_write) and the trailing doupdate in ui_update
with the wrapper, collapsing their inline guards. ui_suspend() now
flushes via prof_doupdate() before flipping ui_suspended, so upstream's
pre-endwin flush is preserved.

The flag-based gate in ui_update / ui_resize / ui_redraw is kept as
belt-and-suspenders: even if a future raw doupdate slips in elsewhere,
ui_update has not been refreshing the virtual screen, so the blit finds
stale content rather than a fresh status-bar clock or chat line.
jabber.developer2 added 1 commit 2026-05-29 06:36:14 +00:00
fix(editor): avoid Ctrl-Z hang on the editor child, add SIGUSR1 escape
All checks were successful
CI Code / Check spelling (pull_request) Successful in 18s
Details
CI Code / Check coding style (pull_request) Successful in 32s
Details
CI Code / Code Coverage (pull_request) Successful in 2m36s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m38s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m49s
Details
CI Code / Linux (arch) (pull_request) Successful in 5m35s
Details
86d924eaac 
The upstream signal reset in launch_editor set SIGTSTP back to SIG_DFL in
the child, letting Ctrl-Z (or vim's :stop) stop the editor. GLib's
g_child_watch uses waitpid(WNOHANG) without WUNTRACED, so a STOPPED
editor never wakes the watch and the app freezes until killed externally.
Other TUI chat clients carry this open problem for years (see
weechat/weechat#985, open since 2017); shells solve it with full job
control (setpgid + tcsetpgrp + WUNTRACED), which is heavy for our use
case.

Drop just the SIGTSTP reset. SIGINT and SIGPIPE are still set to SIG_DFL
— Neovim's libuv needs SIGINT-as-default to recognise an interactive
terminal (per upstream commit dd76f9087). SIGTSTP stays inherited as
SIG_IGN from profanity's _init, so Ctrl-Z and vim's :stop become no-ops
inside /editor. Losing vim's :stop during message composition is an
acceptable trade for not freezing the whole app.

As a universal escape hatch for any other "hung editor" scenario
(looping $EDITOR, corrupted tty, anything we cannot detect), install a
SIGUSR1 handler via g_unix_signal_add that sends SIGTERM to the editor
child via editor_emergency_kill(). The existing g_child_watch then
dispatches the normal exit callback, ui_resume runs, and the user is
back in profanity. Recovery from another terminal: `pkill -USR1
profanity`.
jabber.developer reviewed 2026-05-29 19:11:36 +00:00
jabber.developer left a comment
Owner
Copy Link

generally LGTM. Please address minor issues

generally LGTM. Please address minor issues
src/profanity.c Outdated
@@ -184,1 +185,4 @@
// Universal "I'm stuck in the editor" escape hatch. Dispatched in the main
// loop by GLib (g_unix_signal_add), so it is safe to call into editor.c
// directly. Trigger from another terminal: `pkill -USR1 profanity`.
jabber.developer commented 2026-05-29 19:05:00 +00:00
Owner
Copy Link

excessive comment length + inefficient/inappropriate tone. please cut here and elsewhere

excessive comment length + inefficient/inappropriate tone. please cut here and elsewhere
jabber.developer marked this conversation as resolved
src/tools/editor.c Outdated
@@ -35,0 +38,4 @@
// 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.
jabber.developer commented 2026-05-29 19:04:15 +00:00
Owner
Copy Link

excessive comment length

excessive comment length
jabber.developer marked this conversation as resolved
src/ui/inputwin.c Outdated
@@ -318,1 +324,4 @@
ui_update();
// Let GLib sources (notably the editor child watch) dispatch so
// ui_resume can run if the prompt was raised mid-editor-session.
while (g_main_context_iteration(NULL, FALSE))
jabber.developer commented 2026-05-29 19:11:08 +00:00
Owner
Copy Link

is it safe from infinite looping?

is it safe from infinite looping?
jabber.developer2 commented 2026-05-31 12:22:55 +00:00
Author
Collaborator
Copy Link

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.
jabber.developer marked this conversation as resolved
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from 86d924eaac to e4422ac7a1 2026-05-31 11:08:06 +00:00 Compare
jabber.developer2 added 1 commit 2026-05-31 11:25:44 +00:00
fix(ui): clearok(stdscr) in ui_resume to clear vim screen residue
Some checks failed
CI Code / Check spelling (pull_request) Successful in 16s
Details
CI Code / Check coding style (pull_request) Failing after 30s
Details
CI Code / Code Coverage (pull_request) Successful in 3m31s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m45s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m56s
Details
CI Code / Linux (arch) (pull_request) Successful in 6m46s
Details
b05961e5a4 
ncurses didn't know the editor clobbered the physical screen, so the
first refresh after vim exit was a no-op diff. Stale frame stayed
visible until the next keystroke triggered readline redisplay.
jabber.developer requested changes 2026-05-31 11:47:55 +00:00
src/profanity.c Outdated
@@ -72,1 +73,4 @@
// 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 commented 2026-05-31 11:39:08 +00:00
Owner
Copy Link

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.
jabber.developer2 commented 2026-05-31 12:23:14 +00:00
Author
Collaborator
Copy Link

corrected

corrected
jabber.developer marked this conversation as resolved
src/profanity.c Outdated
@@ -95,1 +102,4 @@
log_stderr_handler();
// Resume from fg after SIGSTOP. Vim handles its own refresh.
if (cont_requested) {
jabber.developer commented 2026-05-31 11:39:38 +00:00
Owner
Copy Link

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.
jabber.developer2 commented 2026-05-31 12:23:19 +00:00
Author
Collaborator
Copy Link

corrected

corrected
jabber.developer marked this conversation as resolved
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from b05961e5a4 to 3f91cbc8b0 2026-05-31 11:54:20 +00:00 Compare
jabber.developer2 added 1 commit 2026-05-31 12:12:44 +00:00
remove SIGUSR1 escape; pkill <editor> is the standard recovery
All checks were successful
CI Code / Check spelling (pull_request) Successful in 17s
Details
CI Code / Check coding style (pull_request) Successful in 33s
Details
CI Code / Code Coverage (pull_request) Successful in 3m33s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m47s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 5m1s
Details
CI Code / Linux (arch) (pull_request) Successful in 6m59s
Details
9213272527 
The SIGUSR1 -> editor_emergency_kill wiring was a contrived escape
hatch. A console user with a stuck editor reaches for kill / pkill /
SIGKILL on the editor, not undocumented signals on the chat client.
editor_emergency_kill() stays for the internal Ctrl-Z abort path.
jabber.developer2 force-pushed fix/editor-ui-suspend-redraw from 9213272527 to b90dec5be3 2026-05-31 12:16:35 +00:00 Compare
jabber.developer2 added 1 commit 2026-05-31 12:24:14 +00:00
refactor(profanity): extract _handle_suspend_signals; tighten comments
All checks were successful
CI Code / Check spelling (pull_request) Successful in 17s
Details
CI Code / Check coding style (pull_request) Successful in 32s
Details
CI Code / Code Coverage (pull_request) Successful in 3m36s
Details
CI Code / Linux (debian) (pull_request) Successful in 4m45s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m56s
Details
CI Code / Linux (arch) (pull_request) Successful in 6m39s
Details
62d9c57925 
Main loop now calls a single helper for cont/suspend/Ctrl-Z
state instead of inlining the signal-handling block. Comment blocks in
profanity.c / editor.c / inputwin.c shortened to trailing form.
jabber.developer closed this pull request 2026-06-01 12:22:04 +00:00
jabber.developer deleted branch fix/editor-ui-suspend-redraw 2026-06-01 12:22:04 +00:00
All checks were successful
CI Code / Check spelling (pull_request) Successful in 17s
Required
Details
CI Code / Check coding style (pull_request) Successful in 32s
Required
Details
CI Code / Code Coverage (pull_request) Successful in 3m36s
Required
Details
CI Code / Linux (debian) (pull_request) Successful in 4m45s
Required
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 4m56s
Required
Details
CI Code / Linux (arch) (pull_request) Successful in 6m39s
Required
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
jabber.developer
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
Kind/Bug
Something is not working
Kind/Documentation
Documentation changes
Kind/Enhancement
Improve existing functionality
Kind/Feature
New functionality
Kind/Security
This is security issue
Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin jabber.developer jabber.developer2
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: devs/cproof#131
No description provided.
Delete Branch "fix/editor-ui-suspend-redraw"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?