Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

SemanticsUpdateBuilder.updateNode(): make TextDirection non-nullable #52421

Closed
wants to merge 2 commits into from
Closed

SemanticsUpdateBuilder.updateNode(): make TextDirection non-nullable #52421

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca13e5a
SemanticsUpdateBuilder.updateNode(): make TextDirection non-nullable
bartekpacia Apr 27, 2024
3fd9547
semantics.dart: remove unnecessary conditions
bartekpacia Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/ui/semantics.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ abstract class SemanticsUpdateBuilder {
required String hint,
required List<StringAttribute> hintAttributes,
required String tooltip,
required TextDirection? textDirection,
required TextDirection textDirection,
Copy link
Contributor

@chunhtai chunhtai Apr 29, 2024

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?

Copy link
Member Author

@bartekpacia bartekpacia Apr 30, 2024 •
edited
Loading

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.

Copy link
Contributor

@chunhtai chunhtai Apr 30, 2024

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

required Float64List transform,
required Int32List childrenInTraversalOrder,
required Int32List childrenInHitTestOrder,
Expand Down Expand Up @@ -888,7 +888,7 @@ base class _NativeSemanticsUpdateBuilder extends NativeFieldWrapperClass1 implem
required String hint,
required List<StringAttribute> hintAttributes,
required String tooltip,
required TextDirection? textDirection,
required TextDirection textDirection,
required Float64List transform,
required Int32List childrenInTraversalOrder,
required Int32List childrenInHitTestOrder,
Expand Down Expand Up @@ -927,7 +927,7 @@ base class _NativeSemanticsUpdateBuilder extends NativeFieldWrapperClass1 implem
hint,
hintAttributes,
tooltip,
textDirection != null ? textDirection.index + 1 : 0,
textDirection.index + 1,
Copy link
Contributor

@chunhtai chunhtai Apr 29, 2024

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?

Copy link
Member Author

@bartekpacia bartekpacia Apr 30, 2024

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?

Copy link
Contributor

@chunhtai chunhtai Apr 30, 2024 •
edited
Loading

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

Copy link
Member Author

@bartekpacia bartekpacia Apr 30, 2024

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.

Copy link
Contributor

@chunhtai chunhtai Apr 30, 2024

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

Copy link
Contributor

@chunhtai chunhtai Apr 30, 2024

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

transform,
childrenInTraversalOrder,
childrenInHitTestOrder,
Expand Down