-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix exception in menuitem extension #156
Conversation
WalkthroughThe changes made to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MenuExtensions
participant FocusedControl
User->>MenuExtensions: Attach to Visual Tree
MenuExtensions->>FocusedControl: Check if focusedControl is not null
alt focusedControl is not null
MenuExtensions->>FocusedControl: GetLogicalAncestors()
MenuExtensions->>MenuItems: Retrieve menu items
else focusedControl is null
MenuExtensions->>User: No action taken
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/MenuExtensions.cs (1)
42-50
: Consider adding debug logging for null focus casesWhile the null check prevents the exception, it might be helpful to log when
focusedControl
is null for debugging purposes. Additionally, consider whether menu items should be closed when no control has focus.if (focusedControl != null) { var focusedTree = focusedControl.GetLogicalAncestors(); var menuItems = visual.GetLogicalAncestors().OfType<MenuItem>(); foreach (MenuItem menuItem in menuItems.Where(item => !focusedTree.Contains(item)) .ToArray()) menuItem.Close(); +} else { + // Consider adding debug logging here + Logger.Debug("No control has keyboard focus"); + + // Consider whether to close all menu items when nothing has focus + // var menuItems = visual.GetLogicalAncestors().OfType<MenuItem>(); + // foreach (MenuItem menuItem in menuItems.ToArray()) + // menuItem.Close(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/MenuExtensions.cs
(1 hunks)
🔇 Additional comments (1)
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/MenuExtensions.cs (1)
42-50
: LGTM! Null check prevents NullReferenceException
The null check for focusedControl
properly prevents the NullReferenceException that would occur when calling GetLogicalAncestors()
on a null reference. The fix maintains the original menu closing behavior when a control has focus.
Co-authored-by: <[email protected]>
Fix for #151
Summary by CodeRabbit
Bug Fixes
New Features