refactor(datagrid): decouple cell-edit model mutation for one-reload column fill (#1304)

datlechin · datlechin · commit 8b18b9aaac1a · 2026-05-23T15:58:50.000+07:00
diff --git a/TablePro/Views/Results/Extensions/DataGridView+CellCommit.swift b/TablePro/Views/Results/Extensions/DataGridView+CellCommit.swift
@@ -15,17 +15,39 @@ extension TableViewCoordinator {
     }
 
     func commitTypedCellEdit(row: Int, columnIndex: Int, newValue typedNewValue: PluginCellValue) {
-        cellCommitLogger.debug("commitTypedCellEdit(row: \(row, privacy: .public), columnIndex: \(columnIndex, privacy: .public)) isCommitting=\(self.isCommittingCellEdit, privacy: .public) delegate=\(self.delegate == nil ? "nil" : "present", privacy: .public)")
-        guard !isCommittingCellEdit else { return }
         guard let tableView else { return }
+        guard let delta = recordCellEdit(row: row, columnIndex: columnIndex, newValue: typedNewValue) else { return }
+
+        invalidateDisplayCache()
+        visualIndex.updateRow(row, from: changeManager, sortedIDs: sortedIDs)
+
+        guard let tableColumnIndex = DataGridView.tableColumnIndex(
+            for: columnIndex,
+            in: tableView,
+            schema: identitySchema
+        ) else { return }
+        if case .cellChanged = delta {
+            tableRowsController.apply(.cellChanged(row: row, column: tableColumnIndex))
+        } else {
+            tableView.reloadData(
+                forRowIndexes: IndexSet(integer: row),
+                columnIndexes: IndexSet(integer: tableColumnIndex)
+            )
+        }
+    }
+
+    @discardableResult
+    func recordCellEdit(row: Int, columnIndex: Int, newValue typedNewValue: PluginCellValue) -> Delta? {
+        cellCommitLogger.debug("recordCellEdit(row: \(row, privacy: .public), columnIndex: \(columnIndex, privacy: .public)) isCommitting=\(self.isCommittingCellEdit, privacy: .public) delegate=\(self.delegate == nil ? "nil" : "present", privacy: .public)")
+        guard !isCommittingCellEdit else { return nil }
         let tableRows = tableRowsProvider()
-        guard columnIndex >= 0 && columnIndex < tableRows.columns.count else { return }
-        guard let displayRowValues = displayRow(at: row) else { return }
-        guard columnIndex < displayRowValues.values.count else { return }
+        guard columnIndex >= 0 && columnIndex < tableRows.columns.count else { return nil }
+        guard let displayRowValues = displayRow(at: row) else { return nil }
+        guard columnIndex < displayRowValues.values.count else { return nil }
         let oldValue = displayRowValues.values[columnIndex]
         guard oldValue != typedNewValue else {
-            cellCommitLogger.debug("commitTypedCellEdit - value unchanged, guard returned")
-            return
+            cellCommitLogger.debug("recordCellEdit - value unchanged, guard returned")
+            return nil
         }
 
         isCommittingCellEdit = true
@@ -49,23 +71,8 @@ extension TableViewCoordinator {
                 delta = tableRows.edit(row: storageRow, column: columnIndex, value: typedNewValue)
             }
         }
-        cellCommitLogger.debug("commitTypedCellEdit - about to call delegate.dataGridDidEditCell, delegate=\(self.delegate == nil ? "nil" : "present", privacy: .public)")
+        cellCommitLogger.debug("recordCellEdit - about to call delegate.dataGridDidEditCell, delegate=\(self.delegate == nil ? "nil" : "present", privacy: .public)")
         delegate?.dataGridDidEditCell(row: row, column: columnIndex, newValue: typedNewValue.asText)
-        invalidateDisplayCache()
-        visualIndex.updateRow(row, from: changeManager, sortedIDs: sortedIDs)
-
-        guard let tableColumnIndex = DataGridView.tableColumnIndex(
-            for: columnIndex,
-            in: tableView,
-            schema: identitySchema
-        ) else { return }
-        if storageRow != nil, case .cellChanged = delta {
-            tableRowsController.apply(.cellChanged(row: row, column: tableColumnIndex))
-        } else {
-            tableView.reloadData(
-                forRowIndexes: IndexSet(integer: row),
-                columnIndexes: IndexSet(integer: tableColumnIndex)
-            )
-        }
+        return delta
     }
 }
