Fix handling of fragmented terminal background color responses #272

pilgrimlyieu · 2025-02-22T15:28:15Z

This PR resolves #271.

Key Changes:

Response Buffering
- Added incompleteResponse buffer to accumulate partial escape sequences
```
+ var incompleteResponse []byte
```

Enhanced Parsing Logic

Modified parseTerminalBgColorResponse() to return completion status:

- func parseTerminalBgColorResponse(...) *Color
+ func parseTerminalBgColorResponse(...) (*Color, bool)

Verification:
Tested with Gruvbox Dark some shcemes, no accidental search mode activation observed:

time="Feb 22 20:56:42.453124" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 22 20:56:42.454905" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 22 20:56:42.454928" level=debug msg="Counted 14 lines in 6.478µs at 462ns/line"
time="Feb 22 20:56:42.454941" level=info msg="Stream read in 8.163µs"
time="Feb 22 20:56:42.455058" level=info msg="ttyin terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 22 20:56:42.455066" level=info msg="ttyout terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 22 20:56:42.455098" level=info msg="No known terminal with arrow keys emulation detected, assuming mouse tracking is needed"
time="Feb 22 20:56:42.455107" level=info msg="Entering Twin main loop..."
time="Feb 22 20:56:42.456013" level=debug msg="Terminal background color detected as #282828 after 890.63µs"
time="Feb 22 20:56:42.456023" level=debug msg="Using style <native>"
time="Feb 22 20:56:42.456038" level=info msg="Pager starting"
time="Feb 22 20:56:42.456150" level=debug msg="No lexer set for highlighting"
time="Feb 22 20:56:42.456156" level=debug msg="highlightFromMemory() took 1.212662ms"
time="Feb 22 20:56:42.456158" level=debug msg="Tailing file /home/fesmoph/.condarc"
time="Feb 22 20:56:42.456774" level=info msg="Entering pager main loop..."
time="Feb 22 20:56:43.063147" level=trace msg="ttyin high watermark bumped to 1 bytes"
time="Feb 22 20:56:43.063314" level=trace msg="Handling rune event 'q'/0x0071..."
time="Feb 22 20:56:43.063419" level=info msg="ttyin read error, twin giving up: EOF"

This fix resolves my immediate issue but would benefit from maintainer review. Happy to adjust based on feedback.

twin/screen.go

- Refactor `incompleteResponse` to directly store buffered data - Simplify color validation to use `*Color` and `bool` exclusively - Remove embedded anonymous function for clarity

pilgrimlyieu · 2025-02-23T02:07:56Z

Thank you for the thorough review! I've implemented your suggested changes:

Additional Corrections: During finally manual testing, I realized my initial reports omitted some key details:

Windows Terminal, Zsh, WSL Ubuntu 24.04 not always reporing "background color detected" when moar didn't enter search mode at first.

After the fix, moar still sometimes complains that it didn't detect terminal background color though it no longer enter search mode at the beginning.

I turned my terminal scheme to Gruvbox Light to make it more clear:

And the trace infomation is as follow:

time="Feb 23 10:02:23.314040" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 10:02:23.316171" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 10:02:23.316220" level=debug msg="Counted 14 lines in 6.423µs at 458ns/line"
time="Feb 23 10:02:23.316249" level=info msg="Stream read in 19.919µs"
time="Feb 23 10:02:23.316416" level=info msg="ttyin terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 10:02:23.316427" level=info msg="ttyout terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 10:02:23.316465" level=info msg="No known terminal with arrow keys emulation detected, assuming mouse tracking is needed"
time="Feb 23 10:02:23.316474" level=info msg="Entering Twin main loop..."
time="Feb 23 10:02:23.317149" level=debug msg="Got unexpected prefix in bg color response from terminal: f1/c7c7\x1b\\"
time="Feb 23 10:02:23.317153" level=trace msg="ttyin high watermark bumped to 2 bytes"
time="Feb 23 10:02:23.317158" level=debug msg="Unhandled multi character terminal escape sequence(s): {<0x1b>\\}"
time="Feb 23 10:02:23.366719" level=debug msg="Terminal background color still not detected after 50.237805ms, giving up"
time="Feb 23 10:02:23.366733" level=debug msg="Using style <native>"
time="Feb 23 10:02:23.366761" level=debug msg="No lexer set for highlighting"
time="Feb 23 10:02:23.366765" level=debug msg="highlightFromMemory() took 50.514106ms"
time="Feb 23 10:02:23.366767" level=debug msg="Tailing file /home/fesmoph/.condarc"
time="Feb 23 10:02:23.366742" level=info msg="Pager starting"
time="Feb 23 10:02:23.367560" level=info msg="Entering pager main loop..."
time="Feb 23 10:02:24.081307" level=trace msg="Handling rune event 'q'/0x0071..."

