feat: Allow setting rows/cols for dimensions instead of just pixels - #763
Open
DaltonSW wants to merge 5 commits into
Open
feat: Allow setting rows/cols for dimensions instead of just pixels#763DaltonSW wants to merge 5 commits into
DaltonSW wants to merge 5 commits into
Conversation
Member
|
@DaltonSW mind fixing the build errors? |
Contributor
Author
|
@meowgorithm Done! I think I misunderstood LFS stuff. 😅 Did you want me to clean up any of the |
Member
|
Apologies for the slow reply @DaltonSW — yes, please. We'll need CI green in order to merge this. |
DaltonSW
force-pushed
the
feat/rows-cols
branch
2 times, most recently
from
August 4, 2026 12:42
0380b12 to
1b0ea2c
Compare
Member
|
Hey @DaltonSW, thank you for this PR. Regarding the token variables, I talked to Christian, lets add an exception for this rule (in this repo) |
DaltonSW
force-pushed
the
feat/rows-cols
branch
from
August 11, 2026 13:39
1b0ea2c to
661b3bb
Compare
andrinoff
requested changes
Aug 12, 2026
Comment on lines
+239
to
+253
| var cellWidth, cellHeight float64 | ||
| for range dimensionProbeAttempts { | ||
| cols, rows := measure(probeWidth, probeHeight) | ||
|
|
||
| tooSmall := cols <= 0 || rows <= 0 || | ||
| (style.Columns > 0 && cols < style.Columns) || | ||
| (style.Rows > 0 && rows < style.Rows) | ||
| if !tooSmall { | ||
| cellWidth = float64(probeWidth) / float64(cols) | ||
| cellHeight = float64(probeHeight) / float64(rows) | ||
| break | ||
| } | ||
| probeWidth = double(probeWidth) | ||
| probeHeight = double(probeHeight) | ||
| } |
Member
There was a problem hiding this comment.
silent failure. if !tooSmall is never satisfied cellWidth and cellHeight stay 0
Comment on lines
+255
to
+262
| contentWidth := probeWidth | ||
| contentHeight := probeHeight | ||
| if style.Columns > 0 { | ||
| contentWidth = int(math.Round(cellWidth * float64(style.Columns))) | ||
| } | ||
| if style.Rows > 0 { | ||
| contentHeight = int(math.Round(cellHeight * float64(style.Rows))) | ||
| } |
Member
There was a problem hiding this comment.
it will result in an 0x0 image, reasons in comment 1
Comment on lines
+267
to
+282
| for range dimensionCorrectionAttempts { | ||
| cols, rows := measure(contentWidth, contentHeight) | ||
|
|
||
| converged := true | ||
| if style.Columns > 0 && cols != style.Columns { | ||
| contentWidth += int(math.Round(float64(style.Columns-cols) * cellWidth)) | ||
| converged = false | ||
| } | ||
| if style.Rows > 0 && rows != style.Rows { | ||
| contentHeight += int(math.Round(float64(style.Rows-rows) * cellHeight)) | ||
| converged = false | ||
| } | ||
| if converged { | ||
| break | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
this will give up silently after 5 attempts
andrinoff
requested changes
Aug 18, 2026
andrinoff
left a comment
andrinoff
left a comment
Member
There was a problem hiding this comment.
We should round up to even numbers, because MP4 and WebM Video dont like odd numbers as the resolution
DaltonSW
force-pushed
the
feat/rows-cols
branch
from
August 23, 2026 16:14
6d22b49 to
38552b1
Compare
Adds `Set Rows`/`Set Columns` commands to size the terminal in character cells instead of pixels. The pixel size is derived by probing the live xterm.js terminal's own fit/measurement APIs and iterating to an exact match, so it stays correct across font/theme changes without depending on any of xterm.js's private internals.
Adds demo tapes for Set Rows and Set Columns (individually and combined) and documents them in examples/settings/README.md, matching the existing width/height example pattern.
resolveRowsColumns gave up quietly in two places: if the probe loop never measured the terminal, cellWidth/cellHeight stayed zero and the viewport was computed as 0x0; and if the correction loop ran out of attempts, the tape rendered at whatever grid it happened to land on. Both now return a descriptive error naming the requested grid and the closest measurement. Setup returns an error so they reach the caller through the existing Evaluate error path.
MP4 and WebM encoders reject odd-numbered resolutions, but the grid-derived width/height from Set Rows/Set Columns could land on an odd pixel count.
DaltonSW
force-pushed
the
feat/rows-cols
branch
from
August 23, 2026 16:15
38552b1 to
bb8c012
Compare
andrinoff
approved these changes
Aug 24, 2026
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
3 participants
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.
Adds
Set Rows/Set Columnscommands to size the terminal in charactercells instead of pixels. The pixel size is derived by probing the live
xterm.js terminal's own fit/measurement APIs and iterating to an exact
match, so it stays correct across font/theme changes without depending
on any of xterm.js's private internals.
CONTRIBUTING.md.