diff --git a/TablePro/Views/Results/Extensions/DataGridView+FillColumn.swift b/TablePro/Views/Results/Extensions/DataGridView+FillColumn.swift
@@ -58,11 +58,15 @@ extension TableViewCoordinator {
         undoManager?.beginUndoGrouping()
         undoManager?.setActionName(String(localized: "Fill Column"))
 
-        for row in targetRows {
-            commitTypedCellEdit(row: row, columnIndex: columnIndex, newValue: value)
+        var didEdit = false
+        for row in targetRows where recordCellEdit(row: row, columnIndex: columnIndex, newValue: value) != nil {
+            didEdit = true
         }
 
         undoManager?.endUndoGrouping()
+
+        guard didEdit else { return }
+        invalidateAllDisplayCaches()
         tableView?.reloadData()
     }
 
diff --git a/TableProTests/Views/Results/Extensions/FillColumnTests.swift b/TableProTests/Views/Results/Extensions/FillColumnTests.swift
@@ -2,10 +2,10 @@
 //  FillColumnTests.swift
 //  TableProTests
 //
-//  Locks the Fill Column decision logic: which loaded rows a fill targets
-//  (editable guard, deleted-row skipping, empty result) and how the dialog
-//  resolves NULL distinctly from an empty string. The apply loop itself
-//  reuses commitTypedCellEdit, which the cell-edit and paste paths exercise.
+//  Locks Fill Column behaviour at two levels: the pure decisions (which loaded
+//  rows a fill targets, how the dialog resolves NULL vs an empty string) and
+//  the effect (applyFillColumn records one staged change per loaded row through
+//  DataChangeManager, skipping deleted rows and read-only result sets).
 //
 
 import Foundation
@@ -14,11 +14,45 @@ import Testing
 
 @testable import TablePro
 
+@MainActor
+private final class NoopColumnLayoutPersister: ColumnLayoutPersisting {
+    func load(for tableName: String, connectionId: UUID) -> ColumnLayoutState? { nil }
+    func save(_ layout: ColumnLayoutState, for tableName: String, connectionId: UUID) {}
+    func clear(for tableName: String, connectionId: UUID) {}
+}
+
 @Suite("Fill Column")
 @MainActor
 struct FillColumnTests {
-    @Test("Fills every loaded row when editable and none are deleted")
-    func fillsAllLoadedRows() {
+    private func makeCoordinator(
+        columns: [String],
+        rowCount: Int,
+        isEditable: Bool = true,
+        manager: DataChangeManager
+    ) -> TableViewCoordinator {
+        let coordinator = TableViewCoordinator(
+            changeManager: AnyChangeManager(manager),
+            isEditable: isEditable,
+            selectedRowIndices: .constant([]),
+            delegate: nil,
+            layoutPersister: NoopColumnLayoutPersister()
+        )
+        let columnTypes: [ColumnType] = Array(repeating: .text(rawType: nil), count: columns.count)
+        let rows = (0..<rowCount).map { i in (0..<columns.count).map { c in "r\(i)c\(c)" } }
+        let tableRows = TableRows.from(
+            queryRows: rows.map { row in row.map { PluginCellValue.text($0) } },
+            columns: columns,
+            columnTypes: columnTypes
+        )
+        coordinator.tableRowsProvider = { tableRows }
+        coordinator.updateCache()
+        return coordinator
+    }
+
+    // MARK: - Target selection
+
+    @Test("Targets every loaded row when editable and none are deleted")
+    func targetsAllLoadedRows() {
         let rows = TableViewCoordinator.fillTargetRows(
             rowCount: 5,
             isEditable: true,
@@ -27,8 +61,8 @@ struct FillColumnTests {
         #expect(rows == [0, 1, 2, 3, 4])
     }
 
-    @Test("Skips rows marked for deletion")
-    func skipsDeletedRows() {
+    @Test("Excludes rows marked for deletion")
+    func excludesDeletedRows() {
         let rows = TableViewCoordinator.fillTargetRows(
             rowCount: 5,
             isEditable: true,
@@ -57,6 +91,8 @@ struct FillColumnTests {
         #expect(rows.isEmpty)
     }
 
+    // MARK: - Value resolution
+
     @Test("Resolves NULL distinctly from an empty string")
     func resolvesNullDistinctFromEmpty() {
         #expect(TableViewCoordinator.fillColumnValue(text: "ignored", setNull: true) == .null)
@@ -69,4 +105,63 @@ struct FillColumnTests {
         #expect(TableViewCoordinator.fillImpactDescription(rowCount: 1).contains("1 loaded row"))
         #expect(TableViewCoordinator.fillImpactDescription(rowCount: 42).contains("42 loaded rows"))
     }
+
+    // MARK: - Apply (staged effect)
+
+    @Test("Records one staged change per loaded row, leaving other columns untouched")
+    func appliesToEveryLoadedRow() {
+        let manager = DataChangeManager()
+        let coordinator = makeCoordinator(columns: ["a", "b"], rowCount: 4, manager: manager)
+
+        coordinator.applyFillColumn(columnIndex: 0, value: .text("X"))
+
+        for row in 0..<4 {
+            #expect(manager.pending.isCellModified(rowIndex: row, columnIndex: 0))
+        }
+        #expect(manager.pending.isCellModified(rowIndex: 0, columnIndex: 1) == false)
+    }
+
+    @Test("Does not touch rows marked for deletion")
+    func skipsDeletedRowsOnApply() {
+        let manager = DataChangeManager()
+        let coordinator = makeCoordinator(columns: ["a"], rowCount: 4, manager: manager)
+        manager.recordRowDeletion(rowIndex: 2, originalRow: [.text("r2c0")])
+
+        coordinator.applyFillColumn(columnIndex: 0, value: .text("X"))
+
+        #expect(manager.pending.isCellModified(rowIndex: 0, columnIndex: 0))
+        #expect(manager.pending.isCellModified(rowIndex: 2, columnIndex: 0) == false)
+    }
+
+    @Test("Records nothing on a read-only result set")
+    func recordsNothingWhenReadOnly() {
+        let manager = DataChangeManager()
+        let coordinator = makeCoordinator(columns: ["a"], rowCount: 4, isEditable: false, manager: manager)
+
+        coordinator.applyFillColumn(columnIndex: 0, value: .text("X"))
+
+        #expect(manager.hasChanges == false)
+    }
+
+    @Test("Writes NULL as a null change, not an empty string")
+    func appliesNull() {
+        let manager = DataChangeManager()
+        let coordinator = makeCoordinator(columns: ["a"], rowCount: 2, manager: manager)
+
+        coordinator.applyFillColumn(columnIndex: 0, value: .null)
+
+        let change = manager.pending.change(forRow: 0, type: .update)
+        #expect(change?.cellChanges.first?.newValue == .null)
+    }
+
+    @Test("Skips rows whose value already equals the fill value")
+    func skipsRowsAlreadyEqual() {
+        let manager = DataChangeManager()
+        let coordinator = makeCoordinator(columns: ["a"], rowCount: 2, manager: manager)
+
+        coordinator.applyFillColumn(columnIndex: 0, value: .text("r0c0"))
+
+        #expect(manager.pending.isCellModified(rowIndex: 0, columnIndex: 0) == false)
+        #expect(manager.pending.isCellModified(rowIndex: 1, columnIndex: 0))
+    }
 }
