Skip to content

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Sep 10, 2020

Fixes #3033

Proposed changes

  • Do not override user-configured font styles in the DataGridView whenever ambient (e.g. form's) font changes.
    This scenario is likely to occur when users migrate their applications from .NET Framework to .NET Core/.NET and wish to retain the original default font.

Whilst the behaviour goes all the way back to .NET Framework, it becomes a significant migration issue for projects that want to keep the original default font (that was changed in #656).

Customer Impact

  • User will be able to migrate existing applications while retaining original default font (for .NET Framework).

Regression?

Risk

  • Small

Screenshots

Before

image

After

image

Test methodology

  • manual (via WinformsControlsTest project)
  • unit tests

dgv-cellstyles

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned RussKie Sep 10, 2020
@RussKie RussKie added this to the 5.0 RC2 milestone Sep 10, 2020
@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Sep 10, 2020
@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #3875 into release/5.0-rc2 will decrease coverage by 31.02774%.
The diff coverage is 0.00000%.

@@                    Coverage Diff                     @@
##           release/5.0-rc2       #3875          +/-   ##
==========================================================
- Coverage         67.60904%   36.58130%   -31.02774%     
==========================================================
  Files                 1408         921         -487     
  Lines               507123      250967      -256156     
  Branches             41178       36862        -4316     
==========================================================
- Hits                342861       91807      -251054     
+ Misses              158290      153877        -4413     
+ Partials              5972        5283         -689     
Flag Coverage Δ
#Debug 36.58130% <0.00000%> (-31.02774%) ⬇️
#production 36.58130% <0.00000%> (+0.10318%) ⬆️
#test ?

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

@merriemcgaw
Copy link
Member

@dreddy-work @Tanya-Solyanik @KlausLoeffelmann - do you all like this fix? Seems like a reasonable approach, but another set of eyes on it would be great. I know @RussKie has a bit more to do to polish things up, but if we can get this in for RC2 that would rock.

@dreddy-work
Copy link
Member

@RussKie , did you investigate other controls that has text input? Are we confirming that this is happening to only DataGridView* controls?

@RussKie

This comment has been minimized.

@RussKie RussKie added the priority-1 Work that is critical for the release, but we could probably ship without label Sep 11, 2020
@RussKie RussKie force-pushed the fix_3033_preserve_user_font_styles branch 3 times, most recently from 9cb0b8f to f90031b Compare September 16, 2020 03:44
@RussKie RussKie marked this pull request as ready for review September 16, 2020 03:44
@RussKie RussKie requested a review from a team as a code owner September 16, 2020 03:44
@RussKie RussKie force-pushed the fix_3033_preserve_user_font_styles branch from f90031b to 463dc93 Compare September 16, 2020 03:45
@RussKie
Copy link
Contributor Author

RussKie commented Sep 16, 2020

@JeremyKuhne I didn't include EMF-based tests as I ran into few issues with those (e.g. it wouldn't size columns, and we don't appear to have Font validators available).
I will add those to the master branch.

@RussKie RussKie added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 16, 2020
@RussKie
Copy link
Contributor Author

RussKie commented Sep 16, 2020

did you investigate other controls that has text input? Are we confirming that this is happening to only DataGridView* controls?

Yes, appears to be only DGV cellstyles-related issue.

@JeremyKuhne
Copy link
Member

I didn't include EMF-based tests as I ran into few issues with those (e.g. it wouldn't size columns, and we don't appear to have Font validators available).

We do have a text out validator- it is how we validate the button text. What records were you getting that you couldn't validate?

@RussKie RussKie removed priority-1 Work that is critical for the release, but we could probably ship without waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Sep 16, 2020
@RussKie
Copy link
Contributor Author

RussKie commented Sep 16, 2020

Tested by CTI.

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Sep 16, 2020

@merriemcgaw, @RussKie , @OliaG - is there a place to document the scenario that this change is breaking? Perhaps in the application migration guide?

@merriemcgaw
Copy link
Member

@merriemcgaw Merrie McGaw FTE, @ru, @OliaG Olia Gavrysh FTE - is there a place to document the scenario that this change is breaking? Perhaps in the application migration guide?

@RussKie , do you have all the info you need to doc the breaking change?

@RussKie
Copy link
Contributor Author

RussKie commented Sep 16, 2020

This will need to be doc'ed in https://docs.microsoft.com/en-us/dotnet/core/compatibility/winforms once we take it.

@RussKie RussKie added the 📖 documentation: breaking please open a breaking change issue https://github.com/dotnet/docs/issues/new?assignees=gewarren label Sep 16, 2020
@RussKie
Copy link
Contributor Author

RussKie commented Sep 17, 2020

I've revisited the fix, and we can do away with the new property. All the existing and interactive test results remained positive:
dgv-font

However I found another related bug or ("feature" if you like) that makes it very awkward user experience. If one of DefaultCellStyle properties is reset (e.g. dataGridView1.DefaultCellStyle.Font = null;) it becomes no longer possible to update it, and the whole style object must be re-assigned.
This is due to the logic here:

else if (_defaultCellStyle.BackColor == Color.Empty ||
_defaultCellStyle.ForeColor == Color.Empty ||
_defaultCellStyle.SelectionBackColor == Color.Empty ||
_defaultCellStyle.SelectionForeColor == Color.Empty ||
_defaultCellStyle.Font is null ||
_defaultCellStyle.Alignment == DataGridViewContentAlignment.NotSet ||
_defaultCellStyle.WrapMode == DataGridViewTriState.NotSet)
{
DataGridViewCellStyle defaultCellStyleTmp = new DataGridViewCellStyle(_defaultCellStyle)
{
Scope = DataGridViewCellStyleScopes.None
};
if (_defaultCellStyle.BackColor == Color.Empty)
{
defaultCellStyleTmp.BackColor = s_defaultBackColor;
}
if (_defaultCellStyle.ForeColor == Color.Empty)
{
defaultCellStyleTmp.ForeColor = base.ForeColor;
_dataGridViewState1[State1_AmbientForeColor] = true;
}
if (_defaultCellStyle.SelectionBackColor == Color.Empty)
{
defaultCellStyleTmp.SelectionBackColor = DefaultSelectionBackBrush.Color;
}
if (_defaultCellStyle.SelectionForeColor == Color.Empty)
{
defaultCellStyleTmp.SelectionForeColor = DefaultSelectionForeBrush.Color;
}
if (_defaultCellStyle.Font is null)
{
defaultCellStyleTmp.Font = base.Font;
_dataGridViewState1[State1_AmbientFont] = true;
}
if (_defaultCellStyle.Alignment == DataGridViewContentAlignment.NotSet)
{
defaultCellStyleTmp.AlignmentInternal = DataGridViewContentAlignment.MiddleLeft;
}
if (_defaultCellStyle.WrapMode == DataGridViewTriState.NotSet)
{
defaultCellStyleTmp.WrapModeInternal = DataGridViewTriState.False;
}
defaultCellStyleTmp.AddScope(this, DataGridViewCellStyleScopes.DataGridView);
return defaultCellStyleTmp;

This isn't something we're fixing in the context of this PR, or in .NET 5.0 timeframe.

@RussKie RussKie force-pushed the fix_3033_preserve_user_font_styles branch from 463dc93 to 59d0fbf Compare September 17, 2020 04:19
@RussKie RussKie added the servicing-approved .NET Shiproom approved the PR for merge label Sep 17, 2020
@RussKie RussKie force-pushed the fix_3033_preserve_user_font_styles branch from 59d0fbf to 31c4c43 Compare September 17, 2020 04:44
@merriemcgaw
Copy link
Member

This isn't something we're fixing in the context of this PR, or in .NET 5.0 timeframe.

@RussKie can you file a separate issue for this related potential problem?

Whenever ambient font changed, this change would stomp over
user-defined DataGridView font styles, such as
* DefaultCellStyle
* ColumnHeadersDefaultCellStyle
* RowHeadersDefaultCellStyle

Whilst the behaviour goes all the way back to .NET Framework, it becomes
a significant migration issue for projects that want to keep the original
default font (that was changed in dotnet#656).

Prevent changes to either of the above cellstyles, if those configured by
a user.

Resolves dotnet#3033
@RussKie RussKie force-pushed the fix_3033_preserve_user_font_styles branch from 31c4c43 to 36f3117 Compare September 18, 2020 08:20
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Sep 21, 2020
@RussKie RussKie merged commit 0f5ecac into dotnet:release/5.0-rc2 Sep 21, 2020
@RussKie RussKie deleted the fix_3033_preserve_user_font_styles branch September 21, 2020 07:04
@RussKie
Copy link
Contributor Author

RussKie commented Sep 28, 2020

Docs: dotnet/docs#20803

@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

📖 documentation: breaking please open a breaking change issue https://github.com/dotnet/docs/issues/new?assignees=gewarren servicing-approved .NET Shiproom approved the PR for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dataGridView doesn't respect font settings if new form's Font != DefaultFont

5 participants