-
Notifications
You must be signed in to change notification settings - Fork 999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Exception being thrown when the space key is pressed on a CheckBoxCell #12821
base: main
Are you sure you want to change the base?
Fixes Exception being thrown when the space key is pressed on a CheckBoxCell #12821
Conversation
fc7d058
to
95f04dc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12821 +/- ##
===================================================
- Coverage 76.21470% 76.13201% -0.08269%
===================================================
Files 3199 3242 +43
Lines 640488 642375 +1887
Branches 47227 47275 +48
===================================================
+ Hits 488146 489053 +907
- Misses 148812 149773 +961
- Partials 3530 3549 +19
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! For the method NotifyMSAAClient
, can we consider remove the Debug.Assert
Lines 970 to 972 in 48c153d
Debug.Assert(DataGridView is not null); | |
Debug.Assert((columnIndex >= 0) && (columnIndex < DataGridView.Columns.Count)); | |
Debug.Assert((rowIndex >= 0) && (rowIndex < DataGridView.Rows.Count)); |
and replcase it with
if (DataGridView is null ||
columnIndex < 0 || columnIndex >= DataGridView.Columns.Count ||
rowIndex < 0 || rowIndex >= DataGridView.Rows.Count)
{
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
95f04dc
to
826374d
Compare
826374d
to
3d18116
Compare
Drafted to investigate failing checks. |
Debug.Assert(DataGridView is not null); | ||
Debug.Assert((columnIndex >= 0) && (columnIndex < DataGridView.Columns.Count)); | ||
Debug.Assert((rowIndex >= 0) && (rowIndex < DataGridView.Rows.Count)); | ||
if (DataGridView is null || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change you don't need null-check on line 842
PaintGrid(g, gridRect, clipRect, SingleVerticalBorderAdded, SingleHorizontalBorderAdded); | ||
|
||
if (CurrentCell is not null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify event dataGridView1_CellPainting
here to see if NullReferenceException
will occur. It seems that the judgment of CurrentCell is not null
here is incorrect.
The event was invoked here
winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewCell.cs
Line 3674 in e3e5ddc
dataGridView.OnCellPainting(dgvcpe); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the CellPainting
event, but it threw a NullReferenceException
when invoked.
To address this, I first added a check before the call to dataGridView.OnCellPainting(dgvcpe);
in the DataGridViewCell
class (line 3674). However, this change alone wasn’t sufficient, as the exception persisted:
After further investigation, I implemented additional checks to ensure the event could be triggered without causing the error:
Below is an example of how I’m using the CellPainting event in the test environment:
using System.ComponentModel;
using System.Diagnostics;
namespace ScratchProject;
// As we can't currently design in VS in the runtime solution, mark as "Default" so this opens in code view by default.
[DesignerCategory("Default")]
public partial class Form1 : Form
{
public Form1()
{
// This call is required by the designer.
InitializeComponent();
// Add any initialization after the InitializeComponent() call.
LoadData();
DataGridView1.CellPainting += DataGridView1_CellPainting;
}
private void DataGridView1_CellContentClick(object sender, DataGridViewCellEventArgs e)
{
LoadData();
}
private void DataGridView1_CellPainting(object? sender, DataGridViewCellPaintingEventArgs e)
{
CoolDown(5000);
}
private readonly Stopwatch _watch = new();
private void CoolDown(int milisseconds)
{
if (!_watch.IsRunning)
_watch.Start();
if (_watch.Elapsed > TimeSpan.FromMilliseconds(milisseconds))
{
LoadData();
_watch.Stop();
_watch.Reset();
}
}
private void LoadData()
{
DataGridView1.Rows.Clear();
DataGridView1.Rows.Add(1, 0, "ABC");
DataGridView1.Rows.Add(2, 0, "DEF");
}
}
3d18116
to
ebdbaf1
Compare
… null. Fixes dotnet#12752 ## Root Cause - The issue occurs because the `NotifyMSAAClient` method is called without checking if the `DataGridView` instance is `null`. This leads to a potential `NullReferenceException`. ## Proposed changes - Add a `null` check for the `DataGridView` instance before calling the `NotifyMSAAClient` method. - Adds another `null` check for the `CurrentCell` property before calling the `PaintGrid` method. - Add further checks for DataGridView in paint events ## Customer Impact - Prevents runtime crashes when the `DataGridView` or `CurrentCell` are null. ## Regression? - No ## Risk - Minimal ## Screenshots ### Before ### After ## Test methodology - Manual testing ## Test environment(s) - `10.0.100-alpha.1.25064.3`
ebdbaf1
to
7e74b89
Compare
Fixes #12752
Root Cause
NotifyMSAAClient
method is called without checking if theDataGridView
instance isnull
. This leads to a potentialNullReferenceException
.Proposed changes
null
check for theDataGridView
instance before calling theNotifyMSAAClient
method.Customer Impact
DataGridView
isnull
.Regression?
Risk
Screenshots
Before
After
Test methodology
Test environment(s)
10.0.100-alpha.1.25064.3