Improve reasoning and tool call UI - #13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the “reasoning” and “tool call” chat message bubbles to use a more structured two-column layout (icon + text) via designer-generated WinForms/DevExpress UI, and tweaks message/status presentation in the demos.
Changes:
- Moved
ReasoningMessageControlandFunctionCallMessageControl(plus DevExpress equivalents) to designer-driven TableLayoutPanel layouts with icon columns. - Updated
FunctionCallMessageControl/DXFunctionCallMessageControlto refresh UI onFunctionCallMessageContent.PropertyChanged(e.g., executing → result). - Added small UI polish changes (sender label padding, demo data chaining via
ReasoningMessageContent.SetDone(), DevExpress form field renames).
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SubControls/ReasoningMessageControl.cs | Converted to partial and uses designer initialization; updates reasoning header text behavior. |
| src/SubControls/ReasoningMessageControl.Designer.cs | New TableLayoutPanel-based layout with icon column and click-to-toggle wiring. |
| src/SubControls/FunctionCallMessageControl.cs | Converted to partial, added PropertyChanged-driven UI updates, new label structure and visibility rules. |
| src/SubControls/FunctionCallMessageControl.Designer.cs | New TableLayoutPanel-based layout for call/args/result rows. |
| src/SubControls/FunctionCallMessageControl.resx | New resx for WinForms designer resource plumbing. |
| src/SubControls/ChatSplitContainerControl.resx | New resx for designer resource plumbing. |
| src/SubControls/ChatMessageControl.cs | Adds padding under sender label for readability. |
| src/Messages/ReasoningMessageContent.cs | Makes SetDone() chainable by returning this. |
| WinFormsDemo/OllamaDemoForm.cs | Removes unused using TinyChat;. |
| WinFormsDemo/DemoData.cs | Uses SetDone() chaining to mark reasoning complete. |
| DevExpressDemo/SubControls/DXReasoningMessageControl.cs | Converted to partial, designer-driven layout; updates title binding output. |
| DevExpressDemo/SubControls/DXReasoningMessageControl.Designer.cs | New TableLayoutPanel/padding panel layout with SVG icon resources. |
| DevExpressDemo/SubControls/DXReasoningMessageControl.resx | Adds embedded SVG icon resource(s). |
| DevExpressDemo/SubControls/DXFunctionCallMessageControl.cs | Converted to partial, PropertyChanged-driven UI updates, visibility rules. |
| DevExpressDemo/SubControls/DXFunctionCallMessageControl.Designer.cs | New TableLayoutPanel/padding panel layout with SVG icon resources. |
| DevExpressDemo/SubControls/DXFunctionCallMessageControl.resx | Adds embedded SVG icon resource(s). |
| DevExpressDemo/SubControls/DXChatMessageControl.cs | Adds padding under sender label for readability. |
| DevExpressDemo/DXOllamaDemoForm.cs | Updates references after designer field renames. |
| DevExpressDemo/DXOllamaDemoForm.Designer.cs | Designer-generated rename/relayout updates. |
Files not reviewed (5)
- DevExpressDemo/DXOllamaDemoForm.Designer.cs: Language not supported
- DevExpressDemo/SubControls/DXFunctionCallMessageControl.Designer.cs: Language not supported
- DevExpressDemo/SubControls/DXReasoningMessageControl.Designer.cs: Language not supported
- src/SubControls/FunctionCallMessageControl.Designer.cs: Language not supported
- src/SubControls/ReasoningMessageControl.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Unsubscribe from the previous content's change notifications. | ||
| if (_message?.Content is FunctionCallMessageContent oldFc) | ||
| oldFc.PropertyChanged -= OnContentPropertyChanged; | ||
|
|
||
| _message = value; | ||
|
|
||
| binding = _headerLabel.DataBindings.Add(nameof(_headerLabel.Text), Message.Content, nameof(FunctionCallMessageContent.IsFunctionExecuting)); | ||
| binding.Format += (_, e) => | ||
| { | ||
| var bullet = _expanded ? "-" : "+"; | ||
| var value = $"{bullet} {fc.Name}"; | ||
|
|
||
| if (fc.IsFunctionExecuting) | ||
| value += " (working)"; | ||
|
|
||
| e.Value = value; | ||
| }; | ||
| // Subscribe to the new content so we redraw when IsFunctionExecuting | ||
| // or Result changes (raised by FunctionCallMessageContent.SetResult). | ||
| if (_message?.Content is FunctionCallMessageContent newFc) | ||
| newFc.PropertyChanged += OnContentPropertyChanged; |
There was a problem hiding this comment.
The control subscribes to FunctionCallMessageContent.PropertyChanged, but the subscription is only removed when Message is reassigned. If the control is disposed while still bound to a message, the event handler can keep the control alive and/or fire after disposal. Consider overriding Dispose(disposing) to detach from the current content and guard BeginInvoke/UpdateDisplay when Disposing/IsDisposed.
| base.MaximumSize = value; | ||
| _headerLabel.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); | ||
| _detailLabel.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); | ||
| var textWidth = Math.Max(0, value.Width - Padding.Horizontal - ICON_WIDTH); |
There was a problem hiding this comment.
MaximumSize calculation ignores the internal table padding (_table.Padding is set in the designer). As a result, the label MaximumSize can be wider than the actual available text area, preventing wrapping and causing horizontal overflow. Subtract the table padding (and any other inner offsets) in addition to ICON_WIDTH when computing textWidth.
| var textWidth = Math.Max(0, value.Width - Padding.Horizontal - ICON_WIDTH); | |
| var textWidth = Math.Max(0, value.Width - Padding.Horizontal - _table.Padding.Horizontal - ICON_WIDTH); |
| } | ||
|
|
||
| if (fc.Result is not null) | ||
| _resultLabel.Text = fc.Result.ToString() + " "; |
There was a problem hiding this comment.
Similarly, _resultLabel.Text is only assigned when Result is non-null. If Message changes (or Result is cleared), the old result string remains in the label instance. Set _resultLabel.Text to empty when Result is null to prevent stale state.
| _resultLabel.Text = fc.Result.ToString() + " "; | |
| _resultLabel.Text = fc.Result.ToString() + " "; | |
| else | |
| _resultLabel.Text = string.Empty; |
| _table.CellBorderStyle = TableLayoutPanelCellBorderStyle.None; | ||
| _table.ColumnCount = 2; | ||
| _table.ColumnStyles.Add(new ColumnStyle(SizeType.Absolute, IconColumnWidth)); | ||
| _table.ColumnStyles.Add(new ColumnStyle(SizeType.Percent, 100f)); | ||
| _table.Controls.Add(_iconLabel, 0, 0); | ||
| _table.Controls.Add(_headerLabel, 1, 0); | ||
| // Row 1, col 0 intentionally left empty - no spacer, so the row fully | ||
| // collapses when _detailLabel is hidden. | ||
| _table.Controls.Add(_detailLabel, 1, 1); | ||
| _table.Cursor = Cursors.Hand; | ||
| _table.Dock = DockStyle.Fill; | ||
| _table.Margin = Padding.Empty; | ||
| _table.Padding = new Padding(8, 0, 0, 0); | ||
| _table.RowCount = 2; |
There was a problem hiding this comment.
The TableLayoutPanel reserves an icon column and adds left padding, but ReasoningMessageControl.MaximumSize currently only subtracts the control's Padding when constraining the header/detail labels. That makes text wrapping inaccurate (labels can be wider than the real text column). Adjust the MaximumSize propagation to subtract IconColumnWidth and the table padding so long reasoning lines wrap correctly.
| // Unsubscribe from the previous content's change notifications. | ||
| if (_message?.Content is FunctionCallMessageContent oldFc) | ||
| oldFc.PropertyChanged -= OnContentPropertyChanged; | ||
|
|
||
| _detailLabel.DataBindings.Clear(); | ||
| _headerLabel.DataBindings.Clear(); | ||
| if (Message is not null && Message.Content is FunctionCallMessageContent fc) | ||
| { | ||
| var binding = _detailLabel.DataBindings.Add(nameof(_detailLabel.Text), Message.Content, nameof(Message.Content.Content)); | ||
| binding.Format += (_, e) => | ||
| { | ||
| var maxArgKeyLength = fc.Arguments?.Any() ?? false ? fc.Arguments.Keys.Max(k => k.Length) : 0; | ||
| var args = fc.Arguments?.Count > 0 | ||
| ? string.Join("\n", fc.Arguments.Select(kv => $"{(kv.Key + ":").PadRight(maxArgKeyLength + 1)} {kv.Value}")) | ||
| : ""; | ||
|
|
||
| var result = fc.Result is not null ? $"\n\n🡪 {fc.Result}" : ""; | ||
|
|
||
| e.Value = (args + result).TrimStart('\n'); | ||
| }; | ||
|
|
||
|
|
||
| binding = _headerLabel.DataBindings.Add(nameof(_headerLabel.Text), Message.Content, nameof(FunctionCallMessageContent.IsFunctionExecuting)); | ||
| binding.Format += (_, e) => | ||
| { | ||
| var bullet = _expanded ? "-" : "+"; | ||
| var value = $"{bullet} {fc.Name}"; | ||
|
|
||
| if (fc.IsFunctionExecuting) | ||
| value += " (working)"; | ||
| _message = value; | ||
|
|
||
| e.Value = value; | ||
| }; | ||
| // Subscribe to the new content so we redraw when IsFunctionExecuting | ||
| // or Result changes (raised by FunctionCallMessageContent.SetResult). | ||
| if (_message?.Content is FunctionCallMessageContent newFc) | ||
| newFc.PropertyChanged += OnContentPropertyChanged; |
There was a problem hiding this comment.
DXFunctionCallMessageControl subscribes to FunctionCallMessageContent.PropertyChanged but only unsubscribes when Message is reassigned. If the control is disposed while still bound, the handler can keep it alive and/or run after disposal. Consider detaching in Dispose(disposing) and guarding BeginInvoke/UpdateDisplay when Disposing/IsDisposed.
| fc.Arguments.Select(kv => $"{(kv.Key + ":").PadRight(maxKeyLen + 1)} {kv.Value}")); | ||
| } |
There was a problem hiding this comment.
UpdateDisplay only sets _argsLabel.Text when Arguments are present. When switching Message instances, a new message with no arguments will keep the previous arguments text (even though the label is hidden). Clear _argsLabel.Text when there are no arguments to avoid stale content lingering and to simplify future changes (e.g., accessibility/automation).
| fc.Arguments.Select(kv => $"{(kv.Key + ":").PadRight(maxKeyLen + 1)} {kv.Value}")); | |
| } | |
| fc.Arguments.Select(kv => $"{(kv.Key + ":").PadRight(maxKeyLen + 1)} {kv.Value}")); | |
| } | |
| else | |
| { | |
| _argsLabel.Text = string.Empty; | |
| } |
| lblTitle.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); | ||
| lblDetail.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); |
There was a problem hiding this comment.
DXReasoningMessageControl.MaximumSize only subtracts the control Padding when constraining lblTitle/lblDetail, but the layout also reserves a fixed icon column (IconColumnWidth). This can prevent wrapping and cause horizontal overflow. Subtract IconColumnWidth (and any inner padding) when setting lblTitle/lblDetail.MaximumSize.
| lblTitle.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); | |
| lblDetail.MaximumSize = new Size(value.Width - Padding.Horizontal, 0); | |
| // Reserve space for the icon column and respect both the control and label padding | |
| var availableWidth = Math.Max(0, value.Width - Padding.Horizontal - IconColumnWidth); | |
| var titleMaxWidth = Math.Max(0, availableWidth - lblTitle.Padding.Horizontal); | |
| var detailMaxWidth = Math.Max(0, availableWidth - lblDetail.Padding.Horizontal); | |
| lblTitle.MaximumSize = new Size(titleMaxWidth, 0); | |
| lblDetail.MaximumSize = new Size(detailMaxWidth, 0); |
| var maxKeyLen = fc.Arguments.Keys.Max(k => k.Length); | ||
| lblArguments.Text = string.Join("\n", | ||
| fc.Arguments.Select(kv => $"{(kv.Key + ":").PadRight(maxKeyLen + 1)} {kv.Value}")); | ||
| } |
There was a problem hiding this comment.
UpdateDisplay only assigns lblArguments.Text when Arguments are present. If Message changes to a call without arguments, the previous arguments string remains in the label instance (even if hidden). Clear lblArguments.Text when there are no arguments to avoid stale state.
| } | |
| } | |
| else | |
| { | |
| lblArguments.Text = string.Empty; | |
| } |
| InitializeComponent(); | ||
|
|
||
| _headerLabel = new Label | ||
| { | ||
| AutoSize = true, | ||
| Font = MonospaceFont, | ||
| UseMnemonic = false, | ||
| Dock = DockStyle.Top, | ||
| Cursor = Cursors.Hand, | ||
| }; | ||
|
|
||
| _detailLabel = new Label | ||
| { | ||
| AutoSize = true, | ||
| Font = MonospaceFont, | ||
| UseMnemonic = false, | ||
| Dock = DockStyle.Fill, | ||
| Visible = false, | ||
| Padding = new Padding(14, 4, 0, 0), | ||
| }; | ||
|
|
||
| Controls.Add(_detailLabel); | ||
| Controls.Add(_headerLabel); | ||
| _headerLabel.BringToFront(); | ||
| _detailLabel.BringToFront(); | ||
| AutoSize = true; | ||
|
|
||
| _headerLabel.Click += Toggle; | ||
| _detailLabel.Click += Toggle; | ||
| Click += Toggle; | ||
| _iconLabel.Font = new Font("Arial", 11); | ||
| } |
There was a problem hiding this comment.
This creates a new Font instance for every ReasoningMessageControl and never disposes it. In message-heavy scenarios this can leak GDI handles. Prefer reusing an existing font (e.g., derive from the control font) or store and dispose the created Font in Dispose(disposing).
| lblToolIcon.Font = new Font(lblTitle.Font.FontFamily, lblTitle.Font.Size); | ||
| lblResultIcon.Font = lblToolIcon.Font; | ||
|
|
||
| _headerLabel = new LabelControl | ||
| { | ||
| AutoSize = true, | ||
| Font = MonospaceFont, | ||
| UseMnemonic = false, | ||
| Dock = DockStyle.Top, | ||
| Cursor = Cursors.Hand, | ||
| }; | ||
|
|
||
| _detailLabel = new LabelControl | ||
| { | ||
| AutoSize = true, | ||
| Font = MonospaceFont, | ||
| UseMnemonic = false, | ||
| Dock = DockStyle.Fill, | ||
| Visible = false, | ||
| Padding = new Padding(14, 4, 0, 0), | ||
| }; | ||
|
|
||
| borderPanel.Controls.Add(_detailLabel); | ||
| borderPanel.Controls.Add(_headerLabel); | ||
| Controls.Add(borderPanel); | ||
| _headerLabel.BringToFront(); | ||
| _detailLabel.BringToFront(); | ||
|
|
||
| borderPanel.Click += Toggle; | ||
| _headerLabel.Click += Toggle; | ||
| _detailLabel.Click += Toggle; | ||
| Click += Toggle; | ||
| lblTitle.Font = new Font("Consolas", lblTitle.Font.Size); | ||
| lblArguments.Font = new Font(lblTitle.Font.FontFamily, lblArguments.Font.Size - 1); |
There was a problem hiding this comment.
These per-instance Font allocations (new Font(...)) are never disposed. If many tool-call controls are created, this can leak GDI handles over time. Prefer reusing existing fonts or dispose the created Font objects in Dispose(disposing).
Show reasoning and tool calls within the chat.