-
Notifications
You must be signed in to change notification settings - Fork 6k
SemanticsUpdateBuilder.updateNode(): make TextDirection non-nullable #52421
SemanticsUpdateBuilder.updateNode(): make TextDirection non-nullable #52421
Conversation
|
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
| required List<StringAttribute> hintAttributes, | ||
| required String tooltip, | ||
| required TextDirection? textDirection, | ||
| required TextDirection textDirection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised this is not breaking the framework https://github.com/flutter/flutter/blob/28b81dc575da37247cb0719b17883b8db65560f6/packages/flutter/lib/src/semantics/semantics.dart#L2799
data.textDirection is nullable.
do you have another pr to migrate framework code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't.
I am also surprised. I was suspecting that this PR will have red CI, and then I'd submit a separate framework PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like they are disabled here #50800
| hintAttributes, | ||
| tooltip, | ||
| textDirection != null ? textDirection.index + 1 : 0, | ||
| textDirection.index + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happen to the case where textDirection used to be null? I think we want to send 0 to the native side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the native side has to handle TextDirection being null, and it's a valid use case, then I think this PR doesn't make sense and can be closed. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only took a quick glance of the code. We only enforce TextDirection to be non-null when there is and text data in the SemanticsData. I think this pr may not be the right move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a new test should be added, so when in the future someone tries to do the same PR as this one, that test would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird I remember we used to have a framework smoke test, but it was gone from the ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was disabled here #50800
|
Closing as per #52421 (comment) |
This PR is a very tiny cleanup that attempts to make all params of
SemanticsUpdateBuilder.updateNode()non-nullable.Pre-launch Checklist
///).