Fix & Improve ListView's broken ItemClick, ItemClicked behavior, without causing breaking change.

[spiriapbergeron]

Background and motivation

Note: This issue is an "API Change Request" version of issue: 12013.

Much of the contents of this description mirrors the contents of that issue, please the for more/other details. Reading the other ticket will expose you to even more of my incessant verbiage.

THE PROBLEM NEEDING TO BE FIXED

The problem is that the ItemCheck and ItemChecked event handlers are broken when you click on the checkbox of a ListViewItem if you have more than 2 images in your StateImageList.

In fact, ListView behaves very oddly when clicking on the checkbox if you have more than 2 images in the StateImageList:

Obviously, things work fine for the typical case of 2 states (checked, unchecked) with 2 images in StateImageList.

Unchecked (Image0) -> Checked (Image1) : Image changes, ItemCheck & ItemChecked fired.
Unchecked (Image1) -> Checked (Image0) : Image changes, ItemCheck & ItemChecked fired.

But things break if there are more than 2 images in StateImageList. The image changes as expected, but events are not fired.

Unchecked (Image0) -> Checked (Image1) : Image changes, ItemCheck & ItemChecked fired.
Checked (image1) -> Checked (Image2) : Image changes, but events not fired because both states are Checked
Checked (image2) -> Checked (Image3) : Image changes, but events not fired because both states are Checked
Checked (image3) -> Checked (Image4) : Image changes, but events not fired because both states are Checked
Checked (Image4) -> Unchecked(Image0) : Image changes, ItemCheck & ItemChecked fired.

Why would a developer have more than 2 State images anyway?

Well, we certainly all know about TriState checkboxes. Also, TreeViews often have a TriState state:

• "Checked" usually means all children are also checked
• "Unchecked" usually means all children are also unchecked.
• "Square" (as in, Indeterminate), usually means some children checked, some unchecked.

ListView would support this just fine, too, but right now it would be missing an event. To fix this, a developer would need to subclass ListView, and repair the broken WmReflectNotify, not an easy task since that method calls private stuff.

Anyway, the fact that that there is an "Indeterminate" state in the CheckedState implies there should at least be some level of support for it. Also, the actual Win32 control has no such problem of supporting multi states, and happily handles many states. In fact, the state ID is simply the ImageIndex in the StateImageList.

This is a super useful feature. For example, it would certainly be used by a Potato app, to represent and cycle through the 200 different kinds of potatoes.

Obviously, when we have such a 200-option multi-state, it's more appropriate to talk about a "User-Defined" state, not a "True-False" state. Multi states are much better represented by an int rather than an enum with just 3 possible values (Unchecked, Checked, Indeterminate).

Therefore, I believe it's required to fix this problem in the general form (support the multi-state case) rather than the sad "lets-not-support-anything-other-than-true-or-false", or even the "let's-support-true-false-and-one-indererminate"

It would have been easy to initially support the general case and fire an even every time the image changes/cycles, and to have the current/new values be defined as "int" rather than the CheckedState enum in the ItemCheckEventArgs, and ItemCheckedEventArgs. Sadly, the enum is what these handlers receive, and we must avoid breaking changes, so we must keep them.... sort-of.

OPTIONS TO "FIX" THE BUGGY BEHAVIOR OF EVENTS NOT FIRING BUT IMAGE STILL CHANGING.

LET'S START WITH BAD OPTIONS FIRST:

Option 1: Force checkbox values to go from checked<-->unchecked, and force image to only cycle through checked/unchecked.

After all, it might have been the original developer's intent to forcefully limit the checked states to checked/unchecked. This idea is reinforced by the comment in the code (WmReflectNotify)

// Because the state image mask is 1-based, a value of 1 means unchecked, anything else means checked. We convert this to the more standard 0 or 1

Unfortunately, this developer didn't test his code. If he did, he would have seen that he could cycle through many images but that his events didn't get fired when getting to the Indeterminate state.

The rationale for fixing it like this is simply that the handlers only receive a Unchecked or Checked value anway, never even an Indeterminate state. All you'd have to do to fix this is to also force the ImageIndex to cycle through 0-1 only, and to not change to anything beyond that, even if there are multiple images in the StateImageList.

I do not recommend this solution, because while "forcing the handlers into a 0/1 world" might have been the developer's initial intent, the fact is that what's in the wild since at last 2007 is that the images will cycle when clicking on the checkbox area. Thus, it would certainly be seen as a breaking change, including the Potato app: its users could no longer cycle through each of the possible 200 kinds of potatoes, and instead be limited to Russets and Yams, a situation that is unlikely to please Ratte and RedGold potato farmers (probably all 198 others, in fact).

Option 2: Same as Option1, but at least add correct behavior for the "Indeterminate" CheckedState enum.

What I mean by this option 2 is to add support for Image number 2, aka the indeterminate state.

That is also not recommended, because while this time you would also get the Yellow Potato farmers on your side, it still does nothing for the 197 other potato farmers who are still getting the shaft. Doing this is likely to cause anger, because come-on.