Writting "Terminal background color still not detected after 50.237805ms, giving up".

However it sometimes does get terminal background color, while the WT, PowerShell, Windows environment always does:

time="Feb 23 10:05:52.716270" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 10:05:52.718142" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 10:05:52.718168" level=debug msg="Counted 14 lines in 7.509µs at 536ns/line"
time="Feb 23 10:05:52.718194" level=info msg="Stream read in 18.957µs"
time="Feb 23 10:05:52.718412" level=info msg="ttyin terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 10:05:52.718423" level=info msg="ttyout terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 10:05:52.718469" level=info msg="No known terminal with arrow keys emulation detected, assuming mouse tracking is needed"
time="Feb 23 10:05:52.718491" level=info msg="Entering Twin main loop..."
time="Feb 23 10:05:52.719473" level=debug msg="Terminal background color detected as #fbf1c7 after 960.462µs"
time="Feb 23 10:05:52.719486" level=debug msg="Using style <tango>"
time="Feb 23 10:05:52.719502" level=info msg="Pager starting"
time="Feb 23 10:05:52.719602" level=debug msg="No lexer set for highlighting"
time="Feb 23 10:05:52.719608" level=debug msg="highlightFromMemory() took 1.410724ms"
time="Feb 23 10:05:52.719609" level=debug msg="Tailing file /home/fesmoph/.condarc"
time="Feb 23 10:05:52.720873" level=info msg="Entering pager main loop..."
time="Feb 23 10:05:53.393218" level=trace msg="ttyin high watermark bumped to 1 bytes"
time="Feb 23 10:05:53.393304" level=trace msg="Handling rune event 'q'/0x0071..."
time="Feb 23 10:05:53.393369" level=info msg="ttyin read error, twin giving up: EOF"

This issue seems to exist before this PR so I have no ideas about it. Should this problem be included in this PR too or report it in a new issue?

Please let me know if further adjustments are needed. Your expertise knowledge is invaluable!

twin/screen.go

walles · 2025-02-23T15:25:34Z

This looks like the second half of an actual response. Maybe this would get more obvious with logging of incomplete-but-valid prefixes?

"Got unexpected prefix in bg color response from terminal: f1/c7c7\x1b\\"

pilgrimlyieu · 2025-02-23T15:42:43Z

Hi @walles,

Acknowledgement & Correction: Upon re-verification, I realized my earlier report about issues reproduction was based on outdated build artifacts (my local build cache wasn't properly cleaned). After rebuilding, All cases now handle background queries correctly.

Trace information now:

time="Feb 23 23:39:29.576456" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 23:39:29.578207" level=debug msg="File is assumed to be uncompressed: /home/fesmoph/.condarc"
time="Feb 23 23:39:29.578238" level=debug msg="Counted 14 lines in 12.312µs at 879ns/line"
time="Feb 23 23:39:29.578257" level=info msg="Stream read in 13.918µs"
time="Feb 23 23:39:29.578386" level=info msg="ttyin terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 23:39:29.578396" level=info msg="ttyout terminal state: &{state:{termios:{Iflag:0 Oflag:4 Cflag:191 Lflag:2608 Line:0 Cc:[3 28 127 21 4 0 1 0 17 19 26 0 18 15 23 22 0 0 0] Ispeed:0 Ospeed:0}}}"
time="Feb 23 23:39:29.578434" level=info msg="No known terminal with arrow keys emulation detected, assuming mouse tracking is needed"
time="Feb 23 23:39:29.578443" level=info msg="Entering Twin main loop..."
time="Feb 23 23:39:29.579670" level=debug msg="Terminal bg color response received so far: fbfb/f1"
time="Feb 23 23:39:29.579934" level=debug msg="Terminal bg color response received so far: fbfb/f1f1/c7c7"
time="Feb 23 23:39:29.580431" level=debug msg="Terminal background color detected as #fbf1c7 after 1.963191ms"
time="Feb 23 23:39:29.580441" level=debug msg="Using style <tango>"
time="Feb 23 23:39:29.580454" level=info msg="Pager starting"
time="Feb 23 23:39:29.580583" level=debug msg="No lexer set for highlighting"
time="Feb 23 23:39:29.580588" level=debug msg="highlightFromMemory() took 2.327846ms"
time="Feb 23 23:39:29.580590" level=debug msg="Tailing file /home/fesmoph/.condarc"
time="Feb 23 23:39:29.581802" level=info msg="Entering pager main loop..."
time="Feb 23 23:39:29.989129" level=trace msg="ttyin high watermark bumped to 1 bytes"
time="Feb 23 23:39:29.989227" level=trace msg="Handling rune event 'q'/0x0071..."

This confirms the issue is fully resolved in the current implementation. My apologies for the earlier inaccurate context – appreciate your patience as I navigate these toolchain nuances.

The PR remains ready for final review when convenient.

pilgrimlyieu · 2025-02-24T01:17:49Z

Thanks for the suggestion! When implementing your feedback, I encountered a CI linting issue:

Error: twin/screen.go:442:4: ineffectual assignment to expectingTerminalBackgroundColor (ineffassign)
			expectingTerminalBackgroundColor = false

To resolve this, there are two options:

Option 1: Keep original implementation

			}
			// Not valid
			incompleteResponse = nil
-           expectingTerminalBackgroundColor = false
		}

(This is the currently committed version)

Option 2: Remove assignment outside if block

-		// We only expect this on entry, it's requested right before we start
-		// the main loop in NewScreenWithMouseModeAndColorCount().
-		expectingTerminalBackgroundColor = false

I temporarily chose Option 1 to keep CI passing, but happy to implement Option 2 if you prefer that approach. Please advise which direction you'd like me to pursue.

All tests passed locally now:

$ ./test.sh
Building sources...
Linting, repro any errors locally using "golangci-lint run"...
  Linting without tests...
  Linting with tests...
Running unit tests...
ok      github.com/walles/moar  (cached)
ok      github.com/walles/moar/m        (cached)
ok      github.com/walles/moar/m/linenumbers    (cached)
ok      github.com/walles/moar/m/textstyles     (cached)
ok      github.com/walles/moar/twin     (cached)
Testing cross compilation...
  Linux i386...
  Linux amd64...
  Linux arm32...
  macOS amd64...
  Windows amd64...
Doing a local build so we can continue testing...
Test reading from redirected stdin, writing to redirected stdout...
Test redirecting a file by name into file by redirecting stdout...
Test redirecting multiple files by name into redirected stdout...
Test redirecting non-existing file by name into redirected stdout...
Testing not crashing with different argument orders...
Please post the following report at <https://github.com/walles/moar/issues>,
or e-mail it to johan.walles@gmail.com.

Version     : v1.31.3-5-g7c3ca86-dirty
LANG        : en_US.UTF-8
TERM        : xterm-256color
MOAR        :
EDITOR      :
TERM_PROGRAM:

GOOS    : linux
GOARCH  : amd64
Compiler: gc
NumCPU  : 16

Stdin  is a terminal: true
Stdout is a terminal: false

time="Feb 24 09:23:53.269890" level=debug msg="File is assumed to be uncompressed: moar.go"
Please post the following report at <https://github.com/walles/moar/issues>,
or e-mail it to johan.walles@gmail.com.

Version     : v1.31.3-5-g7c3ca86-dirty
LANG        : en_US.UTF-8
TERM        : xterm-256color
MOAR        :
EDITOR      :
TERM_PROGRAM:

GOOS    : linux
GOARCH  : amd64
Compiler: gc
NumCPU  : 16

Stdin  is a terminal: true
Stdout is a terminal: false

time="Feb 24 09:23:53.279464" level=debug msg="File is assumed to be uncompressed: moar.go"
Please post the following report at <https://github.com/walles/moar/issues>,
or e-mail it to johan.walles@gmail.com.

