Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ricardobossan
Copy link
Member

@ricardobossan ricardobossan commented Jan 22, 2025

Fixes #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.

Customer Impact

  • Prevents runtime crashes when the DataGridView is null.

Regression?

  • No

Risk

  • Minimal

Screenshots

Before

before

After

after

Test methodology

  • Manual testing

Test environment(s)

  • 10.0.100-alpha.1.25064.3

@ricardobossan ricardobossan self-assigned this Jan 22, 2025
@ricardobossan ricardobossan force-pushed the Issue_12752_Exception_Space_Key_On_CheckBoxCell branch from fc7d058 to 95f04dc Compare January 22, 2025 02:34
@ricardobossan ricardobossan marked this pull request as ready for review January 22, 2025 02:34
@ricardobossan ricardobossan requested a review from a team as a code owner January 22, 2025 02:34
@ricardobossan ricardobossan added the waiting-review This item is waiting on review by one or more members of team label Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.21594%. Comparing base (77ec5c9) to head (95f04dc).
Report is 3 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #12821         +/-   ##
===================================================
+ Coverage   76.21470%   76.21594%   +0.00124%     
===================================================
  Files           3199        3199                 
  Lines         640488      640492          +4     
  Branches       47227       47228          +1     
===================================================
+ Hits          488146      488157         +11     
+ Misses        148812      148806          -6     
+ Partials        3530        3529          -1     
Flag Coverage Δ
Debug 76.21594% <100.00000%> (+0.00124%) ⬆️
integration 18.16458% <0.00000%> (-0.00202%) ⬇️
production 50.15642% <100.00000%> (+0.00245%) ⬆️
test 97.04180% <ø> (+0.00055%) ⬆️
unit 47.58381% <100.00000%> (+0.00670%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@LeafShi1 LeafShi1 left a 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

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;
}

@Tanya-Solyanik Tanya-Solyanik added waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Jan 22, 2025

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.

@ricardobossan ricardobossan force-pushed the Issue_12752_Exception_Space_Key_On_CheckBoxCell branch from 95f04dc to 826374d Compare January 22, 2025 23:37
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 22, 2025
… 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.

## 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`
@ricardobossan ricardobossan force-pushed the Issue_12752_Exception_Space_Key_On_CheckBoxCell branch from 826374d to 3d18116 Compare January 22, 2025 23:43
@ricardobossan ricardobossan marked this pull request as draft January 23, 2025 18:58
@ricardobossan
Copy link
Member Author

Drafted to investigate failing checks.

@dotnet-policy-service dotnet-policy-service bot added the draft draft PR label Jan 23, 2025
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 ||
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik Jan 23, 2025

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)
{
Copy link
Member

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

, so the judgment should be added before Paint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draft draft PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An exception pops up when pressing the space key on the checkbox cell
3 participants