Merge pull request zkSNACKs#10346 from ichthus1604/UI-Decoupling11

[VDG] UI Decoupling #11

CONTRIBUTING.md

@@ -151,7 +151,20 @@ myUiControl.Text = result;
 
 - [ConfigureAwait FAQ](https://devblogs.microsoft.com/dotnet/configureawait-faq/)
 
-## Disposing Subscriptions in ReactiveObjects
+## Never throw AggregateException and Exception in a mixed way
+It causes confusion and awkward catch clauses.
+[Example](https://github.com/zkSNACKs/WalletWasabi/pull/10353/files) 
+
+---
+
+# UI Coding Conventions
+
+The following is a list of UI specific coding conventions. Follow these any time you are contributing code in the following projects:
+ - `WalletWasabi.Fluent`
+ - `WalletWasabi.Fluent.Desktop`
+ - `WalletWasabi.Fluent.Generators`
+
+ ## Disposing Subscriptions in ReactiveObjects
 
 **DO** follow [ReactiveUI's Subscription Disposing Conventions](https://reactiveui.net/docs/guidelines/framework/dispose-your-subscriptions).
 
@@ -239,9 +252,103 @@ If it seems not possible to implement something without breaking some of this ad
 ## Avoid using Grid as much as possible, Use Panel instead 
 If you don't need any row or column splitting for your child controls, just use `Panel` as your default container control instead of `Grid` since it is a moderately memory and CPU intensive control.
 
-## Never throw AggregateException and Exception in a mixed way
-It causes confusion and awkward catch clauses.
-[Example](https://github.com/zkSNACKs/WalletWasabi/pull/10353/files) 
+## ViewModel Hierarchy
+
+The ViewModel structure should reflect the UI structure as much as possible. This means that ViewModels can have *child* ViewModels directly referenced in their code, just like Views have direct reference to *child* views.
+
+❌ **DO NOT** write ViewModel code that depends on *parent* or *sibling* ViewModels in the logical UI structure. This harms both testability and maintainability.
+
+Examples: 
+
+ - ✔️ `MainViewModel` represents the Main Wasabi UI and references `NavBarViewModel`.
+ - ✔️ `NavBarViewModel` represents the left-side navigation bar and references `WalletListViewModel`.
+ - ❌ `NavBarViewModel` code must NOT reference `MainViewModel` (its logical parent).
+ - ❌ `WalletListViewModel` code must NOT reference `NavBarViewModel` (its logical parent).
+ - ❌ `WalletListViewModel` code must NOT reference other ViewModels that are logical children of `NavBarViewModel` (its logical siblings).
+
+## UI Models
+
+The UI Model classes (which comprise the *Model* part of the MVVM pattern) sit as an abstraction layer between the UI and the larger Wasabi Object Model (which lives in the `WalletWasabi` project). This layer is responsible for:
+
+ - Exposing Wasabi data and functionality in a UI-friendly manner. Usually in the form of Observables.
+
+ - Avoiding tight coupling between UI code and business logic. This is critical for testability of UI code, mainly ViewModels.
+
+❌ **DO NOT** write ViewModel code that depends directly on `WalletWasabi` objects such as `Wallet`, `KeyManager`, `HdPubKey`, etc.
+
+✔️ **DO** write ViewModel code that depends on `IWalletModel`, `IWalletListModel`, `IAddress`, etc.
+
+❌ **DO NOT** convert regular .NET properties from `WalletWasabi` objects into observables or INPC properties in ViewModel code. 
+
+❌ **DO NOT** convert regular .NET events from `WalletWasabi` objects into observables in ViewModel code.
+
+✔️ If such conversions are required, **DO** write them into the UI Model layer.
+
+## UiContext
+
+ViewModels that depend on external components (such as Navigation, Clipboard, QR Reader, etc) can access these via the `ViewModelBase.UIContext` property. For instance:
+
+ - Get text from clipboard: `var text = await UIContext.Clipboard.GetTextAsync();`
+
+ - Generate QR Code: `await UIContext.QrGenerator.Generate(data);`
+
+ - Open a popup or navigate to another Viewmodel: `UIContext.Navigate().To(....)`
+
+This is done to facilitate unit testing of viewmodels, since all dependencies that live inside the `UiContext` are designed to be mock-friendly.
+
+❌ **DO NOT** write Viewmodel code that directly depends on external device-specific components or code that might otherwise not work in the context of a unit test.
+
+## Source-Generated ViewModel Constructors
+
+Whenever a ViewModel references its `UiContext` property, the `UiContext` object becomes an actual **dependency** of said ViewModel. It must therefore be initialized, ideally as a constructor parameter.
+
+In order to minimize the amount of boilerplate required for such initialization, several things occur in this case:
+ - A new constructor is generated for that ViewModel, including all parameters of any existing constructor plus the UiContext. 
+ - This generated constructor initializes the `UiContext` *after* running the code of the manually written constructor (if any).
+ - A Roslyn Analyzer inspects any manually written constructors in the ViewModel to prevent references to `UiContext` in the constructor body, before the above mentioned initialization can take place, resulting in `NullReferenceException`s.
+ - The Analyzer demands the manually written constructor to be declared `private`, so that external instatiation of the ViewModel is done by calling the source-generated constructor.
+
+❌ Writing code that directly references `UiContext` in a ViewModel's constructor body will result in a compile-time error.
+
+❌ Writing code that indirectly references `UiContext` in ViewModel's constructor body will result in a run-time `NullReferenceException`.
+
+✔️ Writing code that directly or indirectly references `UiContext` inside a lambda expression in a ViewModel's constructor body is okay, since this code is deferred to a later time at run-time when the `UiContext` property has already been properly initialized.
+
+Example:
+
+```csharp
+    // ❌ BAD, constructor should be private
+    public AddressViewModel(IAddress address)
+	{
+		if (condition)
+		{
+			//❌ BAD, UiContext is null at this point.
+			UiContext.Navigate().To(someOtherViewModel); 
+		}
+	}
+
+    // ✔️ GOOD, constructor is private
+    private AddressViewModel(IAddress address)
+	{
+		//✔️ GOOD, UiContext is already initialized when the Command runs
+		NextCommand = ReactiveCommand.Create(() => UiContext.Navigate().To(someOtherViewModel))); 
+	}
+```
+
+If you absolutely must reference `UiContext` in the constructor, you can create a public constructor explicitly taking `UiContext` as a parameter:
+
+```csharp
+    // ✔️ GOOD, 
+    public AddressViewModel(UiContext uiContext, IAddress address)
+	{
+		UiContext = uiContext;
+
+		// ✔️Other code here can safely use the UiContext since it's explicitly initialized above.
+	}
+```
+
+In this case, no additional constructors will be generated, and the analyzer will be satisfied.
+
 
 
 

WalletWasabi.Fluent.Generators/AnalyzerExtensions.cs

@@ -0,0 +1,114 @@
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+namespace WalletWasabi.Fluent.Generators;
+
+public static class AnalyzerExtensions
+{
+	public static List<IdentifierNameSyntax> GetUiContextReferences(this SyntaxNode node, SemanticModel semanticModel)
+	{
+		return
+			node.DescendantNodes()
+			 .OfType<IdentifierNameSyntax>()
+			 .Where(x => x.Identifier.ValueText == "UiContext")                                                   // faster verification
+			 .Where(x => semanticModel.GetTypeInfo(x).Type?.ToDisplayString() == UiContextAnalyzer.UiContextType) // slower, but safer. Only runs if previous verification passed.
+			 .ToList();
+	}
+
+	public static bool IsPrivate(this ConstructorDeclarationSyntax node)
+	{
+		return node.Modifiers.Any(m => m.IsKind(SyntaxKind.PrivateKeyword));
+	}
+
+	public static bool IsPublic(this ConstructorDeclarationSyntax node)
+	{
+		return node.Modifiers.Any(m => m.IsKind(SyntaxKind.PublicKeyword));
+	}
+
+	public static bool IsSourceGenerated(this SyntaxNode node)
+	{
+		var filePath = node.SyntaxTree.FilePath;
+
+		return filePath is null ||
+			   filePath.EndsWith(UiContextAnalyzer.UiContextFileSuffix);
+	}
+
+	public static bool IsSubTypeOf(this SyntaxNode node, SemanticModel model, string baseType)
+	{
+		if (node is not ClassDeclarationSyntax cls)
+		{
+			return false;
+		}
+
+		var currentType = model.GetDeclaredSymbol(cls);
+		while (currentType != null)
+		{
+			if (currentType.ToDisplayString() == baseType)
+			{
+				return true;
+			}
+			currentType = currentType.BaseType;
+		}
+
+		return false;
+	}
+
+	public static bool IsAbstractClass(this ClassDeclarationSyntax cls, SemanticModel model)
+	{
+		var typeInfo =
+			model.GetDeclaredSymbol(cls)
+			?? throw new InvalidOperationException($"Unable to get Declared Symbol: {cls.Identifier}");
+
+		return typeInfo.IsAbstract;
+	}
+
+	public static bool IsRoutableViewModel(this SyntaxNode node, SemanticModel model)
+	{
+		return node.IsSubTypeOf(model, "WalletWasabi.Fluent.ViewModels.Navigation.RoutableViewModel");
+	}
+
+	public static bool HasUiContextParameter(this ConstructorDeclarationSyntax ctor, SemanticModel model)
+	{
+		return ctor.ParameterList.Parameters.Any(p => p.Type.IsUiContextType(model));
+	}
+
+	public static bool IsUiContextType(this TypeSyntax? typeSyntax, SemanticModel model)
+	{
+		if (typeSyntax is null)
+		{
+			return false;
+		}
+
+		return model.GetTypeInfo(typeSyntax).Type?.ToDisplayString() == UiContextAnalyzer.UiContextType;
+	}
+
+	public static List<string> GetNamespaces(this ITypeSymbol? typeSymbol)
+	{
+		return GetNamespaceSymbols(typeSymbol)
+				.Where(x => !x.IsGlobalNamespace)
+				.Select(x => x.ToDisplayString())
+				.ToList();
+	}
+
+	private static IEnumerable<INamespaceSymbol> GetNamespaceSymbols(this ITypeSymbol? typeSymbol)
+	{
+		if (typeSymbol is null)
+		{
+			yield break;
+		}
+
+		yield return typeSymbol.ContainingNamespace;
+
+		if (typeSymbol is INamedTypeSymbol namedType)
+		{
+			foreach (var typeArg in namedType.TypeArguments)
+			{
+				yield return typeArg.ContainingNamespace;
+			}
+		}
+	}
+}

WalletWasabi.Fluent.Generators/NavigationMetaDataGenerator.cs

@@ -11,7 +11,7 @@ namespace WalletWasabi.Fluent.Generators;
 [Generator]
 public class NavigationMetaDataGenerator : ISourceGenerator
 {
-	private const string NavigationMetaDataAttributeDisplayString = "WalletWasabi.Fluent.NavigationMetaDataAttribute";
+	public const string NavigationMetaDataAttributeDisplayString = "WalletWasabi.Fluent.NavigationMetaDataAttribute";
 
 	private const string NavigationMetaDataDisplayString = "WalletWasabi.Fluent.NavigationMetaData";
 
@@ -129,8 +129,7 @@ public void Execute(GeneratorExecutionContext context)
 
 	private static string? ProcessClass(Compilation compilation, INamedTypeSymbol classSymbol, ISymbol attributeSymbol, ISymbol metadataSymbol)
 	{
-		if (!classSymbol.ContainingSymbol.Equals(classSymbol.ContainingNamespace,
-			SymbolEqualityComparer.Default))
+		if (!classSymbol.ContainingSymbol.Equals(classSymbol.ContainingNamespace, SymbolEqualityComparer.Default))
 		{
 			return null;
 		}
@@ -139,8 +138,7 @@ public void Execute(GeneratorExecutionContext context)
 
 		var format = new SymbolDisplayFormat(
 			typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypes,
-			genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters | SymbolDisplayGenericsOptions.IncludeTypeConstraints | SymbolDisplayGenericsOptions.IncludeVariance
-		);
+			genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters | SymbolDisplayGenericsOptions.IncludeTypeConstraints | SymbolDisplayGenericsOptions.IncludeVariance);
 
 		var source = new StringBuilder($@"// <auto-generated />
 #nullable enable

WalletWasabi.Fluent.Generators/Properties/launchSettings.json

@@ -0,0 +1,8 @@
+{
+  "profiles": {
+    "Profile 1": {
+      "commandName": "DebugRoslynComponent",
+      "targetProject": "..\\WalletWasabi.Fluent\\WalletWasabi.Fluent.csproj"
+    }
+  }
+}