Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate ViewModel layer to C# #1894

Open
1 of 11 tasks
mcooley opened this issue Sep 22, 2022 · 3 comments
Open
1 of 11 tasks

Migrate ViewModel layer to C# #1894

mcooley opened this issue Sep 22, 2022 · 3 comments

Comments

@mcooley
Copy link
Member

mcooley commented Sep 22, 2022

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:

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

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
Copy link
Member Author

mcooley commented Sep 22, 2022

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

@tian-lt
Copy link
Contributor

tian-lt commented Sep 28, 2022

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 CalcManager Packaging  #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?

@mcooley
Copy link
Member Author

mcooley commented Sep 28, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants