WPF Themes - Ensuring Hot Keys Display properly

In going through the themes in the WPF Themes project on CodePlex I've encountered many inconsistencies.  Sometimes these inconsistencies are annoying and other times they require much more complicated fixes.  All these issues are instructive because they teach us things to be aware of when building our own themes or even our own templates.


I've written about some of the more complex issues I've encountered in the past.  Today I'm just going to talk about one of the more simple problems.  Some themes in the WPF Themes do not properly display the access keys.  The underscore used to indicate the key was being displayed instead of removed.  This is a simple problem to fix.  ContentPresenters have a property RecognizesAccessKey.  If this property is not set, it defaults to false and the access keys will not work.  Writing RecognizesAccessKey="True" will make the access keys work properly.


I would have preferred the default for this property to true.  In the general case I want the access keys to be active.  An even better solution would be if there was a global way to specify the default for it.  Using access keys is usually what you want to be application wide and it'd eliminate little presentation bugs to if this was possible. Its easy to forget to set this property on a ContentPresenter which can manifest in hard to find locations.  Its an easy problem to fix, but annoying when you discover it.

Neat Way to Write a Read-Only Extendable List

The other day one of my coworkers wanted to to do the following:

• Have a base class that has a property that returns a list
• The list should be immutable
• The list will contain a list of items to exclude elsewhere in the code
• Sub-classes should be able to add items to the list but not remove them

One way to implement this would be something like the following:


public class Base
{
    private  IList<string> _excludeList = (new List<string>(){ "a", "b", "c" }).AsReadOnly();
    public virtual IList<string> ExcludeList
    {
        get { return _excludeList; }
    }
}


public class Subclass : Base
{


    public override IList<string> ExcludeList
    {
        get
        {
            List<string> excludeList = new List<string>(base.ExcludeList);
            excludeList.Add("d");
            return excludeList.AsReadOnly();
        }
    }
}


My inclination was that this might be inefficient because of the  repeated creation of the list.  Let's examine a couple pieces of this code and see if we can clean it up.  The first is we use a list because the framing of the question lead us there.  We just want to be able to iterate over the list, how we accomplish that is unimportant, therefore List is not the type we want.  There really is no reason that this needs to be something other than an IEnumerable.  If we return an IEnumerable rather than a list we can use the yield keyword to do some fancy rewriting.  We also are no longer creating new collections every time we call the subclass version:



public class Base
{
     private IList<string> _excludeList = (new List<string>() { "a", "b", "c" }).AsReadOnly();
     public virtual IEnumerable<string> ExcludeList
     {
         get { return _excludeList; }
     }
}


public class Subclass : Base
{


    public override IEnumerable<string> ExcludeList
    {
        get
        {
            foreach (string s in base.ExcludeList)
            {
                yield return s;
            }


            yield return "d";
        }
    }
}


We now are avoiding creation of new lists and the way it works is fairly clean.  The unfortunate part of this is that its not very efficient in the large case.  In talking about this I originally thought this would be a more efficient way of doing meeting our requirements because there would be fewer lists being created.  My quick performance testing showed quite the opposite.  In the small cases the difference in performance is negligible; I could not get a consistent result of one being quicker than the other.  In the case where I make the initial list "a" - "z" and the added items "aa" - "mm" the performance difference was significant.  The rewritten version took about twice as long on average.  I did not test a small initial list and a long additional list nor did I test a long initial list and a short additional list.  The test case I ran proved to me that my theory about it being more efficient was wrong so I saw no reason to continue.


Since the performance of this method turns out to be poor this remains nothing more than an interesting way to use the yield operator.

The Art of Commenting Code

When I first started programming I was horrible with comments.  I honestly didn't write them because I didn't see the point.  The reason I did not do it was because I was never forced to maintain any of my code.  In school we often wrote little programs that were a couple hundred lines long that was discarded after each assignment.  Despite my teacher's best efforts it never struck me as to why comments are necessary.  I would comment my code when forced to by my teacher.  This lead to inane commenting like the following:

//Assign 5 to foo

int foo = 5;

If anything my commenting here has hurt the readability of the code because it clutters it with needless information.  If you know the language, you know what an assignment statement looks like, making the comment unnecessary.  It was only once I started working on an J2EE application with approximately 200,000 lines of code that I understood why you comment.  After much thinking about it I've come to the conclusion that you comment to justify the code.  What I mean is if someone reading your code was to ask "Why does this code exist?" the comment should provide the answer.  My comments tend to be long winded compared to most other developers I've encountered, because of this approach.  I often make statements about the origin or the history of a piece of code to give context.  I also believe in making the comment conversational or tell a story.  This makes it both easier to read, more fun to read, and simplifies the communication.  I'll write something like the following:

