DittoView: Enhancement to support unwrapping & downcasting view-models

[leekelleher]

@mattbrailsford - I'd found an scenario a while back where if a DittoView<T> page referenced another DittoView<T> page, but the view-model was castable, the code wouldn't cast it and Ditto would recreate the object.

That might sound like a mouthful, but I'll try to explain...

Say you've got these POCOs...

public class MasterModel
{
  public string Something { get; set; }
}

public class OtherModel : MasterModel
{
  public string Whatever { get; set; }
}

If you had DittoView<OtherModel> on a page template, and DittoView<MasterModel> on the layout template, the DittoView code would be creating the object twice, rather than downcasting it to the inherited type.

This PR adds in a check to see if the model object is a DittoViewModel<> and to unwrap the inner view-model, so it can be reused.

Hopefully this makes sense? (and is backwards compatible / a non-breaking change)

[mattbrailsford]

Just a couple of comments

[mattbrailsford]

I think there needs to be some checks here to ensure it's actually castable to expected view model type before assigning it

[leekelleher]

Interestingly, I had included an IsAssignableFrom check in there, but then thought that the as casting further down would handle that...

var typedModel = model as TViewModel;

as it would cast as null if it wasn't castable, (which is how it currently works).

[mattbrailsford]

Ahh good point, I didn't look past the change. Happy to leave as is then.

[mattbrailsford]

Not saying this is bad, but whats the reason for this addition? When would this be different? (can't really recall)

[leekelleher]

Yeah, I couldn't think of how they'd be different. I guess it's just because they were available on the BaseDittoViewModel?
I'm happy to remove that part.

[mattbrailsford]

If it passes all the tests, I'm not against it as it probably does make sense. I was just querying the change. I can't think it through.

[leekelleher]

I wasn't overly happy with the ?? null-coalescing operator here. I'm airing on the side of removing those 2 lines now.

[mattbrailsford]

I dunno. I can't think of a time they would be different, but I do think reusing them make sense given we will be in an inner context by then. I say leave it.

[leekelleher]

@mattbrailsford - re: unit-test - I couldn't figure out how to unit-test the DittoView class - guessing you hit the same problem (since there aren't any existing tests for it)?

[leekelleher]

Merging in. We can look at the unit-test later. Thanks for the quick turnaround @mattbrailsford #h5yr

[mattbrailsford]

Yea, that's when it starts to get confusing to me as your start bringing in web contexts etc