Property Store Refactor #11779

elachlan · 2024-07-30T20:55:53Z

Related: #9508

Microsoft Reviewers: Open in CodeFlow

codecov · 2024-07-30T21:33:00Z

Codecov Report

Attention: Patch coverage is 72.60274% with 40 lines in your changes missing coverage. Please review.

Project coverage is 74.97851%. Comparing base (fa00708) to head (588fa23).
Report is 25 commits behind head on feature/10.0.

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/10.0      #11779         +/-   ##
======================================================
+ Coverage      74.94672%   74.97851%   +0.03179%     
======================================================
  Files              3026        3032          +6     
  Lines            630129      631581       +1452     
  Branches          46738       47088        +350     
======================================================
+ Hits             472261      473550       +1289     
- Misses           154520      154649        +129     
- Partials           3348        3382         +34

Flag	Coverage Δ
Debug	`74.97851% <72.60274%> (+0.03179%)`	⬆️
integration	`18.03112% <42.06897%> (+0.04917%)`	⬆️
production	`48.23660% <72.41379%> (+0.17343%)`	⬆️
test	`97.01518% <100.00000%> (+0.00024%)`	⬆️
unit	`45.28632% <67.58621%> (+0.18895%)`	⬆️

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

elachlan · 2024-07-31T01:01:02Z

@lonitra PropertyStore has private static int s_currentKey;. we use it via: public static int CreateKey() => s_currentKey++;. Should we use Interlocked.Increment here?

lonitra · 2024-07-31T18:33:00Z

Should we use Interlocked.Increment here?

Comment on this seems to indicate that we are always calling this in class initializer and we do not have same class hierarchy initializing on multiple threads. I looked for the callers of this and that seems to still be true, so I think we should be ok to leave it as is here.

JeremyKuhne · 2024-08-01T17:06:18Z

Should we use Interlocked.Increment here?

Comment on this seems to indicate that we are always calling this in class initializer and we do not have same class hierarchy initializing on multiple threads. I looked for the callers of this and that seems to still be true, so I think we should be ok to leave it as is here.

After we're done with moving to the new API we should probably create a global enum with "keys" as a more efficient replacement. Then we won't have hundreds of these calls on startup, no threading concerns, and greatly simplified static field content. Maybe something like:

internal enum PropertyKey
{
    BackgroundBrush,   // Instead of s_backBrushProperty
    FontHeight,        // Instead of s_fontHeightProperty
}

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

JeremyKuhne · 2024-08-01T17:18:26Z

src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs

-
-            return DrawMode.Normal;
-        }
+        get => Properties.TryGetValue(s_propDrawMode, out DrawMode drawMode) ? drawMode : DrawMode.Normal;


This could just be the OrDefault overload as DrawMode.Normal is the default.

JeremyKuhne · 2024-08-01T17:24:33Z

