Skip to content

Commit 7b45a8c

Browse files
Address first batch of review suggestions.
Also: Fix edge cases where dark mode did not get applied for ... * CheckBoxes, * RadioButtons * Tabs. * Move ApplyThemingImplicitly flag definition to CreateParams. * Update approach to set ApplyThemingImplicitly control style. * Delegate FlatStyle.Standard Button rendering to the System due to HighDpi issues in the VisualStyle-based renderers.
1 parent 2aa45cd commit 7b45a8c

28 files changed

+1297
-1254
lines changed

.github/AI-Prompts/GDI-Refactoring/AIP-cache_pens-and_brushes.md

Lines changed: 0 additions & 80 deletions
This file was deleted.

.github/Copilot/GDIPlus-copilot-instructions.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ Available cached objects:
6565

6666
Always use `var` for brevity and always apply `using` to ensure proper disposal.
6767

68+
IMPORTANT: You cannot use pen- or brush-scoped, when the pen needs additional features, for example, when during its lifetime, the pen needs to be modified (e.g., `Inset`, `DashStyle`, `CustomStartCap`, etc.). In these cases, you need to use the non-cached version of the pen or brush.
69+
6870
### 1.3 Transforming Helper Methods
6971

7072
When refactoring existing code:

.github/copilot-instructions.md

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -215,8 +215,11 @@
215215

216216
- **Use `var` only for complex types when type is apparent from context**
217217
```csharp
218-
var control = new Button(); // OK - type is obvious
219-
var items = GetComplexCollection(); // OK - long type name
218+
var control = new Button(); // OK - type is obvious, but better is:
219+
Button control = new(); // Preferred
220+
221+
// OK - long type name
222+
var items = GetComplexCollectionWithRidiculousLongTypeName();
220223
int primitiveValue = 42; // Required for primitives
221224
```
222225

@@ -239,6 +242,11 @@
239242
```csharp
240243
private static readonly int s_defaultBorderWidth = 1;
241244
```
245+
An exception to this rule is when the field is attributed with `[ThreadStatic]`, in which case the prefix should be `t_`:
246+
```csharp
247+
[ThreadStatic]
248+
private static int t_threadLocalValue;
249+
```
242250

243251
- **Prefix private/internal instance fields with `_`** (suggestion level)
244252
```csharp
@@ -286,7 +294,7 @@
286294
private bool IsVisible() => Visible && Parent?.Visible == true;
287295
```
288296

289-
- For longer expressions, use line breaks with proper alignment:
297+
For longer expressions, use line breaks with proper alignment:
290298
```csharp
291299
internal int SomeFooIntegerProperty =>
292300
_someFooIntegerProperty;
@@ -295,6 +303,19 @@
295303
size.Width > 0
296304
&& size.Height > 0;
297305
```
306+
**IMPORTANT**
307+
308+
In C#, `public Foo FooInstance => new Foo();` and `public Foo FooInstance { get; } = new Foo();` are NOT interchangeable.
309+
310+
* The first is an expression-bodied property that returns a new value each time it is accessed.
311+
* The second is a read-only auto-property with initializer that returns the same value every time, created once.
312+
313+
When editing, generating, or suggesting C# properties, always maintain the original intent:
314+
315+
* Use an expression-bodied property (`=>`) for computed or dynamic values.
316+
* Use an auto-property with initializer (`{ get; } = ...`) for stored, initialized-once values.
317+
318+
Never "correct" or convert one style to the other unless you are certain the semantic behavior is intended to change because it was wrong with 100% confidence to begin with. If you encounter a case, where you have a expression-bodied property that returns a new instance, and you encounter this in a code review, you should add a comment explaining why this is intended, and that it is not a mistake.
298319

299320
### 1.5 Error Handling and Argument Validation
300321

@@ -345,19 +366,38 @@
345366
- **Handle platform-specific code appropriately**
346367
```csharp
347368
[SupportedOSPlatform("windows")]
348-
private void WindowsSpecificMethod() { ... }
369+
private void WindowsSpecificMethod()
370+
{
371+
...
372+
}
349373
```
350374

