ConventionManager.ApplyItemTemplate - doesn't apply template if non-generic collection

Topics: Conventions
Feb 24, 2013 at 7:52 AM
Edited Feb 26, 2013 at 4:17 PM
In ConventionManager.ApplyItemTemplate the property type of the items collection is checked before applying the DefaultItemTemplate: if it is not a generic type, the method returns. This seems too strict, since it excludes ICollectionView from receiving conventions.

To be sure, if the property type is generic the type parameter is checked to ensure that we can handle ValueType and String without trying to apply the default template, since the default WPF template is the best convention in this case.

I propose the following change, to get the benefit of using ICollectionView in scenarios that benefit from it: move the generic type check into a conditional that gates the property type check.

Currently:
// ConventionManager.ApplyItemTemplate, ConventionManager.cs:438

if (!property.PropertyType.IsGenericType)
    return;

var itemType = property.PropertyType.GetGenericArguments().First();
if (itemType.IsValueType || typeof(string).IsAssignableFrom(itemType))
{
    return;
}
Proposed:
if (property.PropertyType.IsGenericType)
{
    var itemType = property.PropertyType.GetGenericArguments().First();
    if (itemType.IsValueType || typeof(string).IsAssignableFrom(itemType))
    {
        return;
    }
}
This change seems to preserve the original intent to use a sensible convention for String and ValueType instances while allowing non-generic collections, such as ICollectionView, to participate in convention application.

I'm happy to issue a pull request if this is an acceptable change.
Feb 26, 2013 at 2:09 PM
Conventions for ICollectionView would be nice ;-)
Please ensure that there will not be any side effects from your change.

PS: In ConductorWithCollectionOneActiveTests.cs are tests for some issues you reported, I think. Any news about them?
Feb 26, 2013 at 4:14 PM
Edited Feb 26, 2013 at 5:10 PM
Please ensure that there will not be any side effects from your change.
I can't foresee any, and I'm using the above change in production for a complex app which is based on CM. That said, the reason I posted this is because it's not clear the motivation to exit the convention application so quickly - perhaps I'm missing a key scenario and I just don't know about it because I don't use what it supports. In fact, I am considering another implementation which keeps the early bailout and just adds a call to some replaceable Func<Boolean> which checks if we want to apply conventions to non-generic collections. But the problem with that is that's another configuration point, making it harder to use CM - definitely not a desirable direction.

__
Any news about them?
I started some discussion about them (http://caliburnmicro.codeplex.com/discussions/276374 and http://caliburnmicro.codeplex.com/discussions/276373) but they didn't replies. Since it's mostly just trying to restrict the valid state of the types, there's not a great deal of urgency, since those who benefit from these kinds of clear restrictions are new users. It's just a nice hallmark of mature libraries to clearly guard valid state, so I wanted to document the laxity of the current code if someone gets around to enforcing validity.
Feb 26, 2013 at 5:34 PM
I went ahead and submitted a pull req, since it should make it easier for you to evaluate this change. See: http://caliburnmicro.codeplex.com/SourceControl/network/forks/codekaizen/caliburnmicro/contribution/4143
Mar 12, 2013 at 11:40 PM
Hey, @tibel -

Just checking to see if you have any thoughts on the pull request. I'm happy to give you any prep before you go into it, so you can get an idea of what it represents as painlessly and quickly as possible.
Mar 19, 2013 at 10:15 PM
Just wanted to say using your fork and not having any issues. I tried to merge it into the latest HEAD myself (1.5.0) but there were too many conflicts for me to deal with.
Mar 19, 2013 at 10:17 PM
Thanks for the heads up - I'll try a pull/rebase on the fork and push it. I'll check in here when I've got that done.
Mar 19, 2013 at 10:42 PM
Edited Mar 19, 2013 at 10:43 PM
Actually (and this may just be from lack of experience) I am having trouble getting an ICollectionView bound by convention to stay in sync when a Filter predicate is applied. If I manually call its Refresh method or re-apply the Filter then the bound ListViews update correctly, but it seems like this should not be necessary. The backing collection is an ObservableCollection<T> and its view is a ListCollectionView. Have you played with Filters on your collection views and seen the same thing?
Mar 19, 2013 at 10:45 PM
That does sound odd, but let me check I understand: you make a change to the underlying IObservableCollection and the ICollectionView instance doesn't change when the Filter property is set?
Mar 19, 2013 at 11:07 PM
I'll need to do some more testing on my end, I think it's just my design at fault. I don't want to derail your thread with my (probably overcomplicated) design details.

If it works for you then it's probably just me :)
Mar 19, 2013 at 11:19 PM
Ok, I rebased and pushed to my fork. All 1.5 changesets should now be available, as well as the additional fork changesets.