Skip to content

Fix three P1 Android-only bugs (B4/B5/B6)#5

Open
afernandes wants to merge 3 commits into
mainfrom
claude/round4-remaining-p2
Open

Fix three P1 Android-only bugs (B4/B5/B6)#5
afernandes wants to merge 3 commits into
mainfrom
claude/round4-remaining-p2

Conversation

@afernandes

@afernandes afernandes commented May 31, 2026

Copy link
Copy Markdown
Owner

B4: Detect Win as modifier on Android via IsMetaPressed, not only when the
Window keycode is pressed. Matches the Windows handler which derives the
Win modifier from GetKeyState. Hotkeys like "Win+A" now fire on both
platforms.

B5: Detect PrintScreen and PauseBreak on Android (Keycode.Sysrq /
Keycode.Break). Both were already wired on the Windows side but silently
absent on Android.

B6: Remove the Space char mapping from the Android KeyboardHelper. On
Android, SpaceKey was set by the handler AND Character was set to ' ' by
the helper, producing ToString() = " +Space". On Windows the same event
yields "Space" (Character stays null). This made "Space" / "Ctrl+Space"
hotkeys unreachable on Android. The cross-platform contract now matches.

Bumps to 1.0.4 with release notes.

Summary by CodeRabbit

  • New Features

    • Added hotkey unregistration/removal APIs
    • Added StopOnHandled option to control handler dispatch chain behavior
  • Bug Fixes

    • Key dispatches after service disposal no longer throw exceptions
    • Hotkey action exceptions are now caught and logged instead of propagating
    • Barcode buffer clears properly even when event subscribers throw
    • Improved Android key detection for PrintScreen, PauseBreak, Windows, and Space keys
    • Fixed race condition in Android key event processing
  • Breaking Changes

    • KeyHandlerService constructor now requires KeyHandlerOptions parameter

claude added 3 commits May 31, 2026 15:08
B4: Detect Win as modifier on Android via IsMetaPressed, not only when the
Window keycode is pressed. Matches the Windows handler which derives the
Win modifier from GetKeyState. Hotkeys like "Win+A" now fire on both
platforms.

B5: Detect PrintScreen and PauseBreak on Android (Keycode.Sysrq /
Keycode.Break). Both were already wired on the Windows side but silently
absent on Android.

B6: Remove the Space char mapping from the Android KeyboardHelper. On
Android, SpaceKey was set by the handler AND Character was set to ' ' by
the helper, producing ToString() = " +Space". On Windows the same event
yields "Space" (Character stays null). This made "Space" / "Ctrl+Space"
hotkeys unreachable on Android. The cross-platform contract now matches.

Bumps to 1.0.4 with release notes.
B7: KeyHandlerService.HandleKeyPress now returns silently when disposed
instead of calling ThrowIfDisposed. The method is invoked from the
platform input thread for events already in flight after Dispose;
throwing there crashes the platform input pipeline.

