Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SemanticsFlag/SemanticsAction cleanup (part 1) #40571

Merged
merged 4 commits into from Mar 30, 2023
Merged

SemanticsFlag/SemanticsAction cleanup (part 1) #40571

merged 4 commits into from Mar 30, 2023

Conversation

bernaferrari
Copy link
Contributor

@bernaferrari bernaferrari commented Mar 23, 2023 •
edited

Add temporary API.
cc @chunhtai

part of flutter/flutter#123346

Steps:
part 1: [engine] add valuesAsList <-- we are here
part 2: [flutter] use valuesAsList
part 3: [engine] convert to enum
part 4: [flutter] use values again.
part 5: [engine] remove valuesAsList.
part 6: [flutter] deprecate describeEnum.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 23, 2023
@flutter-dashboard
Copy link

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.

@bernaferrari bernaferrari changed the title SemanticsFlag/SemanticsAction migration part 1 SemanticsFlag/SemanticsAction enum migration (part 1) Mar 23, 2023
@bernaferrari bernaferrari changed the title SemanticsFlag/SemanticsAction enum migration (part 1) SemanticsFlag/SemanticsAction enum migration (part 1) Mar 23, 2023
This was referenced Mar 23, 2023
Merged
Merged
@goderbauer
Copy link
Member

part 5: [engine] remove valuesAsList.

@bernaferrari
Copy link
Contributor Author

bernaferrari commented Mar 23, 2023 •
edited

Yes. lol. I just edited the discord message adding the part 5. And added 6, deprecate describeEnum. Thanks! Also added a brief description on breakiness (was in part 3, before I knew I would need to break into so many parts).

goderbauer
goderbauer reviewed Mar 23, 2023
View reviewed changes
lib/ui/semantics.dart Outdated
Comment on lines 231 to 232
/// Temporary API until [values] return a list.
List<SemanticsAction> get valuesAsList => values.values.toList(growable: false);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this have to be a static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, lol. Doing now.

Copy link
Member

Choose a reason for hiding this comment

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

To make sure this is actually the right change, can you implement part 2 already locally and run this change against it to make sure it works? See https://github.com/flutter/flutter/wiki/Debugging-the-engine#running-a-flutter-app-with-a-local-engine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We want static. Debugging the engine is sooooo hard and obscure. I figured out just calling the class in the same package would also work.

lib/ui/semantics.dart Outdated
Comment on lines 231 to 232
/// Temporary API until [values] return a list.
List<SemanticsAction> get valuesAsList => values.values.toList(growable: false);
Copy link
Member

Choose a reason for hiding this comment

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

Mark this API as deprecated so no-one starts accidentally using it during the migration outside of the use case we have for it?

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, thanks.

@goderbauer
Copy link
Member

Do you have part 4 and 3 up somewhere as a draft already? Just curious to see what the "final" result of this would look like.

@bernaferrari
Copy link
Contributor Author

bernaferrari commented Mar 23, 2023 •
edited

This is the final result in the engine (before I knew the migration was needed). 200 LOC removed.
#40567

This is the final result in Flutter. Almost no change, but cleaner, because thing.values.values become thing.values: flutter/flutter#123329

@bernaferrari bernaferrari mentioned this pull request Mar 23, 2023
Closed
6 tasks
@bernaferrari
Copy link
Contributor Author

I marked as deprecated and made them static.

@bernaferrari bernaferrari force-pushed the Semantics2 branch 2 times, most recently from 7661adc to e7f28f7 Compare March 23, 2023 19:20
@bernaferrari
Copy link
Contributor Author

bernaferrari commented Mar 27, 2023 •
edited

This should be ready to go (if approved). keys will become SemanticAction.values((d) => d.value), and valueAsList will become values.

chunhtai
chunhtai approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@Hixie
Copy link
Contributor

Hixie commented Mar 30, 2023

test-exempt: temporary code

you might want to consider naming these properties something horrible like DO_NOT_USE_WILL_BE_DELETED_WITHOUT_WARNING_values or whatever

@bernaferrari
Copy link
Contributor Author

hahaha I love that idea!!

goderbauer
goderbauer approved these changes Mar 30, 2023
View reviewed changes
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

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023
@auto-submit auto-submit bot merged commit d68444f into flutter:main Mar 30, 2023
34 checks passed
@bernaferrari bernaferrari deleted the Semantics2 branch March 30, 2023 19:28
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 30, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 30, 2023
@engine-flutter-autoroll
d68444fa5 SemanticsFlag/SemanticsAction enum migration (part 1) (f…
935dffd 
…lutter/engine#40571)
@bernaferrari bernaferrari changed the title SemanticsFlag/SemanticsAction enum migration (part 1) SemanticsFlag/SemanticsAction cleanup (part 1) Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer approved these changes

@chunhtai chunhtai chunhtai approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
4 participants
@bernaferrari @goderbauer @Hixie @chunhtai