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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

### Added

- [SIL.Windows.Forms] Added KeysExtensions class with the IsNavigationKey extension method.

### 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.

-
## [16.0.0] - 2025-05-20

### Added
Expand Down
1 change: 1 addition & 0 deletions Palaso.sln.DotSettings
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=GUI/@EntryIndexedValue">GUI</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=GUID/@EntryIndexedValue">GUID</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=IMDI/@EntryIndexedValue">IMDI</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=IME/@EntryIndexedValue">IME</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=ISO/@EntryIndexedValue">ISO</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=LDML/@EntryIndexedValue">LDML</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=LIFT/@EntryIndexedValue">LIFT</s:String>
Expand Down
123 changes: 68 additions & 55 deletions SIL.Windows.Forms.Keyboarding/Linux/IbusKeyboardSwitchingAdaptor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
context.SetEngine(ibusKeyboard.IBusKeyboardEngine.LongName);
}

private void SetImePreeditWindowLocationAndSize(Control control)
private void SetImePreEditWindowLocationAndSize(Control control)
{
var eventHandler = GetEventHandlerForControl(control);
if (eventHandler == null)
Expand Down Expand Up @@ -165,8 +165,7 @@
e.Control.PreviewKeyDown += HandlePreviewKeyDown;
e.Control.KeyPress += HandleKeyPress;

var scrollableControl = e.Control as ScrollableControl;
if (scrollableControl != null)
if (e.Control is ScrollableControl scrollableControl)
scrollableControl.Scroll += HandleScroll;
}

Expand All @@ -182,8 +181,7 @@
e.Control.KeyPress -= HandleKeyPress;
e.Control.KeyDown -= HandleKeyDownAfterIbusHandledKey;

var scrollableControl = e.Control as ScrollableControl;
if (scrollableControl != null)
if (e.Control is ScrollableControl scrollableControl)
scrollableControl.Scroll -= HandleScroll;

var eventHandler = GetEventHandlerForControl(e.Control);
Expand All @@ -202,7 +200,7 @@

