Skip to content

Conversation

@justinmc
Copy link
Contributor

Currently, if a TextInputClient.performPrivateCommand platform message is sent that has no data key, the framework will not know how to handle it and will throw an error. This PR properly handles this situation and eliminates the error.

The engine purposely sends a map with no data key in situations where the data map isn't used (code).

This PR allows receiving a null value and passing it through to onAppPrivateCommand.

@justinmc justinmc self-assigned this Sep 28, 2022
@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Sep 28, 2022
_currentConnection!._client.performPrivateCommand(
firstArg['action'] as String,
firstArg['data'] as Map<String, dynamic>,
firstArg['data'] == null
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: as Map<String, dynamic>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh duh, that's awesome that we can do that 👍

/// which is the Android documentation for sendAppPrivateCommand, used to
/// send a command to the input method.
void performPrivateCommand(String action, Map<String, dynamic> data);
void performPrivateCommand(String action, Map<String, dynamic>? data);
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks internal tests I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try running the Google tests on this PR when it goes green. I thought about just creating an empty map if it's null and keeping this parameter the same non-nullable, but it felt dirty. I'll consider just migrating any breakages myself if it's not too crazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've requested Google tests but it's not showing up here... This never works for me. Maybe because the PR has no LGTM yet?

@LongCatIsLooong
Copy link
Contributor

I LGTM'd earlier, google testing is now finished.

@justinmc
Copy link
Contributor Author

Ah sorry you're right. Well it looks like there are tons of breaking changes from that. I should have assumed lots of people are implementing TextInputClient. I will use an empty Map to avoid the breaking change.

@justinmc justinmc force-pushed the private-command-no-data branch from 94bb048 to 359d567 Compare September 29, 2022 22:49
@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 4, 2022

auto label is removed for flutter/flutter, pr: 112590, due to - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

@absar
Copy link

absar commented Oct 29, 2022

@LongCatIsLooong @justinmc given the small size of the fix can it be cherry picked into stable or beta, as it's flooding crashlytics on old Android devices with each tap in a text field, related issue #113973

@justinmc
Copy link
Contributor Author

justinmc commented Nov 1, 2022

I agree with that, it would be a very low-risk cherry pick. @absar Can you create a new cherry pick request issue? (New Issue, then choose "Request a cherry-pick"). Tag me and @itsjustkevin please.

@absar
Copy link

absar commented Nov 9, 2022

@justinmc I don't see this in beta branch, it should be cherry-picked there as well

@justinmc
Copy link
Contributor Author

@absar Now it looks like it's released on beta.

firstArg['data'] == null
? <String, dynamic>{}
: firstArg['data'] as Map<String, dynamic>,

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

Labels

a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants