Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions .github/skills/building-code/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ metadata:

# Building the WinForms Repository

> ## 🛑 TENET — Build the solution ONLY with `build.cmd`
>
> **Never** build, validate, or declare the WinForms solution "clean" with a plain
> `dotnet build` / `dotnet msbuild` of `Winforms.sln`. Only **`build.cmd`** (Arcade) applies the
> repository's CI configuration — the **PublicAPI analyzer (RS0016/RS0017)**, the code-style and
> documentation analyzers, and **`-warnAsError`**. A plain `dotnet build` silently downgrades or
> skips these, so **"0 warnings" there does NOT mean CI is green** — the very same change can fail
> the official build with errors.
>
> * **Full / release / package / "is it clean?" verification → always `build.cmd`** (see §2).
> * A single-project `dotnet build` (see §3) is an **inner-loop convenience only**. It is fine while
> iterating, but you **must re-verify with `build.cmd` before claiming a change builds cleanly**.
> * If `build.cmd` cannot run in your environment, the closest fallback is
> `dotnet build <project> /p:ContinuousIntegrationBuild=true /p:TreatWarningsAsErrors=true` — and
> you must say so explicitly rather than implying a `build.cmd` result.

## Prerequisites

* Windows is required for WinForms runtime scenarios, test execution, and Visual
Expand Down Expand Up @@ -45,6 +61,12 @@ You can pass any extra `Build.ps1` flags after `Restore.cmd`, e.g.

## 2 Full Solution Build (preferred)

> **Always use `build.cmd` (Arcade) for full, release, and package builds.** Do **not** use a plain
> `dotnet build` of the solution for these — only `build.cmd` guarantees the Arcade-supported build
> options and the download of the correct base SDK (`global.json`) needed to compile. Plain
> `dotnet build` is reserved for the fast single-project inner loop (see Section 3), and even then
> only after at least one successful `build.cmd` / `Restore.cmd`.

```
.\build.cmd
```
Expand All @@ -57,6 +79,56 @@ Under the hood this runs:
eng\common\Build.ps1 -NativeToolsOnMachine -restore -build -bl
```

### 2.1 Full (clean) test build — required workflow

For a full, clean build of the whole solution, **clean the artifacts first, then build**:

```powershell
# 1. Clean the artifacts folder.
.\build -clean

# 2. Build the full solution.
.\build
```

**Reporting requirement:** a full build is long-running, so while it runs **report progress back to
the user in the console to bridge the wait and give early orientation.** As assemblies complete,
report which assemblies have been **built successfully** and which **failed and with how many
errors**. Prefer running the build with a binary log (the default `-bl`) and/or stream the console
output so per-project results can be surfaced as they happen rather than only at the end.

### 2.2 Release build

```powershell
.\build -configuration release
```

### 2.3 Creating packages

```powershell
# Debug packages
.\build -pack

# Release packages
.\build -configuration release -pack
```

### 2.4 Full `Build.ps1` parameter list

`build.cmd` forwards every extra argument to `eng\common\Build.ps1`. The full surface is:

```
Build.ps1 [-configuration <string>] [-platform <string>] [-projects <string>]
[-verbosity <string>] [-msbuildEngine <string>] [-warnAsError <bool>]
[-warnNotAsError <string>] [-nodeReuse <bool>] [-buildCheck] [-restore]
[-deployDeps] [-build] [-rebuild] [-deploy] [-test] [-integrationTest]
[-performanceTest] [-sign] [-pack] [-publish] [-clean] [-productBuild]
[-fromVMR] [-binaryLog] [-binaryLogName <string>] [-excludeCIBinarylog]
[-ci] [-prepareMachine] [-runtimeSourceFeed <string>]
[-runtimeSourceFeedKey <string>] [-excludePrereleaseVS]
[-nativeToolsOnMachine] [-help] [-properties <string[]>] [<CommonParameters>]
```

### Common flags

| Flag | Short | Description |
Expand Down Expand Up @@ -90,6 +162,12 @@ eng\common\Build.ps1 -NativeToolsOnMachine -restore -build -bl

## 3 Optimized Building a Single Project (fast inner-loop)

> **Inner-loop only.** Use plain `dotnet build` of a single project **only** for quick iteration on
> one project, and **only after** at least one successful `.\build.cmd` / `.\Restore.cmd`. It does
> **not** guarantee the Arcade-supported build options or the download of the correct base SDK, so it
> must **never** be used for a full solution build, a release build, packaging, or any build whose
> result you intend to report as authoritative. For those, always use `build.cmd` (Section 2).

Prefer rebuilding just the project(s) with recent changes by using the
standard `dotnet build` command, **after** at least one initial successful
full restore (via `.\Restore.cmd` or `.\build.cmd`).
Expand Down
22 changes: 22 additions & 0 deletions .github/skills/control-api-tests/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,28 @@ The project uses **xUnit** with **FluentAssertions**. Key attributes:
These are custom xUnit attributes that ensure tests run on an STA thread,
which WinForms requires for COM interop and UI operations.

### 1.4 Async tests: pass a CancellationToken, respect `#nullable`