/// <summary>
/// Passes the key event to ibus. This method deals with the special keys (Cursor up/down,
/// backspace etc) that usually shouldn't cause a commit.
/// backspace, etc.) that usually shouldn't cause a commit.
/// </summary>
private bool PassSpecialKeyEventToIbus(Control control, Keys keyChar, Keys modifierKeys)
{
Expand All @@ -212,8 +210,11 @@

private bool PassKeyEventToIbus(Control control, char keyChar, Keys modifierKeys)
{
if (keyChar == 0x7f) // we get this for Ctrl-Backspace
keyChar = '\b';
const char asciiDel = (char)0x7F;
const char backspace = '\b';

if (keyChar == asciiDel) // Ctrl+Backspace arrives as DEL; normalize to Backspace
keyChar = backspace;

return PassKeyEventToIbus(control, keyChar, modifierKeys, true);
}
Expand All @@ -235,7 +236,7 @@

if (resetIfUnhandled)
{
// If ProcessKeyEvent doesn't consume the key, we need to kill any preedits and
// If ProcessKeyEvent doesn't consume the key, we need to kill any pre-edits and
// sync before continuing processing the keypress. We return false so that the
// control can process the character.
ResetAndWaitForCommit(control);
Expand All @@ -254,8 +255,7 @@
// and the other time because we have to call the original window proc. However, only
// the second time will the control report as being focused (or when we not intercept
// the message then the first time) (see SimpleRootSite.OriginalWndProc).
var control = sender as Control;
if (control == null || !control.Focused)
if (!(sender is Control control) || !control.Focused)
return;

_ibusComm.FocusIn();
Expand All @@ -276,7 +276,7 @@

/// <summary>
/// Inform input bus of Keydown events
/// This is useful to get warning of key that should stop the preedit
/// This is useful to get warning of key that should stop the pre-edit
/// </summary>
private void HandlePreviewKeyDown(object sender, PreviewKeyDownEventArgs e)
{
Expand All @@ -290,49 +290,62 @@

if (_needIMELocation)
{
SetImePreeditWindowLocationAndSize(control);
SetImePreEditWindowLocationAndSize(control);
_needIMELocation = false;
}

var key = e.KeyCode;
switch (key)
if (key == Keys.Escape)
{
case Keys.Escape:
// These should end a preedit, so wait until that has happened
// before allowing the key to be processed.
ResetAndWaitForCommit(control);
return;
case Keys.Up:
case Keys.Down:
case Keys.Left:
case Keys.Right:
case Keys.Delete:
case Keys.PageUp:
case Keys.PageDown:
case Keys.Home:
case Keys.End:
case Keys.Back:
if (PassSpecialKeyEventToIbus(control, key, e.Modifiers))
{
// 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;
}
return;
// These should end a pre-edit, so wait until that has happened
// before allowing the key to be processed.
ResetAndWaitForCommit(control);
return;
}

if (IsKeyHandledByIbus(key))
{
if (PassSpecialKeyEventToIbus(control, key, e.Modifiers))
{
// 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 6 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.
}
return;
}

// pass function keys onto ibus since they don't appear (on mono at least) as WM_SYSCHAR
if (key >= Keys.F1 && key <= Keys.F24)
PassSpecialKeyEventToIbus(control, key, e.Modifiers);
}

/// <summary>
/// Handles a key down. While a preedit is active we don't want the control to handle
/// Handles a key down. While a pre-edit is active we don't want the control to handle
/// any of the keys that IBus deals with.
/// </summary>
private void HandleKeyDownAfterIbusHandledKey(object sender, KeyEventArgs e)
{
switch (e.KeyCode)
if (IsKeyHandledByIbus(e.KeyCode) && sender is Control control)
{
var eventHandler = GetEventHandlerForControl(control);
if (eventHandler != null)
e.SuppressKeyPress = eventHandler.IsPreeditActive;
control.KeyDown -= HandleKeyDownAfterIbusHandledKey;
}
}

/// <summary>
/// Gets whether the given key is expected to be handled by IBus when a pre-edit is active.
/// </summary>
/// <remarks>
/// 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.
Comment on lines +342 to +344
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)".

/// </remarks>
private bool IsKeyHandledByIbus(Keys key)
{
switch (key & Keys.KeyCode)
{
case Keys.Up:
case Keys.Down:
Expand All @@ -344,13 +357,10 @@
case Keys.Home:
case Keys.End:
case Keys.Back:
var control = sender as Control;
var eventHandler = GetEventHandlerForControl(control);
if (eventHandler != null)
e.SuppressKeyPress = eventHandler.IsPreeditActive;
control.KeyDown -= HandleKeyDownAfterIbusHandledKey;
break;
return true;
}

return false;
}

/// <summary>
Expand All @@ -360,17 +370,20 @@
/// this method gets called. We forward the key press to IBus. If IBus swallowed the key
/// it will return true, so no further handling is done by the control, otherwise the
/// control will process the key and update the selection.
/// If IBus swallows the key event, it will either raise the UpdatePreeditText event,
/// allowing the event handler to insert the composition as preedit (and update the
/// selection), or it will raise the CommitText event so that the event handler can
/// remove the preedit, replace it with the final composition string and update the
/// selection. Some IBus keyboards might raise a ForwardKeyEvent (handled by
/// <see cref="IIbusEventHandler.OnIbusKeyPress"/>) prior to calling CommitText to
/// simulate a key press (e.g. backspace) so that the event handler can modify the
/// existing text of the control.
/// If IBus swallows the key event, it will either raise the
/// <see cref="IIbusCommunicator.UpdatePreeditText"/> event, allowing the event handler to
/// insert the composition as pre-edit (and update the selection), or it will raise the
/// <see cref="IIbusCommunicator.CommitText"/> event so that the event handler can
/// remove the pre-edit, replace it with the final composition string and update the
/// selection. Some IBus keyboards might raise a
/// <see cref="IBusDotNet.IInputContext.ForwardKeyEvent"/> (handled by
/// <see cref="IIbusEventHandler.OnIbusKeyPress"/>) prior to raising
/// <see cref="IIbusCommunicator.CommitText"/> to simulate a key press (e.g. backspace)
/// so that the event handler can modify the existing text of the control.
/// IBus might also open a pop-up window at the location we told it
/// (<see cref="IIbusEventHandler.SelectionLocationAndHeight"/>) to display possible
/// compositions. However, it will still call UpdatePreeditText.</remarks>
/// compositions. However, it will still raise
/// <see cref="IIbusCommunicator.UpdatePreeditText"/>.</remarks>
private void HandleKeyPress(object sender, KeyPressEventArgs e)
{
e.Handled = PassKeyEventToIbus(sender as Control, e.KeyChar, Control.ModifierKeys);
Expand All @@ -390,7 +403,7 @@
if (!_ibusComm.Connected)
return;

SetImePreeditWindowLocationAndSize(sender as Control);
SetImePreEditWindowLocationAndSize(sender as Control);
}

#endregion
Expand Down
39 changes: 39 additions & 0 deletions SIL.Windows.Forms/Extensions/KeysExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
using System.Windows.Forms;

namespace SIL.Windows.Forms.Extensions
{
public static class KeysExtensions
{
/// <summary>
/// Determines whether the specified key is considered a navigation key.
/// </summary>
/// <param name="key">The key (or key-combination) to examine.</param>
/// <param name="ctrlPressed">If <c>true</c>, the letter <c>Keys.A</c> will be treated as a
/// navigation key (even if the <paramref name="key"/> value does not explicitly include
/// <c>Keys.Control</c>). In contexts where Ctrl-A is not a shortcut for Select All or that
/// behavior is not relevant, pass false or omit this optional parameter.</param>
/// <remarks>If <paramref name="key"/> represents the combination Ctrl-A (i.e.,
/// <c>Keys.Control | Keys.A</c>), but <paramref name="ctrlPressed"/> is <c>false</c>,
/// this method will return <c>false</c>.</remarks>
public static bool IsNavigationKey(this Keys key, bool ctrlPressed = false)
{
switch (key & Keys.KeyCode)
{
case Keys.Left:
case Keys.Up:
case Keys.Down:
case Keys.Right:
case Keys.Home:
case Keys.End:
case Keys.PageDown:
case Keys.PageUp:
return true;
case Keys.A:
return ctrlPressed;
default:
return false;
}
}
}

}
25 changes: 5 additions & 20 deletions SIL.Windows.Forms/Widgets/BetterGrid/CalendarEditingControl.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System;
using System.Drawing;
using System.Windows.Forms;
using SIL.Windows.Forms.Extensions;

namespace SIL.Windows.Forms.Widgets.BetterGrid
{
Expand Down Expand Up @@ -114,21 +115,8 @@ public void ApplyCellStyleToEditingControl(DataGridViewCellStyle dataGridViewCel
/// ------------------------------------------------------------------------------------
public bool EditingControlWantsInputKey(Keys key, bool dataGridViewWantsInputKey)
{
// Let the DateTimePicker handle the keys listed.
switch (key & Keys.KeyCode)
{
case Keys.Left:
case Keys.Up:
case Keys.Down:
case Keys.Right:
case Keys.Home:
case Keys.End:
case Keys.PageDown:
case Keys.PageUp:
return true;
default:
return !dataGridViewWantsInputKey;
}
// Let the DateTimePicker handle navigation keys.
return key.IsNavigationKey() || !dataGridViewWantsInputKey;
}

/// ------------------------------------------------------------------------------------
Expand All @@ -138,10 +126,7 @@ public void PrepareEditingControlForEdit(bool selectAll)
}

/// ------------------------------------------------------------------------------------
public bool RepositionEditingControlOnValueChange
{
get { return false; }
}
public bool RepositionEditingControlOnValueChange => false;

#endregion
}
Expand Down
Loading