Allow Developers Bypass the Default Fallback Behavior (resolves #3713)

[newbienewbie]

This changes applys to the Maui/Wpf/XamarinForm platform.

What kind of change does this PR introduce?

• Feature Request. See ViewModelViewHost: Better to Allow Developers Bypass the Default Fallback Behavior That Resovles View With No Contract  #3713

What is the current behavior?

1. The ViewModelViewHost resolves view by the ViewContract property. Currently ignores the ViewContract condition if nothing found.

What is the new behavior?

1. Add a property of ContractFallbackByPass so that we can bypass this fallback behavior.
2. Expose a virtual method , i.e. protected virtual void ResolveViewForViewModel(object? viewModel, string? contract) , which allows developers override this behavior.

What might this PR break?

As far as I can see, it does not break anying.

Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:

For WPF/MAUI/XamForms/WinUI, the ContractFallbackByPass is set to false by default. So it won't breaking existing apps.

However, I find the current WinForms implementation has no default fallback behaivor as same as WPF

   var viewLocator = ViewLocator ?? ReactiveUI.ViewLocator.Current;
   var view = viewLocator.ResolveView(x.ViewModel, x.Contract);
   if (view is not null)
   {
       view.ViewModel = x.ViewModel;
       Content = view;
   }

So I didn't add such a property for WinForms. Is it better to add such a property that is set to true by default ?

[glennawatson]

The win forms scenario likely we do want the property added but keep the default the same as current win form functionality.

[newbienewbie]

The win forms scenario likely we do want the property added but keep the default the same as current win form functionality.

Fine.

And should I also add a new method of protected virtual void ResolveViewForViewModel(object? viewModel, string? contract)?

[glennawatson]

Yeah. Make it all consistent

[ChrisPulman]

Please rerun the tests and update the verified files in the API checks, thank you.

[newbienewbie]

Please rerun the tests and update the verified files in the API checks, thank you.

Sorry, at that time, the api vertified tests always failed if I didn't add the following API to verified.txt. :

namespace XamlGeneratedNamespace
{
    public sealed class GeneratedInternalTypeHelper : System.Windows.Markup.InternalTypeHelper
    {
        public GeneratedInternalTypeHelper() { }
        protected override void AddEventHandler(System.Reflection.EventInfo eventInfo, object target, System.Delegate handler) { }
        protected override System.Delegate CreateDelegate(System.Type delegateType, object target, string handler) { }
        protected override object CreateInstance(System.Type type, System.Globalization.CultureInfo culture) { }
        protected override object GetPropertyValue(System.Reflection.PropertyInfo propertyInfo, object target, System.Globalization.CultureInfo culture) { }
        protected override void SetPropertyValue(System.Reflection.PropertyInfo propertyInfo, object target, object value, System.Globalization.CultureInfo culture) { }
    }
}

I don't known why the VS generated this GeneratedInternalTypeHelper. I'm also pretty new to the Verify. So I added the above snippet to verified.txt and then all the tests succeeded in my local dev environments.

However, it's quite strange that when I try to add the ContractFallbackByPass property for WinForm today, I have to remove all the XamlGeneratedNamespace again otherwise the tests fails. I have no idea about the GeneratedInternalTypeHelper phantom for the present.

Anyway, I'll remove them from the verified.txt later together with the commits for WinForm

[ChrisPulman]

@glennawatson are you able to merge this then do another PR to fix the 6 API files that are are failing.

[glennawatson]

Believe @newbienewbie is still working on the PR at the moment with some winforms changes

[newbienewbie]

Yeah. Make it all consistent

Hi, I'm currently blocked. I find the current WinForm's behavior is quite different from the WPF's.

CacheViews

The WinForm's implementation has a unique CacheViews flag. Currently, it will return early if the ViewModel's type remains the same.

  if (CacheViews)
  {
      // when caching views, check the current viewmodel and type
      var c = _content as IViewFor;

      if (c?.ViewModel is not null && c.ViewModel.GetType() == viewModel?.GetType())
      {
          c.ViewModel = viewModel;

          // return early here after setting the viewmodel
          // allowing the view to update it's bindings
          return;
      }
  }

At the same time, the ViewContractObservable is initialized in the SetupBindings() method

ViewContractObservable = Observable.Return(string.Empty);

In the case where I have CacheViews=true, even if I change the ViewContractObservable to Observable.Return("ContractB") later, the view instance remains the same (instead of ViewB)

An easy way is the asumming the ContractFallbackByPass is only available when CacheViews=false. But I don't know whether it is suitable.

DefaultContent

The WinForm applies the DefaultContent only if ViewModel is null and won't assign the Content property when view becomes null:

if (ViewModel is null)       // ********************* the DefaultContent is applied only if ViewModel is NULL******************
{
    if (DefaultContent is not null)
    {
        Content = DefaultContent;
    }

    return;
}

if (CacheViews)
{
    // ...
}

 var viewLocator = ViewLocator ?? ReactiveUI.ViewLocator.Current;
 var view = viewLocator.ResolveView(x.ViewModel, x.Contract);
 if (view is not null)     // ********************* change the content only if view is not null ************************
 {
     view.ViewModel = x.ViewModel;
     Content = view;
 } 

While the WPF will set the Content = DefaultCotent in both cases :

    if (viewModel is null)
    {
        Content = DefaultContent;
        return;
    }
  ...

  if (viewInstance is null)
  {
      Content = DefaultContent;
      this.Log().Warn($"The {nameof(ViewModelViewHost)} could not find a valid view for the view model of type {viewModel.GetType()} and value {viewModel}.");
      return;
  }

My concern is: in a WinForm App, if I dynamically change the ViewContractObservable to a contract that is not registered before (e.g. "Contract_That_Doesnot_Exist"), the above Content = view; within if(view is not null) { ... } won't be executed.

I don't know how to go futher without breaking existing behavoir.

[glennawatson]

Let's just keep winforms as is at the moment. Maybe open a issue with your findings and next major release we can consider changing the functionality. that also allows time for users to see if the new functionality etc.

[newbienewbie]

Maybe open a issue with your findings and next major release we can consider changing the functionality. that also allows time for users to see if the new functionality etc.

I open a discussion and describe the inconsistent behavior of DefaultContent that blocks me.

[codecov]

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (e28392d) 58.31% compared to head (db238ee) 58.44%.

Files	Patch %	Lines
src/ReactiveUI.XamForms/ViewModelViewHost.cs	0.00%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3718      +/-   ##
==========================================
+ Coverage   58.31%   58.44%   +0.12%     
==========================================
  Files         160      160              
  Lines        5842     5847       +5     
  Branches     1031     1031              
==========================================
+ Hits         3407     3417      +10     
+ Misses       2046     2042       -4     
+ Partials      389      388       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.