The repository runs **xUnit v3** and enforces the relevant analyzers as **errors** under the CI
build (`build.cmd`). Two pitfalls fail CI even though a plain `dotnet build` may not flag them:

* **CA2016 / xUnit1051 — always pass a `CancellationToken` to async calls.** Methods such as
`Task.Delay` must receive a token so a cancelled test run stops promptly. In xUnit v3 use
`TestContext.Current.CancellationToken`:

```csharp
await Task.Delay(25, TestContext.Current.CancellationToken);
```

When you receive a `CancellationToken ct` (e.g. in a callback), **forward it** rather than dropping it.

* **CS8632 — nullable annotations need a `#nullable` context.** If a test file uses `?` reference
annotations (e.g. `object? sender`) but the project does not enable nullable, add `#nullable enable`
at the top of the file (or remove the annotation). Match the surrounding files' convention.

> Verify with `build.cmd` (CI parity) — see the `building-code` skill's build tenet. A plain
> single-project `dotnet build` can report these as 0 warnings while CI fails them as errors.

---

## 2. Test Method Naming
Expand Down
124 changes: 87 additions & 37 deletions .github/skills/new-control-api/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,34 @@ System.Windows.Forms.MyEnum.Value2 = 1 -> System.Windows.Forms.MyEnum
**Nullable annotations:** `?` = nullable reference, `!` = non-nullable
reference. Value types do not carry these markers unless `Nullable<T>`.

### 2.4 Publicly accessible interfaces
### 2.4 New `override` members must be tracked too

The PublicAPI analyzer (RS0016) treats a **newly introduced `override`** of a public or
protected member as new API surface — even though the base member is already public. Whenever
you **add an `override` that did not previously exist on that type**, add a line for it to
`PublicAPI.Unshipped.txt` with the `override` prefix. This is easy to miss for paint/lifecycle
overrides added to support a feature. Examples:

```text
override System.Windows.Forms.CheckBox.OnPaint(System.Windows.Forms.PaintEventArgs! pevent) -> void
override System.Windows.Forms.CheckBox.Dispose(bool disposing) -> void
override System.Windows.Forms.ButtonBase.OnVisualStylesModeChanged(System.EventArgs! e) -> void
```

