[macOS, Keyboard] Enqueue event processing (#32120)
* Impl
Remove log
add sync; clean code
Format
* Docs
* Tests
* Apply suggestions from code review
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponder.mm b/shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponder.mm
index 101dfe2..905d6fb 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponder.mm
+++ b/shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponder.mm
@@ -3,6 +3,7 @@
// found in the LICENSE file.
#import <objc/message.h>
+#include <memory>
#import "FlutterEmbedderKeyResponder.h"
#import "KeyCodeMap_Internal.h"
@@ -248,40 +249,11 @@
/**
* The invocation context for |HandleResponse|, wrapping
* |FlutterEmbedderKeyResponder.handleResponse|.
- *
- * The embedder functions only accept C-functions as callbacks, as well as an
- * arbitrary user_data. In order to send an instance method of
- * |FlutterEmbedderKeyResponder.handleResponse| to the engine's |SendKeyEvent|,
- * the embedder wraps the invocation into a C-function |HandleResponse| and
- * invocation context |FlutterKeyPendingResponse|.
- *
- * When this object is sent to the engine's |SendKeyEvent| as |user_data|, it
- * must be attached with |__bridge_retained|. When this object is parsed
- * in |HandleResponse| from |user_data|, it will be attached with
- * |__bridge_transfer|.
*/
-@interface FlutterKeyPendingResponse : NSObject
-
-@property(nonatomic) FlutterEmbedderKeyResponder* responder;
-
-@property(nonatomic) uint64_t responseId;
-
-- (nonnull instancetype)initWithHandler:(nonnull FlutterEmbedderKeyResponder*)responder
- responseId:(uint64_t)responseId;
-
-@end
-
-@implementation FlutterKeyPendingResponse
-- (instancetype)initWithHandler:(FlutterEmbedderKeyResponder*)responder
- responseId:(uint64_t)responseId {
- self = [super init];
- if (self != nil) {
- _responder = responder;
- _responseId = responseId;
- }
- return self;
-}
-@end
+struct FlutterKeyPendingResponse {
+ FlutterEmbedderKeyResponder* responder;
+ uint64_t responseId;
+};
/**
* Guards a |FlutterAsyncKeyCallback| to make sure it's handled exactly once
@@ -598,11 +570,10 @@
callback:(FlutterKeyCallbackGuard*)callback {
_responseId += 1;
uint64_t responseId = _responseId;
- FlutterKeyPendingResponse* pending =
- [[FlutterKeyPendingResponse alloc] initWithHandler:self responseId:responseId];
+ // The `pending` is released in `HandleResponse`.
+ FlutterKeyPendingResponse* pending = new FlutterKeyPendingResponse{self, responseId};
[callback pendTo:_pendingResponses withId:responseId];
- // The `__bridge_retained` here is matched by `__bridge_transfer` in HandleResponse.
- _sendEvent(event, HandleResponse, (__bridge_retained void*)pending);
+ _sendEvent(event, HandleResponse, pending);
callback.sentAnyEvents = TRUE;
}
@@ -809,8 +780,9 @@
namespace {
void HandleResponse(bool handled, void* user_data) {
- // The `__bridge_transfer` here is matched by `__bridge_retained` in sendPrimaryFlutterEvent.
- FlutterKeyPendingResponse* pending = (__bridge_transfer FlutterKeyPendingResponse*)user_data;
- [pending.responder handleResponse:handled forId:pending.responseId];
+ // Use unique_ptr to release on leaving.
+ auto pending = std::unique_ptr<FlutterKeyPendingResponse>(
+ reinterpret_cast<FlutterKeyPendingResponse*>(user_data));
+ [pending->responder handleResponse:handled forId:pending->responseId];
}
} // namespace
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.h b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.h
index af723a7..7c6b037 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.h
+++ b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.h
@@ -4,17 +4,8 @@
#import <Cocoa/Cocoa.h>
-#import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"
#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyboardViewDelegate.h"
-namespace {
-// Someohow this pointer type must be defined as a single type for the compiler
-// to compile the function pointer type (due to _Nullable).
-typedef NSResponder* _NSResponderPtr;
-}
-
-typedef _Nullable _NSResponderPtr (^NextResponderProvider)();
-
/**
* Processes keyboard events and cooperate with |TextInputPlugin|.
*
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.mm b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.mm
index c240682..65227b4 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.mm
+++ b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManager.mm
@@ -9,6 +9,15 @@
#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h"
#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterKeyPrimaryResponder.h"
+namespace {
+typedef void (^VoidBlock)();
+
+// Someohow this pointer type must be defined as a single type for the compiler
+// to compile the function pointer type (due to _Nullable).
+typedef NSResponder* _NSResponderPtr;
+typedef _Nullable _NSResponderPtr (^NextResponderProvider)();
+}
+
@interface FlutterKeyboardManager ()
/**
@@ -21,13 +30,39 @@
*/
@property(nonatomic) NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders;
+@property(nonatomic) NSMutableArray<NSEvent*>* pendingTextEvents;
+
+@property(nonatomic) BOOL processingEvent;
+
/**
* Add a primary responder, which asynchronously decides whether to handle an
* event.
*/
- (void)addPrimaryResponder:(nonnull id<FlutterKeyPrimaryResponder>)responder;
-- (void)dispatchToSecondaryResponders:(NSEvent*)event;
+/**
+ * Start processing the next event if not started already.
+ *
+ * This function might initiate an async process, whose callback calls this
+ * function again.
+ */
+- (void)processNextEvent;
+
+/**
+ * Implement how to process an event.
+ *
+ * The `onFinish` must be called eventually, either during this function or
+ * asynchronously later, otherwise the event queue will be stuck.
+ *
+ * This function is called by processNextEvent.
+ */
+- (void)performProcessEvent:(NSEvent*)event onFinish:(nonnull VoidBlock)onFinish;
+
+/**
+ * Dispatch an event that's not hadled by the responders to text input plugin,
+ * and potentially to the next responder.
+ */
+- (void)dispatchTextEvent:(nonnull NSEvent*)pendingEvent;
@end
@@ -38,6 +73,7 @@
- (nonnull instancetype)initWithViewDelegate:(nonnull id<FlutterKeyboardViewDelegate>)viewDelegate {
self = [super init];
if (self != nil) {
+ _processingEvent = FALSE;
_viewDelegate = viewDelegate;
_primaryResponders = [[NSMutableArray alloc] init];
@@ -57,6 +93,7 @@
getBinaryMessenger]
codec:[FlutterJSONMessageCodec
sharedInstance]]]];
+ _pendingTextEvents = [[NSMutableArray alloc] init];
}
return self;
}
@@ -66,6 +103,9 @@
}
- (void)handleEvent:(nonnull NSEvent*)event {
+ // The `handleEvent` does not process the event immediately, but instead put
+ // events into a queue. Events are processed one by one by `processNextEvent`.
+
// Be sure to add a handling method in propagateKeyEvent when allowing more
// event types here.
if (event.type != NSEventTypeKeyDown && event.type != NSEventTypeKeyUp &&
@@ -73,8 +113,35 @@
return;
}
+ [_pendingTextEvents addObject:event];
+ [self processNextEvent];
+}
+
+#pragma mark - Private
+
+- (void)processNextEvent {
+ @synchronized(self) {
+ if (_processingEvent || [_pendingTextEvents count] == 0) {
+ return;
+ }
+ _processingEvent = TRUE;
+ }
+
+ NSEvent* pendingEvent = [_pendingTextEvents firstObject];
+ [_pendingTextEvents removeObjectAtIndex:0];
+
+ __weak __typeof__(self) weakSelf = self;
+ VoidBlock onFinish = ^() {
+ weakSelf.processingEvent = FALSE;
+ [weakSelf processNextEvent];
+ };
+ [self performProcessEvent:pendingEvent onFinish:onFinish];
+}
+
+- (void)performProcessEvent:(NSEvent*)event onFinish:(VoidBlock)onFinish {
if (_viewDelegate.isComposing) {
- [self dispatchToSecondaryResponders:event];
+ [self dispatchTextEvent:event];
+ onFinish();
return;
}
@@ -86,12 +153,16 @@
__weak __typeof__(self) weakSelf = self;
__block int unreplied = [_primaryResponders count];
__block BOOL anyHandled = false;
+
FlutterAsyncKeyCallback replyCallback = ^(BOOL handled) {
unreplied -= 1;
NSAssert(unreplied >= 0, @"More primary responders replied than possible.");
anyHandled = anyHandled || handled;
- if (unreplied == 0 && !anyHandled) {
- [weakSelf dispatchToSecondaryResponders:event];
+ if (unreplied == 0) {
+ if (!anyHandled) {
+ [weakSelf dispatchTextEvent:event];
+ }
+ onFinish();
}
};
@@ -100,9 +171,7 @@
}
}
-#pragma mark - Private
-
-- (void)dispatchToSecondaryResponders:(NSEvent*)event {
+- (void)dispatchTextEvent:(NSEvent*)event {
if ([_viewDelegate onTextInputKeyEvent:event]) {
return;
}
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm
index 9ee515d..14446ac 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm
+++ b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardManagerUnittests.mm
@@ -14,18 +14,32 @@
namespace {
typedef BOOL (^BoolGetter)();
-typedef void (^AsyncKeyCallback)(BOOL handled);
-typedef void (^AsyncKeyCallbackHandler)(AsyncKeyCallback callback);
+typedef void (^AsyncKeyCallbackHandler)(FlutterAsyncKeyCallback callback);
+typedef BOOL (^TextInputCallback)(NSEvent*);
-NSEvent* keyDownEvent(unsigned short keyCode) {
+// When the Vietnamese IME converts messages into "pure text" messages, their
+// key codes are set to "empty".
+//
+// The 0 also happens to be the key code for key A.
+constexpr uint16_t kKeyCodeEmpty = 0x00;
+
+constexpr uint16_t kKeyCodeKeyO = 0x1f;
+constexpr uint16_t kKeyCodeBackspace = 0x33;
+
+// Constants used for `recordCallTypesTo:forTypes:`.
+constexpr uint32_t kEmbedderCall = 0x1;
+constexpr uint32_t kChannelCall = 0x2;
+constexpr uint32_t kTextCall = 0x4;
+
+NSEvent* keyDownEvent(unsigned short keyCode, NSString* chars = @"", NSString* charsUnmod = @"") {
return [NSEvent keyEventWithType:NSEventTypeKeyDown
location:NSZeroPoint
modifierFlags:0x100
timestamp:0
windowNumber:0
context:nil
- characters:@""
- charactersIgnoringModifiers:@""
+ characters:chars
+ charactersIgnoringModifiers:charsUnmod
isARepeat:NO
keyCode:keyCode];
}
@@ -68,11 +82,36 @@
@interface KeyboardTester : NSObject
- (nonnull instancetype)init;
+
+// Set embedder calls to respond immediately with the given response.
- (void)respondEmbedderCallsWith:(BOOL)response;
+
+// Record embedder calls to the given storage.
+//
+// They are not responded to until the stored callbacks are manually called.
- (void)recordEmbedderCallsTo:(nonnull NSMutableArray<FlutterAsyncKeyCallback>*)storage;
+
+// Set channel calls to respond immediately with the given response.
- (void)respondChannelCallsWith:(BOOL)response;
+
+// Record channel calls to the given storage.
+//
+// They are not responded to until the stored callbacks are manually called.
- (void)recordChannelCallsTo:(nonnull NSMutableArray<FlutterAsyncKeyCallback>*)storage;
+// Set text calls to respond with the given response.
+- (void)respondTextInputWith:(BOOL)response;
+
+// At the start of any kind of call, record the call type to the given storage.
+//
+// Only calls that are included in `typeMask` will be added. Options are
+// kEmbedderCall, kChannelCall, and kTextCall.
+//
+// This method does not conflict with other call settings, and the recording
+// takes place before the callbacks are (or are not) invoked.
+- (void)recordCallTypesTo:(nonnull NSMutableArray<NSNumber*>*)typeStorage
+ forTypes:(uint32_t)typeMask;
+
@property(nonatomic) FlutterKeyboardManager* manager;
@property(nonatomic) NSResponder* nextResponder;
@property(nonatomic, assign) BOOL isComposing;
@@ -93,7 +132,10 @@
@implementation KeyboardTester {
AsyncKeyCallbackHandler _embedderHandler;
AsyncKeyCallbackHandler _channelHandler;
- BOOL _textInputResponse;
+ TextInputCallback _textCallback;
+
+ NSMutableArray<NSNumber*>* _typeStorage;
+ uint32_t _typeStorageMask;
}
- (nonnull instancetype)init {
@@ -129,31 +171,39 @@
}
- (void)respondEmbedderCallsWith:(BOOL)response {
- _embedderHandler = ^(AsyncKeyCallback callback) {
+ _embedderHandler = ^(FlutterAsyncKeyCallback callback) {
callback(response);
};
}
- (void)recordEmbedderCallsTo:(nonnull NSMutableArray<FlutterAsyncKeyCallback>*)storage {
- _embedderHandler = ^(AsyncKeyCallback callback) {
+ _embedderHandler = ^(FlutterAsyncKeyCallback callback) {
[storage addObject:callback];
};
}
- (void)respondChannelCallsWith:(BOOL)response {
- _channelHandler = ^(AsyncKeyCallback callback) {
+ _channelHandler = ^(FlutterAsyncKeyCallback callback) {
callback(response);
};
}
- (void)recordChannelCallsTo:(nonnull NSMutableArray<FlutterAsyncKeyCallback>*)storage {
- _channelHandler = ^(AsyncKeyCallback callback) {
+ _channelHandler = ^(FlutterAsyncKeyCallback callback) {
[storage addObject:callback];
};
}
- (void)respondTextInputWith:(BOOL)response {
- _textInputResponse = response;
+ _textCallback = ^(NSEvent* event) {
+ return response;
+ };
+}
+
+- (void)recordCallTypesTo:(nonnull NSMutableArray<NSNumber*>*)typeStorage
+ forTypes:(uint32_t)typeMask {
+ _typeStorage = typeStorage;
+ _typeStorageMask = typeMask;
}
#pragma mark - Private
@@ -161,6 +211,9 @@
- (void)handleEmbedderEvent:(const FlutterKeyEvent&)event
callback:(nullable FlutterKeyEventCallback)callback
userData:(nullable void*)userData {
+ if (_typeStorage != nil && (_typeStorageMask & kEmbedderCall) != 0) {
+ [_typeStorage addObject:@(kEmbedderCall)];
+ }
if (callback != nullptr) {
_embedderHandler(^(BOOL handled) {
callback(handled, userData);
@@ -171,6 +224,9 @@
- (void)handleChannelMessage:(NSString*)channel
message:(NSData* _Nullable)message
binaryReply:(FlutterBinaryReply _Nullable)callback {
+ if (_typeStorage != nil && (_typeStorageMask & kChannelCall) != 0) {
+ [_typeStorage addObject:@(kChannelCall)];
+ }
_channelHandler(^(BOOL handled) {
NSDictionary* result = @{
@"handled" : @(handled),
@@ -181,7 +237,10 @@
}
- (BOOL)handleTextInputKeyEvent:(NSEvent*)event {
- return _textInputResponse;
+ if (_typeStorage != nil && (_typeStorageMask & kTextCall) != 0) {
+ [_typeStorage addObject:@(kTextCall)];
+ }
+ return _textCallback(event);
}
@end
@@ -193,6 +252,7 @@
- (bool)textInputPlugin;
- (bool)forwardKeyEventsToSystemWhenComposing;
- (bool)emptyNextResponder;
+- (bool)racingConditionBetweenKeyAndText;
@end
namespace flutter::testing {
@@ -220,6 +280,10 @@
ASSERT_TRUE([[FlutterKeyboardManagerUnittestsObjC alloc] emptyNextResponder]);
}
+TEST(FlutterKeyboardManagerUnittests, RacingConditionBetweenKeyAndText) {
+ ASSERT_TRUE([[FlutterKeyboardManagerUnittestsObjC alloc] racingConditionBetweenKeyAndText]);
+}
+
} // namespace flutter::testing
@implementation FlutterKeyboardManagerUnittestsObjC
@@ -402,4 +466,80 @@
return true;
}
+// Regression test for https://github.com/flutter/flutter/issues/82673.
+- (bool)racingConditionBetweenKeyAndText {
+ KeyboardTester* tester = [[KeyboardTester alloc] init];
+
+ // Use Vietnamese IME (GoTiengViet, Telex mode) to type "uco".
+
+ // The events received by the framework. The engine might receive
+ // a channel message "setEditingState" from the framework.
+ NSMutableArray<FlutterAsyncKeyCallback>* keyCallbacks =
+ [NSMutableArray<FlutterAsyncKeyCallback> array];
+ [tester recordEmbedderCallsTo:keyCallbacks];
+
+ NSMutableArray<NSNumber*>* allCalls = [NSMutableArray<NSNumber*> array];
+ [tester recordCallTypesTo:allCalls forTypes:(kEmbedderCall | kTextCall)];
+
+ // Tap key U, which is converted by IME into a pure text message "ư".
+
+ [tester.manager handleEvent:keyDownEvent(kKeyCodeEmpty, @"ư", @"ư")];
+ EXPECT_EQ([keyCallbacks count], 1u);
+ EXPECT_EQ([allCalls count], 1u);
+ EXPECT_EQ(allCalls[0], @(kEmbedderCall));
+ keyCallbacks[0](false);
+ EXPECT_EQ([keyCallbacks count], 1u);
+ EXPECT_EQ([allCalls count], 2u);
+ EXPECT_EQ(allCalls[1], @(kTextCall));
+ [keyCallbacks removeAllObjects];
+ [allCalls removeAllObjects];
+
+ [tester.manager handleEvent:keyUpEvent(kKeyCodeEmpty)];
+ EXPECT_EQ([keyCallbacks count], 1u);
+ keyCallbacks[0](false);
+ EXPECT_EQ([keyCallbacks count], 1u);
+ EXPECT_EQ([allCalls count], 2u);
+ [keyCallbacks removeAllObjects];
+ [allCalls removeAllObjects];
+
+ // Tap key O, which is converted to normal KeyO events, but the responses are
+ // slow.
+
+ [tester.manager handleEvent:keyDownEvent(kKeyCodeKeyO, @"o", @"o")];
+ [tester.manager handleEvent:keyUpEvent(kKeyCodeKeyO)];
+ EXPECT_EQ([keyCallbacks count], 1u);
+ EXPECT_EQ([allCalls count], 1u);
+ EXPECT_EQ(allCalls[0], @(kEmbedderCall));
+
+ // Tap key C, which results in two Backspace messages first - and here they
+ // arrive before the key O messages are responded.
+
+ [tester.manager handleEvent:keyDownEvent(kKeyCodeBackspace)];
+ [tester.manager handleEvent:keyUpEvent(kKeyCodeBackspace)];
+ EXPECT_EQ([keyCallbacks count], 1u);
+ EXPECT_EQ([allCalls count], 1u);
+
+ // The key O down is responded, which releases a text call (for KeyO down) and
+ // an embedder call (for KeyO up) immediately.
+ keyCallbacks[0](false);
+ EXPECT_EQ([keyCallbacks count], 2u);
+ EXPECT_EQ([allCalls count], 3u);
+ EXPECT_EQ(allCalls[1], @(kTextCall)); // The order is important!
+ EXPECT_EQ(allCalls[2], @(kEmbedderCall));
+
+ // The key O up is responded, which releases a text call (for KeyO up) and
+ // an embedder call (for Backspace down) immediately.
+ keyCallbacks[1](false);
+ EXPECT_EQ([keyCallbacks count], 3u);
+ EXPECT_EQ([allCalls count], 5u);
+ EXPECT_EQ(allCalls[3], @(kTextCall)); // The order is important!
+ EXPECT_EQ(allCalls[4], @(kEmbedderCall));
+
+ // Finish up callbacks.
+ keyCallbacks[2](false);
+ keyCallbacks[3](false);
+
+ return true;
+}
+
@end
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardViewDelegate.h b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardViewDelegate.h
index 970d75f..2a6e0bd 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterKeyboardViewDelegate.h
+++ b/shell/platform/darwin/macos/framework/Source/FlutterKeyboardViewDelegate.h
@@ -4,6 +4,7 @@
#import <Cocoa/Cocoa.h>
+#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
#import "flutter/shell/platform/embedder/embedder.h"
/**
