Bug fixing and making test project compatible#2459
Merged
emmche merged 17 commits intoMay 21, 2026
Conversation
- Remove broken MetadataDirectories from CalcManager.Interop.vcxproj (CppWinRT NuGet handles metadata automatically) - Add AssetTargetFallback=native to Calculator.ViewModels and Calculator projects to fix NU1201 errors with native project references - Convert Calculator.Tests from net8.0 SDK-style to UWP test project: - Use SDKReference TestPlatform.Universal for protocol-matched testhost - Add Package.appxmanifest, UnitTestApp.xaml/.cs, and Assets - Keep MSTest NuGet packages for test attributes/adapter - Update Calculator.sln platform mappings for Calculator.Tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port the C++ GetTokenToReadableNameMap logic to C# LocalizationSettings.
Maps engine display tokens (e.g. 'sqr(', '-') to narrator-friendly
names (e.g. 'square (', 'Minus (') for the expression automation name.
This fixes UI test failures where tests expected readable names like
'square (9)' and '4 Minus (' but got raw engine strings 'sqr(9)' and '4 -'.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The CurrencyHttpClient was hitting dead Microsoft fwlink URLs that now redirect to Bing HTML pages instead of returning JSON currency data. The upstream C++ code uses hardcoded mock data (fictional currencies) since the real currency service is internal to Microsoft. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
emmche
changed the title
Refactor GetTokenToReadableNameMap to use simple helper methods instead of ValueTuple arrays to avoid potential .NET Native AOT compilation issues in Release builds. Add double-checked locking pattern to prevent race conditions during lazy initialization. Wrap initialization in try-catch to prevent app crash if resources are unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit removed the Windows.Web.Http.HttpClient static field, which caused the app to crash on startup when compiled with .NET Native AOT (Release mode). This retains the HttpClient field for assembly reference stability while still returning mock currency data (matching C++ behavior). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix same-currency conversion returning wrong values by wiring GetOrderedUnits/LoadOrderedRatios delegation to CurrencyDataLoader - Always call SetCurrentUnitTypes in OnUnitChanged (matching C++) - Add ResetCategoriesAndRatios + Calculate in OnCurrencyDataLoadFinished - Implement ConvertToLocalizedString with CurrencyFormatter to pad decimal places (e.g. '20.' displays as '20.00') - Implement UpdateInputBlocked to limit decimal digit entry - Add input blocking check in OnButtonPressed - Implement UpdateIsDecimalEnabled based on FractionDigits - Add UpdateCurrencyFormatter call in OnUnitChanged - Add value1cp switching in OnSwitchActive Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Build unit display name as 'CountryName - CurrencyName' matching C++ - Build accessible name as 'CountryName CurrencyName' matching C++ - Add TruncateFractionDigits to truncate (not round) decimal digits - Call OnPaste with truncated value in UpdateCurrencyFormatter when switching currencies (matching C++ behavior) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace nuget restore with msbuild -t:Restore to properly handle mixed native C++ and managed C# project references without needing AssetTargetFallback or SkipGetTargetFrameworkProperties workarounds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…perties NuGet restore fails with NU1201 when a managed UAP project references a native vcxproj. Two properties are needed: - AssetTargetFallback=native: tells NuGet to accept 'native' as a compatible framework for UAP projects - SkipGetTargetFrameworkProperties=true: prevents MSBuild from querying native projects for their target framework during restore Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
edwcry
previously approved these changes
May 14, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple functional and compatibility issues across currency conversion/formatting, accessibility narrator output, programmer BitFlip behavior, and CI/test project setup to make the test project deployable and the pipeline compatible with newer toolsets.
Changes:
- Updated currency conversion pipeline to support always-available mock data, per-currency fraction digit formatting, and correct unit display/accessibility naming.
- Implemented expression narrator token mapping and improved Date Calculator live region automation event raising.
- Converted
Calculator.Testsinto a deployable UWP test project and updated CI restore/build configuration for newer VS/toolset environments.
Reviewed changes
Copilot reviewed 18 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CalculatorUITestFramework/UnitConverterPage.cs | Updates UI test unit selection to click list items by accessible name. |
| src/Calculator/Views/DateCalculator.xaml.cs | Raises LiveRegionChanged events on load and hardens automation peer invocation. |
| src/Calculator/Calculator.csproj | Adds asset fallback and adjusts VCX project reference properties for restore/build behavior. |
| src/Calculator.ViewModels/UnitConverterViewModel.cs | Adds currency formatters, fraction-digit truncation/input blocking, and refresh logic after currency data loads. |
| src/Calculator.ViewModels/StandardCalculatorViewModel.cs | Changes paste error behavior to show localized “Invalid input” via engine strings. |
| src/Calculator.ViewModels/DataLoaders/UnitConverterDataLoader.cs | Delegates currency unit/ratio loading to CurrencyDataLoader when appropriate. |
| src/Calculator.ViewModels/DataLoaders/CurrencyHttpClient.cs | Replaces web fetch with embedded mock currency metadata/ratios to match C++ offline behavior. |
| src/Calculator.ViewModels/Common/NumbersAndOperatorsEnum.cs | Aligns BINPOS command ids with native engine command values. |
| src/Calculator.ViewModels/Common/LocalizationSettings.cs | Adds narrator-readable token mapping infrastructure and readable-string generation. |
| src/Calculator.ViewModels/Calculator.ViewModels.csproj | Adds asset fallback and adjusts VCX project reference properties. |
| src/Calculator.Tests/UnitTestApp.xaml.cs | Adds UWP test app entry point for running MSTest in AppContainer. |
| src/Calculator.Tests/UnitTestApp.xaml | Adds UWP application definition for the test project. |
| src/Calculator.Tests/Package.appxmanifest | Adds manifest needed to deploy/run the UWP test app. |
| src/Calculator.Tests/DateCalculatorTests.cs | Minor update (adds System using) to support test project changes. |
| src/Calculator.Tests/Calculator.Tests.csproj | Converts unit tests to a UWP test project with manifest/assets/SDK references. |
| src/Calculator.sln | Fixes solution configuration/deploy mappings and adds test project dependency. |
| src/CalcManager.Interop/CalcManager.Interop.vcxproj | Adjusts MIDL metadata directory settings for build compatibility. |
| .github/workflows/action-ci.yml | Updates CI runner image and restore flow to use MSBuild restore. |
Comments suppressed due to low confidence (1)
src/Calculator.ViewModels/Common/LocalizationSettings.cs:375
- GetNarratorReadableString now maps each character via GetNarratorReadableToken(). The token map is primarily built for multi-character engine tokens (e.g., "sin(") and this per-character iteration will never match those; additionally it inherits the "append open-paren" behavior, so results like "-" become "minus (". Consider either keeping raw-number behavior here (as before) or implementing tokenization appropriate for results (and ensure operator mappings don’t add parentheses).
public static string GetNarratorReadableString(string value)
{
var sb = new StringBuilder();
foreach (char c in value)
{
sb.Append(GetNarratorReadableToken(c.ToString()));
}
return sb.ToString();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (s_tokenToReadableNameMap.TryGetValue(token, out string readableName)) | ||
| { | ||
| return readableName + " " + s_openParen; |
Comment on lines
+990
to
+1052
| int lastCurrencyFractionDigits = currencyFormatter.FractionDigits; | ||
|
|
||
| currencyFormatter.IsDecimalPointAlwaysDisplayed = false; | ||
| currencyFormatter.FractionDigits = 0; | ||
|
|
||
| string result; | ||
|
|
||
| // Handle scientific notation | ||
| int posOfE = stringToLocalize.IndexOf('e'); | ||
| if (posOfE >= 0) | ||
| { | ||
| int posOfSign = posOfE + 1; | ||
| char signOfE = stringToLocalize[posOfSign]; | ||
| string significandStr = stringToLocalize.Substring(0, posOfE); | ||
| string exponentStr = stringToLocalize.Substring(posOfSign + 1); | ||
|
|
||
| result = ConvertToLocalizedString(significandStr, allowPartialStrings, currencyFormatter) | ||
| + "e" + signOfE | ||
| + ConvertToLocalizedString(exponentStr, allowPartialStrings, currencyFormatter); | ||
| } | ||
| else | ||
| { | ||
| int posOfDecimal = stringToLocalize.IndexOf('.'); | ||
| bool hasDecimal = posOfDecimal >= 0; | ||
|
|
||
| if (hasDecimal) | ||
| { | ||
| if (allowPartialStrings && lastCurrencyFractionDigits > 0) | ||
| { | ||
| currencyFormatter.IsDecimalPointAlwaysDisplayed = true; | ||
| } | ||
|
|
||
| // Force post-decimal digits so trailing zeroes aren't cut off | ||
| currencyFormatter.FractionDigits = lastCurrencyFractionDigits; | ||
| } | ||
|
|
||
| if (IsCurrencyCurrentCategory) | ||
| { | ||
| string currencyResult = currencyFormatter.Format(double.Parse(stringToLocalize, System.Globalization.CultureInfo.InvariantCulture)); | ||
| string currencyCode = currencyFormatter.Currency; | ||
|
|
||
| // CurrencyFormatter always includes LangCode or Symbol. Remove the currency code. | ||
| int pos = currencyResult.IndexOf(currencyCode); | ||
| if (pos >= 0) | ||
| { | ||
| currencyResult = currencyResult.Remove(pos, currencyCode.Length); | ||
| // Trim any leading/trailing spaces (including non-breaking spaces) | ||
| currencyResult = currencyResult.Trim(' ', '\u00A0', '\u202F'); | ||
| } | ||
|
|
||
| result = currencyResult; | ||
| } | ||
| else | ||
| { | ||
| // Non-currency: just localize characters | ||
| LocalizationSettings.GetInstance().LocalizeDisplayValue(ref stringToLocalize); | ||
| result = stringToLocalize; | ||
| } | ||
| } | ||
|
|
||
| // Restore formatter state | ||
| currencyFormatter.FractionDigits = lastCurrencyFractionDigits; | ||
|
|
Comment on lines
+804
to
+823
| var currencyRatios = CurrencyDataLoader.LoadOrderedRatios(unit.Id); | ||
| if (currencyRatios.Count > 0) | ||
| { | ||
| var currencyUnits = CurrencyDataLoader.GetOrderedUnits(0); | ||
| var entries = new List<UnitConversionEntry>(); | ||
| foreach (var kvp in currencyRatios) | ||
| { | ||
| var targetUnit = currencyUnits.Find(u => u.Id == kvp.Key); | ||
| if (targetUnit != null) | ||
| { | ||
| entries.Add(new UnitConversionEntry | ||
| { | ||
| Unit = new UnitWrapper { Id = targetUnit.Id, Name = targetUnit.Name, Abbreviation = targetUnit.Abbreviation, AccessibleName = targetUnit.CountryName }, | ||
| Ratio = kvp.Value.Ratio, | ||
| Offset = kvp.Value.Offset, | ||
| OffsetFirst = kvp.Value.OffsetFirst, | ||
| }); | ||
| } | ||
| } | ||
| return entries.ToArray(); |
Comment on lines
53
to
75
| return $"https://go.microsoft.com/fwlink/?LinkID=828053&clcid={_responseLanguage}"; | ||
| await Task.CompletedTask; | ||
| return MockCurrencyConverterData; | ||
| } |
- LocalizationSettings: bake " (" suffix into paren-entry map values so
GetNarratorReadableToken returns mapped values as-is, fixing spurious
trailing "(" on no-paren entries (LeftShift, RightShift, LogBaseY,
YRoot) and on `-` -> `minus`.
- UnitConverterViewModel.ConvertToLocalizedString: wrap formatter
mutations in try/finally so both IsDecimalPointAlwaysDisplayed and
FractionDigits are restored on every path.
- UnitConverterDataLoader.LoadOrderedRatios: build a Dictionary keyed by
currency unit id once per call instead of running List.Find per ratio,
and call GetOrderedUnits(0) exactly once.
- CurrencyHttpClient.GetCurrencyMetadataAsync / GetCurrencyRatiosAsync:
drop unnecessary async/await Task.CompletedTask state machine; return
Task.FromResult for success and Task.FromException for the
VIEWMODEL_FOR_UT forced-failure branch.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
UI tests (Standard/History/Scientific) assert on expression text that
includes a trailing " (" for every mapped token returned by
GetNarratorReadableToken for example "4 Minus (", "2 Minus ( 3=",
and "square (9)". The C++ original (LocalizationService.cpp) also
unconditionally appends " " + openParen, so this is the intended
contract, not a bug.
The Copilot review comment that asked us to remove the spurious paren
on no-paren entries (LeftShift/RightShift/LogBaseY/YRoot) and on the
minus mapping was wrong: those tokens follow the same narrator
convention as the paren entries, and the call sites expect it.
This restores the previous behavior verbatim and lets the CI UI tests
pass again. The other three review fixes (try/finally formatter
restore, dictionary-based ratio lookup, async/await simplification) are
unaffected and remain in place.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
arcadiogarcia
approved these changes
May 19, 2026
edwcry
approved these changes
May 20, 2026
karska-test
approved these changes
May 20, 2026
emmche
merged commit 925861b
into
microsoft:feature/calcviewmodel-to-csharp
5 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Description of the changes:
Currency conversion & formatting (UnitConverterViewModel.cs, CurrencyHttpClient.cs, UnitConverterDataLoader.cs):
Programmer mode paste error (StandardCalculatorViewModel.cs):
CCalcEngine::DisplayError(CALC_E_DOMAIN))
Accessibility (StandardCalculatorViewModel.cs, LocalizationSettings.cs):
Build & CI (.github/workflows/action-ci.yml, Calculator.Tests.csproj, Calculator.sln):
UI test updates (UnitConverterPage.cs):
How changes were validated:
Before
After