-
Notifications
You must be signed in to change notification settings - Fork 45
fix: incorrect parsing on WSL due to buffering of osc esc sequences #499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes OSC 11 parsing when the sequence is split across multiple read() calls. Previously, incomplete OSC sequences would be treated as UnknownEvent, causing the first part to be ignored and the terminator to be misinterpreted as an Alt+\ key press. This change adds buffering to the Reader struct to accumulate partial string-terminated sequences (OSC, DCS, APC, SOS, PM) until they are complete.
Adds TestSplitSequences to key_test.go with multiple test cases covering: - OSC 11 background color with ST and BEL terminators - OSC 10 foreground color split across reads - OSC 12 cursor color split across reads - DCS and APC sequences split across reads - Multiple chunk scenarios - Mixed OSC + regular key sequences Removes separate osc11_linux_test.go file to follow existing test patterns.
|
Can confirm that this seems to fix the issue for me locally with WSL2 + Windows Terminal. Still testing on other terminals, but looks promising. EDIT: seems as though with WezTerm & WSL2, and Hyper (js) & WSL2 (only other terminal emulators I have locally), neither seem to receive the background color message still. Though, they also don't seem to respond to |
|
unfortunately i can only test with windows terminal because of work computer restrictions. EDIT: the below probably isnt true. one of the original issues that led me down this rabbithole mentioned they were having this issue on Wezterm as well (sst/opencode#127 (comment)). @lrstanley would you be able to test the repro in the first issue linked in the description on wezterm? from my very limited googling, it seems like wezterm does not support the osc 11 query, but this could be incorrect. |
Not sure which repro you're referring to, unless you mean running opencode itself on that specific commit (before the patch was implemented?) I did see wezterm/wezterm#5899 -- and although this is supported through ConPTY now I think, wezterm hasn't released in a while and might depend on something older with such an old build. Unless there is a usecase where we know it should return a response that I can test with, I'd say it's probably fine to continue with this fix, unless someone else notices specific issues. |
would you be able to try this @lrstanley ? package main
import (
"fmt"
"image/color"
"log"
"os"
"path/filepath"
tea "github.com/charmbracelet/bubbletea/v2"
"github.com/charmbracelet/x/input"
)
type model struct {
bgColor color.Color
keypresses string
logpath string
}
func (m model) Init() tea.Cmd {
return tea.RequestBackgroundColor
}
func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
log.Printf("Received msg: %#v with type %T", msg, msg)
switch msg := msg.(type) {
case tea.BackgroundColorMsg:
// this never happens on windows terminal
log.Printf("got a BackgroundColorMsg with color %#v", msg.Color)
m.bgColor = msg.Color
return m, nil
case input.UnknownEvent:
log.Printf("got an unknown event '%v'", msg.String())
return m, nil
case tea.KeyPressMsg:
switch keypress := msg.String(); keypress {
case "q", "ctrl+c":
return m, tea.Quit
default:
m.keypresses += msg.String()
return m, nil
}
}
return m, nil
}
func (m model) View() string {
return fmt.Sprintf("logging to %s\nkeypresses: %s\nbg color: %#v\n", m.logpath, m.keypresses, m.bgColor)
}
func main() {
logpath := filepath.Join(os.TempDir(), "repro.log")
f, err := tea.LogToFile(logpath, "")
if err != nil {
fmt.Println("fatal:", err)
os.Exit(1)
}
defer f.Close()
if _, err := tea.NewProgram(model{logpath: logpath}).Run(); err != nil {
fmt.Println("Error running program:", err)
os.Exit(1)
}
} |
That's effectively what my testing has already been doing. WezTerm/Hyper, no background color received (but doesn't look like they support it from OSC query testing). Works on Windows Terminal + WSL2, with the |
|
@aymanbagabas any movement on this issue? |
|
I think it's pending primarily due to the new https://github.com/charmbracelet/ultraviolet package, that I think they're about to switch to using in the v2-exp branches of bubbletea/lipgloss. I haven't had time to confirm yet if it solves the issues though. |
|
FYI, I think the issue still occurs with the ultraviolet implementation, due to the same issues outlined here (the escape sequences caught at the end of 1 buffer don't get cleanly grouped into the next received buffer). Working with Ayman on the issue. |
it passes all tests and fixes the repro described in the bubbletea issue (first link below).
related:
my conversation with o3 to diagnose this issue: https://gist.github.com/rgodha24/3c6934b9ec94df4ab0bf75588d1042dc (t3.chat doesnt have share links yet)
NOTE: this is completely vibe coded. I would not get mad at all if this is closed bc of subpar code quality