Skip to content

+semver:minor Added KeysExtensions class with the IsNavigationKey ext… #1437

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented May 23, 2025

…ension method.

Fixed a subtle bug in IbusKeyboardSwitchingAdaptor when determining whether IBus would have handled a key event while a pre-edit is active. The code now accounts for the possibility of modifier keys (particularly Ctrl), which IBus would presumably have handled when in combination with navigation keys, Backspace, and Delete.


This change is Reviewable

…ension method.

Fixed a subtle bug in IbusKeyboardSwitchingAdaptor when determining whether IBus would have handled a key event while a pre-edit is active. The code now accounts for the possibility of modifier keys (particularly Ctrl), which IBus would presumably have handled when in combination with navigation keys, Backspace, and Delete.
Copy link

github-actions bot commented May 23, 2025

Palaso Tests

     4 files  ±0       4 suites  ±0   10m 43s ⏱️ -24s
 4 978 tests ±0   4 744 ✅ +1  234 💤  - 1  0 ❌ ±0 
16 186 runs  ±0  15 482 ✅ +1  704 💤  - 1  0 ❌ ±0 

Results for commit 4242a32. ± Comparison against base commit 3590b5a.

♻️ This comment has been updated with latest results.

// If IBus handled the key we don't want the control to get it. However,
// we can't do this in PreviewKeyDown, so we temporarily subscribe to
// KeyDown and suppress the key event there.
control.KeyDown += HandleKeyDownAfterIbusHandledKey;

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
control
may be null at this access because of
this
assignment.

Copilot Autofix

AI 4 days ago

To fix the issue, we need to ensure that control is not null before it is dereferenced on line 313. This can be achieved by adding a null check for control after it is assigned on line 286. If control is null, the method should return early, as further operations depend on control being valid.

This fix ensures that the method behaves safely even if sender is not of type Control, preventing a potential NullReferenceException.


Suggested changeset 1
SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs b/SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs
--- a/SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs
+++ b/SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs
@@ -286,2 +286,4 @@
 			var control = sender as Control;
+			if (control == null)
+				return;
 			var eventHandler = GetEventHandlerForControl(control);
EOF
@@ -286,2 +286,4 @@
var control = sender as Control;
if (control == null)
return;
var eventHandler = GetEventHandlerForControl(control);
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


### Fixed

- [SIL.Windows.Kwyboarding] Fixed a subtle bug in IbusKeyboardSwitchingAdaptor when determining whether IBus would have handled a key event while a pre-edit is active. The code now accounts for the possibility of modifier keys (particularly Ctrl), which IBus would presumably have handled when in combination with navigation keys, Backspace, and Delete.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [SIL.Windows.Kwyboarding] Fixed a subtle bug in IbusKeyboardSwitchingAdaptor when determining whether IBus would have handled a key event while a pre-edit is active. The code now accounts for the possibility of modifier keys (particularly Ctrl), which IBus would presumably have handled when in combination with navigation keys, Backspace, and Delete.
- [SIL.Windows.Keyboarding] Fixed a subtle bug in IbusKeyboardSwitchingAdaptor when determining whether IBus would have handled a key event while a pre-edit is active. The code now accounts for the possibility of modifier keys (particularly Ctrl), which IBus would presumably have handled when in combination with navigation keys, Backspace, and Delete.

Comment on lines +342 to +344
/// REVIEW: During pre-edit, I assume that IBus handles both basic and modified navigation
/// keys like Ctrl+Left. ChatGPT says this is true, but I have not been able to verify this
/// since I don't know how to set up a Linux/IBus environment where I could test this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test this on Linux:

  • install Ubuntu 24.04 in a VM
  • install an IME that uses pre-edit: in a terminal run sudo apt update && sudo apt install ibus-pinyin
  • open Keyboard settings, click "+ Add Input Source", search for Pinyin by clicking on the three dots below English. If you type the search term "pinyin" you'll get "Other". Clicking that shows "Chinese (Pinyin)" which you then can add.
  • close the settings
  • in the terminal run ibus restart (or reboot)
  • the language/keyboard picker in the top bar should now show "Chinese (Pinyin)".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants