fix(datagrid): make the grid, the date picker and the write-back agree on a value - #2471
Merged
Merged
Conversation
|
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.
Found while investigating #2454. Unlike the Snowflake work that issue produced, these are app-side and affect every driver, so this is based on
mainrather than stacked.Three defects with one shape: the grid, the cell editor and the write-back each decided for themselves what a temporal value meant, and they disagreed.
A time you typed was thrown away
The picker chooses its fields from the column type, but
DateEditingService.string(from:like:)chose what to write from the shape of the stored text:A SQLite column declared
DATETIMEholding2024-03-15showed hour, minute and second steppers, becauseDATETIMEmaps to.dateAndTime. Setting 14:30:00 and pressing OK wrote2024-03-15, identical to the stored value, so the commit was discarded as a no-op. No error, no dirty marker, no redraw. The time simply vanished.The write-back now takes the fields the editor actually offered as well as the shape the value arrived in, so a field the user was allowed to fill is a field that gets written.
The grid and the picker showed different days
DateFormattingServiceset its formatter's zone once, at creation:while the picker opened the value in the zone the value itself carried. A PostgreSQL
timestamptzrendered by the server as2024-03-15 01:00:00+05:30, viewed on a Mac in Europe/London, read2024-03-14 19:30:00in the cell and opened on 15 March in the calendar. The same row, two days, and no way to tell which one the database held.The value is now formatted in the zone it was written in. That is what
DatabaseDateParseralready documents as the intent: the zone travels with the value so the grid, the chart's axis and the picker all show it as written. A value with no offset is naive and still resolves in the reader's own zone, so nothing changes forDATETIMEand friends.The cache key needs no zone, because the zone is read off the value and the string therefore determines it.
Opening a picker and pressing OK rewrote the cell
onCommitalways synthesized a string and committed it, so merely opening the editor and confirming could replace the stored value. The clearest case is a wall clock inside the reader's spring-forward gap:2024-03-10 02:30:00in a MySQLDATETIMEis legal and common when the data was written elsewhere, butCalendarrolls it to 03:30, and confirming unchanged committed2024-03-10 03:30:00, moving the row an hour.Confirming without changing the value now writes nothing, which is the rule #2454 asked for.
Tests
TemporalEditingConsistencyTests, 7 cases: an entered time survives on a date-only value, the arrival shape still drives the default when no field set is offered, a date-only editor does not truncate a value carrying time, an offset suffix survives the round trip, an offset-bearing value formats on its own day, a naive value still reads in the reader's zone, and formatting one value does not change how the next reads (the shared formatter's zone does not leak).Verified: build PASS, 108/108 across every suite that touches
DateFormattingService,CellDisplayFormatter,ResultChartProjector,DateEditingServiceandDatabaseDateParser, SwiftLint 0 violations.No UI automation. The flows need a cell whose stored text disagrees with its column type and a Mac in a specific time zone, neither of which the UI test harness can set deterministically. The behaviour is pinned at the three services instead.
Two related things deliberately left alone
The DST-gap display.
DatabaseDateParser.keepsItsDaycompares only year, month and day, so a wall clock rolled forward by an hour still passes and the grid shows 03:30 for a stored 02:30. The comment above it defends that on purpose: such a value is still the day it says it is. Rejecting it would make those cells fall back to raw text. The damaging half, the silent rewrite, is fixed above; changing what the grid displays is a product decision.The 1582 calendar.
DatabaseDateParserbuilds a Foundation.gregoriancalendar, which applies the Julian cutover, so a date from1582-10-05to1582-10-14does not parse and an older one resolves to a different absolute day. Snowflake, and ISO 8601, use the proleptic calendar. Making the shared parser proleptic changes behaviour for every driver at once and deserves its own change.