B8: HotkeyHandler.HandleKey wraps the user action in a try/catch inside
the MainThread closure. Exceptions from user code no longer escape onto
the UI thread (where KeyHandlerService's outer catch cannot reach them).

B9: KeyEventCallback.DispatchKeyEvent catches ObjectDisposedException
from the AndroidKeyHandler and falls through to the original dispatcher.
Closes a race window between Dispose() restoring the original callback
and an event already on the wire.

B13: BarcodeHandler.OnBarcodeScanned wraps the invocation in
try/finally so the internal buffer is cleared even when a subscriber
throws. Otherwise the failed value would prefix the next scan.

B14: KeyEventArgs.PlatformEvent is now declared object? to match its
actual nullable usage (ToString already used the null-conditional
operator on it, and HotkeyHandler creates instances without setting it).

Tests in P2FixesTests cover B7/B8/B13/B14. B9 is Android-specific and
not exercisable from the netstandard test project.

Release notes updated under 1.0.4 (same version as the P1 batch — both
ship together).
B10 (feature): HotkeyHandler.UnregisterHotkey(string), UnregisterHotkey
(KeyEventArgs) and ClearHotkeys(). The string overload uses the same
NormalizeHotkey path as Register, so modifier order and aliases match
("Control+Shift+X" unregisters a "Shift+Ctrl+X" registration).

B11 (feature, opt-in): KeyHandlerOptions.StopOnHandled. When set,
KeyHandlerService stops invoking subsequent handlers as soon as one
sets key.Handled = true. Default is false to preserve the historical
fan-out behavior where every handler sees every event.

  Constructor change: KeyHandlerService now takes a KeyHandlerOptions
  parameter. This is breaking for code that instantiates the service
  manually, but transparent for the DI path. KeyHandlerOptions has been
  a singleton in the container since 1.0.0.

B12 (perf): KeyboardHelper.ToChar(char) overload removes the
char -> string -> span -> char round-trip on the Android hot path.
Math operator chars ('.', '/', '+', '-', '*') were added to
SingleCharKeys so the new overload can short-circuit without a
dictionary lookup, preserving the behavior of the string overload for
every input that DisplayLabel can actually produce.

Tests in Round4FixesTests cover B10 (all overloads + normalization +
null/empty guards) and B11 (default off, on, and on-but-not-handled).
B12 is exercised indirectly via the existing Android code path.

Bumps to 1.1.0 (feature bump, not a fix bump, because of the new API
surface and the opt-in dispatch-stop behavior).
Copilot AI review requested due to automatic review settings May 31, 2026 21:54
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces P2 and P4 robustness improvements across keyboard event handling. It adds a StopOnHandled dispatch-chain control option, hotkey unregister lifecycle methods, exception isolation in handlers, and Android platform optimizations for key character extraction and system key mapping. The service now gracefully handles post-disposal key events instead of throwing. All changes are covered by new test suites.

Changes

P2/P4 Robustness and Hotkey Lifecycle Improvements

Layer / File(s) Summary
Configuration and type surface
GlobalKeyboardCapture.Maui/Configuration/KeyHandlerOptions.cs, GlobalKeyboardCapture.Maui/Core/Models/KeyEventArgs.cs, GlobalKeyboardCapture.Maui/Core/Services/KeyHandlerService.cs (using)
KeyHandlerOptions gains StopOnHandled property to control whether handler dispatch stops after an event is marked handled; KeyEventArgs.PlatformEvent is changed to nullable object? to reflect optional platform event presence.
KeyHandlerService core updates
GlobalKeyboardCapture.Maui/Core/Services/KeyHandlerService.cs
Constructor now accepts KeyHandlerOptions instance; key handling gracefully returns when invoked after disposal instead of throwing; handler iteration conditionally stops when StopOnHandled is true and event is marked handled.
HotkeyHandler unregister and exception isolation
GlobalKeyboardCapture.Maui/Handlers/HotkeyHandler.cs
Adds public UnregisterHotkey(string) and UnregisterHotkey(KeyEventArgs) methods to remove previously registered hotkeys with consistent modifier normalization; wraps hotkey action execution in try/catch to isolate user-code exceptions and log them instead of propagating.
BarcodeHandler buffer clearing robustness
GlobalKeyboardCapture.Maui/Handlers/BarcodeHandler.cs
OnBarcodeScanned uses try/finally to guarantee _buffer.Clear() executes even if a BarcodeScanned event subscriber throws an exception.
Android platform key handling and performance optimizations
GlobalKeyboardCapture.Maui/Platforms/Android/AndroidKeyHandler.cs, GlobalKeyboardCapture.Maui/Platforms/Android/KeyboardHelper.cs, GlobalKeyboardCapture.Maui/Platforms/Android/KeyEventCallback.cs
Updates Windows key detection to check IsMetaPressed in addition to key code; adds PrintScreenKey and PauseBreakKey system-key mappings; optimizes character extraction by deferring ToString() conversion; adds allocation-free ToChar(char) overload; excludes space from character maps; wraps handler dispatch in try/catch to handle disposal-race ObjectDisposedException.
Test infrastructure and comprehensive robustness coverage
GlobalKeyboardCapture.Maui.Tests/KeyHandlerServiceTests.cs, GlobalKeyboardCapture.Maui.Tests/P2FixesTests.cs, GlobalKeyboardCapture.Maui.Tests/Round4FixesTests.cs
Test factory updated to wire KeyHandlerOptions explicitly; new P2FixesTests suite validates post-disposal event ignoring, hotkey action exception isolation with continued firing, barcode buffer cleanup on subscriber exceptions, and nullable PlatformEvent safety; new Round4FixesTests suite covers hotkey unregister semantics with modifier normalization and StopOnHandled dispatch-chain behavior across multiple handlers.
Version and release notes
GlobalKeyboardCapture.Maui/GlobalKeyboardCapture.Maui.csproj
Version bumped to 1.1.0 with release notes documenting hotkey removal APIs, StopOnHandled dispatch control, Android platform improvements, and DI constructor breaking change.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • afernandes/GlobalKeyboardCapture.Maui#3: Modifies BarcodeHandler buffer management via TimeProvider refactor; this PR further hardens the same handler's buffer cleanup to survive subscriber exceptions.

Poem

🐰 Hotkeys now unregister with grace,
Exceptions caught and logged in place,
Android keys map right and swift,
Disposal safe—no crashes drift,
Stop-on-handled chains the way,
Robustness blooms this fine release day! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title "Fix three P1 Android-only bugs (B4/B5/B6)" references specific Android fixes but the changeset includes substantial cross-platform features (hotkey removal APIs, StopOnHandled option) and other P2/P3 fixes (robustness, nullable PlatformEvent) that go well beyond the three mentioned P1 bugs. Update the title to reflect the full scope: include version bump to 1.1.0 and mention robustness/feature additions (e.g., "Add hotkey removal, StopOnHandled option, and robustness fixes for P2/P3") or clarify if this PR should be split into separate feature/fix PRs.
Docstring Coverage ⚠️ Warning Docstring coverage is 21.21% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/round4-remaining-p2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several features, bug fixes, and robustness improvements to the global keyboard capture library, including hotkey unregistration, a StopOnHandled option to break the dispatch chain, and better exception isolation. It also optimizes Android key handling by introducing an allocation-free ToChar overload. A review comment points out that function keys (F1-F12) will not be detected on Android because KeyEvent.DisplayLabel.ToString() is a single character, which fails to match the multi-character function key names in ToFunction.

Comment on lines 132 to 137
if (keyEvent.Character == null)
{
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel);
// Function keys still go through the string lookup — DisplayLabel for
// F1..F12 is typically '\0', and ToFunction matches by name only.
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel.ToString());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

On Android, KeyEvent.DisplayLabel is a single char. Therefore, displayLabel.ToString() will always produce a string of length 1 (or 0). Since KeyboardHelper.ToFunction expects function key names like "F1" to "F12" (which are 2 or 3 characters long), ToFunction will always return null. This means function keys (F1-F12) are never detected on Android.

Instead, we should map function keys directly using e.KeyCode which contains sequential values from Keycode.F1 to Keycode.F12.

        if (keyEvent.Character == null)
        {
            if (e.KeyCode >= Keycode.F1 && e.KeyCode <= Keycode.F12)
            {
                keyEvent.FunctionKey = e.KeyCode.ToString();
            }
        }

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

The PR is described as three Android-only P1 fixes (Win modifier via IsMetaPressed, PrintScreen/PauseBreak detection, and removing the Space → Character=' ' mapping that broke cross-platform Space hotkeys), but the actual diff also lands a broader 1.1.0 release: a constructor change to KeyHandlerService to accept KeyHandlerOptions, a new StopOnHandled option, HotkeyHandler.UnregisterHotkey/ClearHotkeys + user-action exception isolation, BarcodeHandler finally-clear, an Android KeyEventCallback ObjectDisposedException race guard, a KeyboardHelper.ToChar(char) allocation-free overload, and KeyEventArgs.PlatformEvent made nullable. Release notes for both 1.0.4 and 1.1.0 are added, and CurrentVersion is set to 1.1.0.

