fix(datagrid): show column comments in headers on first load (#1861)#1863
Merged
Conversation
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.
Fixes #1861.
Column comments were missing from the data grid header until the query was run again, and once they appeared the header was too short, so the comment was cut off until a column was resized.
That one report turned out to be three defects in the header comment feature added in #1842.
1. Comments never reached the header on first load
Column comments are not part of the row result. They arrive from the background schema fetch (phase 2), which mutates
TableRows.columnCommentsin place. The grid rebuild gate,DataGridUpdateSnapshot, had no field derived from comment content, only the globalshowObjectCommentstoggle. So the snapshot compared equal,reconcileColumnPoolwas skipped, and that is the only code that writesheaderComment.Re-running the query worked by accident:
configureForTablebumpsreloadVersion, which trips the gate for an unrelated reason.The snapshot now carries the effective
columnComments(setting applied), which subsumes the old bool and makes the gate reflect what the header actually draws.2. The header height never applied
applyHeaderHeight()calledenclosingScrollView?.tile(), andenclosingScrollViewis always nil on anNSTableHeaderView: the header lives in the scroll view's header clip view, not itscontentView, andNSView.enclosingScrollViewonly resolves through the content clip. The header view grew to 40pt while the scroll view's header region stayed at 28pt, which is what clipped the comment.Only an
NSScrollViewtiling pass picks up header height.NSTableView.tile()does not, despite what its documentation implies. A column resize drag happens to make the scroll view re-tile, which is why dragging appeared to fix it.The tile now goes through
tableView?.enclosingScrollView.Two follow-ons from that:
setFrameSizeclamp, added with a comment claimingtile()would otherwise shrink the header back, was a workaround for this misdiagnosis. The height survivestableView.tile(), scroll view resize, column width changes,addTableColumn,reloadData, and layout passes without it, so it is removed.commentHeaderHeightis no longer a hardcoded40. It is the natural header height plus the measured comment line height. AppKit's default header height has changed twice (17 to 23 to 28), so the compact baseline should not be assumed.3. The first reconcile ran too early
In
makeNSView,reconcileColumnPoolran before theSortableHeaderViewwas installed and before the table view joined the scroll view, so the first pass cast the header to nil and could never apply a height. The header anddocumentVieware now set up before the initial reconcile.Also: an edited comment now repaints the header. Mutating a header cell's content does not set
needsDisplay, so a comment change at an unchanged header height would otherwise keep drawing the old text.Tests
SortableHeaderViewTests(new): with a realNSScrollViewandNSTableView, asserts that showing comments grows both the header view and the scroll view's header clip, that hiding restores the platform height, and that a later resize/reload keeps the grown header. The clip assertion is the point: the old code already set the header frame correctly, so the existing test (a bareNSTableViewwith no scroll view) passed while the feature was visibly broken.DataGridUpdateSnapshotTests: comments arriving after the rows, and an edited comment, must both change the snapshot.DataGridColumnPoolTests: comments arriving on a second reconcile (the phase 2 case) grow the header; an edited comment reaches the cell and tooltip at an unchanged height.28 tests pass,
swiftlint lint --strictis clean.Notes
The two-line header is a database client convention rather than an Apple one; the HIG's mechanism for supplementary column information is
headerToolTip, which this feature already sets alongside the inline line. Mature clients converge on the tooltip, and the inline comment line is a long-standing open request in DataGrip and DBeaver, so keeping it is the right call. The header still only grows when a visible column actually has a comment, so tables without comments cost no vertical space.