Fixes #5043 - View.ScrollBars now only show if they fit - Fixes View.Viewport bug#5045
Fixes #5043 - View.ScrollBars now only show if they fit - Fixes View.Viewport bug#5045
View.ScrollBars now only show if they fit - Fixes View.Viewport bug#5045Conversation
Refactor Shortcut and related code to make CommandView nullable (View?) and update all usages to handle null values safely. Update property setters, event handlers, layout logic, and tests to use null-safe access. Initialize HelpView and KeyView with default Frames. Clean up redundant null checks and improve robustness against null reference exceptions when CommandView is replaced or disposed.
- Remove BorderStyle from TreeViewFileSystem tree view - Prevent invalid viewport/frame updates in View - Refine scrollbar visibility checks for valid content area - Remove quit key binding from MenuBar, comment out in PopoverMenu - Hide scrollbars if visible content size < 2 in auto mode
Refactored TreeViewFileSystem to use Runnable and improved layout logic. Enhanced View.Content.cs to prevent Frame growth when adornments consume the viewport. Expanded ViewportTests with cases for adornment edge cases. Applied code style and correctness improvements.
There was a problem hiding this comment.
Pull request overview
This PR targets issue #5043 by preventing built-in scrollbars from becoming visible when there isn’t enough space to render them without consuming the entire content area, and by addressing a Viewport/adornment interaction that could incorrectly grow Frame when re-applying the current Viewport.
Changes:
- Add scrollbar visibility guards (and a minimum-size rule) so scrollbars only show when they can fit.
- Adjust
View.SetViewportbehavior and add viewport/adornment regression tests for the “adornments consume viewport” scenario. - Update
Shortcut/Menu example and unit test code to accommodate the behavior changes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/Views/ShortcutTests.cs | Updates Shortcut tests for CommandView nullability/layout expectations. |
| Tests/UnitTestsParallelizable/Views/ShortcutTests.KeyDown.cs | Updates Shortcut input tests; adds null-conditional usage in assertions. |
| Tests/UnitTestsParallelizable/Views/EnableForDesignEventTests.cs | Uses null-forgiving operator when casting CommandView. |
| Tests/UnitTestsParallelizable/ViewBase/Layout/ViewportTests.cs | Adds regression tests ensuring Viewport = Viewport doesn’t grow Frame when adornments consume viewport. |
| Terminal.Gui/Views/Shortcut.cs | Makes CommandView nullable and adds default Frames to subviews; adjusts layout logic. |
| Terminal.Gui/Views/ScrollBar/ScrollBar.cs | Prevents Auto scrollbars from showing when too small to render both buttons. |
| Terminal.Gui/Views/Menu/MenuBar.cs | Tweaks design-time Quit menu item creation. |
| Terminal.Gui/ViewBase/View.ScrollBars.cs | Adds VisibleChanging cancellation logic to prevent scrollbars showing when they can’t fit. |
| Terminal.Gui/ViewBase/View.Content.cs | Adds an early-return guard in SetViewport for “invalid” sizes / no-op scenarios. |
| Terminal.Gui/ViewBase/Adornment/AdornmentImpl.cs | Adds (commented-out) debug assertion concept for unreasonable thickness values. |
| Examples/UICatalog/Scenarios/TreeViewFileSystem.cs | Updates layout and replaces Window with Runnable for the scenario root. |
| Examples/UICatalog/Scenarios/Shortcuts.cs | Updates event handler/lambda parameters and adds null-conditional access for CommandView. |
| Examples/UICatalog/Scenarios/PopoverMenus.cs | Makes checkbox cast/assignment safe via as + null-conditional. |
Comments suppressed due to low confidence (2)
Terminal.Gui/Views/Shortcut.cs:260
CommandViewis now nullable, but this block dereferences it (CommandView.SuperView,Add(CommandView)) based onCommandView?.Visible == true. This will produce nullable warnings and can still pass a null toAdd(...). Consider cachingView commandView = CommandView!after a null check, or keepCommandViewnon-nullable if it is required by design.
if (CommandView?.Visible == true)
{
if (CommandView.SuperView is null)
{
Add (CommandView);
SetCommandViewDefaultLayout ();
Examples/UICatalog/Scenarios/Shortcuts.cs:339
- The condition uses
framedShortcut.CommandView?.Margin is { }, but inside the blockframedShortcut.CommandViewis dereferenced without a null check. WithCommandViewnow nullable this will produce nullable warnings. Consider cachingView commandView = framedShortcut.CommandView!;after an explicit not-null check, then usecommandViewinside the block.
if (framedShortcut.CommandView?.Margin is { })
{
framedShortcut.CommandView.SchemeName = SchemeManager.SchemesToSchemeName (Schemes.Dialog);
framedShortcut.HelpView.SchemeName = SchemeManager.SchemesToSchemeName (Schemes.Error);
framedShortcut.KeyView.SchemeName = SchemeManager.SchemesToSchemeName (Schemes.Base);
When the scheme name is "Accent", the code now derives the accent scheme from the "Base" scheme using the driver's default attribute, rather than returning the named "Accent" scheme directly. This ensures consistent accent theming based on the current base scheme.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…inal.Gui into issue-5043-scrollbar
Removed null check and updated usages to support CommandView being null. Used null-conditional operators to prevent exceptions and set Title to an empty string when CommandView is null.
… layout logic Removed arbitrary screenX4 upper bounds in dimension and layout calculations, using int.MaxValue instead. This change affects maximum content dimension, text formatting constraints, and layout for centered and anchored subviews, improving flexibility and correctness for large dimension scenarios.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
Terminal.Gui/Views/Shortcut.cs:446
- The XML docs for
CommandViewstill include anArgumentNullException<exception>tag, but the setter no longer throws on null and the property is now nullable. Please update the docs to reflect the actual behavior (or reintroduce the null guard ifCommandViewis intended to be non-null).
/// <summary>
/// Gets or sets the View that displays the command text and hotkey.
/// </summary>
/// <exception cref="ArgumentNullException"></exception>
/// <remarks>
- Arrangement scenario now sets SchemeName via SchemeManager instead of assigning a custom Scheme directly. - DropDownList overrides OnMouseEvent to handle popover activation when ReadOnly and left mouse button is pressed.
Fix Accent scheme fallback, cleanup, and disposal logic - Derive Accent scheme from Base if missing background color - Simplify ConstrainToSize assignment in DimAuto.cs - Reformat namespace in View.Content.cs - Use _commandView field consistently in Shortcut disposal
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Terminal.Gui/Views/Shortcut.cs:519
CommandViewwas changed from non-nullable toView?and the setter no longer rejectsnull, but the class docs/remarks describeShortcutas a composite containing a CommandView and command dispatch relies on it as the relay target. This is a public API surface change for nullable-reference-type consumers and may be unintentionally loosening an invariant. Ifnullis not a supported state, consider restoring the non-nullable type +ThrowIfNull; if it is supported, consider updating the XML docs and ensuring all callers (and derived types likeMenuItem) have a clear contract for the null case.
public View? CommandView
{
get => _commandView;
set
{
// Clean up old
_commandView?.GettingAttributeForRole -= SubViewOnGettingAttributeForRole;
Remove (_commandView);
_commandView?.Dispose ();
// Set new
_commandView = value;
#if DEBUG
if (string.IsNullOrEmpty (_commandView?.Id))
{
_commandView?.Id = "_commandView";
}
#endif
_commandView?.GettingAttributeForRole += SubViewOnGettingAttributeForRole;
// If the CommandView has a hotkey, we use that. Otherwise, we use '_' to indicate the hotkey is in the Title.
if (_commandView?.HotKey != Key.Empty)
{
HotKeySpecifier = (Rune)'\xffff';
}
else
{
HotKeySpecifier = (Rune)'_';
}
Title = _commandView?.Text ?? string.Empty;
UpdateKeyBindings (Key.Empty);
UpdateMouseBindings ();
ShowHide ();
}
…ort no-op case Agent-Logs-Url: https://github.com/gui-cs/Terminal.Gui/sessions/7f22400b-e587-4b21-a4a7-90a776f8784d Co-authored-by: tig <585482+tig@users.noreply.github.com>
BDisp
left a comment
There was a problem hiding this comment.
That sounds good to me. I just have two questions that I'm a little unsure about.
Fixes
View.ScrollBarshould no make scrollbars visible ifPaddingcan't fit #5043Dim.Autorestricts size to 4x screen size #5046Pull request overview
This PR targets issue #5043 by preventing built-in scrollbars from becoming visible when there isn’t enough space to render them without consuming the entire content area, and by addressing a
Viewport/adornment interaction that could incorrectly growFramewhen re-applying the currentViewport.Changes:
View.SetViewportbehavior and add viewport/adornment regression tests for the “adornments consume viewport” scenario.Shortcut/Menu example and unit test code to accommodate the behavior changes.