Changes:

  • B4/B5/B6 Android fixes in AndroidKeyHandler and KeyboardHelper (Win/Meta modifier, PrintScreen/PauseBreak keycodes, drop space → char mapping).
  • New public API on HotkeyHandler/KeyHandlerOptions and a breaking constructor change on KeyHandlerService; new tests cover B7/B8/B10/B11/B13/B14.
  • Version bump to 1.1.0 with combined 1.1.0 + 1.0.4 release notes.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Platforms/Android/AndroidKeyHandler.cs Derive WindowsKey from IsMetaPressed, add PrintScreenKey/PauseBreakKey, use char DisplayLabel directly.
Platforms/Android/KeyboardHelper.cs Remove Space from SingleCharKeys/KeyMap, add char overload ToChar(char), register math-operator chars in SingleCharKeys.
Platforms/Android/KeyEventCallback.cs Catch ObjectDisposedException from the handler and fall through to the original dispatcher.
Handlers/HotkeyHandler.cs Add UnregisterHotkey(string)/UnregisterHotkey(KeyEventArgs)/ClearHotkeys; wrap user actions in try/catch with Debug trace.
Handlers/BarcodeHandler.cs Clear _buffer in finally so a throwing BarcodeScanned subscriber doesn't contaminate the next scan.
Core/Services/KeyHandlerService.cs New KeyHandlerOptions ctor param; dispatch is silent no-op when disposed; honor StopOnHandled.
Core/Models/KeyEventArgs.cs PlatformEvent declared object?.
Configuration/KeyHandlerOptions.cs Add StopOnHandled option (default false).
GlobalKeyboardCapture.Maui.csproj Bump CurrentVersion to 1.1.0 and add 1.1.0 + 1.0.4 release notes.
Tests/Round4FixesTests.cs New tests for UnregisterHotkey/ClearHotkeys and StopOnHandled.
Tests/P2FixesTests.cs New tests for dispatch-after-dispose, hotkey action isolation, barcode buffer clearing, nullable PlatformEvent.
Tests/KeyHandlerServiceTests.cs Update Build helper for new KeyHandlerService constructor signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<TargetPlatformMinVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'windows'">10.0.17763.0</TargetPlatformMinVersion>

<CurrentVersion>1.0.3</CurrentVersion>
<CurrentVersion>1.1.0</CurrentVersion>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
GlobalKeyboardCapture.Maui/Core/Services/KeyHandlerService.cs (1)

70-86: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Make handler dispatch order deterministic before short-circuiting.

StopOnHandled makes handler order observable, but currentHandlers comes from a HashSet, so the handler that gets to consume the event is implementation-dependent. Two handlers registered in A/B order can execute in B/A order and stop the chain before A ever sees the key.

Suggested direction
-    private readonly HashSet<IKeyHandler> _handlers;
+    private readonly List<IKeyHandler> _handlers;
...
-        _handlers = new HashSet<IKeyHandler>(INITIAL_HANDLERS_CAPACITY);
+        _handlers = new List<IKeyHandler>(INITIAL_HANDLERS_CAPACITY);
...
-            _handlers.Add(handler);
+            if (!_handlers.Contains(handler))
+            {
+                _handlers.Add(handler);
+            }

