SemanticsFlag/SemanticsAction cleanup (part 3)

[bernaferrari]

These two classes should be an enum, because they work as enum, but they are older than the enhanced enum, so this wasn't possible before. Now it is.

This is kind of breaking because Flutter uses semanticsFlag.values.values, but I searched on code search and couldn't find a repo that uses that, so it seems really internal.
We could do in these ways:

• Add deserialize/indexAt/[] on them, migrate Flutter, convert to enum;
• Convert to enum and land the Flutter change at the same time (is this possible?);
• Don't do anything, lol

[kevmoo]

@yjbanov ?

[bernaferrari]

Bot tagged as web because there are two SemanticsFlag, one on the web and one everywhere else. I don't know who is responsible, lol. This could fall under accessibility. But Greg likes to review my PRs, if he is on engine and Yegor doesn't fit, you could assign him.

Ps. I saw some tests are failing. I'm into it. Just figuring out how to get gclient sync to work, this is tricky.
Ps2. I'm really struggling to get things working.
Ps3. The issue is probably getDartClassFields returning invalid because it is an enum.

[loic-sharma]

/cc @chunhtai @hangyujin

[chunhtai]

It looks like ci is failing, can you fix it?

[bernaferrari]

Yes, but I need your help. Getting gsync to work is too much for me. Can you help in Discord? I don't know how to get api_check to appear.

[chunhtai]

based on some of the error message, you may need to do a migration

Try correcting the name to the name of an existing getter, or defining a getter or field named 'values'.
      for (final SemanticsAction action in SemanticsAction.values.values)
                                                                  ^^^^^^
lib/src/semantics/semantics.dart:683:61: Error: The getter 'values' isn't defined for the class 'List<SemanticsFlag>'.
 - 'List' is from 'dart:core'.
 - 'SemanticsFlag' is from 'dart:ui'.
Try correcting the name to the name of an existing getter, or defining a getter or field named 'values'.
      for (final SemanticsFlag flag in SemanticsFlag.values.values)
                                                            ^^^^^^
lib/src/semantics/semantics.dart:2758:53: Error: The getter 'values' isn't defined for the class 'List<SemanticsFlag>'.
 - 'List' is from 'dart:core'.
 - 'SemanticsFlag' is from 'dart:ui'.
Try correcting the name to the name of an existing getter, or defining a getter or field named 'values'.
    final List<String> flags = SemanticsFlag.values.values.where((SemanticsFlag flag) => hasFlag(flag)).map((SemanticsFlag flag) => flag.toString().substring('SemanticsFlag.'.length)).toList();
                                                    ^^^^^^

[bernaferrari]

Do I make the migration in this PR? Or do I make one in Flutter and tie them together? I thought the error message was something else. This one is easy. I have it here: flutter/flutter#123329

[chunhtai]

Unfortunately, this can't be migrate like this because engine and framework pr can't go in at the same time.

You would need to create an temporarily API so that engine and framework pr can go in separately.

[bernaferrari]

I went ahead. This now is blocked by #40571 😀

[bernaferrari]

I just rebased on top of master.

Do you think SemanticsAction.deserialize(flag) is a good replacement for SemanticsAction.values[flag]? AndroidSemanticsAction used "deserialize", which was my inspiration.

I'm also using value as the value. field or something else would also be valid names (even action, flag or actionId could be good names! But value works for both, so the cognitive load might be lower. It also works as the individual unit of values).

[goderbauer]

Looks like some checks are failing

[bernaferrari]

Hold on, testing here (I need a YouTube tutorial on how to debug the engine lol). But almost there.

[goderbauer]

Is there a good reason to change the name of this property from index to value?

[bernaferrari]

enum has its own index (https://api.dart.dev/stable/2.19.4/dart-core/Enum/index.html), so you can't have two index with the same name. They are kind of similar, but the original index is 1 << index (so 2^index), and the "new index" matches the list position.

[goderbauer]

Oh, right! Makes sense.

[goderbauer]

LGTM

[chunhtai]

I am not super familiar with this but could this just be SemanticsAction(value)?

[bernaferrari]

No, from [] maybe. Could be potentially confusing, but I need to check.

[chunhtai]

same here

[bernaferrari]

I have a marriage now, I'll probably only finish this on the weekend.

The test failing is SemanticsFlag enums match, probably getDartClassFields doesn't know how to get an enum, that's my guess. If you want to help, feel free, else I'll take a look later. Engine Ninja took 17min to compile here, hopefully that's it.

[bernaferrari]

I agree. But...are there users using SemanticsAction? I searched code search and found a single usage, which the person copied Flutter bindings for Go. I doesn't seem to have that big of a user-base. But we could deprecate index, move to value, then migrate, or just don't do it. 99% of the (few) users use SemanticsAction.tap, not SemanticsAction.tap.index.

We could make half migration, deprecate index, move to value, then in a few months/years remove the value and put the enum in place without anyone seeing.

[goderbauer]

You could:

a) add value to the SemanticsAction class (not the enum)
b) migrate the framework
c) remove index from the SemanticsAction class and see if any of our customer tests (or google testing) break. If nothing breaks, we can continue with this migration and change the class to an enum. If things break, we may have to undo all that.

It's upto you if you're up for doing all that extra work :)

[bernaferrari]

Well, we came until here, we can get there (as long as your patience allows)... value is 1 LOC. Let's try and see.

Hixie will get tired of pressing ignore tests.

[flutter-dashboard]

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]

After a lot of discussion and nice ideas, this will be the end result. It is not an enum, and might probably never be, but it is almost there.

Do you prefer from, actionAt, fromAction, fromIndex or deserialize for the => values[index] getter? Somewhere else in Flutter uses deserialize, but I don't like. from or fromAction sounds better because SemanticsAction.fromAction(action) is more readable than SemanticsAction.fromIndex(action). But anything is fine.

Thanks for all the help!

[goderbauer]

LGTM

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #40567 at sha eb423ae

[bernaferrari]

The bot went crazy.

[chunhtai]

LGTM

[bernaferrari]

Thanks, everybody for this long journey. Feel free to add the "autosubmit".