From 9245015d1a005611801d7393e4d7e3cdf5fbca0c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tau=20G=C3=A4rtli?= <git@tau.garden>
Date: Thu, 22 Aug 2024 16:19:24 +0200
Subject: [PATCH] terminal: Retain relative order of responses (#16456)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Partially addresses #8497 (namely, the occurring with `delta`)

As I mentioned in
https://github.com/zed-industries/zed/issues/8497#issuecomment-2226896371,
zed currently replies to OSC color requests (`OSC 10`, `OSC 11`, ...)
out of order when immediately followed by another request (for example
`CSI c`). All other terminals that [I have
tested](https://github.com/bash/terminal-colorsaurus/blob/main/doc/terminal-survey.md)
maintain relative order when replying to requests.

## Solution
Respond to the `ColorRequest` in `process_event` (in the same place
where other PTY writes happen) instead of queuing it up in the internal
event queue.

## Alternative
I initially thought that I could handle the color request similarly to
the `TextAreaSizeRequest` where the size is stored in `last_content` and
updated on `sync`. However this causes the terminal to report
out-of-date values when a "set color" sequence is followed by a color
request.

## Tests

1. `OSC 11; ?` (request bg color) + `CSI c` (request device attributes):
   ```shell
   printf '\e]11;?\e\\ \e[c' && cat -v
   # Expected result: ^[]11;rgb:dcdc/dcdc/dddd^[\^[[?6c
# Current result: ^[[?6c^[]11;rgb:dcdc/dcdc/dddd^[\ (❌)
# Result with this PR: ^[]11;rgb:dcdc/dcdc/dddd^[\^[[?6c (✅)
# Result with alternative: ^[]11;rgb:dcdc/dcdc/dddd^[\^[[?6c (✅)
   ```
2. `OSC 11; rgb:f0f0/f0f0/f0f0` (set bg color) + `OSC 11; ?` (request bg
color)
   ```shell
   printf '\e]11;rgb:f0f0/f0f0/f0f0\e\\ \e]11;?\e\\' && cat -v
   # Expected result: ^[]11;rgb:f0f0/f0f0/f0f0^[\
# Current result: ^[]11;rgb:f0f0/f0f0/f0f0^[\ (✅)
# Result with this PR: ^[]11;rgb:f0f0/f0f0/f0f0^[\ (✅)
# Result with alternative: ^[]11;rgb:OUT_OF_DATE_COLOR_HERE^[\ (❌)
   ```

Release Notes:

- N/A
---
 crates/terminal/src/terminal.rs | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs
index ce6b9bd6ac4402ecb26ed37d6739a15ccf52aaf7..b9eddd8bb3d0f3b9472ad78364430cb3501ccf2c 100644
--- a/crates/terminal/src/terminal.rs
+++ b/crates/terminal/src/terminal.rs
@@ -18,7 +18,7 @@ use alacritty_terminal::{
         Config, RenderableCursor, TermMode,
     },
     tty::{self, setup_env},
-    vte::ansi::{ClearMode, Handler, NamedPrivateMode, PrivateMode, Rgb},
+    vte::ansi::{ClearMode, Handler, NamedPrivateMode, PrivateMode},
     Term,
 };
 use anyhow::{bail, Result};
@@ -127,7 +127,6 @@ pub enum MaybeNavigationTarget {
 
 #[derive(Clone)]
 enum InternalEvent {
-    ColorRequest(usize, Arc<dyn Fn(Rgb) -> String + Sync + Send + 'static>),
     Resize(TerminalSize),
     Clear,
     // FocusNextMatch,
@@ -688,9 +687,19 @@ impl Terminal {
                     cx.emit(Event::TitleChanged);
                 }
             }
-            AlacTermEvent::ColorRequest(idx, fun_ptr) => {
-                self.events
-                    .push_back(InternalEvent::ColorRequest(*idx, fun_ptr.clone()));
+            AlacTermEvent::ColorRequest(index, format) => {
+                // It's important that the color request is processed here to retain relative order
+                // with other PTY writes. Otherwise applications might witness out-of-order
+                // responses to requests. For example: An application sending `OSC 11 ; ? ST`
+                // (color request) followed by `CSI c` (request device attributes) would receive
+                // the response to `CSI c` first.
+                // Instead of locking, we could store the colors in `self.last_content`. But then
+                // we might respond with out of date value if a "set color" sequence is immediately
+                // followed by a color request sequence.
+                let color = self.term.lock().colors()[*index].unwrap_or_else(|| {
+                    to_alac_rgb(get_color_at_index(*index, cx.theme().as_ref()))
+                });
+                self.write_to_pty(format(color));
             }
             AlacTermEvent::ChildExit(error_code) => {
                 self.register_task_finished(Some(*error_code), cx);
@@ -714,12 +723,6 @@ impl Terminal {
         cx: &mut ModelContext<Self>,
     ) {
         match event {
-            InternalEvent::ColorRequest(index, format) => {
-                let color = term.colors()[*index].unwrap_or_else(|| {
-                    to_alac_rgb(get_color_at_index(*index, cx.theme().as_ref()))
-                });
-                self.write_to_pty(format(color))
-            }
             InternalEvent::Resize(mut new_size) => {
                 new_size.size.height = cmp::max(new_size.line_height, new_size.height());
                 new_size.size.width = cmp::max(new_size.cell_width, new_size.width());