Repository navigation
Fix the Commits panel's column widths when scrolling during interactive rebase or bisect - #6022
Merged
Merged
Conversation
Decolorise stores its result in a cache keyed by the string it was given, but the lookup treats an empty result as a miss, so a string that decolorises to nothing is recomputed on every call. Recomputing it means compiling two regexes, and that costs 4534ns against 24.7ns for a string the cache does answer for. Empty cells are everywhere in the list panels, and RenderDisplayStrings measures every cell twice, once to work out the column widths and once to pad it. A commits panel showing 50 lines with one empty column spends about 0.45ms of every render on this. Read the cache with the two-value form, so that an empty result counts as an answer, and compile the two regexes once at package level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s styling These three cells are built and styled in one step, so their width can only be measured by stripping the styling off again. A later commit needs those widths to reserve space for the columns they go into. Pull the text out into getHashText, getActionText and a getBisectStatusText that no longer styles what it returns, and apply the styling at the call site. A commit with no hash, such as a "break" or "update-ref" todo, now gets an empty hash cell instead of one holding nothing but colour codes. Nothing can see the difference: both render as the same number of blank columns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A caller that asks for a padding of zero gets its string back unchanged, but only after WithPadding has measured it, and measuring means a Decolorise lookup and a width scan over the result. A later commit pads four cells of every commit in the list, and asks for a padding of zero for all four whenever no rebase or bisect is in progress and the date column is hidden, so make that case cost nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
formatExpected runs the expected output through strings.TrimSpace to get rid of the newlines that the raw string literal starts and ends with, but that also eats the indentation of the first line. A scenario whose first line starts with an empty column can't express what it expects, and a later commit adds two of those. Trim the newlines only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ines need The commits panel renders only the lines that are on screen, so the width of a column follows what happens to be visible, and a column all of whose visible lines are empty disappears altogether. Scrolling past the pending rebase todos takes the action column away with them, and everything to its right jumps to the left. Cover the same problem for two more columns: the hash column, which goes away while only hashless todos such as "update-ref" are on screen, and the date column in the expanded commits panel, which is as narrow as the time format while only commits from today are on screen. This file is deliberately left un-gofumpt'd: gofumpt indents the body of the commented-out block by a tab, which would fill the diff of the commit that swaps the two blocks with whitespace-only changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
During an interactive rebase, scrolling down far enough that the pending todos leave the screen makes the author and subject of every commit jump to the left; scrolling back up makes them jump back. During a bisect the column that holds "<-- current" and "?" comes and goes the same way, and in the expanded commits panel the date column is only as wide as gui.shortTimeFormat while nothing but today's commits is on screen. A column is as wide as the widest string in it, and a column whose strings are all empty is dropped altogether. The commits and sub-commits panels render only the lines that are on screen, so those widths come from the visible lines alone and follow the scroll position. Pad the hash, bisect, action and date cell of every line to the width that all the commits in the list need. A padded cell is no longer empty, so its column is never dropped, and the column is already as wide as the whole list needs, so its width no longer depends on what is visible. The date column is the exception. Formatting the date of every commit on every render costs too much, so it measures the oldest commit only. That covers the conventional time formats; getReservedColumnWidths says what it misses, and why it can never reserve width that no commit asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reflog commits context slices the visible lines out itself and passes only those, so the function has no way to look at the rest of the list. A following commit needs the oldest entry to work out how much width the date column needs. Take the whole list along with the range to render, the way GetCommitListDisplayStrings already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ines need The reflog panel renders only the lines that are on screen too, so the expanded panel's date column is only as wide as gui.shortTimeFormat while nothing but entries from today is visible, and the message of every entry jumps to the left. A reflog reaches back in time, so scrolling crosses that boundary soon enough. GetReflogCommitListDisplayStrings had no tests at all, so cover the two shapes it can return along the way. This file is deliberately left un-gofumpt'd: gofumpt indents the body of the commented-out block by a tab, which would fill the diff of the commit that swaps the two blocks with whitespace-only changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pad the date of every line to the width that the oldest entry in the reflog asks for, so that the column keeps its width as the user scrolls. This is the same treatment the commits panel's date column gets, with the same limits; getReservedColumnWidths spells them out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 task
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
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.
The commit list only renders the visible rows for performance reasons (see #3687), which means it also uses only the visible rows for determining the widths of the list's columns. In an interactive rebase, when you scroll down so that no rebase todos are visible any more, the author and subject columns would snap to the left, which looks ugly and distracting. A similar thing happened in half-screen mode when only commits from today are visible: today's commits use a shorter date format, so as you scroll down to make an older commit visible, the columns to the right of the date would move to the right to make room for the longer date.
Fix this by taking the width requirements of all commits into account, not just the visible ones, and pad the column texts to those widths. For the date we take a shortcut: measuring the widths of all commits would be too expensive, so we only format the oldest commit's date, on the assumption that this one is the least likely to be from today. This only works when the long time format is actually longer than the short one, and when the long format always results in the same width for all dates; both of these assumptions might be false when users reconfigure their
gui.timeFormatorgui.shortTimeFormatconfigs (e.g. to include a week day), but for the default values of these it works.