A place for spare thoughts

23/08/2011

Checking correctness of DependencyProperty declarations

Filed under: wpf — Ivan Danilov @ 19:44

DependencyProperty declaration is a fairly standard action and it is often performed with some kind of templates or snippets. Name in DependencyProperty.Register call should match property name itself, some methods should be declared, it should be most times static and readonly etc. Also if you declare read-only DP – you want Key to be non-public and DP itself to be public (otherwise why to declare it as read-only in first place?)

So, simple rule to check some of these things is below. It is almost trivial to plug it in SourceChecker I described before (actually we have this rule there).

public class AttachedPropertiesNamedCorrectlyRule
{
    private static readonly Regex PropertyPattern = new Regex(
        @"(?public\s+)?(?(static\s+)?(readonly\s+)?(static\s+)?)" + // match any order/presence of modifiers
        @"DependencyProperty\s+(?\w+)\s*" + // propname is identifier name, should be XxxProperty
        @"(" + // match registration assignment, assignment from property key or just ';' here
        @"   (?" +
        @"      =\s*DependencyProperty\.Register(Attached)?\s*\(\s*" + // match attached properties also
        @"      ""(?[^""]+)""" +
        @"   )" +
        @"|" +
        @"   (?" +
        @"      =\s*(?\w+)\.DependencyProperty\s*;" +
        @"   )" +
        @"|" +
        @"   (?;)" +
        @")",
        RegexOptions.IgnorePatternWhitespace | RegexOptions.Compiled | RegexOptions.ExplicitCapture);

    private static readonly Regex PropertyKeyPattern = new Regex(
        @"(?public\s+)?(?(static\s+)?(readonly\s+)?(static\s+)?)" + // match any order/presence of modifiers
        @"DependencyPropertyKey\s+(?\w+)\s*" + // propname is identifier name, should be XxxProperty
        @"(" + // match assignment or just ';' here
        @"   (?" +
        @"      =\s*DependencyProperty\.Register(Attached)?ReadOnly\s*\(\s*" + // match attached keys also
        @"      ""(?[^""]+)""" +
        @"   )" +
        @"|" +
        @"   (?;)" +
        @")",
        RegexOptions.IgnorePatternWhitespace | RegexOptions.Compiled | RegexOptions.ExplicitCapture);

    public IEnumerable Check(string cscode)
    {
        var errors = new List<string>();

        CheckDependencyProperties(cscode, errors);
        CheckDependencyPropertyKeys(cscode, errors);

        return errors;
    }

    private static void CheckDependencyProperties(string sourceText, List<string> errors)
    {
        Match matchResults = PropertyPattern.Match(sourceText);
        while (matchResults.Success)
        {
            var propname = matchResults.Groups["propname"].Value;
            var modifiers = matchResults.Groups["modifiers"].Value;
            bool isPublic = matchResults.Groups["publicity"].Success;
            bool noInitializer = matchResults.Groups["noInitializer"].Success;
            bool initializedFromKey = matchResults.Groups["assignmentFromKey"].Success;
            bool isStatic = modifiers.Contains("static");
            bool isReadonly = modifiers.Contains("readonly");

            if (!isPublic)
            {
                // probably some internal usage passed in instance constructor, can't say if it is wrong
                matchResults = matchResults.NextMatch();
                continue;
            }

            if (!isStatic)
                errors.Add(string.Format("DependencyProperty {0} is not marked as static", propname));
            if (!isReadonly)
                errors.Add(string.Format("DependencyProperty {0} is not marked as readonly", propname));

            // Initialization in static constructor should be avoided because of performance impact of static ctor presence
            // See here: http://stackoverflow.com/questions/2921828/static-constructors-cause-a-performance-over-head
            // Also SourceChecker can't check such initialization reliably (for example with partial classes static ctor 
            // could be in other file) and ensure that property name matches string given as parameter
            if (noInitializer)
            {
                errors.Add(string.Format("DependencyProperty {0} is not initialized or initialized in static constructor.", propname));
            }
            else if (initializedFromKey)
            {
                var keyName = matchResults.Groups["keyName"].Value;
                var expectedKeyName = propname + "Key";
                if (keyName != expectedKeyName)
                    errors.Add(string.Format("DependencyProperty {0} either named or initialized incorrectly (key name is '{1}', expected '{2}')", propname, keyName, expectedKeyName));
            }
            else
            {
                var stringPropname = matchResults.Groups["stringPropname"].Value;
                var expected = stringPropname + "Property";
                if (propname != expected)
                    errors.Add(string.Format("DependencyProperty {0} either named or initialized incorrectly (param is '{1}', name should be {2})", propname, stringPropname, expected));
            }

            matchResults = matchResults.NextMatch();
        }
    }

