[VDG] UI Decoupling #10

[ichthus1604]

Reasoning:

ViewModels that depend on external components (such as Navigation, Clipboard, QR Reader, etc) can now access them via their UIContext. For instance:

• Get text from clipboard: var text = await UIContext.Clipboard.GetTextAsync();
• Generate QR Code: await UIContext.QrGenerator.Generate(data);
• Navigate to another Viewmodel: UIContext.Navigate().To(....)

Problem:

• UIContext is not a singleton, and while UIContext.Default static property exists, it only does so temporarily until UI Decoupling refactoring is completed. ViewModels cannot really depend on that or any other static/singleton object because this breaks testability, as unit tests run in parallel and cannot initialize such singleton objects in a per-test fashion, which is required to define mocks and expected behavior in each test.

• This means that in practice, UIContext is an actual dependency of most ViewModels.

• As any other dependency, UIContext should therefore be a constructor parameter of every ViewModel.

• This basically means that we now need to do constructor injection for all ViewModels that require UIContext.

• As discussed before, constructor injection is highly undesirable because it quickly propagates throughout the codebase since dependencies need to be passed from parent ViewModel to child ViewModel in an endless nested fashion.

Solution:

This PR introduces a Source Generator that does the following:

1 - Source-generated UIContext constructor

• For every ViewModel that uses UIContext in any part of its code, it will create a source-generated constructor containing all the parameters of the existing hand-written constructor, plus the UIContext itself:

// Hand-written code
public class ReceiveAddressViewModel: RoutableViewModel
{
    public ReceiveAddressViewModel(Wallet wallet, HdPubKey model)
    {
        // Constructor logic
    }

    private void GenerateQrCode()
    {
        UIContext.QrCodeGenerator.Generate(...); // Reference to UIContext
    }
}

The above code will produce the following source-generated code:

// Source-generated code
partial class ReceiveAddressViewModel
{
    public ReceiveAddressViewModel(UIContext uiContext, Wallet wallet, HdPubKey model): this(wallet, model)
    {
        UIContext = uiContext;
    }
}

This newly generated constructor inherits from the hand-written one, therefore the latter's logic is preserved, while initializing UIContext as a final step.

Pit of Success:

"The pit of success is a concept that refers to designing systems or processes in a way that makes it easy for users to do the right thing and difficult to do the wrong thing."
— ChatGPT

Since the UIContext is actually required by the above ViewModel (otherwise GenerateQrCode() method will crash), the hand-written constructor which doesn't initialize the UIContext isn't really valid, therefore this PR also introduces a Roslyn Analyzer that will make it a compilation error if this constructor is made public, and instead will request to make it private:

Coming soon: Ctrl. Code Fix to make the constructor private automatically ;)

Also, since the hand-written constructor actually runs before the UIContext property is initialized, you cannot really use it directly in the constructor's body:

Coming soon: Ctrl. Code Fix to move this code to OnNavigatedTo() or OnActivated() automatically ;)

Notice that this only applies to direct use of UIContext in the constructor code, while referencing it for instance in a lambda expression inside the constructor is perfectly fine:

	private ReceiveAddressViewModel(Wallet wallet, HdPubKey model)
	{
		CopyAddressCommand = ReactiveCommand.CreateFromTask(async () =>
		{
			await UIContext.Clipboard.SetTextAsync(Address); // This is perfectly fine.
		});
	}     

2 - Fluent Navigation:

Approximately half of the occurrences of ViewModel instantiation thoughout the codebase are used for navigation. For instance:

public partial class ReceiveViewModel : RoutableViewModel
{
        // ... Other code omitted

 	private void OnShowExistingAddresses()
	{
		Navigate().To(new ReceiveAddressesViewModel(_wallet));
	}
}

As described above, the ReceiveAddressesViewModel constructor now has a UIContext parameter, therefore the Navigate() line would need to be changed to:

private void OnShowExistingAddresses()
{
    Navigate().To(new ReceiveAddressesViewModel(UIContext, _wallet));
}