That keeps the existing lock/snapshot pattern but makes StopOnHandled honor registration order.

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@GlobalKeyboardCapture.Maui/Handlers/HotkeyHandler.cs`:
- Around line 171-180: UnregisterHotkey(string) relies on NormalizeHotkey but
NormalizeHotkey currently doesn't canonicalize key aliases to match
KeyEventArgs.ToString(), causing mismatches like ESCAPE vs Esc; update
NormalizeHotkey to apply the same canonical aliases and modifier order used by
KeyEventArgs.ToString() (map Control→Ctrl, Windows→Win, ESCAPE/ESC→Esc and
ensure modifiers are ordered per KeyEventArgs.ToString()), so both normalizers
produce identical normalized keys when adding/removing via the string API.
- Around line 177-196: The Dictionary field _hotkeyActions is being mutated by
UnregisterHotkey(string), UnregisterHotkey(KeyEventArgs) and ClearHotkeys while
HandleKey does unsynchronized reads, which can cause thread-safety bugs; fix
this by synchronizing all accesses to _hotkeyActions — either wrap all reads and
writes in a single private lock object (use lock(...) around
NormalizeHotkey/Remove/Clear and around the TryGetValue/read path in HandleKey)
or replace the Dictionary with a thread-safe collection (e.g.,
ConcurrentDictionary) and update HandleKey to use its atomic read methods;
ensure the same synchronization strategy is applied to the other affected region
(the methods around lines 202–218) so all lookups and mutations use the same
lock or thread-safe type.

In `@GlobalKeyboardCapture.Maui/Platforms/Android/AndroidKeyHandler.cs`:
- Around line 132-136: The current branch sets keyEvent.FunctionKey via
KeyboardHelper.ToFunction(displayLabel.ToString()), but DisplayLabel can be '\0'
so that returns null; instead resolve function keys from the Android KeyCode:
when keyEvent.Character is null (or displayLabel == '\0'), derive the function
name from keyEvent.KeyCode (e.g., map Keycode.F1..F12 to "F1".."F12" or call
ToFunction with keyEvent.KeyCode.ToString()) and pass that into
KeyboardHelper.ToFunction to populate keyEvent.FunctionKey; update the
AndroidKeyHandler.cs logic around keyEvent.Character / DisplayLabel and
KeyboardHelper.ToFunction accordingly so Android function keys are detected.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: daf0bff9-03f8-41da-9731-63b6739ec467

📥 Commits

Reviewing files that changed from the base of the PR and between adcddd7 and 4fdb3cf.

📒 Files selected for processing (12)
  • GlobalKeyboardCapture.Maui.Tests/KeyHandlerServiceTests.cs
  • GlobalKeyboardCapture.Maui.Tests/P2FixesTests.cs
  • GlobalKeyboardCapture.Maui.Tests/Round4FixesTests.cs
  • GlobalKeyboardCapture.Maui/Configuration/KeyHandlerOptions.cs
  • GlobalKeyboardCapture.Maui/Core/Models/KeyEventArgs.cs
  • GlobalKeyboardCapture.Maui/Core/Services/KeyHandlerService.cs
  • GlobalKeyboardCapture.Maui/GlobalKeyboardCapture.Maui.csproj
  • GlobalKeyboardCapture.Maui/Handlers/BarcodeHandler.cs
  • GlobalKeyboardCapture.Maui/Handlers/HotkeyHandler.cs
  • GlobalKeyboardCapture.Maui/Platforms/Android/AndroidKeyHandler.cs
  • GlobalKeyboardCapture.Maui/Platforms/Android/KeyEventCallback.cs
  • GlobalKeyboardCapture.Maui/Platforms/Android/KeyboardHelper.cs

Comment on lines +171 to +180
/// <summary>
/// Removes a hotkey previously registered via the string overload.
/// The lookup uses the same normalization, so modifier order and aliases are
/// honored ("Control+Shift+X" matches a registration of "Shift+Ctrl+X").
/// </summary>
/// <returns><c>true</c> if a hotkey was removed; <c>false</c> if no match was registered.</returns>
public bool UnregisterHotkey(string hotKey)
{
ArgumentException.ThrowIfNullOrEmpty(hotKey);
return _hotkeyActions.Remove(NormalizeHotkey(hotKey));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep string hotkey normalization in sync with KeyEventArgs.ToString().

UnregisterHotkey(string) depends on NormalizeHotkey, but that normalization still does not canonicalize Escape/ESCAPE to Esc. A hotkey registered or removed via the string API can therefore miss the same logical key that KeyEventArgs.ToString() produces.

As per coding guidelines, **/HotkeyHandler.cs: In HotkeyHandler.NormalizeHotkey(...), maintain the same modifier order and aliases as KeyEventArgs.ToString() (ControlCtrl, WindowsWin, ESCAPEEsc) and keep both normalizers in sync.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@GlobalKeyboardCapture.Maui/Handlers/HotkeyHandler.cs` around lines 171 - 180,
UnregisterHotkey(string) relies on NormalizeHotkey but NormalizeHotkey currently
doesn't canonicalize key aliases to match KeyEventArgs.ToString(), causing
mismatches like ESCAPE vs Esc; update NormalizeHotkey to apply the same
canonical aliases and modifier order used by KeyEventArgs.ToString() (map
Control→Ctrl, Windows→Win, ESCAPE/ESC→Esc and ensure modifiers are ordered per
KeyEventArgs.ToString()), so both normalizers produce identical normalized keys
when adding/removing via the string API.

