accesskey always on for non windows #207
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new boolean property The implementation varies across different console types, with some returning Accompanying the property addition, there are subtle changes in input handling methods like The changes appear to be part of a broader effort to standardize and enhance input handling across different console platforms, providing a more consistent interface for managing console input and access key behaviors. Possibly related PRs
Suggested labels
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Consolonia.Core/Helpers/Extensions.cs (1)
19-23: Excellent improvement to subscription management!Returning
IDisposablefromSubscribeActionallows proper cleanup of subscriptions, preventing potential memory leaks. This change follows the standard Observable pattern and best practices.Make sure to dispose of the subscription when it's no longer needed, typically in component cleanup or disposal methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Consolonia.Core/Dummy/DummyConsole.cs(1 hunks)src/Consolonia.Core/Helpers/Extensions.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs(1 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(1 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(2 hunks)src/Consolonia.NUnit/UnitTestConsole.cs(1 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs(1 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(1 hunks)
🔇 Additional comments (10)
src/Consolonia.PlatformSupport/CursesConsole.cs (1)
143-143: Correct Override of SupportsAltSolo Property
The SupportsAltSolo property is correctly overridden to return false, which is appropriate for the CursesConsole implementation.
src/Consolonia.Core/Infrastructure/IConsole.cs (1)
23-23: Addition of SupportsAltSolo Property to IConsole Interface
The SupportsAltSolo property has been added to the IConsole interface, ensuring that all console implementations specify their support for Alt solo mode.
src/Consolonia.Core/Dummy/DummyConsole.cs (1)
36-36: Implementation of SupportsAltSolo Property in DummyConsole
The SupportsAltSolo property is implemented and returns false, which is suitable for the DummyConsole.
src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs (1)
56-57: Override of SupportsAltSolo Property in DefaultNetConsole
The SupportsAltSolo property is correctly overridden to return false, aligning with the behavior of DefaultNetConsole.
src/Consolonia.NUnit/UnitTestConsole.cs (1)
42-42: LGTM! Property implementation is appropriate for test scenarios.
Setting SupportsAltSolo to true in the test console allows testing both supported and unsupported alt key scenarios through dependency injection.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
14-14: Well-structured architectural changes!
Making the class abstract and adding the abstract SupportsAltSolo property enforces proper implementation in derived classes, ensuring explicit handling of alt key support across different platforms.
Let's verify that all derived classes implement the property:
Also applies to: 51-51
✅ Verification successful
All derived classes properly implement the abstract property
The verification confirms that all three derived classes (Win32Console, CursesConsole, and DefaultNetConsole) correctly implement the SupportsAltSolo property with appropriate platform-specific values:
- Windows implementation returns
true - Curses and default implementations return
false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all console implementations that should implement SupportsAltSolo
ast-grep --pattern 'class $_ : InputLessDefaultNetConsole {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Let's try a different approach to find derived classes
rg "class.*:.*InputLessDefaultNetConsole" -l
Length of output: 199
Script:
#!/bin/bash
# Also search for potential implementations with whitespace variations
rg "class\s+\w+\s*:\s*InputLessDefaultNetConsole" --type cs
Length of output: 397
Script:
#!/bin/bash
# Let's also check if there are any references to this base class
rg "InputLessDefaultNetConsole" --type cs
Length of output: 618
Script:
#!/bin/bash
# Let's check if these derived classes implement SupportsAltSolo
for file in src/Consolonia.PlatformSupport/WindowsConsole.cs src/Consolonia.PlatformSupport/CursesConsole.cs src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs; do
echo "=== $file ==="
rg "SupportsAltSolo" "$file" -B 1 -A 1
done
Length of output: 1297
src/Consolonia.PlatformSupport/WindowsConsole.cs (1)
64-64: LGTM! Property implementation aligns with Windows console capabilities.
The implementation correctly indicates that Windows console supports Alt solo key combinations.
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (3)
33-34: LGTM! Fields properly encapsulate access key state.
The fields are appropriately scoped and named to manage the access key state.
52-57: LGTM! Robust implementation of access key property change handler.
The method correctly:
- Guards against non-input root changes
- Maintains access key visibility when required
- Uses early returns for better readability
68-69: LGTM! Proper initialization of access keys state.
The implementation correctly sets the initial state of access keys when setting the input root.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
52-57: Add XML documentation to clarify the method's purpose.While the implementation is correct, adding documentation would help explain why we're forcing access keys to stay visible.
+ /// <summary> + /// Ensures access keys remain visible when they are required to be always on. + /// This prevents access keys from being hidden when running on platforms that don't support Alt solo keys. + /// </summary> + /// <param name="args">The property change event arguments.</param> private void OnShowAccessKeyPropertyChanged(AvaloniaPropertyChangedEventArgs<bool> args)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)
🔇 Additional comments (4)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (4)
33-34: LGTM! Field declarations are well-structured.
The new fields follow proper naming conventions and encapsulation principles.
68-69: LGTM! Consistent with the access keys always-on behavior.
The modification ensures access keys are visible when required, maintaining consistency with the class's purpose.
448-448: Previous null reference issue has been properly addressed.
The use of the null-conditional operator ensures safe disposal of the subscription.
47-50: Verify consistent behavior across different console implementations.
The initialization logic is correct, but let's verify that all console implementations properly implement the SupportsAltSolo property.
✅ Verification successful
SupportsAltSolo property is consistently implemented across all console implementations
The verification shows that all console implementations properly handle the SupportsAltSolo property:
- Windows Console:
true(native Alt key support) - Curses Console:
false(no Alt key support) - Default .NET Console:
false - Unit Test Console:
true(for testing purposes) - Dummy Console:
false
The initialization logic in ConsoleWindow.cs correctly handles both supported and unsupported cases by enabling always-on access keys when Alt key is not supported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SupportsAltSolo implementation across console classes
# Expected: Find all console implementations and their SupportsAltSolo property implementations
# Search for console classes implementing SupportsAltSolo
ast-grep --pattern 'class $_ : $_ {
$$$
SupportsAltSolo $$$
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach to find SupportsAltSolo implementations
# First, let's find files containing "SupportsAltSolo"
rg -l "SupportsAltSolo"
# Then get the actual implementation context
rg "SupportsAltSolo" -B 2 -A 2
Length of output: 3713
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Consolonia.PlatformSupport/CursesConsole.cs (2)
Line range hint
261-262: Remove redundant comment about key combination limitation.The comment about the inability to differentiate between Ctrl+Alt+alfa and Ctrl+Shift+Alt+alfa is repeated. Consider documenting this limitation in a central location, such as the class documentation.
Line range hint
1-585: Consider improving test coverage and documentation.Given the complexity of key handling logic, especially around modifier key combinations, consider:
- Adding comprehensive unit tests for various key combinations
- Documenting platform-specific limitations in the class XML documentation
- Creating a test matrix for all supported key combinations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs(1 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs(1 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Infrastructure/IConsole.cs
- src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
Learnt from: jinek
PR: jinek/Consolonia#207
File: src/Consolonia.Core/Infrastructure/ConsoleWindow.cs:0-0
Timestamp: 2024-12-15T01:48:55.826Z
Learning: Once the issue has been solved, I should be able to mark it as resolved myself.
🔇 Additional comments (7)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (4)
26-27: LGTM! Field declarations are well-designed.
The new fields follow good practices with appropriate types and immutability.
46-49: LGTM! Constructor initialization is correct.
The access key management is properly initialized based on console capabilities, with appropriate conditional subscription setup.
60-61: LGTM! SetInputRoot modification is appropriate.
The ShowAccessKeys property is correctly set based on the _accessKeysAlwaysOn flag.
448-448: LGTM! Proper disposal of subscription.
The _accessKeysAlwaysOnDisposable is correctly disposed with a null check.
src/Consolonia.PlatformSupport/CursesConsole.cs (3)
118-119: LGTM: Property addition is correct and aligns with PR objective.
The SupportsAltSolo property correctly indicates that this Curses-based console implementation doesn't support solo Alt key usage.
118-119: Verify the input handling during IO pause.
The addition of Curses.unget_wch((int)Key.Unknown) looks correct. This ensures proper input handling during IO pausing by injecting a sentinel key event.
✅ Verification successful
Input handling during IO pause is correctly implemented
The verification confirms that Key.Unknown is appropriately used as a sentinel value in the codebase:
- It's the default value for key events
- It's mapped to
ConsoleKey.NoNamein key mappings - It's used as a fallback in key processing
- The
unget_wchcall withKey.UnknowninPauseIOfollows the established input handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of PauseIO to verify consistency
ast-grep --pattern 'public override void PauseIO(Task task) {
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Search for PauseIO method implementations with a more flexible pattern
ast-grep --pattern 'PauseIO($$$) {
$$$
}'
# Also search for any references to unget_wch to understand the input handling pattern
rg "unget_wch" -A 2 -B 2
Length of output: 1852
Script:
#!/bin/bash
# Let's look for the base implementation and Key.Unknown usage
rg "class.*Console.*PauseIO" -A 5
# Also check for Key.Unknown definition and usage
rg "Key\.Unknown" -A 2 -B 2
Length of output: 2052
Line range hint 306-306: Investigate and resolve ReSharper warning.
The comment // ReSharper disable once ConditionIsAlwaysTrueOrFalse todo: check why indicates an unresolved issue. This should be investigated and either fixed or properly documented.
✅ Verification successful
ReSharper warning can be safely suppressed - condition is valid
The warning appears to be a false positive. Looking at the Key enum definition, we can see that uppercase letters A-Z are defined with values starting from 65 (A) and continuing sequentially. The condition wch >= (uint)Key.A && wch <= (uint)Key.Z is a valid range check for uppercase letters, as it compares against these enum values. The comment can be updated to explain why the condition is valid instead of marking it as a todo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the condition context
ast-grep --pattern 'if (wch >= (uint)Key.A && wch <= (uint)Key.Z)
{
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Search for the ReSharper warning comment and surrounding context
rg -B 2 -A 2 "ReSharper disable once ConditionIsAlwaysTrueOrFalse" src/Consolonia.PlatformSupport/CursesConsole.cs
Length of output: 470
Script:
#!/bin/bash
# Let's get more context around this condition to understand the logic better
rg -B 10 -A 10 "ReSharper disable once ConditionIsAlwaysTrueOrFalse" src/Consolonia.PlatformSupport/CursesConsole.cs
# Also check for Key enum definition to understand the value ranges
rg -l "enum Key"
Length of output: 1372
Script:
#!/bin/bash
# Let's check the Key enum definition to understand the values
rg -A 30 "enum Key" src/Consolonia.GuiCS/Event.cs
Length of output: 984
Script:
#!/bin/bash
# Let's get more of the Key enum definition to see the A-Z values
rg -A 100 "enum Key" src/Consolonia.GuiCS/Event.cs
# Also get the full method context where this condition is used
ast-grep --pattern 'MapCursesKey($_) {
$$$
}'
Length of output: 2205
btw what do you mean disable? I'm assuming not to disable anything in this PR |
|
When I tried the branch on ubuntu it didn't display any accelerators...it wasn't clear if you were trying to disable showing the accelerators or trying to turn them on permentaly. |
|
Let me investigate further then |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Consolonia.PlatformSupport/CursesConsole.cs (2)
Line range hint
149-426: Consider improving documentation and maintainability of input handling.The input handling logic is complex with many special cases. Consider:
- Adding documentation explaining the key mapping strategy
- Breaking down the large
ProcessInputmethod into smaller, focused methods- Adding unit tests for different key combinations
Example structure:
private void ProcessInput() { + // Get the next character/key int code = Curses.get_wch(out int wch); if (code == Curses.ERR) return; _keyModifiers = new KeyModifiers(); - var k = Key.Unknown; + Key k = ProcessKeyCode(code, wch); + RaiseKeyPressInternal(k); } +/// <summary> +/// Processes a key code and returns the corresponding Key value +/// </summary> +private Key ProcessKeyCode(int code, int wch) +{ + if (code == Curses.KEY_CODE_YES) + return ProcessKeyCodeYes(wch); + + return ProcessRegularKey(wch); +}
Line range hint
428-489: Consider adding XML documentation for key translation methods.The key translation logic in
RaiseKeyPressInternalis crucial for proper input handling. Consider adding XML documentation to explain:
- The purpose of key translation
- The relationship between different key representations
- The handling of special cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Helpers/Extensions.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Core/Helpers/Extensions.cs
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
🔇 Additional comments (2)
src/Consolonia.PlatformSupport/CursesConsole.cs (2)
118-119: LGTM! Property implementation aligns with platform limitations.
The implementation correctly indicates that ALT key combinations are not supported in Curses-based terminals, which aligns with the PR objective of making access keys always visible on non-Windows platforms.
Line range hint 141-145: Verify the impact of injecting Key.Unknown during IO pause.
While the implementation looks correct, injecting Key.Unknown into the input stream could potentially affect subsequent input handling. Please verify that this change:
- Doesn't interfere with normal key processing after resuming IO
- Doesn't cause any input buffering issues
✅ Verification successful
Let me analyze the findings and generate one more verification to fully understand the input processing flow.
Based on the analysis of the code, I can now provide a final response:
The injection of Key.Unknown during IO pause is safe and correctly handled.
The verification shows that:
Key.Unknownis already a well-handled value in the input processing flow, used as a default/fallback key in multiple places- The input processing system properly handles
Key.Unknownthrough the key mapping system, where it's explicitly mapped toConsoleKey.NoName - The
unget_wchis only used once in the codebase, specifically for this IO pause scenario, which means it won't interfere with other key processing logic - The key event handling system is designed to properly process both known and unknown keys, making this implementation safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of unget_wch to understand the impact
ast-grep --pattern 'Curses.unget_wch($_)'
# Search for Key.Unknown handling to verify proper processing
rg -A 5 'Key\.Unknown'
Length of output: 2346
Script:
#!/bin/bash
# Search for ProcessInput method implementation to understand how Key.Unknown is handled
ast-grep --pattern 'ProcessInput() {
$$$
}'
# Search for any input processing or key handling methods that might be affected
rg -A 10 'ProcessKey|HandleKey|OnKey'
Length of output: 23697
default ubuntu terminal? Could you please check if |
|
it's good. I didn't notice there was a -linux and -linux2 branch, I was looking at the wrong one. |
No description provided.