//The item has to be added to the collection before the call to the next function

//because the control throws an exception if the item is not in the collection

//yet.  It'd be better if we could do it later on but we are limited by the existing

//structure.  If we refactor the control to use WPF we should revisit this.

This of course is completely contrived and doesn't relate to any real code.  I am trying to demonstrate what I believe is a good commenting style. I am doing the following things:

• I justify the positioning of the code and why it exists there.  
• This lets a future developer (or which might be me) know why the code is set-up as it is and that the order has a logic to it and what that is
• It also prevents a developer who is refactoring the code from unwittingly creating an error.  I hate when I move something that appears to have no effect on the logic but then a strange edge case appears that causes an error.  Even worse is when you talk to the original developer and they knew about the error potential but did not document it
• I also acknowledge any short comings.  
• This is partly me protecting myself from future developers being to angry at me.  I cannot count the number of times I've read someone's code and just questioned their sanity for coding something in a certain manner.  There might have been logical justification for doing it that way and if I understood that I probably would accept it.  I still may dislike the code but at least I now have context
• This also provides information to someone who want to improve the code in the future.  At some point in the future someone will be changing that code.  There may be a way to perform the operation in a better manner.  If that person may be less experienced with the code base or less experienced in general they may just assume that this is a good piece of code.
• It raises a do not copy and paste this flag for people.  Nothing is worse than seeing a poor piece of code copy and pasted.  Inexperienced developers make this mistake often.  They know a piece of code works but they don't understand it.  They therefore copy, paste it, and never examine it closely.  A comment that acknowledges short comings encourages a deeper level of thinking.
• Provide an upgrade path
• If there is some point in the future that the code be changed to remove the short comings and I know what that point is I mention it.  I don't necessarily think that those short comings will be removed right away when that upgrade point change occurs but it provides a reference point to make the change.

Its also important to remember that comments are how future developers will be able to understand what the code does and why it does it.  Reading comments is one way you can perform archeology on code to understand it.  You therefore should leave behind artifacts for the future archeologists.  Code  doesn't exist in a vacuum.  We use many other tools like requirements and defect tracking systems to help us manage development.  We stage implementation in to releases.  We have meetings to discuss functionality.  We write using architectures and designs.  All of these are important to the code, but rarely are they referenced in a meaningful way.  Providing well placed linage to the outside materials can provide a wealth of information with the minimal actual writing.


It is impossible to link every piece of code to meaningful external references, but sometimes providing that link is vital.  The easiest link to provide is to requirements management systems and defect tracking systems that give unique identifiers that can be easily referenced.  If a change for a requirement or a defect is fairly esoteric and isolated to a small code section  I will often include the ID of the requirement or the ID in the comments for the code.  Sure, a tool like subversion can be used to gain the same information, but that requires an additional step that most people will not undertake.  It also gives me traceability back to the defect tracking system, which occasionally comes in handy on its own.  Defects and requirements are not the only links that can be put in comments.  If you keep meeting minutes or a record of conversations you can provide a link by putting in the date of the conversation that was the basis for the change.  Architecture and design documents can be referenced in similar manners.  Instead of trying to reexplain in detail why something works the way it does, reference the existing documentation.  If a change significantly changes behavior starting at a certain version number, it doesn't hurt to mention that with that version number the functionality has changed.


You can go overboard with this to the point that it becomes annoying.  Just like the first example of commenting an assignment statement, you don't want to make it so the comments lose meaning.  For example I don't link to a defect number if the defect occurs because of a null reference that is avoided by a simple null check.  Generally, I add the link and level of detail if

•  The change required significant archeology on my part
•  If a fix for a defect was non-intuitive
•  It seems likely that similar changes will need to be made in the future
• The way something was functioned was specifically prescribed by a meeting or a document

These are not hard and fast rules to follow, but more guidelines.  I do not use the previous list as a checklist.  In fact I had to reverse engineer when I actually do add links to outside information.  It all comes down to a feel for the code and when its useful.  You do not want the linkage to become pedantic, but want to provide others and yourself tools for the future.  After all, you are as likely to be helping yourself as you are to helping others. Numerous times I have had to return to a piece of code I wrote and had to perform archeology on it because I couldn't remember writing it.  We always feel we will remember the code we write.  For the most part that just can't be true because of the sheer volume of code we write.  It will save you time an energy in the long run to write good comments.  Nobody really debates the merits of comments, so I won't continue down that line of thought. I do think the quality of comments is extremely important.  I hope I've given you some thoughts on how to write good comments.