Comment on lines +177 to +196
public bool UnregisterHotkey(string hotKey)
{
ArgumentException.ThrowIfNullOrEmpty(hotKey);
return _hotkeyActions.Remove(NormalizeHotkey(hotKey));
}

/// <summary>
/// Removes a hotkey previously registered via the <see cref="KeyEventArgs"/> overload.
/// </summary>
/// <returns><c>true</c> if a hotkey was removed; <c>false</c> if no match was registered.</returns>
public bool UnregisterHotkey(KeyEventArgs hotKey)
{
ArgumentNullException.ThrowIfNull(hotKey);
return _hotkeyActions.Remove(hotKey.ToString());
}

/// <summary>
/// Removes every registered hotkey.
/// </summary>
public void ClearHotkeys() => _hotkeyActions.Clear();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Synchronize hotkey mutations with dispatch lookups.

These new unregister/clear APIs add more runtime writes to _hotkeyActions, but HandleKey still does unsynchronized reads from the same Dictionary. Since key dispatch runs on the platform input thread and registration APIs can run on the UI thread, concurrent Remove/Clear/TryGetValue here is not safe.

Suggested direction
+    private readonly object _hotkeysLock = new();
...
     public void RegisterHotkey(KeyEventArgs hotKey, Action action)
     {
         ArgumentNullException.ThrowIfNull(hotKey);
         ArgumentNullException.ThrowIfNull(action);
-        _hotkeyActions[hotKey.ToString()] = action;
+        lock (_hotkeysLock)
+        {
+            _hotkeyActions[hotKey.ToString()] = action;
+        }
     }
...
     public void RegisterHotkey(string hotKey, Action action)
     {
         ArgumentException.ThrowIfNullOrEmpty(hotKey);
         ArgumentNullException.ThrowIfNull(action);
-
-        _hotkeyActions[NormalizeHotkey(hotKey)] = action;
+        lock (_hotkeysLock)
+        {
+            _hotkeyActions[NormalizeHotkey(hotKey)] = action;
+        }
     }
...
     public bool UnregisterHotkey(string hotKey)
     {
         ArgumentException.ThrowIfNullOrEmpty(hotKey);
-        return _hotkeyActions.Remove(NormalizeHotkey(hotKey));
+        lock (_hotkeysLock)
+        {
+            return _hotkeyActions.Remove(NormalizeHotkey(hotKey));
+        }
     }
...
     public bool UnregisterHotkey(KeyEventArgs hotKey)
     {
         ArgumentNullException.ThrowIfNull(hotKey);
-        return _hotkeyActions.Remove(hotKey.ToString());
+        lock (_hotkeysLock)
+        {
+            return _hotkeyActions.Remove(hotKey.ToString());
+        }
     }
...
-    public void ClearHotkeys() => _hotkeyActions.Clear();
+    public void ClearHotkeys()
+    {
+        lock (_hotkeysLock)
+        {
+            _hotkeyActions.Clear();
+        }
+    }
...
-        if (_hotkeyActions.TryGetValue(key.ToString(), out var action))
+        Action? action;
+        lock (_hotkeysLock)
+        {
+            _hotkeyActions.TryGetValue(key.ToString(), out action);
+        }
+
+        if (action is not null)