> **CI catches this, a plain `dotnet build` may not.** RS0016 is enforced as an **error** under
> the CI/Arcade build (`build.cmd`); a single-project `dotnet build` can report it as 0 warnings.
> Always re-verify API tracking with `build.cmd` (see the `building-code` skill's build tenet).

### 2.5 Related pitfalls when adding members to a control

* **Hiding an inherited member (CS0114):** if your new member intentionally hides an inherited
one (e.g. a `private new bool ShouldSerializePadding()` shadowing `Control.ShouldSerializePadding()`),
you **must** use the `new` keyword, or the CI build fails.
* **`cref` to internal types in another assembly (CS1574):** XML-doc `<see cref="..."/>` cannot
resolve a type that is `internal` in a *different* assembly (even via `InternalsVisibleTo`). Use
`<c>TypeName</c>` (plain code font) instead of a `cref` for such references.

### 2.6 Publicly accessible interfaces

If a new **public or protected interface** is introduced (or an existing one
gains new members), every member that is publicly accessible must also appear
Expand Down Expand Up @@ -468,51 +495,73 @@ protected virtual void OnMyPropertyChanged(EventArgs e)

---

## 7. .NET Version Guard — Mandatory
## 7. API Stability: Experimental vs. Stable — and Version Guards

All new public APIs **must** be guarded with a preprocessor directive for the
target .NET version. Currently, new APIs target at least **.NET 11**:
### 7.1 New APIs are STABLE by default — do NOT mark them `[Experimental]`

```csharp
#if NET11_0_OR_GREATER
/// <summary>
/// Gets or sets the corner radius for the control's border.
/// </summary>
public int CornerRadius
{
get => Properties.GetValueOrDefault(s_cornerRadiusProperty, 0);
set
{
ArgumentOutOfRangeException.ThrowIfNegative(value);
New public APIs ship as **normal, stable APIs by default**. Do **not** add the
`[Experimental(...)]` attribute, a `WFO5xxx` diagnostic ID, or `[WFO5xxx]`
PublicAPI prefixes unless the work item **explicitly** asks for an experimental
API.

if (Properties.GetValueOrDefault(s_cornerRadiusProperty, 0) != value)
{
Properties.AddOrRemoveValue(s_cornerRadiusProperty, value, defaultValue: 0);
OnCornerRadiusChanged(EventArgs.Empty);
}
}
}
#endif
```
> **Never make an API experimental implicitly.** Experimental status is a
> deliberate, requested decision (it changes the customer contract and requires a
> diagnostic ID + suppression to consume). If the context does not explicitly call
> for it, the API is stable.

### 7.2 When an experimental API *is* explicitly requested

Only when the task explicitly requests an experimental API:

1. Add (or reuse) a diagnostic ID in the `WFO500x` group in
`src\System.Windows.Forms.Analyzers\src\System\Windows\Forms\Analyzers\Diagnostics\DiagnosticIDs.cs`
(e.g. `ExperimentalDarkMode = "WFO5001"`, `ExperimentalAsync = "WFO5002"`,
`ExperimentalAsyncDropTarget = "WFO5003"`). New IDs continue the sequence.
2. Decorate the API:
```csharp
[Experimental(DiagnosticIDs.ExperimentalXxx, UrlFormat = DiagnosticIDs.UrlFormat)]
```
3. Prefix every PublicAPI entry for that API with the diagnostic ID, e.g.
`[WFO5001]System.Windows.Forms.SomeNewApi.get -> ...`.
4. Add a row to **both** `docs\analyzers\Experimental.Help.md` and
`docs\list-of-diagnostics.md`.
5. Suppress the diagnostic where the framework itself consumes the API
(`#pragma warning disable WFOxxxx` / `#Disable Warning WFOxxxx` in VB).

> **Why?** Version guards ensure new APIs are only available on the .NET version
> they were approved for, preventing accidental use on older runtimes. The guard
> applies to the entire API surface: property, event, `On` method, and any
> associated types.
When the API later **graduates to stable** (typically the next release), reverse
all five steps: remove the attribute, the `[WFOxxxx]` PublicAPI prefixes, the
suppressions, the docs rows, and the unused diagnostic ID.

The matching tests must use the **same** preprocessor guard:
### 7.3 Version guards

This repository **single-targets the current in-development .NET** (see
`TargetFramework` / `NetCurrent`), so source is **not** wrapped in
`#if NETxx_0_OR_GREATER` guards — there are none in `System.Windows.Forms`. Do
**not** add `#if NET11_0_OR_GREATER` blocks around new APIs. Add the member
directly:

```csharp
#if NET11_0_OR_GREATER
[WinFormsFact]
public void MyControl_CornerRadius_Set_GetReturnsExpected()
/// <summary>
/// Gets or sets the corner radius for the control's border.
/// </summary>
public int CornerRadius
{
get => Properties.GetValueOrDefault(s_cornerRadiusProperty, 0);
set
{
using MyControl control = new() { CornerRadius = 5 };
Assert.Equal(5, control.CornerRadius);
ArgumentOutOfRangeException.ThrowIfNegative(value);

if (Properties.GetValueOrDefault(s_cornerRadiusProperty, 0) != value)
{
Properties.AddOrRemoveValue(s_cornerRadiusProperty, value, defaultValue: 0);
OnCornerRadiusChanged(EventArgs.Empty);
}
}
#endif
}
```

Tests do not need a version guard either.

---

## 8. Checklist Before Submitting
Expand All @@ -521,7 +570,8 @@ Before considering the implementation complete, verify:

* [ ] API proposal issue exists (upstream or fork) with full proposal format
* [ ] All new public/protected members are in `PublicAPI.Unshipped.txt`
* [ ] New APIs guarded with `#if NET11_0_OR_GREATER` (or appropriate version)
* [ ] API is **stable** (no `[Experimental]`/`WFO5xxx`) unless experimental was
explicitly requested; no `#if NETxx_0_OR_GREATER` guards
* [ ] Property values stored via `PropertyStore` (not backing fields)
* [ ] Every property has a CodeDOM serialization strategy
* [ ] Every property has `On[Property]Changed` + `[Property]Changed` event
Expand All @@ -533,7 +583,7 @@ Before considering the implementation complete, verify:
* [ ] XML documentation on every new public/protected member
* [ ] Naming follows precedent on the control and its base classes
* [ ] Publicly accessible interface members are tracked in PublicAPI files
* [ ] Unit tests cover the new API surface (with matching version guard)
* [ ] Unit tests cover the new API surface

### 8.1 API issue checklist

Expand Down
38 changes: 38 additions & 0 deletions docs/Net11Api_04_KioskManagerComponent.HighRiskReview.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Net11Api_04 KioskModeManager High-Risk Review

This document records review findings that require an API or lifecycle design decision before they can be patched safely.

## FullScreen requested state and actual state differ

`KioskModeManager.FullScreen` currently returns `true` when fullscreen has been requested but no target form is available. `FullScreenChanged`, however, is raised only when the resolved form actually enters or exits fullscreen. This creates two observable state models:

- The property reports requested state through `_pendingFullScreen || _isFullScreen`.
- The event reports actual window state through changes to `_isFullScreen`.

Consequently, setting `FullScreen = true` without a resolved form changes the property without raising the event. When a form later becomes available, the event is raised even though the property value remains `true`.

Before changing this behavior, decide whether `FullScreen` represents requested state or actual window state. The decision must cover:

- initialization and delayed parenting;
- missing, replaced, reparented, and disposed container controls;
- event ordering and two-way data binding;
- `ToggleFullScreen` behavior while a request is pending;
- disposal and failed fullscreen transitions;
- compatibility for applications already observing the property or event.

Tests should define the complete transition matrix for requested, pending, entered, exited, reparented, and disposed states.

## Session-notification registration is not retried

`KioskModeFormObserver.RegisterSessionNotifications` records a failed `WTSRegisterSessionNotification` call as `false` and retries only after form handle recreation. During Windows startup, registration can fail with `RPC_S_INVALID_BINDING` before `Global\TermSrvReadyEvent` is signaled. A kiosk application started during that interval can therefore miss session notifications for the lifetime of its form handle.

A safe fix needs a non-blocking retry design that specifies:

- which Win32 errors are retryable and which are terminal;
- how waiting for `Global\TermSrvReadyEvent` is cancelled;
- how completion is marshalled to the form's UI thread;
- how stale callbacks are rejected after handle recreation;
- how registration and unregistration remain paired;
- how disposal races with an outstanding wait.

Tests should cover service-not-ready startup, successful retry, handle recreation during the wait, and disposal before the wait completes.
2 changes: 1 addition & 1 deletion src/BuildAssist/BuildAssist.msbuildproj
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

<!-- Work out VS installation path, so we can find MSBuild.exe -->
<Exec
Command="vswhere.exe -latest -prerelease -property installationPath -requires Microsoft.Component.MSBuild"
Command='"$(VSWherePath)\vswhere.exe" -latest -prerelease -property installationPath -requires Microsoft.Component.MSBuild'
WorkingDirectory="$(VSWherePath)"
EchoOff="true"
ConsoleToMsBuild="true"
Expand Down
7 changes: 6 additions & 1 deletion src/System.Windows.Forms.Primitives/src/NativeMethods.txt
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,9 @@ PD_RESULT_*
PostQuitMessage
PostQuitMessage
PostThreadMessage
PowerCreateRequest
PowerClearRequest
PowerSetRequest
PRF_*
PROGRESS_CLASS
PROPERTYKEY
Expand Down Expand Up @@ -692,9 +695,11 @@ WC_TABCONTROL
WC_TREEVIEW
WHEEL_DELTA
WindowFromPoint
WTSRegisterSessionNotification
WTSUnRegisterSessionNotification
WINDOWPOS
WINEVENT_INCONTEXT
WSF_VISIBLE
XBUTTON1
XBUTTON2
XFORMCOORDS
XFORMCOORDS
Loading
Loading