🏗️ Forge: ListBox Parity Integration#148
Conversation
- Created `ListBoxItem` inheriting from `ContentControl`. - Added `IsSelected` dependency property and `Selected`/`Unselected` routed events to `ListBoxItem`. - Replaced `ListBox` manual rendering and layout (`MeasureOverride`, `ArrangeOverride`, `Render`) with a `ControlTemplate` utilizing `ScrollViewer` and `ItemsPresenter`. - Configured `ListBox` to act as an `ItemsControl` generator by overriding `GetContainerForItemOverride` and `IsItemItsOwnContainerOverride`. - Synchronized `ListBox.SelectedIndex` with the visual state (`IsSelected`) of generated `ListBoxItem` containers. - Updated layout and behavior test cases in `Tedd.TUI.Tests`. Co-authored-by: tedd <493224+tedd@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Refactors ListBox to follow the framework’s ItemsControl templating architecture (using an ItemsPresenter hosted in a ScrollViewer) and introduces a dedicated ListBoxItem container to support selection via routed events—aiming for closer WPF structural parity and better XAML customization.
Changes:
- Added
ListBoxItemas aContentControlcontainer withIsSelectedand bubblingSelected/Unselectedrouted events. - Refactored
ListBoxto use a defaultControlTemplate(ScrollViewer+ItemsPresenter) and container generation overrides instead of manual render/measure/scroll logic. - Updated
ListBoxmeasurement tests to account for templating and always-reserved scrollbar width.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Tedd.TUI/ListBoxItem.cs | New item container type with selection state + routed events, plus default template/content wiring. |
| src/Tedd.TUI/ListBox.cs | Migrates ListBox to templated ItemsControl pattern and selection synchronization via container events. |
| src/Tedd.TUI.Tests/ListBoxMeasureTests.cs | Updates measurement tests for templated behavior and scrollbar width reservation. |
| .jules/forge.md | Documents the architectural refactor and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // In Selector, OnSelectionChanged is fired when SelectedIndex/SelectedItem changes. | ||
| // We override it to sync IsSelected to the containers. | ||
| protected override void OnSelectionChanged() | ||
| { | ||
| base.OnSelectionChanged(); | ||
|
|
||
| if (ItemsPanelRoot != null) | ||
| { | ||
| _scrollOffset = index - RenderSize.Height + 1; | ||
| for (int i = 0; i < ItemsPanelRoot.Children.Count; i++) | ||
| { | ||
| if (ItemsPanelRoot.Children[i] is ListBoxItem lbi) | ||
| { | ||
| lbi.IsSelected = (i == SelectedIndex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Keyboard navigation changes SelectedIndex but never moves focus to the newly selected ListBoxItem. If focus is currently on a ListBoxItem (e.g., after a mouse click), arrow-key navigation will leave focus on the old item, so the new selection won’t be treated as “focused selected” by ListBoxItem.UpdateVisualState. Consider updating focus along with selection (e.g., focus the container at SelectedIndex when selection changes due to keyboard), or otherwise introducing a notion of focus-within on ListBox so items can style correctly without relying on per-item focus.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Listen to SelectedEvent from children to update selection | ||
| AddHandler(ListBoxItem.SelectedEvent, new RoutedEventHandler(OnItemSelected)); | ||
| } | ||
|
|
||
| private void OnScroll(object? sender, EventArgs e) | ||
| private void OnItemSelected(object? sender, RoutedEventArgs e) | ||
| { | ||
| _scrollOffset = _scrollBar.Value; | ||
| Invalidate(); | ||
| if (e.OriginalSource is ListBoxItem item) | ||
| { | ||
| int index = ItemsPanelRoot?.Children.IndexOf(item) ?? -1; | ||
| if (index >= 0 && index != SelectedIndex) | ||
| { | ||
| SelectedIndex = index; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new selection synchronization path (ListBoxItem.SelectedEvent bubbling to ListBox.OnItemSelected + container IsSelected updates in OnSelectionChanged) isn’t covered by existing tests. Consider adding tests that simulate selecting an item via ListBoxItem (e.g., setting IsSelected / raising the event) and assert SelectedIndex updates, and that changing SelectedIndex updates the generated containers’ IsSelected states.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| Template = new ControlTemplate(parent => | ||
| { | ||
| var cp = new ContentPresenter(); | ||
| cp.TemplatedParent = parent; | ||
|
|
||
| var contentBinding = new Binding("Content"); | ||
| contentBinding.RelativeSource = RelativeSource.TemplatedParent; | ||
| cp.SetBinding(ContentPresenter.ContentProperty, contentBinding); | ||
|
|
||
| var templateBinding = new Binding("ContentTemplate"); | ||
| templateBinding.RelativeSource = RelativeSource.TemplatedParent; | ||
| cp.SetBinding(ContentPresenter.ContentTemplateProperty, templateBinding); | ||
|
|
||
| return cp; | ||
| }); |
There was a problem hiding this comment.
ListBoxItem sets Background/Foreground in UpdateVisualState, but neither Control nor the current template paints the Background (Control.Render just renders TemplateRoot, and Background is not an inherited DP). This means selected items won’t actually show a background highlight (and the child TextBlock won’t pick up Background either). Consider rendering a background fill in ListBoxItem.Render (fill row with Background) or changing the default template to include an element that draws the background based on the templated parent’s Background/Foreground.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| protected override void OnPropertyChanged(DependencyProperty dp) | ||
| { | ||
| base.OnPropertyChanged(dp); | ||
| if (dp == IsSelectedProperty || dp == IsFocusedProperty) | ||
| { | ||
| UpdateVisualState(); | ||
| } | ||
|
|
||
| if (dp == IsSelectedProperty) | ||
| { | ||
| bool isSelected = (bool)GetValue(IsSelectedProperty); | ||
| if (isSelected) | ||
| { | ||
| RaiseEvent(new RoutedEventArgs(SelectedEvent, this)); | ||
| } | ||
| else | ||
| { | ||
| RaiseEvent(new RoutedEventArgs(UnselectedEvent, this)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void UpdateVisualState() | ||
| { | ||
| // Try to get colors from Parent ListBox if we are inside one. | ||
| // If not, use defaults. | ||
| var parentListBox = Parent as ListBox; | ||
|
|
||
| // Sometimes parent is not set yet, so search up the visual tree | ||
| if (parentListBox == null) | ||
| { | ||
| var curr = Parent; | ||
| while(curr != null && !(curr is ListBox)) | ||
| { | ||
| curr = curr.Parent; | ||
| } | ||
| parentListBox = curr as ListBox; | ||
| } | ||
|
|
||
| ConsoleColor selectedBg = parentListBox?.SelectionBackground ?? ConsoleColor.White; | ||
| ConsoleColor selectedFg = parentListBox?.SelectionForeground ?? ConsoleColor.Black; | ||
|
|
||
| ConsoleColor focusedSelectedBg = parentListBox?.FocusedSelectionBackground ?? ConsoleColor.Blue; | ||
| ConsoleColor focusedSelectedFg = parentListBox?.FocusedSelectionForeground ?? ConsoleColor.White; | ||
|
|
||
| ConsoleColor normalBg = parentListBox?.Background ?? ConsoleColor.Black; | ||
| ConsoleColor normalFg = parentListBox?.Foreground ?? ConsoleColor.Gray; | ||
|
|
||
| bool showSelection = parentListBox?.ShowSelection ?? true; | ||
|
|
||
| if (IsSelected) | ||
| { | ||
| if (IsFocused || (parentListBox != null && parentListBox.IsFocused)) | ||
| { | ||
| // Note: WPF ListBoxItem is focused when selected usually. | ||
| // Or if ListBox is focused. | ||
| Background = focusedSelectedBg; |
There was a problem hiding this comment.
UpdateVisualState is only triggered when the ListBoxItem’s IsSelected/IsFocused changes. The visual state also depends on the parent ListBox’s focus (parentListBox.IsFocused) and ShowSelection, but ListBoxItem won’t recompute when the ListBox gains/loses focus, so colors can get “stuck” in the unfocused state. Consider recomputing on each render, or reacting to parent focus changes (e.g., override OnParentChanged and hook/unhook parent focus events or have ListBox invalidate/update its containers on focus changes).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| public ListBoxItem() | ||
| { | ||
| Focusable = true; | ||
|
|
||
| Template = new ControlTemplate(parent => | ||
| { | ||
| var cp = new ContentPresenter(); | ||
| cp.TemplatedParent = parent; | ||
|
|
||
| var contentBinding = new Binding("Content"); | ||
| contentBinding.RelativeSource = RelativeSource.TemplatedParent; | ||
| cp.SetBinding(ContentPresenter.ContentProperty, contentBinding); | ||
|
|
||
| var templateBinding = new Binding("ContentTemplate"); | ||
| templateBinding.RelativeSource = RelativeSource.TemplatedParent; | ||
| cp.SetBinding(ContentPresenter.ContentTemplateProperty, templateBinding); | ||
|
|
||
| return cp; | ||
| }); |
There was a problem hiding this comment.
ListBoxItem duplicates ContentControl’s default template (ContentPresenter with bindings to Content/ContentTemplate). Since ContentControl already sets an identical Template in its constructor, this extra Template assignment is redundant and adds maintenance burden if the base template changes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Set content correctly based on ItemTemplate or fallback | ||
| if (ItemTemplate != null) | ||
| { | ||
| var pixelBg = Background ?? buffer.GetPixel(x + dx, y + i).Background; | ||
| buffer.SetPixel(x + dx, y + i, ' ', ConsoleColor.White, pixelBg); | ||
| lbi.ContentTemplate = ItemTemplate; | ||
| lbi.Content = item; | ||
| } | ||
|
|
||
| if (itemIndex < Items.Count) | ||
| else | ||
| { | ||
| bool isSelected = (itemIndex == SelectedIndex); | ||
| var bg = Background ?? buffer.GetPixel(x, y + i).Background; | ||
| var fg = Foreground; | ||
| if (isSelected) | ||
| { | ||
| if (IsFocused) | ||
| { | ||
| // Focused: selected item is blue | ||
| bg = FocusedSelectionBackground; | ||
| fg = FocusedSelectionForeground; | ||
| } | ||
| else if (ShowSelection) | ||
| { | ||
| // Not focused but ShowSelection enabled: inverted black/white | ||
| bg = SelectionBackground; | ||
| fg = SelectionForeground; | ||
| } | ||
| // else: ShowSelection is false and not focused, use default colors | ||
| } | ||
|
|
||
| if (ItemTemplate != null) | ||
| { | ||
| // Fill row with selection background first, then render template content on top | ||
| for (int dx = 0; dx < effectiveW; dx++) | ||
| { | ||
| buffer.SetPixel(x + dx, y + i, ' ', fg, bg); | ||
| } | ||
| var container = GetContainerForItemCore(); | ||
| PrepareContainerForItemOverride(container, Items[itemIndex]); | ||
| container.Measure(new Size(effectiveW, 1)); | ||
| container.Arrange(new Rect(0, 0, effectiveW, 1)); | ||
| container.Render(buffer, x, y + i); | ||
| } | ||
| else | ||
| { | ||
| string content = GetItemText(Items[itemIndex]); | ||
| if (content.Length > effectiveW) content = content.Substring(0, effectiveW); | ||
|
|
||
| for (int dx = 0; dx < content.Length; dx++) | ||
| { | ||
| buffer.SetPixel(x + dx, y + i, content[dx], fg, bg); | ||
| } | ||
| // Fill rest of line with bg | ||
| for (int dx = content.Length; dx < effectiveW; dx++) | ||
| { | ||
| buffer.SetPixel(x + dx, y + i, ' ', fg, bg); | ||
| } | ||
| } | ||
| lbi.Content = GetItemText(item); | ||
| } |
There was a problem hiding this comment.
When ItemTemplate is null, PrepareContainerForItemOverride sets ListBoxItem.Content = GetItemText(item). Because ListBox overrides IsItemItsOwnContainerOverride to not treat arbitrary UIElement items as their own containers, UIElement items (e.g., a Button) will be wrapped in a ListBoxItem and then coerced to string, so they won’t render/interact as UIElements. Consider using the raw item as Content when item is a UIElement (and only fall back to GetItemText for non-UIElement items).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Sync IsSelected | ||
| int index = Items.IndexOf(item); | ||
| if (index == SelectedIndex) | ||
| { | ||
| X = e.X - (RenderSize.Width - 1), | ||
| Y = e.Y, | ||
| Handled = false | ||
| }; | ||
| _scrollBar.OnMouseDown(sbArgs); | ||
| e.Handled = true; | ||
| return; | ||
| } | ||
|
|
||
| // e.Y is already local relative to this control | ||
| int itemIndex = e.Y + _scrollOffset; | ||
|
|
||
| if (itemIndex >= 0 && itemIndex < Items.Count) | ||
| { | ||
| SelectedIndex = itemIndex; | ||
| // SelectionChanged is raised by base.SelectedIndex setter | ||
| lbi.IsSelected = true; | ||
| } | ||
| else | ||
| { | ||
| lbi.IsSelected = false; | ||
| } |
There was a problem hiding this comment.
PrepareContainerForItemOverride calls Items.IndexOf(item) for every container to decide IsSelected. This makes container preparation O(n²) for large lists. Consider passing the item index into container generation (e.g., incrementing an index in ItemsPresenter.PopulatePanel) or otherwise avoiding repeated IndexOf calls when populating/refreshing containers.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…ForItemOverride Co-authored-by: tedd <493224+tedd@users.noreply.github.com>
…ex and SelectedIndex→IsSelected paths Co-authored-by: tedd <493224+tedd@users.noreply.github.com>
Co-authored-by: tedd <493224+tedd@users.noreply.github.com>
…focus Co-authored-by: tedd <493224+tedd@users.noreply.github.com>
[WIP] Address feedback from review on ListBox Parity Integration PR
Add tests for ListBox selection synchronization (item→SelectedIndex and SelectedIndex→IsSelected)
[WIP] WIP Address feedback from review on ListBox Parity Integration PR
[WIP] WIP Address feedback on ListBox Parity Integration PR
Remove redundant Template assignment in ListBoxItem
Fix ListBox coercing UIElement items to string when no ItemTemplate is set
…f4-a4e2-4f891462c6ec [WIP] Address feedback on ListBox Parity Integration PR
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sync IsSelected | ||
| int index = Items.IndexOf(item); | ||
| if (index == SelectedIndex) | ||
| { | ||
| lbi.IsSelected = true; | ||
| } | ||
| else | ||
| { | ||
| lbi.IsSelected = false; | ||
| } |
There was a problem hiding this comment.
PrepareContainerForItemOverride uses Items.IndexOf(item) to determine the container's index. This is O(n) per item during panel population (O(n^2) overall) and is also incorrect when Items contains duplicates (IndexOf returns the first occurrence, so selection can be applied to the wrong container). Consider syncing IsSelected based on the container's actual position in ItemsPanelRoot.Children (e.g., a post-population pass iterating children by index), instead of using Items.IndexOf here.
| // Sync IsSelected | |
| int index = Items.IndexOf(item); | |
| if (index == SelectedIndex) | |
| { | |
| lbi.IsSelected = true; | |
| } | |
| else | |
| { | |
| lbi.IsSelected = false; | |
| } | |
| // Sync IsSelected without O(n) lookup or duplicate ambiguity | |
| lbi.IsSelected = Equals(item, SelectedItem); |
| RaiseEvent(new RoutedEventArgs(UnselectedEvent, this)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal void UpdateVisualState() | ||
| { |
There was a problem hiding this comment.
ListBoxItem visual state depends on parentListBox.IsFocused (focused selection colors), but UpdateVisualState() is only triggered when IsSelected or the item's own IsFocused changes. When focus moves to/from the ListBox itself (e.g., via Tab navigation), the selected item's visuals won't refresh. Consider having ListBox notify realized containers on its IsFocused changes (and possibly selection color DP changes), and making UpdateVisualState callable from ListBox (e.g., internal) or using bindings/triggers instead of caching colors into local Background/Foreground.
|
|
||
| public class ListBox : Selector |
There was a problem hiding this comment.
This file includes an unused using System.Reflection; import (at the top of the file). Please remove it to avoid analyzer warnings and keep imports clean.
💡 Hypothesis: The current
ListBoximplementation manually handles rendering, measurement, and scrolling instead of adhering to the standard WPFItemsControltemplating architecture (ItemsPresenterwithin aScrollViewer). This violates the target architectural isomorphism and prevents declarative customization.🎯 Execution: Engineered
ListBoxItemas aContentControlsupportingIsSelectedand bubbling routed events. RefactoredListBoxto abandon manual graphics operations and rely on a defaultControlTemplatewrapping anItemsPresenterinside aScrollViewer. SynchronizedSelector.SelectedIndexchanges with the generated container logic natively via routed events.📊 Functional Impact: Fully restores XAML and WPF architectural parity for list selection workflows. Developers can now utilize standard
ControlTemplatebehavior, while visual styling falls back robustly onto dependency properties.🔬 Verification Protocol:
dotnet test src/Tedd.TUI.Teststo verifyListBoxlayout bounds correctly incorporateScrollViewerpadding and scrollbars.ApplyTemplatelifecycle validation and correct property invalidation.PR created automatically by Jules for task 5850515012165907113 started by @tedd