Version     : v1.31.3-5-g7c3ca86-dirty
LANG        : en_US.UTF-8
TERM        : xterm-256color
MOAR        :
EDITOR      :
TERM_PROGRAM:

GOOS    : linux
GOARCH  : amd64
Compiler: gc
NumCPU  : 16

Stdin  is a terminal: true
Stdout is a terminal: false

time="Feb 24 09:23:53.288931" level=debug msg="File is assumed to be uncompressed: moar.go"
Test auto quitting on single screen...
  (success)
Test decompressing while piping
Test --version...
Test that the man page and --help document the same set of options...
Testing PAGER= suggestion in moar --help output...

All tests passed!

pilgrimlyieu · 2025-02-25T14:05:04Z

Hi @walles 👋

Just a quick ping that this PR is now:
✅ All CI checks passed
🔀 No merge conflicts
📝 Updated with your earlier feedback

The change remains small and low-risk. Would you like me to add any additional test cases or apply new suggestions?

Happy to make final tweaks if needed! Appreciate your time reviewing this.

walles · 2025-02-25T17:40:03Z

Thanks @pilgrimlyieu, now released:
https://github.com/walles/moar/releases/tag/v1.31.4

pilgrimlyieu · 2025-02-26T00:43:52Z

Just verified v1.31.4 works perfectly now. Thanks for maintaining this brilliant tool!

Fix #271

69780cc

pilgrimlyieu mentioned this pull request Feb 22, 2025

Moar enter search mode in Windows Terminal, Zsh, WSL Ubuntu 24.04 #271

Closed

walles reviewed Feb 22, 2025

View reviewed changes

twin/screen.go Outdated Show resolved Hide resolved

walles reviewed Feb 22, 2025

View reviewed changes

twin/screen.go Outdated Show resolved Hide resolved

walles reviewed Feb 22, 2025

View reviewed changes

twin/screen.go Outdated Show resolved Hide resolved

Address PR review feedback

00fd2a9

- Refactor `incompleteResponse` to directly store buffered data - Simplify color validation to use `*Color` and `bool` exclusively - Remove embedded anonymous function for clarity

Fix golint complaint

499746b

walles reviewed Feb 23, 2025

View reviewed changes

twin/screen.go Show resolved Hide resolved

walles reviewed Feb 23, 2025

View reviewed changes

twin/screen.go Show resolved Hide resolved

Drop expectation if prefix is invalid & Add new debug information

b2c2e05

Revert b2c2e05 about "Drop expectation if prefix is invalid"

7c3ca86

walles added 2 commits February 25, 2025 16:41

Unset both variables at once

dbd2be7

Merge branch 'master' into fix-wt

e2277a3

walles merged commit c81557a into walles:master Feb 25, 2025
2 checks passed

pilgrimlyieu deleted the fix-wt branch February 25, 2025 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of fragmented terminal background color responses #272

Fix handling of fragmented terminal background color responses #272

pilgrimlyieu commented Feb 22, 2025 •

edited

Loading

pilgrimlyieu commented Feb 23, 2025 •

edited

Loading

walles commented Feb 23, 2025

pilgrimlyieu commented Feb 23, 2025

pilgrimlyieu commented Feb 24, 2025 •

edited

Loading

pilgrimlyieu commented Feb 25, 2025

walles commented Feb 25, 2025

pilgrimlyieu commented Feb 26, 2025

Fix handling of fragmented terminal background color responses #272

Fix handling of fragmented terminal background color responses #272

Conversation

pilgrimlyieu commented Feb 22, 2025 • edited Loading

pilgrimlyieu commented Feb 23, 2025 • edited Loading

walles commented Feb 23, 2025

pilgrimlyieu commented Feb 23, 2025

pilgrimlyieu commented Feb 24, 2025 • edited Loading

pilgrimlyieu commented Feb 25, 2025

walles commented Feb 25, 2025

pilgrimlyieu commented Feb 26, 2025

pilgrimlyieu commented Feb 22, 2025 •

edited

Loading

pilgrimlyieu commented Feb 23, 2025 •

edited

Loading

pilgrimlyieu commented Feb 24, 2025 •

edited

Loading