351375
- **Use proper disposal patterns**
352-
```csharp
353-
using var brush = new SolidBrush(Color.Red); // Simple using for single resources
354-
355-
// Traditional using for complex scenarios
356-
using (Graphics g = CreateGraphics())
357-
{
358-
// Drawing operations
359-
}
360-
```
376+
Prefer the modern C# "using declaration" syntax (using var ...) over the traditional using statement block.
377+
Use:
378+
379+
```csharp
380+
using var brush = backColor.GetCachedSolidBrushScope();
381+
```
382+
383+
instead of:
384+
385+
```csharp
386+
using (SolidBrushCache.Scope brush = backColor.GetCachedSolidBrushScope())
387+
{
388+
// ...
389+
}
390+
```
391+
392+
This makes code more concise, reduces nesting, and improves readability. Only use the older style if you need to limit the scope of the variable to a smaller section within a larger method, or when the using var pattern is not supported. **Note**, that if you use `using` with the type name over `var` for new instances that you can omit the type name after the `new` like:
393+
394+
```csharp
395+
using Pen focusPen = new(focusColor) // Use 'using' with type name and omit type after 'new'
396+
{
397+
Width = 1.0f,
398+
DashStyle = DashStyle.Dot
399+
};
400+
```
361401

362402
### 1.8 XML Documentation Standards
363403

@@ -402,6 +442,7 @@
402442
### 1.10 Magic Numbers
403443

404444
- **Avoid magic numbers**: Use named constants or enums instead of hard-coded values. This improves readability and maintainability.
445+
405446
```csharp
406447
// Avoid magic numbers
407448
const int MaxItems = 100;
@@ -451,6 +492,9 @@
451492
}
452493
```
453494

495+
Note: If a code line for which these empty-line rules apply has a comment on top of it, the empty line should be placed before the comment, if there isn't one. And of course, NEVER comment that an empty lines need to be inserted at a spot, which would defeat the purpose of the empty line.
496+
497+
454498
### 1.12 Refactoring of existing comments
455499

456500
- **Refactor existing comments**: When refactoring code, also consider updating or removing outdated comments. Comments should accurately reflect the current state of the code and its purpose. Avoid/rephrase inappropriate comments or microaggressions.

src/System.Windows.Forms/System/Windows/Forms/Controls/Buttons/Button.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,11 @@ public AutoSizeMode AutoSizeMode
7575

7676
protected override AccessibleObject CreateAccessibilityInstance() => new ButtonAccessibleObject(this);
7777

78-
internal override ButtonBaseAdapter CreateFlatAdapter() => new ButtonFlatAdapter(this);
78+
internal override ButtonBaseAdapter CreateFlatAdapter() => DarkModeAdapterFactory.CreateFlatAdapter(this);
7979

80-
internal override ButtonBaseAdapter CreatePopupAdapter() => new ButtonPopupAdapter(this);
80+
internal override ButtonBaseAdapter CreatePopupAdapter() => DarkModeAdapterFactory.CreatePopupAdapter(this);
8181

82-
internal override ButtonBaseAdapter CreateStandardAdapter() => new ButtonStandardAdapter(this);
83-
84-
internal override ButtonBaseAdapter CreateDarkModeAdapter() => new ButtonDarkModeAdapter(this);
82+
internal override ButtonBaseAdapter CreateStandardAdapter() => DarkModeAdapterFactory.CreateStandardAdapter(this);
8583

8684
internal override Size GetPreferredSizeCore(Size proposedConstraints)
8785
{
@@ -115,13 +113,15 @@ protected override CreateParams CreateParams
115113
{
116114
CreateParams cp = base.CreateParams;
117115
cp.ClassName = PInvoke.WC_BUTTON;
116+
118117
if (GetStyle(ControlStyles.UserPaint))
119118
{
120119
cp.Style |= PInvoke.BS_OWNERDRAW;
121120
}
122121
else
123122
{
124123
cp.Style |= PInvoke.BS_PUSHBUTTON;
124+
125125
if (IsDefault)
126126
{
127127
cp.Style |= PInvoke.BS_DEFPUSHBUTTON;

0 commit comments

Comments
 (0)