Skip to content

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Dec 17, 2019

flutter/flutter#47177 is a breaking change no matter how I try to slice it - even if I pull out the test related changes. Provides a guide for migrating tests that this breaks.

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Dec 17, 2019
@dnfield
Copy link
Contributor Author

dnfield commented Dec 17, 2019

CI appears to be failing on unrelated files...

Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

@dnfield, I'm a little surprised to see this PR against the flutter/flutter repo, as we are homing the breaking changes on flutter/website. (Under src/docs/release/breaking-changes)

## Description of change

The Flutter framework tracks when the editing state of an `EditableText` widget
changes. It keeps synchronizes this value to the engine, but only if the value
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete "keeps"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

The Flutter framework tracks when the editing state of an `EditableText` widget
changes. It keeps synchronizes this value to the engine, but only if the value
has changed. The state of this value is tracked by the `TextInputClient`, and
logic exists to prevent sending duplicative values to the engine side.
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicative => duplicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dnfield
Copy link
Contributor Author

dnfield commented Dec 17, 2019

@sfshaza2 - this is against the website repo :) I've linked to the related flutter/flutter PR here.

Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

That was fast, @dnfield!! LGTM. Waiting to merge for eng review (unless you tell me otherwise).

@dnfield
Copy link
Contributor Author

dnfield commented Dec 17, 2019

Thanks - let's give Hixie and goderbauer a chance to see it :)

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM


## Migration guide

Tests that relied on the dirty state will have to be updated. For example, this
Copy link
Member

Choose a reason for hiding this comment

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

maybe call out ".. dirty state from a previously run test..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dnfield dnfield merged commit 39da3cd into flutter:master Dec 17, 2019
@dnfield dnfield deleted the migration branch December 17, 2019 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Contributor has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants