Repository navigation
feat(terminal): colors themes - #85
Merged
Merged
Conversation
- Emulator keeps 256-color and truecolor SGR (both ; and : spellings), plus dim, italic, underline, inverse, hidden, strikethrough and cursor visibility - Six built-in terminal themes resolved at draw time; pixel-snapped background runs - Settings: theme, cursor style, blinking cursor, history length, with a live preview
- Alternate screen (?47/1047/1049), scroll regions, origin mode, IL/DL/ICH/DCH/ECH, SU/SD, REP, autowrap toggle - Application cursor keys, DEC line-drawing charset, DSR/DA replies; ESC ( B no longer prints a stray B - Box-drawing and block characters drawn per cell so frames join; one key table adds Delete, PgUp/PgDn, F1-F12 - Wheel and swipe on the alternate screen send arrow keys
PR Summary by QodoAdd terminal themes and full-screen SSH support
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. One-column terminals crash on backspace
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| case '\b': if (_cursorCol > 0) _cursorCol--; break; | ||
| // VT and FF are line feeds on every terminal that matters. | ||
| case '\n' or '\v' or '\f': LineFeed(); break; | ||
| case '\b': if (_cursorCol > 0) _cursorCol = Math.Min(_cursorCol, Columns - 1) - 1; break; |
There was a problem hiding this comment.
1. One-column terminals crash on backspace 🐞 Bug ≡ Correctness
ProcessGround subtracts one after clamping a pending-wrap column to Columns - 1, producing column -1 in a one-column grid. After a character fills that grid, a backspace followed by printable output indexes the row at -1; wider grids also move back one cell too far at the right edge.
Agent Prompt
## Issue description
Backspace after a pending wrap computes an incorrect column, including `-1` in a one-column terminal.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[296-306]
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[636-657]
## Recommended Fix
Subtract from the pending column before clamping to the valid cell range. Add tests for backspace after filling both one-column and wider rows.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+811
to
+812
| var limit = _cursorRow <= _scrollBottom ? _scrollBottom : Rows - 1; | ||
| _cursorRow = Math.Min(limit, _cursorRow + n); |
There was a problem hiding this comment.
2. Cursor movement stops at wrong margins 🐞 Bug ≡ Correctness
CursorUp and CursorDown choose a scroll margin based on only one side of the cursor position, so each also constrains movement that starts outside the region. With a region spanning rows 3–5, moving down from row 1 stops at row 5 instead of reaching the screen bottom, and moving up from below the region has the symmetric failure.
Agent Prompt
## Issue description
Cursor-up and cursor-down incorrectly stop at scroll margins when movement starts outside the scroll region.
## Fix Focus Areas
- src/OpenIPC.Viewer.Core/Ssh/Terminal/TerminalEmulator.cs[802-812]
## Recommended Fix
Apply margin limits only when the starting row lies between both margins; otherwise use screen limits. Test movements starting above, inside, and below a partial scroll region.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+266
to
+268
| TerminalThemeOptions = TerminalTheme.BuiltIn | ||
| .Select(t => new TerminalThemeOption(Localizer.Instance[$"Settings.Ssh.Theme.{t.Id}"], t)) | ||
| .ToList(); |
There was a problem hiding this comment.
3. Theme labels stay in the old language 🐞 Bug ≡ Correctness
TerminalThemeOptions and TerminalCursorStyleOptions store localized display strings when the settings view model is constructed and never rebuild them. Changing the application language while that page remains open updates localizer-backed labels, but both new terminal pickers keep their previous-language entries until the page is recreated.
Agent Prompt
## Issue description
The new terminal option lists retain their constructor-time language after an in-page language change.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/ViewModels/SettingsPageViewModel.cs[266-275]
- src/OpenIPC.Viewer.App/ViewModels/SettingsPageViewModel.cs[345-346]
## Recommended Fix
Refresh the terminal theme and cursor option displays when the localizer changes, preserving the selected theme and style, or bind their labels directly to live localization.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1177
to
+1181
| if (cell.Char > ' ') | ||
| { | ||
| var (_, bg) = theme.ColorsOf(cell); | ||
| DrawCellChar(context, cell.Char, bg, cell.Attributes, x, y); | ||
| } |
There was a problem hiding this comment.
4. Block cursor hides the character on highlighted cells 🐞 Bug ≡ Correctness
In DrawCursor’s Block case, the character is redrawn in theme.ColorsOf(cell).Background over the theme.Cursor fill, and the redraw does not check TerminalAttributes.Hidden. On inverse cells in most built-in themes, that background matches the cursor colour and the character disappears; on SGR 8 cells, text suppressed by DrawRow becomes visible while the cursor rests there.
Agent Prompt
## Issue description
The block cursor redraws the cell character in its resolved background colour. On inverse cells, that colour can match the cursor fill, making the character disappear; the redraw also exposes characters hidden with SGR 8.
## Fix Focus Areas
- src/OpenIPC.Viewer.App/Controls/TerminalView.cs[1169-1183]
## Recommended Fix
Redraw visible characters in a colour that contrasts with `theme.Cursor`: use `theme.Background`, as suggested by the “redrawn in the screen color” comment, or choose whichever resolved cell colour contrasts more with the cursor fill. Skip the character redraw when `(cell.Attributes & TerminalAttributes.Hidden) != 0`, while keeping the cursor fill visible.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes