-
Notifications
You must be signed in to change notification settings - Fork 0
Consolidate key input to use KeyDown only, removing TextInput handler #21
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
Conversation
Handle printable characters directly in KeyDown using KeySymbol property instead of deferring to the TextInput event. This simplifies the input handling path while maintaining proper keyboard layout support. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Greptile SummaryConsolidated keyboard input handling by removing the separate Key changes:
Potential concerns:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DisplayPanel
participant ViewerView
participant ViewerService
participant Connection
participant PresenterService
participant InputInjection
Note over User,DisplayPanel: Before: Two-path approach
User->>DisplayPanel: Press key
alt Printable character
DisplayPanel->>ViewerView: TextInput event (multi-char support)
ViewerView->>ViewerService: SendTextInputAsync(text)
ViewerService->>Connection: TextInputMessage
Connection->>PresenterService: HandleTextInput(text)
PresenterService->>InputInjection: InjectText(text)
else Special key/shortcut
DisplayPanel->>ViewerView: KeyDown event
ViewerView->>ViewerService: SendKeyDownAsync(keyCode, modifiers)
ViewerService->>Connection: KeyMessage
Connection->>PresenterService: HandleKey(keyCode, modifiers, true)
PresenterService->>InputInjection: InjectKey(keyCode, modifiers, true)
end
Note over User,DisplayPanel: After: Single-path approach
User->>DisplayPanel: Press key
DisplayPanel->>ViewerView: KeyDown event only
alt KeySymbol is single printable char
ViewerView->>ViewerService: SendTextInputAsync(KeySymbol)
ViewerService->>Connection: TextInputMessage
Connection->>PresenterService: HandleTextInput(text)
PresenterService->>InputInjection: InjectText(text)
else Non-printable key
ViewerView->>ViewerService: SendKeyDownAsync(keyCode, modifiers)
ViewerService->>Connection: KeyMessage
Connection->>PresenterService: HandleKey(keyCode, modifiers, true)
PresenterService->>InputInjection: InjectKey(keyCode, modifiers, true)
end
|
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.
2 files reviewed, 1 comment
| if (string.IsNullOrEmpty(e.KeySymbol) is false && | ||
| e.KeySymbol.Length == 1 && | ||
| char.IsControl(e.KeySymbol[0]) is false) | ||
| { | ||
| await this._viewModel.Connection.RequiredViewerService.SendTextInputAsync(e.KeySymbol); |
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.
logic: The single-character check e.KeySymbol.Length == 1 may break IME (Input Method Editor) support for languages like Chinese, Japanese, and Korean. The previous TextInput event could receive multi-character strings from IME composition (up to 100 chars), but KeyDown.KeySymbol only provides single characters. IME users may not be able to input complex characters that require composition. Does Avalonia's KeyDown.KeySymbol handle IME composition, or should IME be handled separately through TextInput events?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/RemoteViewer.Client/Views/Viewer/ViewerView.axaml.cs
Line: 257:261
Comment:
**logic:** The single-character check `e.KeySymbol.Length == 1` may break IME (Input Method Editor) support for languages like Chinese, Japanese, and Korean. The previous `TextInput` event could receive multi-character strings from IME composition (up to 100 chars), but `KeyDown.KeySymbol` only provides single characters. IME users may not be able to input complex characters that require composition. Does Avalonia's KeyDown.KeySymbol handle IME composition, or should IME be handled separately through TextInput events?
How can I resolve this? If you propose a fix, please make it concise.
Handle printable characters directly in KeyDown using KeySymbol property instead of deferring to the TextInput event. This simplifies the input handling path while maintaining proper keyboard layout support.