View locator changes

[olevett]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix

What is the current behavior? (You can also link to an open issue here)
#1299 and #1298 the view locator doesn't correctly resolve a couple of cases, and can't handle oddly named interfaces well.

What is the new behavior (if this is a feature change)?
Handle IRoutableViewModel style VMs, and handle interfaces that don't meet conventions.

Does this PR introduce a breaking change?
It sort of undoes a breaking change introduced in 7.2 with the refactorings around the view locator.

Please check if the PR fulfills these requirements

• The commit follows our guidelines: https://github.com/reactiveui/reactiveui#contribute
• Tests for the changes have been added (for bug fixes / features)

Other information:
This could conceivably be two separate PRs, but I think it's actually clearer to do it in one

[olevett]

The intermediate state is possibly clearer from a review point of view, the big change is adding handling around vm.GetType() for the case that we fall through to ToggleViewModel type (i.e. respect the runtime type)

[kentcb]

I agree it reads better, but I will say that I started with an enumerable in my own implementation, but then removed it to avoid allocs. I haven't double-checked, but I'm pretty sure the previous implementation doesn't alloc (except Type objects, which I believe will be cached by the runtime) whereas this one will alloc with every view resolution.

Do we care? I'm not sure.

[olevett]

That's an interesting point, and quite probably right - I'm happy to not use enumerables, and just do the set of null checks if we're concerned? (The last commit is the refactoring, but shouldn't functionally change things)

I can try and do some benchmarks, not sure how much this will tell us though?

[kentcb]

Yeah, benchmarkdotnet could tell us precisely what's going on in terms of memory allocs. Only problem is, we don't really have the infrastructure in place to run it in the solution as it stands. You could easily add a quick check, but it might be throwaway code until we formally decide how to incorporate perf tests into the solution.

[olevett]

Benchmark results

Host Process Environment Information:
BenchmarkDotNet.Core=v0.9.9.0
OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Xeon(R) CPU E3-1231 v3 3.40GHz, ProcessorCount=8
Frequency=3320309 ticks, Resolution=301.1768 ns, Timer=TSC
CLR=MS.NET 4.0.30319.42000, Arch=32-bit RELEASE
GC=Concurrent Workstation
JitModules=clrjit-v4.7.2046.0

Type=ViewResolutionBenchmark  Mode=Throughput

                Method |    Median |    StdDev | Scaled | Scaled-SD |  Gen 0 | Gen 1 | Gen 2 | Bytes Allocated/Op |
---------------------- |---------- |---------- |------- |---------- |------- |------ |------ |------------------- |
               Default | 5.0061 us | 0.0359 us |   1.00 |      0.00 | 610.00 |     - |     - |             890.47 |
                Pr1311 | 4.9428 us | 0.0277 us |   0.99 |      0.01 | 610.20 |     - |     - |             888.82 |
 EnumerableViewLocator | 5.3197 us | 0.1377 us |   1.06 |      0.03 | 687.45 |     - |     - |           1,000.36 |

Where Pr1311 is (solid naming, I know) the functional changes, without IEnumerable, and the Enumerable test is with wrapping into an IEnumerable.

 public interface IFooViewModel { }

        public class FooViewModel : ReactiveObject, IFooViewModel { }

        public interface IFooView : IViewFor<IFooViewModel> { }

        public class FooView : IFooView
        {
            object IViewFor.ViewModel
            {
                get { return ViewModel; }
                set { ViewModel = (IFooViewModel)value; }
            }
            public IFooViewModel ViewModel { get; set; }
        }

        private readonly ModernDependencyResolver resolver = new ModernDependencyResolver();
        private readonly IDisposable resolverDisposable;
        private readonly DefaultViewLocator defaultViewLocator = new DefaultViewLocator();
        private readonly PR1311ViewLocator pr1311ViewLocator = new PR1311ViewLocator();
        private readonly EnumerableViewLocator enumerableViewLocator = new EnumerableViewLocator();
        private readonly FooViewModel viewModel = new FooViewModel();

        public ViewResolutionBenchmark()
        {
            resolver.InitializeSplat();
            resolver.InitializeReactiveUI();
            resolver.Register(() => new FooView(), typeof(IViewFor<FooViewModel>));
            resolverDisposable = resolver.WithResolver();
        }

        [Benchmark(Baseline = true)]
        public object Default() => defaultViewLocator.ResolveView(viewModel);

        [Benchmark]
        public object Pr1311() => pr1311ViewLocator.ResolveView(viewModel);

        [Benchmark]
        public object EnumerableViewLocator() => enumerableViewLocator.ResolveView(viewModel);

        public void Dispose()
        {
            resolverDisposable.Dispose();
        }

We're making more allocations (10%) but I think it'll depend on route through, I can do more benchmarks if wanted, but on the basis of this I'm happy to not do the Enumerable refactoring.

[kentcb]

Thanks for this @olevett. Yeah, agreed, we should avoid alloc'ing where possible, especially at such a low level of the framework. Every little bit helps and all. I know we haven't been particularly vigilant about this kind of thing elsewhere, but it might be time to tighten up. This will be easier to manage once we formalise the inclusion of Benchmarkdotnet into the tests.

[olevett]

Pushed without the IEnumerable now

[olevett]

I think this refactoring makes it a little clearer what we're actually doing, happy to revert it though.

[olevett]

This short circuit handles #1298 we could instead handle the null better out of ToggleViewModelType, but I think it's a reasonable thing to have.

[coveralls]

Coverage increased (+0.02%) to 66.803% when pulling c5ed68d on olevett:view-locator-changes into f766ddd on reactiveui:develop.

[marcpiechura]

Maybe it would be a good idea to split it into two parts, ViewTypeLocator and ViewLocator.
One idea I have is to use a ordinary ContentControl + TemplaceSelector to create a template for every unique view model and that is currently not possible with the ViewLocator since it always returns a instance of the view while I only need the type.

[olevett]

@Silv3rcircl3 I quite like that as an idea, might be worth raising as a separate ticket, in the spirit of smaller changes.

[marcpiechura]

@olevett cool, I'll send a PR once this one was merged

[kentcb]

Do the API docs need to be updated inline with the functional changes?

[olevett]

Had a quick double check, and have made the only needed change (I think the docs included more functionality than the code did!)

[coveralls]

Changes Unknown when pulling a97270b on olevett:view-locator-changes into ** on reactiveui:develop**.

[coveralls]

Changes Unknown when pulling 34505a9 on olevett:view-locator-changes into ** on reactiveui:develop**.

[kentcb]

Thanks @olevett - much appreciated!