to avoid this, there is now a Fluent Navigation API: for each Routable ViewModel's constructor, a method will be source-generated in the FluentNavigate class, with all the same parameters as the constructor, excluding the UIContext:

// Source-generated code
public partial class FluentNavigate
{
    public void ReceiveAddresses(Wallet wallet,NavigationTarget navigationTarget = NavigationTarget.DialogScreen,NavigationMode navigationMode = NavigationMode.Normal)
    {
        UIContext.Navigate(navigationTarget).To(new ReceiveAddressesViewModel(UIContext, wallet), navigationMode);
    }
}

this Fluent API can be accessed by using UIContext.Navigate().To() and RoutableViewModel.Navigate().To() overload with no parameters. The navigation statement now becomes:

private void OnShowExistingAddresses()
{
    Navigate().To().ReceiveAddresses(_wallet);
}

This not only has the advantage of hiding the UIContext parameter, but also creates a "strongly typed" navigation API where every available ViewModel will be shown in Intellisense when hitting .:

Also notice that you can optionally define both NavigationTarget and NavigationMode just like before ;)

Next Steps:

This PR is not in a workable state. The changes above imply that many ViewModels will require slight code changes, and there are several ones that have compile errors in this PR. The intent is to introduce and discuss these abstractions and make reviewing easier.

I will follow up with the next PR fixing these ViewModel errors, and then several following PRs applying these techniques to ViewModels incrementally until all ViewModels have been covered.

Also, the problem of non navigation-related instantiation of ViewModels remains: for instance, TileViewModels will still need to have the UIContext manually passed to them by MainViewModel or whatever their parent ViewModel is.

There are also further improvements required to these APIs, specifically the addition of async Task NavigateDialogAsync() equivalent to FluentNavigate.

Overall, this is the initial introduction of the Pit of Success philosophy where we can enforce certain practices and patterns in the code via compiler tooling, and also provide instant feedback and useful information to all developers at design time.

[soosr]

@ichthus1604 @SuperJMN Tests are failing here

[SuperJMN]

@ichthus1604 @SuperJMN Tests are failing here

As discussed with @ichthus1604, this PR is only for review purposes, as a interative step for the bigger changes to be easier to reason about.

Thus, the code here should NOT be merged to master. All the changes contained in this PR + some fixes to make it compile are included in part 11.

cc @soosr

[soosr]

I could only review it in general, pointed out some nits.
@wieslawsoltes could you also take a look before we continue?

(not mergeable PR)

[soosr]

	private ReceiveAddressViewModel(Wallet wallet, HdPubKey model)
	{
		CopyAddressCommand = ReactiveCommand.CreateFromTask(async () =>
		{
			await UIContext.Clipboard.SetTextAsync(Address); // This is perfectly fine.
		});
	}     

So it is not possible to reference UiContext from the private ctor, but the code above is fine. What happens if I immediately call CopyAddressCommand.Execute() in ctor?

[ichthus1604]

@soosr a runtime exception will be thrown in that case.

the suggested solution is to use OnActivated() or OnNavigatedTo(). But if you absolutely must do something in the constructor body that requires the UiContext, then you can change the constructor back to public, add the UiContext parameter explicitly and set the corresponding property as a first step:

public ReceiveAddressViewModel(UiContext UiContext, Wallet wallet, HdPubKey model)
	{
                UiContext = uiContext; // after this line UiContext is of course already initialized and might be used without problem.
		CopyAddressCommand = ReactiveCommand.CreateFromTask(async () =>
		{
			await UIContext.Clipboard.SetTextAsync(Address); // This is perfectly fine.
		});

                CopyAddressCommand.Execute(); // this line uses the UiContext safely.
	}     

This makes the analyzer stop complaining about making the ctor private, and the source generator to not generate any additional constructors for this class, because the requirement to initialize the UiContext has already been met.

I need to document this behavior. What would be the right place for such document?

[soosr]

I think https://github.com/zkSNACKs/WalletWasabi/blob/master/CONTRIBUTING.md