diff --git a/flutter/lib/common/widgets/keyboard_shortcuts/shortcut_constants.dart b/flutter/lib/common/widgets/keyboard_shortcuts/shortcut_constants.dart index 3d265199d..a652dfc20 100644 --- a/flutter/lib/common/widgets/keyboard_shortcuts/shortcut_constants.dart +++ b/flutter/lib/common/widgets/keyboard_shortcuts/shortcut_constants.dart @@ -17,6 +17,18 @@ const kShortcutActionRestartRemote = 'restart_remote'; const kShortcutActionResetCanvas = 'reset_canvas'; const kShortcutActionSwitchTabNext = 'switch_tab_next'; const kShortcutActionSwitchTabPrev = 'switch_tab_prev'; + +/// Actions that move keyboard focus to another session. They run when the +/// key that fired them is released, not when it is pressed, so the matcher +/// that consumed the press also sees its repeats and its release. On Linux a +/// legacy-mode session matches in Dart while a map-mode session matches in +/// Rust, and neither knows the other's fired keys. Mirrors `runs_on_release` +/// in `src/keyboard/shortcuts.rs`. +const kShortcutActionsRunOnKeyUp = { + kShortcutActionCloseTab, + kShortcutActionSwitchTabNext, + kShortcutActionSwitchTabPrev, +}; const kShortcutActionToggleMute = 'toggle_mute'; const kShortcutActionPinToolbar = 'pin_toolbar'; const kShortcutActionViewModeOriginal = 'view_mode_original'; diff --git a/flutter/lib/models/local_flutter_shortcuts.dart b/flutter/lib/models/local_flutter_shortcuts.dart index 2733f4058..38840eb4c 100644 --- a/flutter/lib/models/local_flutter_shortcuts.dart +++ b/flutter/lib/models/local_flutter_shortcuts.dart @@ -3,6 +3,8 @@ import 'dart:async'; import 'package:flutter/foundation.dart'; import 'package:flutter/services.dart'; +import '../common/widgets/keyboard_shortcuts/shortcut_constants.dart'; + class LocalFlutterShortcutDispatcher { final void Function(String) onTriggered; // Keys whose press fired a shortcut and whose release has not arrived yet. @@ -11,22 +13,30 @@ class LocalFlutterShortcutDispatcher { // it is never dropped with a session. A key whose release is missed heals // itself: its next press is consumed as a repeat and that release removes it. static final _firedKeys = {}; + // Actions waiting for the release of the key that fired them, with the + // session that fired them. See [kShortcutActionsRunOnKeyUp]. + static final _keyUpActions = + {}; final _releasedModifiers = {}; bool _viewOnlyShortcutPending = false; int _generation = 0; LocalFlutterShortcutDispatcher({required this.onTriggered}); - /// Drops this session's pending action and modifier replay. Fired keys + /// Drops this session's pending actions and modifier replay. Fired keys /// stay owned until their release, whichever session receives it. void clear() { + _keyUpActions.removeWhere((_, pending) => identical(pending.$1, this)); _releasedModifiers.clear(); _viewOnlyShortcutPending = false; _generation++; } @visibleForTesting - static void resetFiredKeys() => _firedKeys.clear(); + static void resetFiredKeys() { + _firedKeys.clear(); + _keyUpActions.clear(); + } void recordReleasedModifiers(Iterable keys) { _releasedModifiers.addAll(keys); @@ -106,23 +116,39 @@ class LocalFlutterShortcutDispatcher { String? Function()? match, Future Function()? releaseModifiers, }) { - if (up) return _firedKeys.remove(key); + if (up) { + if (!_firedKeys.remove(key)) return false; + final pending = _keyUpActions.remove(key); + if (pending != null) { + final (owner, action) = pending; + unawaited(owner._trigger(action, null)); + } + return true; + } if (_firedKeys.contains(key)) return true; if (!down) return false; final action = match?.call(); if (action == null) return false; _firedKeys.add(key); if (viewOnly) _viewOnlyShortcutPending = true; - unawaited(_trigger(action, releaseModifiers)); + final runOnKeyUp = kShortcutActionsRunOnKeyUp.contains(action); + if (runOnKeyUp) { + _keyUpActions[key] = (this, action); + } else { + _keyUpActions.remove(key); + } + unawaited(_trigger(runOnKeyUp ? null : action, releaseModifiers)); return true; } + /// Releases the remote modifiers, then runs [action] unless it is null or + /// this session was cleared meanwhile. Future _trigger( - String action, Future Function()? releaseModifiers) async { + String? action, Future Function()? releaseModifiers) async { final generation = _generation; try { if (releaseModifiers != null) await releaseModifiers(); - if (generation == _generation) onTriggered(action); + if (action != null && generation == _generation) onTriggered(action); } catch (e, st) { debugPrint('Local shortcut failed for $action: $e\n$st'); } diff --git a/flutter/test/local_flutter_shortcuts_test.dart b/flutter/test/local_flutter_shortcuts_test.dart index cf3ec9691..de46188bc 100644 --- a/flutter/test/local_flutter_shortcuts_test.dart +++ b/flutter/test/local_flutter_shortcuts_test.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter_hbb/common/widgets/keyboard_shortcuts/shortcut_constants.dart'; import 'package:flutter_hbb/models/local_flutter_shortcuts.dart'; LocalFlutterShortcutDispatcher dispatcherFor( @@ -224,6 +225,47 @@ void main() { expect(actions, isEmpty); }); + test('focus-changing actions run when the key is released', () async { + // Close tab moves focus before the key is released; the release may then + // land in a session whose keys go through the other matcher. + var releases = 0; + final actions = []; + final dispatcher = _ShortcutHarness( + match: (_) => kShortcutActionCloseTab, + releaseModifiers: () async => releases++, + onTriggered: actions.add, + ); + + expect(dispatcher.tryDispatch(down(PhysicalKeyboardKey.keyW)), isTrue); + expect(dispatcher.tryDispatch(repeat(PhysicalKeyboardKey.keyW)), isTrue); + await Future.value(); + expect(releases, 1, reason: 'remote modifiers are released on the press'); + expect(actions, isEmpty, reason: 'the action waits for the release'); + + final other = dispatcherFor(actions.add); + expect(other.tryDispatch(up(PhysicalKeyboardKey.keyW)), isTrue); + await Future.value(); + expect(actions, [kShortcutActionCloseTab]); + expect(other.tryDispatch(up(PhysicalKeyboardKey.keyW)), isFalse); + }); + + test('closing the session drops its action pending on key release', + () async { + final actions = []; + final dispatcher = _ShortcutHarness( + match: (_) => kShortcutActionSwitchTabNext, + releaseModifiers: () async {}, + onTriggered: actions.add, + ); + + expect(dispatcher.tryDispatch(down(PhysicalKeyboardKey.keyW)), isTrue); + dispatcher.clear(); + expect(dispatcher.tryDispatch(up(PhysicalKeyboardKey.keyW)), isTrue, + reason: 'the key stays owned'); + await Future.value(); + expect(actions, isEmpty); + }); + test('raw legacy shortcuts own repeats and release after modifiers change', () async { addTearDown(RawKeyboard.instance.clearKeysPressed); diff --git a/src/keyboard/shortcuts.rs b/src/keyboard/shortcuts.rs index 46919be94..9f23c4df4 100644 --- a/src/keyboard/shortcuts.rs +++ b/src/keyboard/shortcuts.rs @@ -328,15 +328,35 @@ lazy_static::lazy_static! { /// removes it. static ref FIRED_KEYS: std::sync::Mutex> = Default::default(); + /// Actions waiting for the release of the key that fired them, with the + /// session that fired them. See `runs_on_release`. + static ref RELEASE_ACTIONS: std::sync::Mutex< + std::collections::HashMap, + > = Default::default(); static ref RELEASED_MODIFIERS: std::sync::Mutex< std::collections::HashMap>, > = Default::default(); } -/// Forget the modifiers a session released on its remote. Fired keys are -/// physical and stay owned until their release, whichever session gets it. +/// Actions that move keyboard focus to another session. They run when the +/// key that fired them is released, not when it is pressed, so the matcher +/// that consumed the press also sees its repeats and its release. The next +/// session may route its keys through the other matcher (Flutter's legacy +/// path on Linux), which never saw the press. Mirrors +/// `kShortcutActionsRunOnKeyUp` in `shortcut_constants.dart`. +pub fn runs_on_release(action_id: &str) -> bool { + matches!( + action_id, + action_id::CLOSE_TAB | action_id::SWITCH_TAB_NEXT | action_id::SWITCH_TAB_PREV + ) +} + +/// Forget the modifiers a session released on its remote and the actions it +/// still owes on key release. Fired keys are physical and stay owned until +/// their release, whichever session gets it. #[cfg(feature = "flutter")] pub fn clear_session_state(session_id: &hbb_common::SessionID) { + RELEASE_ACTIONS.lock().unwrap().retain(|_, (sid, _)| sid != session_id); let held = RELEASED_MODIFIERS.lock().unwrap().remove(session_id); if let Some(held) = held { { @@ -361,6 +381,7 @@ pub fn enter_view_only(session_id: &hbb_common::SessionID) { // The Flutter input source stops routing key events here, so the release // of a fired key can no longer reach this matcher. FIRED_KEYS.lock().unwrap().clear(); + RELEASE_ACTIONS.lock().unwrap().clear(); clear_session_state(session_id); } @@ -419,7 +440,18 @@ pub fn try_dispatch( let mut fired = FIRED_KEYS.lock().unwrap(); match event.event_type { EventType::KeyPress(k) if fired.contains(&k) => return true, - EventType::KeyRelease(k) if fired.remove(&k) => return true, + EventType::KeyRelease(k) if fired.remove(&k) => { + drop(fired); + let pending = RELEASE_ACTIONS.lock().unwrap().remove(&k); + if let Some((sid, action_id)) = pending { + crate::flutter::push_session_event( + &sid, + "shortcut_triggered", + vec![("action", &action_id)], + ); + } + return true; + } _ => {} } } @@ -430,6 +462,12 @@ pub fn try_dispatch( release_remote_keys(sid, keyboard_mode, &peer(), &send); if let EventType::KeyPress(k) = event.event_type { FIRED_KEYS.lock().unwrap().insert(k); + let mut pending = RELEASE_ACTIONS.lock().unwrap(); + if runs_on_release(&action_id) { + pending.insert(k, (*sid, action_id)); + return true; + } + pending.remove(&k); } crate::flutter::push_session_event(sid, "shortcut_triggered", vec![("action", &action_id)]); true @@ -1199,6 +1237,48 @@ mod tests { release_chord(chord); } + /// Close tab and tab switching move focus away before the key is + /// released; they run on the release so the same matcher sees the whole + /// press. + #[cfg(feature = "flutter")] + #[test] + fn focus_changing_actions_run_on_release() { + use rdev::Key; + + let _guard = CACHE_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let chord = enable_defaults_and_hold_chord(); + let prefix = || vec![Modifier::Primary, Modifier::Alt, Modifier::Shift]; + *CACHE.write().unwrap() = Arc::new(Bindings { + enabled: true, + pass_through: false, + bindings: vec![ + Binding { action: action_id::CLOSE_TAB.into(), mods: prefix(), key: "w".into() }, + Binding { action: action_id::SCREENSHOT.into(), mods: prefix(), key: "p".into() }, + ], + }); + let dispatch = |sid: &hbb_common::SessionID, e: &rdev::Event| { + try_dispatch(Some(sid), e, "map", || "windows".into(), |_| {}) + }; + let pending_for = |k: Key| RELEASE_ACTIONS.lock().unwrap().get(&k).cloned(); + + assert!(dispatch(&SID_A, &make_press(Key::KeyW))); + assert_eq!(pending_for(Key::KeyW), Some((SID_A, action_id::CLOSE_TAB.to_owned()))); + assert!(dispatch(&SID_A, &make_press(Key::KeyW)), "repeat is consumed"); + assert_eq!(pending_for(Key::KeyW), Some((SID_A, action_id::CLOSE_TAB.to_owned()))); + assert!(dispatch(&SID_B, &make_release(Key::KeyW)), "release is consumed wherever it lands"); + assert_eq!(pending_for(Key::KeyW), None, "the action ran on the release"); + + assert!(dispatch(&SID_A, &make_press(Key::KeyP))); + assert_eq!(pending_for(Key::KeyP), None, "other actions still run on the press"); + assert!(dispatch(&SID_A, &make_release(Key::KeyP))); + + assert!(dispatch(&SID_A, &make_press(Key::KeyW))); + clear_session_state(&SID_A); + assert_eq!(pending_for(Key::KeyW), None, "a closed session owes nothing"); + assert!(dispatch(&SID_A, &make_release(Key::KeyW)), "but its key stays owned"); + release_chord(chord); + } + #[cfg(feature = "flutter")] const SID_A: hbb_common::SessionID = hbb_common::SessionID::from_u128(0xA); #[cfg(feature = "flutter")] @@ -1212,6 +1292,7 @@ mod tests { #[cfg(feature = "flutter")] fn reset_fired_keys() { FIRED_KEYS.lock().unwrap().clear(); + RELEASE_ACTIONS.lock().unwrap().clear(); } #[cfg(feature = "flutter")]