The most dangerous situation here is that Microsoft could easily be accused of playing favorites among potato farmers, because although Bill Gates is clearly a Yellow Potato kinda guy and nobody could be blamed for at least adding support for Yellow potatoes, the fact is that it's still a slam dunk of a case for artificially limiting potato consumer choice. We all realize Microsoft is like the Monsanto of operating system makers, but please please Microsoft, please don't also be the Monsanto of potatoes. You're not even in that space... Yet... And I don't want a Microsoft logo on my potatoes. (god I hope I didn't give them any ideas)

NOW LET'S LOOK AT BETTER OPTIONS:

SOMEWHAT BETTER

Option 3: Always fire events, even when the state goes from one Indeterminate state to another Indeterminate state.

This would not require an API change, but rather, simply requires the removal of the "if" statement that verifies if the before-and-after states are equal, and if so, don't fire the event.

The "solution" would be to just fire the event, always, and if they are greater than "Checked", at least make the value "Indeterminate". Currently, events aren't fired because they go from "Checked(1)" to "Checked(2)", and they remain "Checked" for anything >= Checked.

Removing the "if statement" and support sending "Indeterminate" values would be a good solution (the developer just needs to look at the ImageIndex to know which state he's actually at), if it wasn't for the fact that in the ItemCheck handler, the developer can override the NewValue to prevent a change.

For example:

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
e.NewValue = CheckedState.Unchecked;
}

Now, what happens if the developer wants to override the state? He can't simply write to e.Item.ImageIndex = 30; because that would trigger another ItemCheck event.

So if we want to support overriding the new value to some custom state specified by an index, we need to improve on this solution.

AH, BETTER

Option 4 Augment ItemCheckEventArgs, and ItemCheckedEventArgs with int properties.

class ItemCheckEventArgs
{
CheckedState CurrentValue, NewValue; // existing
int CurrentIntValue, NewIntValue; // added, correspond to the ImageIndex in StateImageList.
}

class ItemCheckedEventArgs
{
CheckedState Value; // existing
int IntValue; // added, corresponds to the ImageIndex in StateImageList.
}

With the above in place, the ListView would now property call the ItemCheck handlers like so:

Unchecked(0) -> Checked(1): Image changes like before, event fired like before.
Checked(1) -> Indeterminate(2): Image changes, event fired (unlike before)
Indeterminate(2) -> Indeterminate(3): Image changes, event fired (unlike before)
Indeterminate(3) -> Indeterminate(4): Image changes, event fired (unlike before)
Indeterminate(4) -> Indeterminate(5): Image changes, event fired (unlike before)
Indeterminate(5) -> Unchecked(0): Image changes, event fired like before

In the previous option, I alluded to the developer wanting to override the new value in the ItemCheck handler.

.void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
e.NewValue = CheckedState.Unchecked;
}

now he could do:

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
if (e.NewValue == CheckedState.Checked)
e.NewIntValue = 5; // force image state to be 5, always.
}

But with your keen eyes, you've noticed the inconsistency in the above code.

For starters, you have a e.NewIntValue which is 5, and e.NewValue which is Checked.

Upon return from the handler, what shall the caller do? Use e.NewValue? or e.NewIntValue?

Well, we could remove the ambiguity by instructing the caller to use either e.NewValue, or e.NewIntValue, by setting e.NewValue to CheckedState.Indeterminate if we want to use e.NewIntValue.

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
if (e.NewValue == CheckedState.Checked)
{
e.NewValue = CheckedState.Indeterminate; // enforce this to tell caller to use e.NewIntValue
e.NewIntValue = 5; // force image state to be 5, always.
}

 // overriding this way would also work, because even though IntValue == 0 means "unchecked"
 // the caller would use e.NewIntValue as the field to use upon returning from the event handler call.
 e.NewValue = CheckedState.Indeterminate; // enforce this to tell caller to use e.NewIntValue
 e.NewIntValue = 0; // force image state to be 5, always.

}

This would not affect old code. After all, old code could simply override by setting to Checked or Unchecked.

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
e.NewValue = CheckedState.Unchecked; // this is fine. Caller uses imageIndex == 0
e.NewValue == CheckedState.Checked; // this is fine. Caller uses ImageIndex == 1
}

There is a risk, albeit a small one, that the developer did this:
void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
e.NewValue = CheckedState.Indeterminate; // Developer wrote this, yuk.
}

With the old implementation, this simply means ImageIndex = 2

With the new implementation, this changes. It means ImageIndex = whatever_is_in_NewIntValue

Now that we'd fire all events, there is a potential bug IF there StateImageList contains more than 3 images:

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
// Potential bug. Upon return, caller no longer uses e.NewValue
// to set the image index. Rather, the caller now uses e.NewIntValue,
// which is not changed in this function.
e.NewValue = CheckedState.Indeterminate; // Developer wrote this, yuk.

  // with the new implementation, to keep backwards compatibility, the developer needs to add this line:
  e.NewIntValue = 2;

}

But, still, this is an edge case.

The next solution addresses this issue, but at the cost of fixing the actual bug propertly and providing a needless workaround.

