calculator icon indicating copy to clipboard operation
calculator copied to clipboard

Migrate ViewModel layer to C#

Open mcooley opened this issue 2 years ago • 3 comments

In 2021, the view layer of the Calculator codebase was ported to C#. I propose that we continue this work and migrate the ViewModel layer to C#.

General reasons for migrating to C# are discussed in #893. I expect that we would see a few benefits specifically from migrating the ViewModel layer:

  • Remove most of the C++/CX code in this repository, making it easier to convert remaining C++ code to C++/WinRT or another supported solution.
  • Make it easier to rewrite some components in this layer which have major issues, like unit converters (#379) and pasted expression parsing (#344).
  • In some cases, possibly improve performance by 1) making the interface between C# and C++ code less chatty, and 2) exposing more code to .NET Native's whole-program optimization.

This work is best done incrementally. Here is how I would break up the work:

  • [X] Set up a CalculatorApp.ViewModel project to host viewmodel-layer C# code (#1895)
  • [ ] Port ApplicationViewModel (#1924)
  • [ ] Port NavCategory, and its tests
  • [ ] Port CopyPasteManager, and its tests
  • [ ] Port UnitConverterViewModel, data loaders, and CalcManager's UnitConverter, and their tests
  • [ ] Port DateCalculatorViewModel, and its tests
  • [ ] Port GraphingCalculatorViewModel, and its tests
  • [ ] Port miscellaneous utilities
    • [ ] NarratorNotifier (#1928)
    • [ ] Utilities (#1927)
    • [ ] NetworkManager

We should avoid modifying the infinite-precision calc engine as part of this work. StandardCalculatorViewModel, and related classes which call into the engine, should remain in C++ for now.

We should port the code as-is, and postpone major refactoring until after the code has been ported.

We should measure the app startup latency after these changes. If these changes cause startup to get slower, we should revert the changes or optimize code to eliminate the regression.

I would like to work on this in my free time, but I'd welcome help from other contributors.

mcooley avatar Sep 22 '22 23:09 mcooley

@tian-lt, @hanzhang54, or @guominrui I'd love your opinion on this proposal :smile:

mcooley avatar Sep 22 '22 23:09 mcooley

Hi @mcooley, the plan looks great. Here are some questions and comments from myself:

  1. AFAIK, unit converter is sitting at CalcManager, and C# cannot connect to it without a glue layer (which is one of the roles that CalcViewModel is now playing). So, to ensure the entire ViewModel to be migrated to C#, shall we consider decouple the unit converter from CalcManager and make the CalcManager as a standalone library in order to allow C# to invoke with the help of P/Invoke? (This is also the task that issue #1545 is tracking.
  2. CalcManager and CalcEngine are being tested by CalculatorUnitTests. CalcViewModel is acting as a bridge to connect them. Shall we create a new UT project to move out of the responsibility of testing CalcEngine from the current CalculatorUnitTests?

tian-lt avatar Sep 28 '22 16:09 tian-lt

@tian-lt

AFAIK, unit converter is sitting at CalcManager, and C# cannot connect to it without a glue layer (which is one of the roles that CalcViewModel is now playing). So, to ensure the entire ViewModel to be migrated to C#, shall we consider decouple the unit converter from CalcManager and make the CalcManager as a standalone library in order to allow C# to invoke with the help of P/Invoke? (This is also the task that issue CalcManager Packaging #1545 is tracking.

Yes, I think we should move unit converter out of CalcManager as part of this work. Unit converter is already mostly separate from the rest of CalcManager--the only common file is Command.h--and I don't think there's any reason to keep the implementation in C++.

For the parts of CalcViewModel which act as a bridge to CalcManager--StandardCalculatorViewModel, History(Item)ViewModel, MemoryItemViewModel, and ExpressionCommand(De)Serializer--I agree that we need to redesign the interface between the ViewModel layer and the "engine" layer as described in #1545. Until we do that work, I think we should leave these classes as they are. We can track complete removal of the CalcViewModel project as part of #1545.

CalcManager and CalcEngine are being tested by CalculatorUnitTests. CalcViewModel is acting as a bridge to connect them. Shall we create a new UT project to move out of the responsibility of testing CalcEngine from the current CalculatorUnitTests?

Yes, we should create a separate project to test CalcEngine in isolation. I think we can track this as a separate issue.

mcooley avatar Sep 28 '22 23:09 mcooley