    private static void CheckDependencyPropertyKeys(string sourceText, List<string> errors)
    {
        Match matchResults = PropertyKeyPattern.Match(sourceText);
        while (matchResults.Success)
        {
            var keyname = matchResults.Groups["propname"].Value;
            var modifiers = matchResults.Groups["modifiers"].Value;
            bool isPublic = matchResults.Groups["publicity"].Success;
            bool noInitializer = matchResults.Groups["noInitializer"].Success;
            bool isStatic = modifiers.Contains("static");
            bool isReadonly = modifiers.Contains("readonly");

            if (isPublic)
                errors.Add(string.Format("DependencyPropertyKey {0} is marked as public. Either make it more restricted or consider making property read-write.", keyname));
            if (!isStatic)
                errors.Add(string.Format("DependencyPropertyKey {0} is not marked as static", keyname));
            if (!isReadonly)
                errors.Add(string.Format("DependencyPropertyKey {0} is not marked as readonly", keyname));

            // Initialization in static constructor should be avoided because of performance impact of static ctor presence
            // See here: http://stackoverflow.com/questions/2921828/static-constructors-cause-a-performance-over-head
            // Also SourceChecker can't check such initialization reliably (for example with partial classes static ctor 
            // could be in other file) and ensure that property name matches string given as parameter
            if (noInitializer)
            {
                errors.Add(string.Format("DependencyPropertyKey {0} is not initialized or initialized in static constructor.", keyname));
            }
            else
            {
                var stringPropname = matchResults.Groups["stringPropname"].Value;

                var expected = stringPropname + "PropertyKey";
                if (keyname != expected)
                    errors.Add(string.Format("DependencyPropertyKey {0} either named or initialized incorrectly (param is '{1}', name should be {2})", keyname, stringPropname, expected));
            }

            matchResults = matchResults.NextMatch();
        }
    }
}

10/08/2011

Git-TFS bridge v0.12 released

Filed under: git-tfs — Ivan Danilov @ 16:00

New features in chronological order:

  • git tfs quick-clone -c 12345 option that allows you to clone not latest, but specified changeset. If project in TFS is large but you want to have part of the history – this will be handy;
  • git tfs rcheckin command implemented. See wiki for details. For short it allows you to checkin strainght series of commits from git to TFS. Also it supports simple merge-preserving but with significant limitations;
  • TFS authentication support. Now you can put auth info to repository configuration, pass it from command line, use your default windows user account or enter password each time;
  • Fetch/Pull commands (git-tfs’ ones of course) now checks your local repository up to HEAD to check if some TFS changesets were fetched with normal git fetch from other git repository. Also some minor changes that allows you to have central git repository and go to TFS only one time per changeset while downloading everything else from central repo.
  • git tfs unshelve and git tfs shelve-list commands that are allowing you to work with TFS’ shelves. Note that not everything is perfect there due to TFS crappiness. Refer for details here.
  • autotag option allows you to suppress creation of a tag for each TFS changeset if you don’t need this. To be precise: before 0.12 tag creation was default and non-optionable behavior. Since 0.12 it is optional and turned off by default.

And several bugs fixed, as always, of course.

In my plans there’s TFS’ branch support implementation, making git-tfs faster, and optionally moving to NDesk.Options (aka Mono.Options. NDesk site is obsolete as author mentioned. Still it has useful documentation) and libgit2sharp.

Also it would be great if someone could help us with keeping wiki up-to-date. It is somewhat behind.

P.S. Please note that I consider v0.12 still as release candidate. So if you have problems with it – feel free to use issue tracker or Google Group to let us know. Feedback is always appreciated!

WPF: Using commands with CommandParameter without CommandManager

Filed under: wpf — Ivan Danilov @ 15:33

Almost always business commands are implemented without RoutedCommand usage. There’re good reasons: business commands are not ‘application-wide’ in general. It is precise action that should be done when user clicked a button, tapped a list or something alike. So, common approach is to implement ICommand interface and CommandManager for CanExecuteChanged event implementation like below:

public event EventHandler CanExecuteChanged
{
    add { CommandManager.RequerySuggested += value; }
    remove { CommandManager.RequerySuggested -= value; }
}

Well, this approach has its downsides.

First, CommandManager knows only about the UI. If you have some external source of information, e.g. direct network connection or something alike, – you should be able to tell command ‘Your state changed! Raise your event!’ manually. It can be done with CommandManager.InvalidateRequerySuggested() method, but with updating of all commands in your app! Which sometimes is somewhat slower than required.

CommandManager is leaky in .NET 3.5. In .NET 4.0 some problems were fixed, but still there’s some criticism about it as a design concept. One could introduce memory leak and don’t notice it at all. Especially in the presence of this problem in data binding. I have also some examples that led to long-and-painful debug sessions but unfortunately can’t disclose them for legal reasons. And it would be too much work to rewrite some part just to make a demo…

At last I’m very suspicious about any static global thing that many pieces in the application subscribed to without strict control and review. WeakReference is harder to understand than normal, strong reference; delegate is harder than reference and WeakDelegate is the hardest of all.

But if you implement CanExecuteChanged event in other way – you have a problem. Many classes relies on the fact that you’re using CommandManager. It was stated in the criticism referenced above btw. You want an example? No problem. Lets take probably the most used class in WPF: Button. It assumes that if Binding changed – CommandManager would re-query commands’ statuses without additional actions. And so, if you have CommandParameter bound to some source and command not subscribed to CommandManager – this command will not be re-queried. It seems very natural for me to assume command status could have changed if parameter is changed. Even if parameter is not bound. But command relies on CommandManager. WTF?!

Well, there’s a solution for the problem. Button and other inheritors of ButtonBase doesn’t override property metadata for CommandParameter dependency property. It is our chance… 🙂

Here I present my implementation of the ICommand interface implementation:

public class GenericRelayCommand<T> : GenericRelayCommand
{
    public GenericRelayCommand(Action<T> execute)
        : base(o => execute((T) o))
    {
    }

    public GenericRelayCommand(Action<T> execute, Func<T, bool> canExecute)
        : base(o => execute((T) o), o => canExecute((T) o))
    {
    }
}

public class GenericRelayCommand : ICommand
{
    static GenericRelayCommand()
    {
        ButtonBase.CommandParameterProperty.OverrideMetadata(typeof(Button), new FrameworkPropertyMetadata(null, CommandParameterChangedCallback));
        ButtonBase.CommandParameterProperty.OverrideMetadata(typeof(DataGridColumnHeader), new FrameworkPropertyMetadata(null, CommandParameterChangedCallback));
        ButtonBase.CommandParameterProperty.OverrideMetadata(typeof(DataGridRowHeader), new FrameworkPropertyMetadata(null, CommandParameterChangedCallback));
        ButtonBase.CommandParameterProperty.OverrideMetadata(typeof(CalendarButton), new FrameworkPropertyMetadata(null, CommandParameterChangedCallback));
        ButtonBase.CommandParameterProperty.OverrideMetadata(typeof(CalendarDayButton), new FrameworkPropertyMetadata(null, CommandParameterChangedCallback));
    }

    private static void CommandParameterChangedCallback(DependencyObject d, DependencyPropertyChangedEventArgs e)
    {
        var command = d.GetValue(ButtonBase.CommandProperty) as GenericRelayCommand;
        if (command != null)
            command.RaiseCanExecuteChanged();
    }

    private readonly Func<object, bool> _canExecute;
    private readonly Action<object> _execute;

    public GenericRelayCommand(Action<object> execute)
        : this(execute, null)
    {
    }

    public GenericRelayCommand(Action<object> execute, Func<object, bool> canExecute)
    {
        if (execute == null)
            throw new ArgumentNullException("execute");

        _execute = execute;
        _canExecute = canExecute;
    }

    [DebuggerStepThrough]
    public bool CanExecute(object parameter)
    {
        return _canExecute == null || _canExecute(parameter);
    }

    public void Execute(object parameter)
    {
        _execute(parameter);
    }

    public event EventHandler CanExecuteChanged = delegate { };

    public void RaiseCanExecuteChanged()
    {
        CanExecuteChanged(this, EventArgs.Empty);
    }
}

Some generic syntax sugar is for free. Most interesting lines are in static constructor (lines 18-22) and in changing handler method (lines 25-30).

Note that despite code works, this code is not a recommended approach. Moreover it is recommended to avoid this practice:

Calls to OverrideMetadata must be performed within the static constructors of the type that provides itself as the forType parameter of OverrideMetadata. If you attempt to change metadata once instances of the owner type exist, this will not raise exceptions, but will result in inconsistent behaviors in the property system. Also, metadata can only be overridden once per type. Subsequent attempts to override metadata on the same type will raise an exception.

Nevertheless I’ve decided to stick with this approach now because in .NET 3.5 and .NET 4.0 these properties are not overridden and I hope it will be so in next version. Or (even better) this problem will be fixed.

Maybe it would be better (at least safer for sure) to subclass Button and use my own button with overridden metadata safely… but my feelings are bad about inventing the wheel. Even if it is subclassing the wheel.

Create a free website or blog at WordPress.com.