Fix copy/paste when scribble enabled #31691
Conversation
…or canperformaction
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -866,6 +866,32 @@ - (void)testUITextInputAvoidUnnecessaryUndateEditingClientCalls { | |||
| XCTAssertEqual(updateCount, 2); | |||
| } | |||
|
|
|||
| - (void)testCanCopyPasteWithScribbleEnabled { | |||
| if (UI_USER_INTERFACE_IDIOM() != UIUserInterfaceIdiomPad) { | |||
There was a problem hiding this comment.
This is probably not ok (the test will only run on iPad?). What I'm trying to do is to get IsScribbleAvailable to return true. It's a static function in FlutterTextInputPlugin. Is there a way I can mock it instead?
There was a problem hiding this comment.
I would suggest not making IsScribbleAvailable a static function. I don't see a particular reason IsScribbleAvailable has to be static.
There was a problem hiding this comment.
Thanks for the suggestion, that worked! This is now ready for full review.
|
I forget if I saw that "[app] pasted" notification. If I remember
correctly, I returned NO for copy paste there to prevent the native text
editing toolbar from showing up. As long as that is also not happening when
selecting text either with scribble or touch (or keyboard) then I think
this should be fine.
…On Sat, Feb 26, 2022, 7:14 PM Chris Yang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.mm
<#31691 (comment)>:
> @@ -866,6 +866,32 @@ - (void)testUITextInputAvoidUnnecessaryUndateEditingClientCalls {
XCTAssertEqual(updateCount, 2);
}
+- (void)testCanCopyPasteWithScribbleEnabled {
+ if (UI_USER_INTERFACE_IDIOM() != UIUserInterfaceIdiomPad) {
I would suggest not making IsScribbleAvailable a static function. I don't
see a particular reason IsScribbleAvailable has to be static.
—
Reply to this email directly, view it on GitHub
<#31691 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYZOO6FOA7FWU55RJXZV33U5F3HVANCNFSM5PLWBLTQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I saw your comment about the native toolbar but I was never able to get it to appear while testing on the iPad. I'll keep an eye out for it but hopefully we're good. |
| @@ -1048,7 +1048,7 @@ - (BOOL)canBecomeFirstResponder { | |||
| - (BOOL)canPerformAction:(SEL)action withSender:(id)sender { | |||
| // When scribble is available, the FlutterTextInputView will display the native toolbar unless | |||
| // these text editing actions are disabled. | |||
| if (IsScribbleAvailable()) { | |||
| if ([self isScribbleAvailable] && sender == NULL) { | |||
There was a problem hiding this comment.
@justinmc do you remember what does sender == NULL mean here?
Also can you explain more about what this is trying to do? When I remove this sender == NULL, CMD+C/V from hardware keyboard still works. So I may be missing somehting. Thanks!
There was a problem hiding this comment.
Sorry I didn't document this better, I'm struggling to remember...
Are you trying this on an iPad with a hardware keyboard attached? And iOS 14+.
The relevant part of the docs for canPerformAction say:
The object calling this method. For the editing menu commands, this is the shared UIApplication object.
Looking at the original scribble PR (#24224), we seem to call performAction a bunch. Maybe it's called with no sender from some of those and/or when hardware keys are being pressed?
Are you considering removing that expression for some reason? I would test a physical iPad and Apple Pencil and hardware keyboard before at least.
There was a problem hiding this comment.
I am not doing anything particular for this code. I was just looking into "secure paste" feature, and trying to understand the codebase.
There was a problem hiding this comment.
interestingly i found that this function is not even called when we copy/paste. The other function in FlutterPlatformPlugin.mm is called instead. This code seems to be obsolete
There was a problem hiding this comment.
Nice, I'm happy to review if you want to remove it.
The recent scribble PR (#24224) broke cmd+c and cmd+v on scribble enabled devices. The reason was this line, which seems to have been added to avoid the "[app] pasted" notification from showing up on every keystroke.
This solution returns YES when it receives a real copy/paste action and not just a key input, so copy and paste work and you won't get the notification on normal keyboard input.
Fixes flutter/flutter#98832
CC @fbcouch