WPF Gird Causing Memory Leak

There is a fundamental problem in WPF that can cause a memory leak in applications using it.  The problem is that a ColumnCollection in a Grid can become pinned in memory. It is not in all cases but in a specific case in the application I work on.  I am not 100% about what in the structure caused it but there are 3 separate potential contributing culprits that I've identified.  Without extensive testing of the different scenarios I won't be able to identify exactly what the cause was.  The three potential contributing culprits are:

1.     User control inside a DataTemplate with a Grid.  It does not matter how deep in the control the grid was in the control all Grids were getting caught

2.     Binding column widths from a parent grid to an inner grid through a border control.  This was done to emulate a grid like behavior but allow for multiple control types within the grid

3.     Binding and unbinding the collection view source every time the source of the observable collection changed. 

In trying to fix this problem it seemed as if it was related to the DataTemplate but it is unclear exactly what caused the issue. 

The actual leak came from the pinning of the Grid.ColumnCollection.  The pinning of that caused all the associated control to be pinned as well.  The Grid.RowCollection did not seem to have the same issue.  This was still an issue even if there were no columns defined.  It also occurred if a Grid was used inside a control from the same assembly.  If the Grid was anywhere inside a user defined control in the same assembly, at any level, the issue would occur, even if it was in another user control.  Crossing the assembly boundary appears to make this not occur. 

To fix the issue I replaced the Grids in the affected controls.  The grids were replaced with a combination of StackPanels and DockPanels which can be used to replicate the grid.  In this case they were actually more appropriate than using a Grid anyway. 

The lesson from this appears to be that the use of a Grid over other LayoutPanel types should be carefully considered.  If memory leaks start to appear with the Grid it needs to be changed to another Layout type.

.Net Framework Bug With Sorting

I came across a bug in the .Net Framework the other day.

When specifying SortDescriptions on a CollectionViewSource there is a problem when using a complex path for the property name. This occurs when part of the path being checked is null. If I have the classes defined below

class Foo{
string Name {get; set;}

}

class Bar{
Foo FooMember {get; set;}
}

If there is a CollectionViewSource that contains a list of Bar's and it is desired to sort each bar by Foo's name a SortDescription can be created as

SortDescription d = new SortDescription("FooMember.Name", ListSortDirection.Ascending);

If that SortDescription is added to the sort descriptions of the CollectionViewSoure it will properly order the items with one exception. This one exception is if there is an instance of Bar in the collection that the CollectionViewSource uses as the source. In this case an ArgumentException stating that there is a type mismatch and the element must be a string occurs.

The reason this occurs is that in the Compare method of the SoftFieldComparer obtains a strange object when its accessing the instance of Bar that has a null FooMember. Rather than just being null, its an MS.Internal.NamedObject with a _name value of "DependencyProperty.UnsetValue". This is a problem because it then tries to do a comparison betwen the string and the NamedObject which cannot be compared, which causes the ArgumentException. This case needs to be handled to be able to sort items with null members.

I have submitted this as an issue to Microsoft https://connect.microsoft.com/VisualStudio/feedback/details/539559/sortfieldcomparer-compare-method-can-throw-an-exception

ContentPresenter, GridViewRowPresenter, and ListViewItems

There are two distinct ways that content can be presented within control templates.  These are the GridViewRowPresenter and the ContentPresenter.  The only place that the GridViewRowPresenter is used is within a GridView to present the cell of data.  This is needed for the binding path to be evaluated properly.  If for example I have the class Foo defined below:

public class Foo{
    public string X {get; set;}
    public string Y {get; set;}
}

If I want to use a ListView presented using a GridView inside it to present the values of both X and Y as columns you would write the following assuming the FooCollectionView is a collection view source or some other collection of Foo's


<ListView ItemsSource={Binding FooCollectionView}">
  <ListView.View>
    <GridView >
      <GridViewColumn Header="X" DisplayMemberBinding="{Binding Path=X}" />
      <GridViewColumn Header="Y" DisplayMemberBinding="{Binding Path=Age}" />
    </GridView>
  </ListView.View>
</ListView>
  
If you want to define a style or control template that applies to this control instead of the normal location, where you'd put a ContentPresenter in the template you put a GridViewRowPresenter.  What happens if you use a ContentPresenter instead?  You get the content presented but its not in a grid and its just the ToString() of the object.  This obviously is not what you want to occur.

Therefore, it seems fairly obvious that you should use a GridViewRowPresenter.  It however, is not that straightforward because what if you want to not use a GridView and instead just want to list them (the same as would be in a ListBox) so you have something like the following?

<ListView ItemsSource={Binding FooCollectionView}">