Also applies to: 202-218

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@GlobalKeyboardCapture.Maui/Handlers/HotkeyHandler.cs` around lines 177 - 196,
The Dictionary field _hotkeyActions is being mutated by
UnregisterHotkey(string), UnregisterHotkey(KeyEventArgs) and ClearHotkeys while
HandleKey does unsynchronized reads, which can cause thread-safety bugs; fix
this by synchronizing all accesses to _hotkeyActions — either wrap all reads and
writes in a single private lock object (use lock(...) around
NormalizeHotkey/Remove/Clear and around the TryGetValue/read path in HandleKey)
or replace the Dictionary with a thread-safe collection (e.g.,
ConcurrentDictionary) and update HandleKey to use its atomic read methods;
ensure the same synchronization strategy is applied to the other affected region
(the methods around lines 202–218) so all lookups and mutations use the same
lock or thread-safe type.

Comment on lines 132 to +136
if (keyEvent.Character == null)
{
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel);
// Function keys still go through the string lookup — DisplayLabel for
// F1..F12 is typically '\0', and ToFunction matches by name only.
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel.ToString());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Resolve function keys from KeyCode, not DisplayLabel.

Line 136 cannot produce "F1"-"F12" from DisplayLabel: when DisplayLabel is '\0' as noted in the comment, displayLabel.ToString() is just "\0", so KeyboardHelper.ToFunction(...) always returns null. That leaves Android function-key hotkeys undetectable on this path.

Proposed fix
         if (keyEvent.Character == null)
         {
-            // Function keys still go through the string lookup — DisplayLabel for
-            // F1..F12 is typically '\0', and ToFunction matches by name only.
-            keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel.ToString());
+            keyEvent.FunctionKey = e.KeyCode switch
+            {
+                Keycode.F1 => "F1",
+                Keycode.F2 => "F2",
+                Keycode.F3 => "F3",
+                Keycode.F4 => "F4",
+                Keycode.F5 => "F5",
+                Keycode.F6 => "F6",
+                Keycode.F7 => "F7",
+                Keycode.F8 => "F8",
+                Keycode.F9 => "F9",
+                Keycode.F10 => "F10",
+                Keycode.F11 => "F11",
+                Keycode.F12 => "F12",
+                _ => null
+            };
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (keyEvent.Character == null)
{
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel);
// Function keys still go through the string lookup — DisplayLabel for
// F1..F12 is typically '\0', and ToFunction matches by name only.
keyEvent.FunctionKey = KeyboardHelper.ToFunction(displayLabel.ToString());
if (keyEvent.Character == null)
{
keyEvent.FunctionKey = e.KeyCode switch
{
Keycode.F1 => "F1",
Keycode.F2 => "F2",
Keycode.F3 => "F3",
Keycode.F4 => "F4",
Keycode.F5 => "F5",
Keycode.F6 => "F6",
Keycode.F7 => "F7",
Keycode.F8 => "F8",
Keycode.F9 => "F9",
Keycode.F10 => "F10",
Keycode.F11 => "F11",
Keycode.F12 => "F12",
_ => null
};
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@GlobalKeyboardCapture.Maui/Platforms/Android/AndroidKeyHandler.cs` around
lines 132 - 136, The current branch sets keyEvent.FunctionKey via
KeyboardHelper.ToFunction(displayLabel.ToString()), but DisplayLabel can be '\0'
so that returns null; instead resolve function keys from the Android KeyCode:
when keyEvent.Character is null (or displayLabel == '\0'), derive the function
name from keyEvent.KeyCode (e.g., map Keycode.F1..F12 to "F1".."F12" or call
ToFunction with keyEvent.KeyCode.ToString()) and pass that into
KeyboardHelper.ToFunction to populate keyEvent.FunctionKey; update the
AndroidKeyHandler.cs logic around keyEvent.Character / DisplayLabel and
KeyboardHelper.ToFunction accordingly so Android function keys are detected.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants