Fix bad sticky scroll when ctrl+l in terminal #274685
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where the terminal sticky scroll overlay was not being hidden when commands were cleared from the screen (e.g., via Ctrl+L or clear command). The fix ensures that when all commands are invalidated, the sticky scroll overlay is properly hidden if it was displaying one of the cleared commands.
Key changes:
- Added a new
_clearAllCommandsmethod to clear all commands when the screen is cleared - Updated PTY heuristics to call
clearAllCommandsinstead ofclearCommandsInViewportfor screen clear operations - Registered an event listener in the sticky scroll overlay to hide the overlay when its displayed command is invalidated
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| terminalStickyScrollOverlay.ts | Adds event listener to hide sticky scroll when the displayed command is invalidated |
| commandDetectionCapability.ts | Introduces _clearAllCommands method and updates heuristics to clear all commands on screen clear |
src/vs/workbench/contrib/terminalContrib/stickyScroll/browser/terminalStickyScrollOverlay.ts
Outdated
Show resolved
Hide resolved
| @@ -523,7 +510,7 @@ class UnixPtyHeuristics extends Disposable { | |||
| super(); | |||
| this._register(_terminal.parser.registerCsiHandler({ final: 'J' }, params => { | |||
There was a problem hiding this comment.
I was thinking more generic fix with clearAllCommands(), but not sure.
For clear, turning off sticky scroll after detecting command clear was enough.
For ctrl+l we can't necessarily do the same since ctrl+l is not a command that the user type in the terminal.
- We do see them (ctrl+l) here, and clearing both commands in scrollback & viewport prevents from bad sticky scroll after
ctrl+l.
I want to be-careful here though, was there special reason we used to only cleared commands in viewport previously and not included commands in scrollback? @Tyriar
There was a problem hiding this comment.
Maybe it depends on platform, but on Windows/pwsh ctrl+l sends back CSI 2 J which only clears the viewport. So all commands in the scrollback are still there are perfectly valid.
| // Clear all commands when the viewport is cleared. | ||
| // Handles both `clear` and `ctrl+l` to prevent bad sticky scroll. | ||
| if (params.length >= 1 && (params[0] === 2 || params[0] === 3)) { | ||
| this._hooks.clearCommandsInViewport(); | ||
| this._hooks.clearAllCommands(); |
There was a problem hiding this comment.
Is the problem we need to handle params[0] = 3 to clear scrollback and 2 to clear viewport?
There was a problem hiding this comment.
@Tyriar Doing this would make the bug (weird sticky scroll) show up again.🥲
Since for ctrl+l, we only hit params[0] === 2, only clearing viewport.
I was thinking that the missing escape sequences on the empty line(specifically added from ctrl+l) is causing things to be messed up? More details: #270029 (comment)
But here is what happens if we handle params[0] === 2,3 separately.
| if (command.command.trim().toLowerCase() === 'clear' || command.command.trim().toLowerCase() === 'cls') { | ||
| this._tryAdjustCommandStartMarkerScheduler?.cancel(); | ||
| this._tryAdjustCommandStartMarkerScheduler = undefined; | ||
| this._hooks.clearCommandsInViewport(); | ||
| this._hooks.clearAllCommands(); |
There was a problem hiding this comment.
clear doesn't necessarily do this. For scrollback, what's meant to happen I think is that the line will get disposed and therefore the command on that line will listen to marker.onDispose and invalidate itself. We should make sure that's working.
There was a problem hiding this comment.
I see, for commands that are in scrollback after clear would not be sticky anymore since they are invalidated, right?
|
Handled with: #277215 |
Resolves: #270029