What happens when you define the style with GridViewRowPresenter to present the content?  The ListView will appear to have no content because there is no GridView to present the content of.  This does not present a huge problem if you are defining a style on a per ListView instance.  However, it is a problem if you want to define a general style like you would in a theme.  This presents a big problem because either you cannot use a GridView or must always use a GridView.  You can get around this by using a ListBox anywhere you don't want to use a GridView.  This strategy would mean letting the style dictate the form of the application which shouldn't be the case.

However, the real problem occurs when defining a reusable theme.  If you want to define a general purpose theme that others can reuse without having to alter thier application such as in the WPFThems project then you need it be able to handle both cases.  On a side-note some of the themes in WPFThemes use the ContentPresenter and some use the GridRowViewPresenter.  This can make it so that the ListBox is shown properly with some themes applied and improperly with others.

The fix to this is a bit of a hack but ultimatley turns out to work.  It basically involves defining both presenters inside the control template.  So where you put the presenters you put code that looks like the following:


  <GridViewRowPresenter x:Name="gridrowPresenter"
                        Content="{TemplateBinding Property=ContentControl.Content}"/>
  <ContentPresenter x:Name="contentPresenter"
                    Content="{TemplateBinding Property=ContentControl.Content}"  Visibility="Collapsed"/>


 To get the content to correctly display doing this.  For the template the following trigger will need to be added:

   <Trigger Property="GridView.ColumnCollection" Value="{x:Null}">
     <Setter TargetName="contentPresenter" Property="Visibility" Value="Visible"/>
  </Trigger>     

This trigger will show the ContentPresenter when the GridViewRowPresenter has no content.  Since there is no GridView the GridViewRowPresenter will not display anything visually.

Obviously, this is a bit of a hack to get around a flaw with how WPF works.  Hopefully, in a future version this will be addressed in how the framework works.

Edit 11/16/10 Added  Visibility="Collapsed" property to the contentPresenter element which was a bug in the implementation

SourceName In MultiTriggers in WPF Themes

There is a bug in the WPF code that can cause a null pointer exception.  The null pointer exception occurs on line 5924 of System.Windows.StyleHelper.cs :

object evaluationValue = evaluationNode.GetValue( conditions[i].Property );

The problem has to do when you are defining a MultiTrigger on an element in a theme.  This bug manifested itself when on our project we were refreshing a collection that was bound to a TreeViewer and the TreeViewItem style had a MultiTrigger in it.  The style in question comes from the WPFThemes project and was the ExpressionDark Theme (the problem occurs in about 7 of the other themes in the project). The one Condition on the MultiTrigger in the theme has a SourceName defined for it.  The SourceName was not needed because it was the element right inside the root element anyway.  This bug was especially subtle because it would only manifest itself after other conditions would have happened that would cause the MultiTrigger to fire.

 I suspect, but have not verified, that the precondition for causing this bug is having a currently selected TreeViewItem that is removed upon refreshing the control and a newly generated item in the refresh was being set as the selected item in its stead.  The trigger was checking the condition but the new item hadn't fully been generated so when it was looking for the child element it didn't yet exist.  This makes sense in context  of the code in the StyleHelper class.  The code that returns the element using the GetChild method defined on line 6388.  The comment inside the method says:


 // Notice that if we are requesting a childIndex that hasn't been
 // instantiated yet we return null. This could happen when we are 
 // invalidating the dependents for a property on a TemplateNode and
 // the dependent properties are meant to be on template nodes that
 // haven't been instantiated yet.


This method was returning null because


if (styledChildren == null || childIndex > styledChildren.Count)
was evaluating to true because styledChilderen was null.

The null pointer exception was occurring because line 5924


object evaluationValue = evaluationNode.GetValue( conditions[i].Property );



was assuming evaluationNode as not null.

It was assuming it could do this because of the Debug.Assert on line 5903:


 Debug.Assert(evaluationNode != null, 
                    "Couldn't find the node corresponding to the ID and name given in the trigger.  This should have been caught somewhere upstream, like StyleHelper.SealTemplate()." );


This brings up a point about using Debug.Asserts in code.  People often use them (I see it on my project often) when they don't want to perform the null check overhead or other conditions that they feel should never happen.  The idea is that you should catch any of these cases in testing.  Obviously this is a case that was over looked in Microsoft's testing.  The method that set the value of evaluationNode before the Assert has a perfectly legitimate reason for returning null (see the comments earlier) then its not reasonable to use the Assert here.  More on a future post about using Asserts versus null checks.

The work around for this bug was to remove the SourceName from the MultiTrigger.  This does not change the style in a meaningful way and removes the null pointer exception