-
Notifications
You must be signed in to change notification settings - Fork 127
[Extensions] AlternateColor without item containers #763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
zubinqayam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avid29:fix/alt-color6
components/Extensions/src/ListViewBase/ListViewExtensions.AlternateRows.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR extends the AlternateColor functionality for ListViewBase to support scenarios where no item container is used. When an item container is unavailable, the extension now falls back to using the item itself if it's a Control.
Key changes:
- Added fallback logic to use the item as a Control when ItemContainer is unavailable
- Refactored code across multiple ListViewExtensions files for consistency and modern C# patterns
- Consolidated event cleanup logic with uniquely named unload handlers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ListViewExtensions.AlternateRows.cs | Added fallback logic to use args.Item when args.ItemContainer is null, refactored to use modern C# patterns, renamed methods/variables for clarity |
| ListViewExtensions.Command.cs | Refactored to use expression-bodied members and modern pattern matching for consistency |
| ListViewExtensions.StretchItemContainer.cs | Implemented previously missing event handlers, changed return type to nullable, refactored for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
components/Extensions/src/ListViewBase/ListViewExtensions.StretchItemContainer.cs
Outdated
Show resolved
Hide resolved
| var itemContainer = args.ItemContainer as Control; | ||
| SetItemContainerBackground(sender, itemContainer, args.ItemIndex); | ||
| } | ||
| _trackedListViews[listViewBase.Items] = listViewBase; |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _trackedListViews dictionary is being updated unconditionally on line 67, even when AlternateColor is being removed (set to null). This means the dictionary will contain entries for ListViews that no longer use AlternateColor, which is a memory leak. The entry should only be added when GetAlternateColor(listViewBase) is not null, and should be removed when it is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no actual memory leak because the item is removed when the ListViewBase is unloaded, but it doesn't hurt to address it earlier
components/Extensions/src/ListViewBase/ListViewExtensions.AlternateRows.cs
Outdated
Show resolved
Hide resolved
components/Extensions/src/ListViewBase/ListViewExtensions.StretchItemContainer.cs
Outdated
Show resolved
Hide resolved
…tchItemContainer.cs Co-authored-by: Copilot <[email protected]>
…rnateRows.cs Co-authored-by: Copilot <[email protected]>
…tchItemContainer.cs Co-authored-by: Copilot <[email protected]>
…n alternate color is unset
Fixes #712
PR Type
What kind of change does this PR introduce?
What is the current behavior?
If a
ListViewBasedoes not use a container, for whatever reason, the item's background will not be altered.What is the new behavior?
If the
ItemContaineris not, the item itself will be used if the type is aControl.PR Checklist
Please check if your PR fulfills the following requirements:
Other information