Port DataGridView control from WinForms to Modern.Forms#121
Conversation
…trolGallery demo panel Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/a8e2d3dd-1cb3-4e19-bb8a-bd53fbff1d30 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
…, and cell editing - Fix visual artifacts at fractional DPI by using Math.Ceiling in GetContentArea - Fix cursor not updating during mouse move by calling FindForm()?.SetCursor() - Implement actual data sorting when column headers are clicked (numeric + string) - Add DataSource property for data binding (auto-generates columns from properties) - Add cell editing via double-click/F2 with embedded TextBox - Add LostFocus event to Control, SelectAll() to TextBox - Add CellBeginEdit, CellEndEdit, CellValueChanged events - Add ReadOnly property, BeginEdit/EndEdit/CancelEdit methods - Update ControlGallery to demonstrate data binding and editing Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/d15aa0dc-cb46-4825-be39-974d7172db65 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
…r comments Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/d15aa0dc-cb46-4825-be39-974d7172db65 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/8e1380c5-1ba4-466b-9482-4ea3f94cb36c Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/0eba3168-7e8a-43c2-8216-bbe7a1f8e2c7 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
…dd cell styles and header cells Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/c3e1e882-d981-4ede-b971-1c1e82a6737c Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/c3e1e882-d981-4ede-b971-1c1e82a6737c Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/c3e1e882-d981-4ede-b971-1c1e82a6737c Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
… and cell styles Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/c3e1e882-d981-4ede-b971-1c1e82a6737c Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
…lumns/Rows, AlternatingRowsDefaultCellStyle, CurrentCell/CurrentRow, rename to DisplayedRowCount/FirstDisplayedScrollingRowIndex, update ControlGallery Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/8838bcbb-e2b2-44d9-8e46-ce37cc4d10b3 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/8838bcbb-e2b2-44d9-8e46-ce37cc4d10b3 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
…tRowAtLocation Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/0bddd37e-b1b1-433f-8dd3-a7939ac11ed2 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new DataGridView control implementation to Modern.Forms (ported/approximated from WinForms-style behavior), including rendering, selection/editing, and a ControlGallery sample to showcase usage.
Changes:
- Introduces a new
DataGridViewcontrol + supporting model types (rows/columns/cells, selection mode, edit events, simple data binding + sorting). - Adds a
DataGridViewRendererand registers it inRenderManager. - Adds Control infrastructure needed by the grid (Control
LostFocusevent, TextBoxSelectAll) and a new ControlGallery panel/sample entry.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Modern.Forms/TextBox.cs | Adds SelectAll() API used by DataGridView editing. |
| src/Modern.Forms/Renderers/RenderManager.cs | Registers DataGridViewRenderer for DataGridView. |
| src/Modern.Forms/Renderers/DataGridViewRenderer.cs | Implements DataGridView drawing (headers, rows, selection, glyphs). |
| src/Modern.Forms/DataGridViewSelectionMode.cs | Adds selection mode enum for grid interaction behavior. |
| src/Modern.Forms/DataGridViewRowCollection.cs | Adds row collection with owner wiring + bulk replace for sorting. |
| src/Modern.Forms/DataGridViewRow.cs | Adds row model (height, cells, selection). |
| src/Modern.Forms/DataGridViewHeaderCell.cs | Adds header cell types for rows/columns. |
| src/Modern.Forms/DataGridViewColumnCollection.cs | Adds column collection with owner wiring. |
| src/Modern.Forms/DataGridViewColumn.cs | Adds column model (width, header, sorting state). |
| src/Modern.Forms/DataGridViewCellCollection.cs | Adds cell collection with owner wiring. |
| src/Modern.Forms/DataGridViewCell.cs | Adds cell model (value, style, selection). |
| src/Modern.Forms/DataGridView.cs | Adds the DataGridView control (layout, scrolling, sorting, editing, input). |
| src/Modern.Forms/Control.Events.cs | Adds LostFocus event plumbing to Control event-list store. |
| src/Modern.Forms/Control.cs | Raises LostFocus when a control is deselected. |
| samples/ControlGallery/Panels/DataGridViewPanel.cs | Adds sample panel demonstrating grid features (editing, binding, styling). |
| samples/ControlGallery/MainForm.cs | Adds ControlGallery navigation entry to open the new DataGridView panel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // If editing, end edit when clicking outside the editor | ||
| if (edit_textbox is not null) { | ||
| var edit_bounds = new Rectangle (edit_textbox.Left, edit_textbox.Top, edit_textbox.Width, edit_textbox.Height); |
There was a problem hiding this comment.
MouseEventArgs.Location is in device pixels (see Control.RaiseMouseDown hit-testing against ScaledBounds). Here edit_bounds is built from edit_textbox.Left/Top/Width/Height (logical units), so the Contains check is wrong on non-100% scaling and can prematurely EndEdit or fail to end it. Use edit_textbox.ScaledBounds for the hit-test, or convert e.Location to logical units before comparing.
| var edit_bounds = new Rectangle (edit_textbox.Left, edit_textbox.Top, edit_textbox.Width, edit_textbox.Height); | |
| var edit_bounds = edit_textbox.ScaledBounds; |
| var row_header_rect = new Rectangle (client.Left, client.Top + (ColumnHeadersVisible ? ScaledHeaderHeight : 0), ScaledRowHeadersWidth, client.Height); | ||
|
|
||
| if (!row_header_rect.Contains (location)) | ||
| return -1; | ||
|
|
||
| var row_top = client.Top + (ColumnHeadersVisible ? ScaledHeaderHeight : 0); |
There was a problem hiding this comment.
row_header_rect uses height = client.Height even though its Y is offset by the header height, which makes the rectangle extend past the content area (and can overlap the horizontal scrollbar area). Compute the height as client.Height - headerOffset (or use client.Bottom - row_top) so row-resize hit testing is limited to the actual row header region.
| var row_header_rect = new Rectangle (client.Left, client.Top + (ColumnHeadersVisible ? ScaledHeaderHeight : 0), ScaledRowHeadersWidth, client.Height); | |
| if (!row_header_rect.Contains (location)) | |
| return -1; | |
| var row_top = client.Top + (ColumnHeadersVisible ? ScaledHeaderHeight : 0); | |
| var header_offset = ColumnHeadersVisible ? ScaledHeaderHeight : 0; | |
| var row_header_rect = new Rectangle (client.Left, client.Top + header_offset, ScaledRowHeadersWidth, Math.Max (0, client.Height - header_offset)); | |
| if (!row_header_rect.Contains (location)) | |
| return -1; | |
| var row_top = client.Top + header_offset; |
| var client = ClientRectangle; | ||
| var content_height = client.Height - (ColumnHeadersVisible ? ScaledHeaderHeight : 0); | ||
| var visible_rows = content_height > 0 && ScaledRowHeight > 0 ? content_height / ScaledRowHeight : 0; | ||
|
|
There was a problem hiding this comment.
UpdateScrollBars() calculates visible_rows from ClientRectangle and ScaledRowHeight, which ignores the horizontal scrollbar height and any per-row Height changes (row resizing uses Rows[i].Height). This can produce incorrect scrollbar visibility/ranges after showing the horizontal scrollbar or resizing rows. Consider basing calculations on GetContentArea() and either (a) enforce uniform row heights by applying RowHeight to rows, or (b) compute visible content height and scroll range from the actual row heights.
| if (old_value != new_value) { | ||
| row.Cells[editing_column_index].Value = new_value; | ||
|
|
||
| // Update the data source if bound | ||
| if (data_source is not null && editing_row_index < data_source.Count) { | ||
| var item = data_source[editing_row_index]; | ||
|
|
||
| if (item is not null && editing_column_index < Columns.Count) { | ||
| var prop = item.GetType ().GetProperty (Columns[editing_column_index].HeaderText, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance); | ||
| if (prop?.CanWrite == true) { | ||
| try { | ||
| var converted = Convert.ChangeType (new_value, prop.PropertyType); | ||
| prop.SetValue (item, converted); | ||
| } catch { | ||
| // Conversion failed - revert cell value | ||
| row.Cells[editing_column_index].Value = old_value; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var changed_args = new DataGridViewCellEditEventArgs (editing_row_index, editing_column_index); | ||
| OnCellValueChanged (changed_args); | ||
| } |
There was a problem hiding this comment.
If data binding conversion fails, the cell value is reverted to old_value but CellValueChanged is still raised. This means listeners can observe a "value changed" event even though the value ultimately did not change. Track whether the commit actually succeeded (including successful conversion) before raising CellValueChanged, or perform conversion first and only update the cell when it succeeds.
| // Sort the rows in-place using a stable sort | ||
| var sorted = Rows.ToList (); | ||
|
|
||
| sorted.Sort ((a, b) => { |
There was a problem hiding this comment.
Comment says "stable sort", but List.Sort is not stable. Either implement a stable sort (e.g., decorate with original index as a tiebreaker) or update the comment so it doesn't promise stability.
| /// <inheritdoc/> | ||
| protected override void ClearItems () | ||
| { | ||
| foreach (var column in this) | ||
| column.SetOwner (null); | ||
|
|
||
| base.ClearItems (); | ||
| owner.Invalidate (); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override void InsertItem (int index, DataGridViewColumn item) | ||
| { | ||
| item.SetOwner (owner); | ||
| base.InsertItem (index, item); | ||
| owner.Invalidate (); | ||
| } |
There was a problem hiding this comment.
Column collection mutations only call owner.Invalidate(), but column changes affect horizontal scrollbar range (TotalColumnsWidth). As a result, adding/removing columns at runtime may not update scrollbar visibility/ranges until some other trigger calls UpdateScrollBars(). Consider adding an owner.OnColumnsChanged() (similar to OnRowsChanged) that updates scrollbars and invalidates, and call it from here.
| owner?.Invalidate (); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Changing Width only invalidates the DataGridView. Since column widths affect TotalColumnsWidth and horizontal scrollbar ranges, programmatic width changes won't update scrollbars unless the caller manually forces it. Consider notifying the owner to recompute scrollbars (e.g., owner.OnColumnsChanged()/owner.UpdateScrollBars()) in addition to invalidation.
| owner?.Invalidate (); | |
| } | |
| } | |
| } | |
| NotifyOwnerColumnLayoutChanged (); | |
| } | |
| } | |
| } | |
| private void NotifyOwnerColumnLayoutChanged () | |
| { | |
| if (owner is null) | |
| return; | |
| var ownerType = owner.GetType (); | |
| var bindingFlags = System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.NonPublic; | |
| ownerType.GetMethod ("OnColumnsChanged", bindingFlags, null, System.Type.EmptyTypes, null)?.Invoke (owner, null); | |
| ownerType.GetMethod ("UpdateScrollBars", bindingFlags, null, System.Type.EmptyTypes, null)?.Invoke (owner, null); | |
| owner.Invalidate (); | |
| } |
| get => height; | ||
| set { | ||
| if (height != value) { | ||
| height = Math.Max (value, 10); |
There was a problem hiding this comment.
Changing row Height only invalidates the DataGridView. Row heights affect vertical scrolling and hit-testing (GetRowAtLocation sums Rows[i].Height), so programmatic Height changes should also trigger scrollbar recomputation (similar to DataGridView.OnRowsChanged()). Consider calling owner?.OnRowsChanged() (or an internal method that updates scrollbars) instead of only Invalidate().
| height = Math.Max (value, 10); | |
| height = Math.Max (value, 10); | |
| owner?.OnRowsChanged (); |
| // Draw text using cell style or default cell style | ||
| var text_bounds = bounds; | ||
| text_bounds.Inflate (-4, 0); | ||
| text_bounds.Height = control.ScaledRowHeight; // Ensure consistent text positioning | ||
|
|
||
| var fg = cellStyle?.ForegroundColor ?? control.DefaultCellStyle.ForegroundColor ?? Theme.ForegroundColor; | ||
| var font = cellStyle?.Font ?? control.DefaultCellStyle.Font ?? Theme.UIFont; | ||
| var font_size = cellStyle?.FontSize ?? control.DefaultCellStyle.FontSize ?? Theme.ItemFontSize; | ||
|
|
||
| e.Canvas.DrawText (value, font, control.LogicalToDeviceUnits (font_size), text_bounds, fg, ContentAlignment.MiddleLeft, maxLines: 1); |
There was a problem hiding this comment.
RenderCell forces text_bounds.Height = control.ScaledRowHeight, but rows can have per-row Height (RenderRows uses row.Height and row resizing changes Rows[i].Height). This will misalign text vertically for resized rows and can also mismatch when RowHeight differs from row.Height. Prefer using the row's actual (unclipped) height for text alignment (e.g., pass the scaled row height into RenderCell) rather than the DataGridView-level ScaledRowHeight.
| protected virtual void RenderSortGlyph (PaintEventArgs e, Rectangle bounds, SortOrder sortOrder) | ||
| { | ||
| var glyph_size = 6; | ||
| var glyph_x = bounds.Right - glyph_size - 8; | ||
| var glyph_y = bounds.Top + (bounds.Height - glyph_size) / 2; | ||
|
|
||
| using var path = new SKPath (); | ||
|
|
||
| if (sortOrder == SortOrder.Ascending) { | ||
| path.MoveTo (glyph_x, glyph_y + glyph_size); | ||
| path.LineTo (glyph_x + glyph_size / 2, glyph_y); | ||
| path.LineTo (glyph_x + glyph_size, glyph_y + glyph_size); | ||
| path.Close (); | ||
| } else { | ||
| path.MoveTo (glyph_x, glyph_y); | ||
| path.LineTo (glyph_x + glyph_size / 2, glyph_y + glyph_size); | ||
| path.LineTo (glyph_x + glyph_size, glyph_y); | ||
| path.Close (); | ||
| } | ||
|
|
||
| using var paint = new SKPaint { Color = Theme.ForegroundColor, IsAntialias = true }; | ||
| e.Canvas.DrawPath (path, paint); | ||
| } |
There was a problem hiding this comment.
Sort glyph is always painted with Theme.ForegroundColor, ignoring ColumnHeadersDefaultCellStyle.ForegroundColor used for the header text. This breaks header style customization (e.g., white header text on an accent background will still draw a dark glyph). Pass the resolved header foreground color into RenderSortGlyph (or resolve it inside RenderSortGlyph) and use that for the paint color.
|
@copilot apply changes based on the comments in this thread |
…ollBars, CellValueChanged, sort comment, OnColumnsChanged, row/col height notifiers, RenderCell text height, sort glyph color Agent-Logs-Url: https://github.com/modern-forms/Modern.Forms/sessions/f6adaf5c-c37f-450d-af6e-903ab0e734b4 Co-authored-by: jpobst <179295+jpobst@users.noreply.github.com>
All review comments addressed in commit 11f07f4:
|
Adds a new
DataGridViewcontrol implementation to Modern.Forms (ported/approximated from WinForms-style behavior), including rendering, selection/editing, and a ControlGallery sample to showcase usage.Changes:
DataGridViewcontrol + supporting model types (rows/columns/cells, selection mode, edit events, simple data binding + sorting).DataGridViewRendererand registers it inRenderManager.LostFocusevent, TextBoxSelectAll) and a new ControlGallery panel/sample entry.