[Essentials] Make Locale constructible by external TextToSpeech backends - #37857
[Essentials] Make Locale constructible by external TextToSpeech backends#37857Redth wants to merge 4 commits into
Conversation
Microsoft.Maui.Media.Locale only had an internal constructor, so an out-of-tree ITextToSpeech implementation could not implement GetLocalesAsync or populate SpeechOptions.Locale without reflection or forking. Community backends (for example Maui.Tizen) had to mark locale enumeration as unsupported. Make the existing constructor public and normalize null arguments to string.Empty so the non-nullable Language/Country/Name/Id contract that PublicAPI already declares is actually honored. Values are otherwise stored verbatim: not parsed, validated, trimmed, or case normalized. Immutability, reference equality, and the default ToString are unchanged. Because locales can now be created with empty Id/Language, tighten the platform voice-selection guards from `!= null` to string.IsNullOrEmpty so a language-only locale still falls back the way it does today. This also fixes a latent NullReferenceException on Tizen where `options?.Locale.Language` dereferenced a null Locale. Add Essentials.ExternalBackend.UnitTests, a test assembly that has no InternalsVisibleTo access to Microsoft.Maui.Essentials, proving an external backend can implement ITextToSpeech, return Locale values from GetLocalesAsync, and round-trip them through SpeechOptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37857Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37857" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
area-essentials
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables out-of-tree ITextToSpeech backends to construct and return Microsoft.Maui.Media.Locale instances by making Locale publicly constructible, normalizing null inputs, and adding a regression test project that runs in CI.
Changes:
- Added a public
Locale(string? language, string? country, string? name, string? id)constructor withnull→string.Emptynormalization and expanded XML docs. - Tightened voice-selection guards in platform TextToSpeech implementations to handle partially populated locales.
- Added
Essentials.ExternalBackend.UnitTestsand wired it into solutions and test runs (Helix + cake).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Essentials/test/ExternalBackendTests/ExternalTextToSpeechBackendTests.cs | Adds “external backend” regression tests validating the new public Locale constructor and round-tripping via SpeechOptions. |
| src/Essentials/test/ExternalBackendTests/Essentials.ExternalBackend.UnitTests.csproj | Introduces a new test project intentionally lacking InternalsVisibleTo access to validate public API usage. |
| src/Essentials/src/TextToSpeech/TextToSpeech.shared.cs | Makes Locale constructor public, normalizes null arguments, and documents cross-platform semantics. |
| src/Essentials/src/TextToSpeech/TextToSpeech.android.cs | Updates locale guard logic before constructing Android Java.Util.Locale. |
| src/Essentials/src/TextToSpeech/TextToSpeech.ios.tvos.watchos.cs | Updates iOS voice selection to treat empty Id as absent. |
| src/Essentials/src/TextToSpeech/TextToSpeech.macos.cs | Updates macOS voice selection to require non-empty Locale.Id. |
| src/Essentials/src/TextToSpeech/TextToSpeech.tizen.cs | Avoids a potential null dereference / improves guard when Locale.Language is missing/empty. |
| src/Essentials/src/PublicAPI/net/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/net-android/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| src/Essentials/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Records the newly public Locale constructor for API tracking. |
| eng/helix.proj | Adds the new external-backend unit test project to Helix execution. |
| eng/cake/dotnet.cake | Includes the new unit test project in dotnet-test globs. |
| Microsoft.Maui.sln | Registers the new test project in the main solution. |
| Microsoft.Maui-windows.slnf | Adds the new test project to the Windows solution filter. |
| Microsoft.Maui-mac.slnf | Adds the new test project to the Mac solution filter. |
| if (!string.IsNullOrEmpty(options?.Locale?.Language)) | ||
| { | ||
| JavaLocale locale = null; | ||
| if (!string.IsNullOrWhiteSpace(options?.Locale.Country)) | ||
| if (!string.IsNullOrWhiteSpace(options.Locale.Country)) | ||
| locale = new JavaLocale(options.Locale.Language, options.Locale.Country); | ||
| else | ||
| locale = new JavaLocale(options.Locale.Language); |
| /// <param name="name">The display name of the locale, as described by <see cref="Name"/>.</param> | ||
| /// <param name="id">The engine specific identifier of the locale, as described by <see cref="Id"/>.</param> | ||
| /// <remarks> | ||
| /// No argument is required and none are validated. A <see langword="null"/> argument is normalized to |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
Fixes the macOS Debug/Release build break introduced by the previous commit: Microsoft.Maui-mac.slnf(1,1): error MSB4025: The project file could not be loaded. Data at the root level is invalid. Line 1, position 1. Microsoft.Maui-mac.slnf and Microsoft.Maui-windows.slnf both filter Microsoft.Maui-dev.sln, not Microsoft.Maui.sln. The new test project was only added to Microsoft.Maui.sln, so the filter referenced a project that its backing solution did not contain. Reproduced locally as: error MSB5028: Solution filter file at ".../Microsoft.Maui-mac.slnf" includes project "src/Essentials/test/ExternalBackendTests/ Essentials.ExternalBackend.UnitTests.csproj" that is not in the solution file at ".../Microsoft.Maui-dev.sln". Add the project to Microsoft.Maui-dev.sln, and to Microsoft.Maui-vscode.sln for consistency with Essentials.UnitTests, which is listed in all three solutions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/Essentials/src/TextToSpeech/TextToSpeech.shared.cs:274
- The XML docs have a grammatical error: "For Android this used..." should be present tense ("uses").
This issue also appears on line 286 of the same file.
/// For Android this used the ISO 639 alpha-2 or alpha-3 language code, or registered language subtags up to 8 alpha letters (for future enhancements).
src/Essentials/src/TextToSpeech/TextToSpeech.shared.cs:286
- The XML docs have a grammatical error: "For Android this used..." should be present tense ("uses").
/// <para>For Android this used the ISO 3166 alpha-2 country code or UN M.49 numeric-3 area code.</para>
src/Essentials/test/ExternalBackendTests/ExternalTextToSpeechBackendTests.cs:55
- The null-forgiving operator (
value!) isn’t needed here (the Locale constructor accepts nullable strings) and makes the intent of the null-normalization test less clear. Passingvaluedirectly still tests the same runtime behavior without suppressing nullability warnings.
var locale = new Locale(value!, value, value, value);
Code review of the public Locale constructor caught a regression the
constructor itself introduced, plus adjacent gaps in the same class of
newly-reachable input.
Windows (regression, was missed): GetSpeakParametersSSMLProsody resolved
the language with
options?.Locale?.Language ?? SpeechSynthesizer.DefaultVoice.Language
Normalizing null to string.Empty defeats that ?? — an externally
constructed Locale with no language yields "", which is not null, so the
default-voice fallback silently stopped firing and the SSML was emitted
with xml:lang=''. Windows was the one platform not hardened in the
previous commit. Verified against a standalone harness: the old
expression emits xml:lang='' for new Locale(null, ...) and
new Locale("", ...); the new one falls back to DefaultVoice.Language for
null/empty/whitespace and preserves a real language unchanged.
iOS: a non-empty but unrecognized Id made AVSpeechSynthesisVoice
.FromIdentifier return null, and because that sat in the true branch of a
ternary with no coalesce, Language was skipped entirely and Voice was set
to null. Since the whole point of this PR is that out-of-tree backends
mint engine-specific ids, that path is now reachable. Rewritten as a
single coalescing chain so identifier -> language -> current-language
each falls through, matching the intent the comment already stated.
macOS: restore the reset semantics the previous commit changed. Assigning
null returns NSSpeechSynthesizer to the system default; the synthesizer is
a cached instance, so skipping the assignment let a voice from a previous
utterance leak into a call that asked for no specific voice. (This file is
not currently compiled for any shipping TFM.)
Use IsNullOrWhiteSpace consistently across the guards, so a whitespace-only
value is treated as absent like an empty one, matching the Country guard
that already did this.
Also drop the test that pinned reference equality and the compiler default
ToString(), and the matching sentence in the XML docs. Neither is part of
what this change establishes, and asserting them would block a future
record/IEquatable conversion. Document instead that empty or whitespace
properties are treated as "not specified" by the platform backends.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Essentials/src/TextToSpeech/TextToSpeech.shared.cs:317
- The constructor XML docs say "No argument is required", which is misleading because the constructor has four required parameters (even though they can be null). Reword to clarify that arguments are optional in value (may be null) rather than optional in arity.
/// No argument is required and none are validated. A <see langword="null"/> argument is normalized to
/// <see cref="string.Empty"/>; every other value is stored exactly as supplied.
/// </remarks>
AI Review Summary
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After FixGate Result:
|
| Rank | Candidate | Implementation | Validation | Assessment |
|---|---|---|---|---|
| 1 | pr-plus-reviewer |
Complete PR plus focused reviewer corrections | Blocked by missing project.assets.json; no behavioral test result |
Best code: retains the minimal API while fixing iOS fallback, macOS stale voice state, and inaccurate fallback documentation. |
| 2 | pr |
Complete submitted fix | Trusted Gate SKIPPED; no detected tests | Correct core API and external-consumer test design, but an unknown iOS identifier discards a valid language and a macOS language-only locale can retain the previous voice. |
| 3 | try-fix-2 |
Design only; empty diff | Blocked, not executed | A static factory preserves immutability, but it is unimplemented, untested, and less direct than ordinary construction for a data-holder type. It also does not resolve the reviewed platform behavior. |
| 4 | try-fix-1 |
Design only; empty diff | Blocked, not executed | A parameterless constructor plus four public init accessors greatly expands the mutable-looking API surface and is unimplemented and untested. Its claim that platform guards could remain unchanged is unsafe once empty values become valid. |
No candidate passed or failed a regression test: the trusted Gate skipped the raw PR, both STEP 5a alternatives stopped before implementation, and pr-plus-reviewer stopped at missing restore assets before compiling. Therefore no failed candidate is ranked above a passing candidate; there are no passing candidates in the available evidence.
Expert review reconciliation
- Blocking: iOS's conditional/
??grouping gives the non-empty identifier branch no language fallback whenAVSpeechSynthesisVoice.FromIdentifierreturns null. - Blocking: macOS's shared
NSSpeechSynthesizerretains its previousVoicewhen a new locale has an empty identifier. - Corrected: the raw XML documentation promises language/country fallback on platforms that actually use the platform default.
- Accepted design tradeoff: null-to-empty normalization changes values observed from existing platform-produced locales, but it enforces the shipped non-null property contract and is explicitly documented and tested at the constructor boundary.
- Remaining gap: the new external-backend tests validate public construction and transport, not native voice-selection branches.
inline-findings.json contains the raw submitted-PR findings. pr-plus-reviewer/reviewer.patch, candidate.patch, and validation.log contain the complete refinement evidence.
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 360 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
71/73 (2 skipped) ✓ | — |
Label |
97/99 (2 skipped) ✓ | — |
Layout |
192/195 (3 skipped) ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — reviewer changes required
The reviewer-enhanced candidate identified changes that are not yet in the submitted PR.
Why: The submitted public-constructor approach is the best API design, but the reviewer refinement also fixes concrete iOS fallback and macOS stale-voice defects while correcting the platform contract documentation. Its focused validation was blocked by a missing assets file, so native behavior remains an explicit uncertainty.
Address the actionable findings in this review before merging.
MauiBot
removed
the
s/agent-review-in-progress
Build Windows (Release) in build 1569551 failed to compile
src/Essentials/src/Passkeys/WindowsWebAuthn.windows.cs with CS0234/CS0246
for Windows.Win32, HRESULT, PCWSTR and the WEBAUTHN_* structs, i.e. the
Microsoft.Windows.CsWin32 source generator did not emit its output.
This is unrelated to the change under review:
* this PR touches no Passkeys, CsWin32 or NativeMethods code;
* no error in the log came from TextToSpeech.windows.cs;
* Essentials compiled cleanly for net11.0, netstandard2.0/2.1,
maccatalyst and android in the same run - only the two Windows TFMs
failed, and only on the generated namespace;
* Build Windows (Debug) compiled the same file successfully in the same
build on a different agent (NetCore-Public 128 vs 111);
* Build Windows (Release) succeeded two runs earlier on build 1569387,
which already had the identical solution graph including the new test
project - the only delta since is the TextToSpeech guard fixes;
* no other build in the recent fleet shows this signature.
The step retried on the same agent and failed identically, which points at
that agent rather than at a race. Pushing an empty commit to get the job
scheduled somewhere else.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Microsoft.Maui.Media.Localeonly had an internal constructor. That makes it impossible for a text-to-speech backend that lives outside of this repo to implementITextToSpeech: it cannot return anything fromGetLocalesAsync(), and it cannot construct aLocaleto hand back throughSpeechOptions.Locale.This is a concrete blocker today — Redth/Maui.Tizen#8 has to mark locale enumeration as unsupported (
GetLocalesAsyncreturns an empty list) purely because of this, even though the TizenTtsClient.GetSupportedVoices()API returns everything needed.Reproduction from an assembly that does not have
InternalsVisibleToaccess toMicrosoft.Maui.Essentials:What changed
Localegains a public constructor (public Locale(string? language, string? country, string? name, string? id)). This is the smallest additive change that unblocks the scenario — no builder, no factory, no new type.nullis normalized tostring.Empty.PublicAPI.Shipped.txtalready declares all four properties as non-nullable (string!), but the in-box platform backends passnullforCountry/Name/Id. Now that arbitrary callers can construct aLocale, the declared contract is actually enforced, so consumers can rely on the properties never beingnull. Values are otherwise stored verbatim — they are not parsed, validated, trimmed, or case normalized, because their meaning is defined by whichever engine produced them.Immutability, reference equality, and
ToString()are unchanged. NoEquals/GetHashCode/ToStringoverrides were added, so existing behavior is preserved exactly; there is a test that pins this.Platform voice-selection guards tightened. Because a caller can now build a locale that only carries a
Language(or only anId), the!= nullchecks in the platform implementations were changed tostring.IsNullOrEmptyso that a partially-populated locale falls back the same way an absent one does today. Platform-produced locales always populated these fields, so in-box behavior is unchanged.NullReferenceExceptionon Tizen, whereoptions?.Locale.Languagedereferenced a nullLocale.Culture semantics documented. The XML docs now state that
Languageis BCP-47 on iOS/Windows and ISO 639 on Android, thatCountryis ISO 3166 / UN M.49 on Android and unused (empty) on iOS/Windows because the region is already carried inLanguage, thatIdis engine-specific and that platforms fall back toLanguage/Countrywhen it is empty, and thatnullnormalizes tostring.Empty.Issues Fixed
Unblocks out-of-tree
ITextToSpeechbackends, e.g. Redth/Maui.Tizen#8.Testing
Added
src/Essentials/test/ExternalBackendTests(Microsoft.Maui.Essentials.ExternalBackend.UnitTests). This assembly deliberately has noInternalsVisibleTogrant fromMicrosoft.Maui.Essentials, so it can only use the public surface — it is the regression guard for exactly the scenario this PR unblocks. It contains a stand-in external backend that implementsITextToSpeech, and covers:nulland""normalizing tostring.Empty;Locales fromGetLocalesAsync(), including a language-only locale;LocalethroughSpeechOptionsintoSpeakAsync.The project is registered in
eng/helix.proj,eng/cake/dotnet.cake,Microsoft.Maui.sln, and both solution filters, so it runs alongside the other unit test assemblies.Verified locally on macOS:
Essentials.ExternalBackend.UnitTests— 6/6 passed (and confirmed to fail to compile withCS1729before the change).Essentials.UnitTests— 513/513 passed.Essentials.csprojbuilds clean (including thePublicAPIanalyzers) fornet11.0,netstandard2.0,netstandard2.1,net11.0-ios,net11.0-maccatalyst, andnet11.0-android.PublicAPI.Unshipped.txtentries were added for all seven TFM folders (net,net-android,net-ios,net-maccatalyst,net-tizen,net-windows,netstandard).Reviewer notes
The
null→string.Emptynormalization is an observable change.PublicAPI.Shipped.txtalready declared all four properties as non-nullablestring!, but the in-box backends passed literalnullforCountry/Name/Id, so the runtime value contradicted the annotation. This change aligns runtime with the declared contract. External consumers who defensively null-checkLocale.Countrywill now see""instead ofnull.Platform voice-selection guards were audited for the newly-reachable inputs. Because a caller can now construct a
Localecarrying only some fields, every platform guard was reviewed:GetSpeakParametersSSMLProsodyresolved the language withoptions?.Locale?.Language ?? SpeechSynthesizer.DefaultVoice.Language. Normalization defeats that??(""is notnull), so the default-voice fallback stopped firing and the SSML was emitted asxml:lang=''. Fixed with anIsNullOrWhiteSpacecheck. Verified against a standalone harness: the previous expression emitsxml:lang=''fornew Locale(null, …)andnew Locale("", …), while the new one falls back toDefaultVoice.Languagefor null/empty/whitespace and preserves a real language unchanged.IdmakesAVSpeechSynthesisVoice.FromIdentifierreturnnull; sitting in the true branch of a ternary with no coalesce, that skippedLanguageentirely and setVoice = null. Now a single coalescing chain: identifier → language → current language, matching the intent the existing comment already described. (The original?:/??precedence was correct; this is a separate fall-through gap.)IsNullOrWhiteSpace. The Tizen change also fixes a latentNullReferenceException:options?.Locale.Languagethrew wheneveroptions != null && options.Locale == null..macos.csis not compiled for any shipping TFM (itsItemGroupinEssentials.csprojis commented out and there is nonet-macosPublicAPI folder), so it is neither type-checked nor CI-verified. Its guard was kept faithful to the original reset semantics: assigningnullreturnsNSSpeechSynthesizerto the system default, and since the synthesizer is a cached instance, skipping the assignment would let a previous utterance's voice leak into a call that asked for no specific voice.Known coverage gap.
GetSpeakParametersSSMLProsodyisstaticand trivially unit-testable, but it compiles only for the Windows TFM, whileEssentials.UnitTeststargets$(_MauiDotNetTfm)(net11.0). There is therefore no in-repo home for a regression test on the Windows fix without adding a Windows-targeted unit test project. Flagging rather than silently skipping; happy to add that project if reviewers want it.Equality/
ToString()are deliberately left unspecified. An earlier revision asserted reference equality and the compiler-defaultToString(). Those assertions were removed, along with the matching sentence in the XML docs, so that a future conversion ofLocaleto arecordor anIEquatable<Locale>implementation is not pre-emptively blocked.