Option 5: Keep the old behavior intact (boooooh) but add a new proprety that explicitely supports the new behavior.

If the risk in solution 4 is too risky despite the edge case I discuss, this solution 5 should put your mind to rest, at the cost of it being a bit of a sad workaround.

Add a property ListView.MultiStateCheckboxes (false by default)

If ListView.Checkboxes is true, OR
If ListView.MultiStateCheckboxes is true, THEN
--- the ListView will show the checkboxes.

MultiStateCheckboxes is a new property, off by default, thereby keeping the old behavior intact unless turned on.

This property cannot be used in conjunction with ListView.Checkboxes = true. It's one, or the other, not both.

If both MultiStateCheckboxes == true and Checkboxes == true, an exception is thrown wherever is appropriate to throw it.

if MultiStateCheckboxes == true, and there is no StateImageList specified, an exception is thrown. If you want checkboxes and no image list, use Checkboxes = true as before.

if MultiStateCheckboxes == true, then ItemMultiStateCheck ItemMultiStateChecked events are sent, with appropriate EventArgs as per below:

class ItemMultiStateCheckEventArgs
{
int CurrentValue;
int NewValue;
}

and

class ItemMultiStateCheckedEventArgs
{
int Value;
}

I believe using Option 5 is the safest, but also a bit sad, option. It has the benefit of at least offering a clear implementation path for developers, without ambiguity for what needs to be done in the event handlers.

With option 5, it remains unclear what should be done to address the bug with the ItemCheck an ItemChecked event handlers not being fired.

Do you want to keep the old behavior? Do you want to clean it up by at least supporting the Indeterminate state?
Do you want to "fix" the bug by restricting which image gets shown to the first 2 images in the image list?

Do you want to keep it as-is, in order to have whatever workaround code written by developers continue to work?

Do you want to document this bug in the listview documentation, and recommend the new handlers instead for anything other than a true/false case? Maybe this would be best.

CONCLUSION

I believe that it's of some importance to correctly support StateImageLists, even those that contain more than 2 images. It would also certainly be of some relief to those who constantly complain that ListView is a necessary evil but sooooo limited in what you can do with them.

In the end, I think I would prefer Option 5, because it's the safest and has the least chance to have any kind of side effect.

As it is, the bug will feel "unpredictable" and who knows what convoluted code the Potato App developer has written to work around it. Maybe there's even a timer in there somewhere. Or a private variable. Or a Mouse click event that is intercepted before the click ever makes it to the checkbox. Who knows, who knows.

API Proposal

In the above I keep talking about Option 4 and Option 5.

this here describes Option5 as IMO the best option, but will describe Option4 in the Alternative section.

// in a Form declaration:

**Option 5:**

// new eventargs class
class ItemMultiStateCheckEventArgs
{
     int CurrentValue;
     int NewValue;
}

// new eventargs class
class ItemMultiStateCheckedEventArgs
{
     int Value;
}

// new property
bool ListView.MultiStateCheckboxes { get; set;}

// new event .
Event ItemMultiStateCheck += listView_ItemMultiStateCheck(object? sender, ItemMultiCheckEventArgs e)

API Usage

Option 4:

Option 5:

Listview listView = new ListView();

listView.Checkboxes = true;
listView.MultiStateCheckboxes = true; // exception because of previous line

listView.MultiStateCheckboxes = true; 
listView.Checkboxes = true; // exception because of previous line

listView.ItemMultiStateCheck += listView_ItemMultiStateCheck;
listView.ItemMultiStateChecked += listView_ItemMultiStateChecked;

... and then later:

void listView_ItemMultiStateCheck(object? sender, ItemMultiStateCheckEventArgs e)
{
     if (e.CurrentValue == 9)
        e.NewValue = 15;  // override the value.
}

void listView_ItemMultiStateChecked(object? sender, ItemMultiStateCheckedEventArgs e)
{
     PerformSomeActionBasedOnUserState(e.CurrentValue);
}

Alternative Designs

Option 4:

class ItemCheckEventArgs is changed to add 2 new properties
{
CheckedState CurrentValue;
CheckedState NewValue;
int CurrentIntValue; // added
int NewIntValue; // added
}

class ItemCheckedArgs is changed to add 1 new property
{
CheckedState Value;
int IntValue; // added
}

The rest of the change is in the internal code of ListView.cs, to always fire events, and to correctly process NewIntValue if it's changed by the ItemCheck event handler upon return.

ListView.ItemCheck += listView_ItemCheck(object? sender, ItemCheckEventArgs e)

void listView_ItemCheck(object? sender, ItemCheckEventArgs e)
{
e.NewValue = CheckedState.Indeterminate; // cause the caller to use NewIntValue property.
e.NewIntValue = 0; // Valid values are from 0 to StateImageList.Count-1
}

Risks

Low, especially with option 5, which is a new code path entirely, but requires the proper syupport in the WmNotifyReflect in ListBox.cs

Will this feature affect UI controls?

With option 4, nothing to do.

With option 5, add a Bool property which needs to be supported by the visual studio property inspector.