Add ConsoloniaAccessText underline #205
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new class, Additionally, the pull request modifies the 🔗 Possibly related PRs
👥 Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
The added conditions prevent text input when Alt or Control modifiers are active, which should fix the issue where pressing Alt+F was incorrectly inputting 'F' into input controls. This is a good solution as it ensures modifier key combinations are handled purely as commands/shortcuts rather than generating text input. Note that this change addresses only the first reported issue. The second issue regarding Alt key toggle behavior for underscores appears to be an Avalonia framework bug as mentioned in the PR comments. Let's verify the fix works as expected: ✅ Verification successfulThe initial results show multiple files handling keyboard events. Let's examine the key handlers in the core infrastructure files to ensure there are no conflicting implementations and that the Alt key handling is consistent across the codebase. The Alt key handling implementation is correctly isolated and consistent The code inspection confirms that the Alt key handling in ConsoleWindow.cs is properly implemented and doesn't conflict with other keyboard event handlers in the codebase. The changes prevent text input when Alt or Control modifiers are active by checking The only other Alt modifier handling found is in DefaultNetConsole.cs, which is just a mapping definition between ConsoleModifiers and RawInputModifiers, further confirming the consistency of the implementation. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any other keyboard handling code that might interfere
# Look for other keyboard event handlers that might affect Alt key behavior
rg -l "KeyEvent|RawKeyEventArgs|RawTextInputEventArgs|Alt.*modifier" --type cs
# Look for any AccessText-related keyboard handling
ast-grep --pattern 'class $_ {
$$$
(KeyDown|KeyUp|TextInput).*{
$$$
}
$$$
}'
Length of output: 1061 Script: #!/bin/bash
# Let's examine the key handling implementations in related files
rg -A 10 "KeyDown|KeyUp|TextInput|RawKeyEventArgs" src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
# Also check for any other Alt modifier handling
rg -A 5 "Alt.*modifier|RawInputModifiers.Alt" src/Consolonia.Core/Infrastructure/
Length of output: 8497 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: 3
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Core/Controls/ConsoloniaAccessText.cs (1)
9-11: Enhance XML documentationThe current documentation is minimal. Consider adding more details about the class's behavior and usage.
Add more comprehensive documentation:
/// <summary> -/// A text block that displays a character prefixed with an underscore as an access key. +/// A text block that displays a character prefixed with an underscore as an access key. +/// The access key character can be underlined when ShowAccessKey is true. /// </summary> +/// <remarks> +/// This control extends Avalonia's AccessText to provide visual feedback for access keys. +/// Example usage: "_File" will display as "File" with 'F' as the access key. +/// </remarks>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Controls/ConsoloniaAccessText.cs(1 hunks)src/Consolonia.Themes/Templates/Controls/Menu.axaml(2 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(2 hunks)
🔇 Additional comments (2)
src/Consolonia.Core/Controls/ConsoloniaAccessText.cs (1)
12-13: LGTM: Class inheritance and purpose are well-defined
The class appropriately extends Avalonia's AccessText and has a clear purpose.
src/Consolonia.Themes/Templates/Controls/Menu.axaml (1)
3-3: LGTM: Proper integration of ConsoloniaAccessText
The namespace declaration and control usage are correctly implemented with appropriate property bindings.
Also applies to: 28-30
| var iPos = Text.IndexOf('_', StringComparison.Ordinal); | ||
| if (iPos >= 0 && iPos < Text.Length - 1) | ||
| { | ||
| inlines.Add(new Run(Text.Substring(0, iPos))); |
There was a problem hiding this comment.
Just a hint: Text[..iPos] or Text[iPos..]
There was a problem hiding this comment.
oh, fancy. I totally forgot about that new feature. It hasn't been integrated into my coding patterns.
There was a problem hiding this comment.
Holy shit, I just tried it...so clean, so readable, so succinct. .Substring always felt so verbose. I love it! Thanks for the tip!
Fix bug on keyup where Alt modifier not being passed for Alt keys. Added Labels page.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
26-43: Consider adding error handling and null checksWhile the implementation looks good overall, consider these improvements:
- Add null check for DataTemplates collection
- Add error handling for ConsoloniaAccessText creation
- Consider performance impact of type checking all unknown data types
Consider this enhanced implementation:
// override AccessText to use ConsoloniaAccessText as default contentpresenter for unknown data types (aka string) +if (this.DataTemplates == null) + throw new InvalidOperationException("DataTemplates collection is not initialized"); + this.DataTemplates.Add(new FuncDataTemplate<object>( (data, s) => { if (data != null && data is string str) { try { var result = new ConsoloniaAccessText(); result.Bind(TextBlock.TextProperty, result.GetObservable(Control.DataContextProperty)); return result; } catch (Exception ex) { // Log error or handle appropriately return null; } } return null; }, true) );src/Consolonia.Gallery/Gallery/GalleryViews/GalleryLabel.axaml (1)
9-9: Remove or update the unrelated commentThe comment references TextBox.xaml from Avalonia's default theme, but this implementation appears to be a custom form layout rather than a TextBox style.
- <!--Copied from https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Themes.Default/TextBox.xaml-->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Consolonia.Core/Controls/ConsoloniaAccessText.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryLabel.axaml(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryLabel.axaml.cs(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(2 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Controls/ConsoloniaAccessText.cs
🔇 Additional comments (2)
src/Consolonia.Gallery/View/ControlsListView.axaml (2)
23-23: LGTM: Access key added for Theme label
The underscore before "Theme" correctly defines 'T' as the access key for keyboard navigation.
23-23: Verify integration with ConsoloniaAccessText
Since these changes are part of introducing ConsoloniaAccessText, we should verify if these controls need to be updated to use the new ConsoloniaAccessText control for consistent underline behavior.
Also applies to: 39-40
✅ Verification successful
ConsoloniaAccessText is only used for menu items, no changes needed
The verification shows that ConsoloniaAccessText is specifically designed for menu-related controls (Menu and MenuItem) to handle their access key underline behavior. Regular Labels don't need this functionality as they use a simpler underscore prefix for access keys. The current implementation in ControlsListView.axaml is correct and consistent with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other XAML files are using ConsoloniaAccessText
rg -l "ConsoloniaAccessText" --type xaml
# Check the style/template definitions for Label and Button
fd -e axaml -e xaml | xargs rg "Style.*Target(Type)?=\"(Label|Button)\""
Length of output: 142
Script:
#!/bin/bash
# Search for ConsoloniaAccessText in all files
rg -l "ConsoloniaAccessText"
# Search for Label and Button styles in all files
rg "Style.*Target(Type)?=\"(Label|Button)\""
# List all XAML/AXAML files to understand the codebase structure
fd -e xaml -e axaml
# Check content of the specific file
cat "src/Consolonia.Gallery/View/ControlsListView.axaml"
Length of output: 8251
Script:
#!/bin/bash
# Check Menu and MenuItem templates since they use ConsoloniaAccessText
cat "src/Consolonia.Themes/Templates/Controls/Menu.axaml" "src/Consolonia.Themes/Templates/Controls/MenuItem.axaml"
# Check Label template/style definitions
fd "Label.axaml" || rg -l "Style.*Label"
# Check the ConsoloniaAccessText implementation
cat "src/Consolonia.Core/Controls/ConsoloniaAccessText.cs"
Length of output: 16067
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (3)
26-43: Consider performance implications of global templateThe template is evaluated for every unknown data type, not just strings. This could impact performance in data-heavy scenarios.
Consider:
- Using a more specific template scope if possible
- Adding a comment explaining why this global approach was chosen
- Document potential performance implications in the class documentation
32-35: Document the binding behaviorThe binding between TextBlock.TextProperty and DataContext should be documented to explain the text rendering process.
Add a comment explaining the binding:
var result = new ConsoloniaAccessText(); +// Bind the text content to the DataContext, allowing dynamic updates result.Bind(TextBlock.TextProperty, result.GetObservable(Control.DataContextProperty));
26-44: Add usage documentation to help other developersGiven the PR comment about difficulty locating features, consider adding:
- XML documentation to the class explaining the template's purpose
- Example usage in README or documentation
- Sample XAML showing how to override this behavior when needed
Would you like me to help create documentation templates or examples?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Consolonia.Core/Controls/ConsoloniaAccessText.cs(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Themes/Templates/Controls/Menu.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Consolonia.Themes/Templates/Controls/MenuItem.axaml
- src/Consolonia.Themes/Templates/Controls/Menu.axaml
- src/Consolonia.Core/Controls/ConsoloniaAccessText.cs
🔇 Additional comments (2)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (2)
5-5: LGTM: Required imports added appropriately
The new imports are necessary for the template and control functionality being introduced.
Also applies to: 7-7
30-40: Verify template behavior with different string scenarios
The template creates a new ConsoloniaAccessText instance for every string. Let's verify the behavior with different scenarios.
✅ Verification successful
The previous attempt failed because of file type restrictions. Let's try a broader search to understand the usage patterns and implementation details of ConsoloniaAccessText and related components.
Template behavior with strings is correctly implemented
The implementation is appropriate and safe for handling string scenarios:
- It's a global DataTemplate that only creates ConsoloniaAccessText for non-null string values
- The binding is properly set up to reflect DataContext changes
- ConsoloniaAccessText is a sealed class inheriting from AccessText, ensuring consistent behavior
- The template is registered at application level as a fallback for string content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing string template usage to understand impact
rg -l "DataTemplate.*string" --type xaml
# Search for potential string-to-AccessText conversions that might be affected
ast-grep --pattern 'AccessText { Text = $_ }'
Length of output: 119
Script:
#!/bin/bash
# Search for ConsoloniaAccessText usage and implementation
rg -A 5 "ConsoloniaAccessText"
# Search for DataTemplate string handling
rg -A 5 "DataTemplate.*string"
# Search for AccessText related code
rg -A 5 "AccessText"
Length of output: 5218
|
it works almost. Toggle of Alt accelerator works only after the FIRST time I use Menu
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Themes/Templates/Controls/Menu.axaml (2)
Line range hint
21-24: Add ConsoloniaAccessText for proper access key handlingThe ContentPresenter is directly binding to Header without handling access keys. To address the underline feature and Alt key behavior issues mentioned in the PR, implement ConsoloniaAccessText here.
Apply this diff:
<ContentPresenter Name="PART_HeaderPresenter" Content="{TemplateBinding Header}" - Margin="{TemplateBinding Padding}"> + Margin="{TemplateBinding Padding}"> + <ContentPresenter.DataTemplates> + <DataTemplate DataType="System.String"> + <controls:ConsoloniaAccessText Text="{Binding}" /> + </DataTemplate> + </ContentPresenter.DataTemplates> </ContentPresenter>
Line range hint
1-93: Consider implementing a centralized access key managerTo address the reported issues comprehensively:
- The Alt+F key input issue suggests we need proper event handling at the control level
- The inconsistent Alt key toggle behavior indicates we need centralized state management
- The access key visibility persistence issue suggests we need proper state storage
Consider implementing a centralized access key manager that:
- Manages the global Alt key state
- Handles access key visibility consistently
- Persists the state across navigation
- Provides proper event handling for access key combinations
This would help ensure consistent behavior across the entire menu system.
Would you like me to help design this centralized access key manager?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Themes/Templates/Controls/Menu.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/MenuItem.axaml(0 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Themes/Templates/Controls/MenuItem.axaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Themes/Templates/Controls/Menu.axaml (1)
Learnt from: jinek
PR: jinek/Consolonia#176
File: src/Consolonia.Themes.TurboVision/Templates/Controls/MenuItem.axaml:131-131
Timestamp: 2024-12-05T18:23:58.201Z
Learning: In the Avalonia UI project, within `src/Consolonia.Themes.TurboVision/Templates/Controls/MenuItem.axaml`, MenuItem controls are intentionally designed without a `:pointerover` state to maintain touch-friendliness and consistency across controls.
🔇 Additional comments (2)
src/Consolonia.Themes/Templates/Controls/Menu.axaml (2)
Line range hint 39-47: Review ItemsPresenter focus behavior for Alt key toggle
The Alt key toggle issue (underscore visibility) might be related to focus handling in ItemsPresenter. The current FocusOnLoad behavior might need adjustment.
Let's check the focus handling implementation:
✅ Verification successful
Let me gather more information about the focus handling implementation to better understand the behavior.
Alt key toggle and focus behavior are correctly implemented
The implementation shows that:
MenuExtensions.FocusOnLoadcorrectly handles focus management by focusing the first MenuItem when attached to the visual treeConsoloniaAccessTextproperly manages the underscore visibility through theShowAccessKeyproperty, which controls text decorations for access keys
The current implementation follows the expected behavior for both focus handling and Alt key toggling of underscores. No adjustments are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for focus-related code
rg -t cs "FocusOnLoad|ShowAccessKey" --glob "src/Consolonia*/**"
# Look for Alt key handling
ast-grep --pattern 'class $_ {
$$$
ShowAccessKey($$$) {
$$$
}
}'
Length of output: 1399
Script:
#!/bin/bash
# Check MenuExtensions implementation
cat "src/Consolonia.Themes/Templates/Controls/Helpers/MenuExtensions.cs"
# Check ConsoloniaAccessText implementation for Alt key handling
cat "src/Consolonia.Core/Controls/ConsoloniaAccessText.cs"
Length of output: 5479
Line range hint 28-29: Review Popup focus handling for Alt+F key issue
The Alt+F key issue where the 'F' character is being input might be related to focus handling in the Popup. Consider handling the key press event here.
Let's verify the key handling implementation:
Co-authored-by: <[email protected]>
|
The second is an avalonia bug. I just repoed in avalonia proper. |
|
First one I can check. |
| if (iPos >= 0 && iPos < Text.Length - 1) | ||
| { | ||
| inlines.Add(new Run(Text[..iPos])); | ||
| _accessRun = new Run(Text[++iPos..++iPos]); |
Could you please push the repro of label+input somewhere? |
|
It should be pushed....there should be a Label page in gallery
|
|
I'm ready to check it in. Sorry. I was ready, then based on feedback on avalonia side I made some changes, and then feedback from you I added the Label/Button support. etc. I will try to be more disclipined about using Draft PR. Sorry. |
no worries. But I want to investigate ALT handling also, the guy you've changed. Thought I have investigated the issue of producing excessive F. |
|
I bet we should be awaiting the InvokeAsync. There are 2 patterns, there is Post() and Invoke(). Invoke returns a task, we should be careful about the semantics of which one we are using. |
This one excluded. Task returned from Invoke properly awaits for access key to be processed. I could not find matching code in Avalonia at all. Thinking may be I just invented this processing: if KeyDown event was not handled, then producing TextInput event. |
|
Yeah, that helped. fixed first issue |
|
Second issue is Avalonia bug. It looks like we can work around by having an empty MENU on the page. |
|
@tomlm |
|
Looks good. |
|
Thanks Evgeny! If you are ever in Seattle I'll buy you a beer! |
|
No, I will buy you a beer. 😃 |
Fix for #196
