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

Remove Global.Instance singleton #1591

Merged
merged 3 commits into from Jun 21, 2019

Conversation

Projects
None yet
2 participants
@NicolasDorier
Copy link
Contributor

commented Jun 19, 2019

So this completely remove the singleton.

The code is quite easy to review in most places I replaced Global.Instance by Global, and made Global a property in the current scope.

Some UI component like converters or behaviors had no way to pass a Global instance easily. This was solved by populating the Application.Current.Resources with Global, UiConfig and Config instances from the MainWindow.

I also export AvaloniaGlobalComponent to MEF so it can be solved at some places. I initially wanted just to export Global, but because we create Global before avalonia app, Avalonia would have created its own instance of Global... this AvaloniaGlobalComponent hack make sure we share the same global that we created outside of avalonia.

Will be able to dramatically increase test coverage and maybe later expose the view model as library for wasabi clients.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/no-global branch 3 times, most recently from bd254df to d7c225d Jun 19, 2019

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/no-global branch from d7c225d to d3a5a87 Jun 19, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2019

  • There are a lot of things here.
  • There's no bug.
  • This will make the code not consistent with the Backend and the Tests project's coding patterns.
  • I'm not sure I'm qualified to review it. I'd like @danwalmsley @molnard or @jmacato to review this, but all of them are unavailable until the 1st of next month.

So here's my proposal.

  • Are there any changes that you can beak into new PRs? If so could you do that? You can work on top of your own PRs, so it wouldn't stop you from progressing forward.
  • Can you make sure the Backend and the Test project is consistent to what you are doing?

Then both @lontivero and me will review them and @Rmartin1104 will test them according to the Manual Testing Document.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Sure I can break it in easy to review units

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

So the Part 1 here is the most difficult part.

The rest is almost exclusively a replace from Global.Instance to Global.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/no-global branch from d3a5a87 to 31073d9 Jun 19, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I rebased this PR on top of Part 1 so you can decide either to merge this in one go or to start by Part 1.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/no-global branch from 31073d9 to 91a4ab3 Jun 20, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@nopara73 this PR is rebased and ready to review and merge, as you can see it do only obvious stuff. (Renaming Global.Instance to Global and passing around the Global instance)

I worked around the GetDocument you reverted by making a replacement of GetOrCreate which use IoC to instanciate WalletViewModel.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

I will make separate PR for the rest of global, they are quite small compared to this one.

nopara73 added some commits Jun 21, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

I worked around the GetDocument you reverted by making a replacement of GetOrCreate which use IoC to instanciate WalletViewModel.

That looks good.

Everything else seems to be basic refactoring.

@nopara73 nopara73 merged commit 5b5f3e1 into zkSNACKs:master Jun 21, 2019

1 of 4 checks passed

Wasabi.Linux in progress
Details
Wasabi.Osx in progress
Details
Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.