src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs

        set
        {
            if (DrawMode != value)
            {
                // valid values are 0x0 to 0x2.
                SourceGenerated.EnumValidator.Validate(value);
                ResetHeightCache();
-                Properties.SetInteger(s_propDrawMode, (int)value);
+                Properties.AddValue(s_propDrawMode, value);


Related to the value being the default, it might be better to update AddOrRemoveValue to not be constrained to class and instead check == default(T). Then we can be removing entries for other default values as well.

JeremyKuhne · 2024-08-01T17:37:00Z

src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewBand.cs

-            int dividerThickness = Properties.GetInteger(s_propDividerThickness, out bool found);
-            return found ? dividerThickness : 0;
-        }
+        get => Properties.TryGetValue(s_propDividerThickness, out int dividerThickness) ? dividerThickness : 0;


Shouldn't this be GetValueOrDefault?

JeremyKuhne · 2024-08-01T17:38:31Z

src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewBand.cs

-            int dividerThickness = Properties.GetInteger(s_propDividerThickness, out bool found);
-            return found ? dividerThickness : 0;
-        }
+        get => Properties.TryGetValue(s_propDividerThickness, out int dividerThickness) ? dividerThickness : 0;
        set
        {
            ArgumentOutOfRangeException.ThrowIfNegative(value);
            ArgumentOutOfRangeException.ThrowIfGreaterThan(value, MaxBandThickness);

            if (value != DividerThickness)


This is one where AddOrRemoveValue simplifies things. You can then check the result to see if value is different. (See my earlier comment about opening this overload up to all T values).

JeremyKuhne · 2024-08-01T17:39:14Z

src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs

        {
            PInvoke.SendMessage(this, PInvoke.CB_SETDROPPEDWIDTH, (WPARAM)dropDownWidth);
        }

-        _ = Properties.GetInteger(s_propItemHeight, out found);
-        if (found)
+        if (Properties.TryGetValue(s_propItemHeight, out int _))


ContainsKey?

JeremyKuhne · 2024-08-01T17:43:02Z

@elachlan I left some comments. I've reviewed about half of it. I'll review again after you ping me.

This is about the largest change size we'd want to make for this. I'd even suggest making them a little smaller in the future so we can review with a bit more confidence that we aren't missing something.

elachlan · 2024-08-01T20:20:18Z

@JeremyKuhne I have made another branch locally to keep the changes and reverted this branch to the last know test pass. This change set should also be smaller now.

Ill open up a follow up PR once this is merged.

JeremyKuhne

Left a few comments

src/System.Windows.Forms/src/System/Windows/Forms/Layout/FlowLayout.cs

src/System.Windows.Forms/src/System/Windows/Forms/Layout/CommonProperties.cs

src/System.Windows.Forms/src/System/Windows/Forms/Form.cs

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

JeremyKuhne · 2024-08-01T21:28:56Z

In general looks good, looks like some tests need updated.

elachlan · 2024-08-01T21:42:14Z

I'm just reviewing it again myself. I've found a few more discards and places where we can use GetValueOrDefault. I'll push it through once the current test run finishes.

elachlan · 2024-08-01T22:45:54Z

@JeremyKuhne can you check CI please? It's got a fail. But no test failures.

JeremyKuhne · 2024-08-01T23:03:13Z

Integration tests failed, but didn't report the failing test

Refactor PropertyStore GetSize/SetSize

5eb33f2

dotnet-policy-service bot assigned elachlan Jul 30, 2024

Remove GetColor/SetColor

5d30cba

elachlan changed the title ~~Property Store Refactor - GetSize/SetSize~~ Property Store Refactor Jul 30, 2024

dotnet-policy-service bot added the draft draft PR label Jul 31, 2024

Refactor ContainsInteger usage

96f178e

elachlan force-pushed the PropertyStore-Refactor branch from 3f96ff7 to 96f178e Compare July 31, 2024 00:48

Refactor RemoveInteger usage

460e0cf

elachlan force-pushed the PropertyStore-Refactor branch 4 times, most recently from 1e65c5e to dbb47f3 Compare July 31, 2024 06:06

Refactor PropertyStore.RemoveObject usage

da14311

elachlan force-pushed the PropertyStore-Refactor branch 2 times, most recently from d5fd031 to e641e6b Compare July 31, 2024 23:23

Refactor PropertyStore.GetInteger

588fa23

elachlan force-pushed the PropertyStore-Refactor branch 2 times, most recently from e2af75c to 715155e Compare August 1, 2024 06:38

JeremyKuhne reviewed Aug 1, 2024

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs Outdated Show resolved Hide resolved

JeremyKuhne reviewed Aug 1, 2024

View reviewed changes

elachlan force-pushed the PropertyStore-Refactor branch from 715155e to 588fa23 Compare August 1, 2024 19:59

elachlan marked this pull request as ready for review August 1, 2024 20:41

elachlan requested a review from a team as a code owner August 1, 2024 20:41

JeremyKuhne requested changes Aug 1, 2024

View reviewed changes

dotnet-policy-service bot added the 📭 waiting-author-feedback The team requires more information from the author label Aug 1, 2024

Changes from review

f09a072

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 1, 2024

elachlan requested a review from JeremyKuhne August 1, 2024 20:48

fix GetWrapContents

e955b5b

dotnet-policy-service bot removed the draft draft PR label Aug 1, 2024

Fix GetWrapContents and simplify ShouldSerializeSelectionForeColor

bcddb08

JeremyKuhne approved these changes Aug 1, 2024

View reviewed changes

JeremyKuhne enabled auto-merge (squash) August 1, 2024 23:14

JeremyKuhne merged commit 7446e03 into dotnet:feature/10.0 Aug 1, 2024
8 checks passed

elachlan deleted the PropertyStore-Refactor branch August 1, 2024 23:54

github-actions bot locked and limited conversation to collaborators Sep 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Property Store Refactor #11779

Property Store Refactor #11779

elachlan commented Jul 30, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

elachlan commented Jul 31, 2024

lonitra commented Jul 31, 2024

JeremyKuhne commented Aug 1, 2024

JeremyKuhne Aug 1, 2024

JeremyKuhne Aug 1, 2024

JeremyKuhne Aug 1, 2024

JeremyKuhne Aug 1, 2024

JeremyKuhne Aug 1, 2024

JeremyKuhne commented Aug 1, 2024

elachlan commented Aug 1, 2024

JeremyKuhne left a comment

JeremyKuhne commented Aug 1, 2024

elachlan commented Aug 1, 2024

elachlan commented Aug 1, 2024

JeremyKuhne commented Aug 1, 2024

Property Store Refactor #11779

Property Store Refactor #11779

Conversation

elachlan commented Jul 30, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

elachlan commented Jul 31, 2024

lonitra commented Jul 31, 2024

JeremyKuhne commented Aug 1, 2024

JeremyKuhne Aug 1, 2024

Choose a reason for hiding this comment

JeremyKuhne Aug 1, 2024

Choose a reason for hiding this comment

JeremyKuhne Aug 1, 2024

Choose a reason for hiding this comment

JeremyKuhne Aug 1, 2024

Choose a reason for hiding this comment

JeremyKuhne Aug 1, 2024

Choose a reason for hiding this comment

JeremyKuhne commented Aug 1, 2024

elachlan commented Aug 1, 2024

JeremyKuhne left a comment

Choose a reason for hiding this comment

JeremyKuhne commented Aug 1, 2024

elachlan commented Aug 1, 2024

elachlan commented Aug 1, 2024

JeremyKuhne commented Aug 1, 2024

elachlan commented Jul 30, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading