fix(datagrid): leave room for a cell's action button when sizing its column - #2309
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 #2303.
A column whose cells draw an action button, such as the up and down arrows on a date, shows its value cut short by roughly the width of that button.
Root cause
Two separate mechanisms, both confirmed against the code and against real saved data on this machine.
1. Widths saved before 0.66 are pinned forever. Up to 0.65.0,
captureColumnLayoutwrotecolumn.widthfor every column with no user-sized filter, anddismantleNSViewpersisted on every grid teardown, so browsing a table once saved a complete map of automatic widths. Those widths were measured byDataGridCellFactorywith a flat 16pt padding and no accessory term at all, which the accessory-aware measurement in #2148 replaced in 0.66.On current
mainthose maps are never re-measured.DataGridColumnPool.reconcile:86setswillRestoreWidthsfromsavedLayout.columnWidths, and:95resolvessavedLayout?.columnWidths[columnName] ?? widthCalculator(columnName, slot), so a name in the map means the calculator is never called.synchronizeUserSizedColumnsthen unions every saved name intouserSizedColumnNames, soliveWidthsForReconciliationwill not drop it either, andshouldRecalculateAutomaticColumnWidthsis set only by an explicit Reset Columns. The measurement code is not on the path, so an accessory-aware fix to the calculator alone changes nothing for these tables.Measured:
~/Library/Application Support/TablePro/ColumnLayouton this machine holds 363 entries, 72 of them in the pre-0.66 shape (columnWidthspopulated,columnContentWidthsnull). Current code cannot produce that shape, becausecaptureColumnLayoutfills both maps in the same branch. The ChinookInvoiceentry storesInvoiceDate: 168.6865234375; solving the header formula gives a character width of 8.0361, and19 * 8.0361 + 16 = 168.6865against19 * 8.0361 + 32 = 184.6865for the same column after an explicit fit. The deficit is exactlyDataGridCellAccessory.chevron.reservedTrailingWidth, or 20pt for a foreign key. These files are iCloud-synced, so a reinstall pulls them back.2. A late accessory can never widen its column.
refreshCellPresentations()repaints and never resizes, andapplyPhase2MetadataandapplyEnumValuesbump onlytab.metadataVersion, which is not a field ofDataGridUpdateSnapshot, so no reconcile runs. Enum and set values are a separate metadata round trip by construction, and foreign keys hit the same latch wheneverSchemaForeignKeyStoremisses. Those columns were permanently short on a fresh install with no saved layout at all.What changed
A layout that records no ownership no longer pins an accessory column.
layoutDiscardingUnownedWidthsdrops a saved width only when the layout carries widths but no content widths, and only for a column that actually draws an action button. Up to 0.65 measured a plain column with the same 16pt it uses today, so a plain column keeps the width it was given, and so does any column the user has taken ownership of. Column order and hidden columns are untouched. Nothing is rewritten on disk: the stale numbers are ignored on read and the entry converges to the current shape the next time anything is saved for that table.An automatic column widens when its accessory arrives.
widenAutomaticColumnsre-measures a column whose presentation changed, but only when the user has not sized it, and only when the new width is larger. A width the user is reading never shrinks, and a width the user chose never moves, which is what #2195 was protecting. Sequel Ace does the same thing: it re-autosizes as rows stream in and skips any column with a saved width.An enum or set column reserves its dropdown at the first paint. The result set already reports the column as
ENUMorSET; only the allowed values need the later round trip.DataGridColumnPresentation.resolvenow reads the type, so the width is right from the start and the widen pass above is the fallback rather than the normal path. This is what made a bulk enum prefetch mirroringprovidesBulkForeignKeyFetchunnecessary: the information was already on the wire.An explicit fit reads the page rather than a sample of it.
measureColumnWidthsampled about 30 rows throughstep = max(1, totalRows / 30), so on a 1000-row page it read 31 rows and stepped over the longest value. Size to Fit, Size All Columns to Fit and the divider double-click now scan against a budget on the whole gesture rather than on one column, so fitting a single column covers any page size a user can configure (the setting goes to 100,000 rows) while fitting every column of a wide result stays a bounded amount of formatting on the main thread. The first paint keeps sampling, because it measures every column of the result before the grid can draw a single row.Both measurement entry points now go through one helper each on the coordinator, so they cannot drift apart on the accessory, display format, database type or null string they pass.
resolvedColumnLayoutgained asaved:variant so the reconcile pass reuses the saved layout the ownership check already looked at, rather than resolving it a second time: a layout built from live widths alone has no content widths either, and would otherwise look like the old shape.From the review pass
Five findings, all real, all fixed in the diff:
makeNSViewits legacy width survived the prune andsynchronizeUserSizedColumnsthen recorded it as user-sized, which made the widen pass skip it forever. The other half of Double-clicking the column edge to auto-fit doesn’t take the arrows button into account. #2303.unownedRestoredColumnNamesnow records which restored widths carry no ownership, and the widen pass is allowed through for those; a real user resize clears the name.applyStructuralUpdateconsumed the presentation diff without widening, so metadata that arrived for a background tab was swallowed when that tab was selected. Both paths now go throughapplyAccessoryWidthChanges.handleChevronActionfell off the end of its chain. It now opens the inline editor for that window.CHANGELOG.mdmade the whole### Fixedlist loose Markdown.Screenshots
The before is the reporter's own shot on the issue, and the second one the project owner posted while testing
main, wherefirst_seen_atis 185pt after an explicit fit andlast_seen_atis 168.69pt, exactly 16pt short. I did not capture an after: it needs a second instance of the app driven on this machine, and the owner was using their own copy at the time. The numbers above are measured from the real saved layout rather than from a screenshot, and the arithmetic is asserted in the tests.Verified
verify.sh build: PASSverify.sh test TableViewCoordinatorLayoutTests DataGridColumnWidthOwnershipTests ColumnWidthOptimizationTests FitToContentWidthTests DataGridColumnPoolTests ColumnLayoutStateTests: PASSverify.sh lint: 0 violationsNew and updated tests cover: a legacy layout dropping only its action-column widths while keeping order, hidden columns and any width the user owns; a modern layout left untouched; a late foreign key and a late enum widening an automatic column while leaving a user-sized one bit-identical; a column that loses its accessory keeping its width; an enum and a set column reserving the dropdown before their values arrive; and an explicit fit finding a value the automatic sample steps over.
Three tests changed meaning rather than being adjusted to fit the code. They pinned the automatic half of the no-resize rule from #2195, which is the defect: a column the user never sized carries no intent to protect, so its width is a derived value whose inputs changed. The user-sized half of every one of those tests is unchanged and still passes.
Not in this PR
The width model is
utf16Length * width("M")fromThemeEngine, while the cell draws withCTLineGetTypographicBounds. Measured against the real fonts, CJK under-measures by about 10pt on a short string and a proportional font over-measures by about 85pt on a 20-character string. That is a separate defect of the same class, with a real cost to fix (a CTLine per sampled row per column), and it is reported rather than shipped here.