Fix out of sync templates files and add a check #145747

TahaTesser · 2024-03-26T11:00:46Z

Description

Add a check to verify template code in the Material library is synced with gen_defaults
Sync the changes to pass the new check.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-03-26T15:33:58Z

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.

guidezpl · 2024-04-02T09:50:56Z

Feel free to get a second review

goderbauer

This will also need a test or a test exception (see bot message).

dev/bots/analyze.dart

TahaTesser · 2024-04-03T15:21:03Z

Add a test in analayze_test.dart

QuncCccccc · 2024-04-03T16:20:58Z

Thanks for adding this check!! It was on my todo list but never got implemented...

goderbauer

LGTM, I leave the final review to @QuncCccccc.

dev/bots/test/analyze-gen-defaults/dev/tools/gen_defaults/bin/gen_defaults.dart

QuncCccccc

Thanks for adding this improvement! Just added one comment below and will take another look later today (sorry still need some time to understand the test for analyser).

QuncCccccc · 2024-04-09T19:00:12Z

dev/tools/gen_defaults/lib/input_decorator_template.dart

      if (states.contains(MaterialState.focused)) {
        return ${mergedBorder('md.comp.filled-text-field.error.focus.active-indicator','md.comp.filled-text-field.focus.active-indicator')};
      }
+      if (states.contains(MaterialState.hovered)) {


Just curious why we have the change here. When it is focused, we still want to see the hover effect, right? So probably hovered state should be put above focused?

This was a change from another PR which wasn't synced with the template class. I synced it here to pass the new check.

cc: @bleroux

Ah I see. I found the PR change: #146127
Looks like this is expected, can we add a comment above so this will not cause any confusion. Usually we'll have a order: pressed->hovered->focused. Here is a little more explanation: #125905 😄

Hey @QuncCccccc!

My PR which introduced this change, #146127, landed just before Taha implemented this new analyser test. Thanks to this new test, Taha spotted that I forgot to update the template. Instead of me filing a PR to update the template we decided that it will be ok to do it in this PR.

About the change itself, the problem with having the hovered state taking precedence over the focused one is especially noticeable on text field: the focused state for a text field increases border width (2dp) and applies bright colors (primary color or error color) while the hover state has the same border than a non focused text field (1dp) and use a color a little darker that a non focused text field. On desktop, it is also very common that a text field is focused and hovered (when users rely mainly on their mouse), so with the previous implementation when the text field was hovered it feels more or less as it lost focus:

Enregistrement.de.l.ecran.2024-04-09.a.22.23.01.mov

The M3 specification did not specify the exact behavior for this kind of visual properties impacted by hovered/focused states. What they explain in details is that hover is meant as a layer above the component but that is applicable for filled area not for text style and border like this.

I compared with other M3 implementations, especially https://material-web.dev/components/text-field/, and focus state takes precedence over hovered state for text field border (and also for the label color, that's not the case for Flutter text field for the moment).

I plan to file a similar PR for the label text color, I think it will be great to discuss that topic together on this coming PR, there might be cases I missed.

Got it. This totally makes sense to me! Thanks for the explanation! I think adding a comment above would be helpful because other widgets have a reversed order:)

+1, focus taking preference over hovered state is the design intention

Got it. This totally makes sense to me! Thanks for the explanation! I think adding a comment above would be helpful because other widgets have a reversed order:)

To avoid blocking Taha's PR, I added the detailed comment on #146572

QuncCccccc · 2024-04-10T19:06:50Z

dev/bots/test/analyze_test.dart

+    String result = await capture(() => verifyMaterialFilesAreUpToDateWithTemplateFiles(
+      testGenDefaultsPath,
+      dartPath,
+    ), shouldHaveErrors: true);


Just a question for my education. Looks like the chip_template.dart has the same content as the chip.dart in dev/bots/test/dev/bots/test/analyze-gen-defaults, why here shouldHaveErrors is true?

Test string description is different. I think i didn't make it different enough.

Ah chip_template.dart vs chip.dart! Got it!

QuncCccccc

LGTM! Thanks again for the fix:)!

QuncCccccc · 2024-04-10T19:24:19Z

dev/bots/test/analyze_test.dart

+    String result = await capture(() => verifyMaterialFilesAreUpToDateWithTemplateFiles(
+      testGenDefaultsPath,
+      dartPath,
+    ), shouldHaveErrors: true);


Ah chip_template.dart vs chip.dart! Got it!

guidezpl · 2024-04-11T08:44:04Z

dev/bots/test/analyze-gen-defaults/dev/tools/gen_defaults/bin/token_logger.dart

seems a bit strange to copy this file, is this common?

This is just for the test folder. Generator in the gen_defaults cannot update test file in the dev/bots

There are similar test folders in the dev/bots folder

https://github.com/flutter/flutter/tree/master/dev/bots/test/analyze-test-input/root/packages/flutter/lib

…utter#145747)

flutter/flutter@97cd47a...557fbf5 2024-04-11 barpac02@gmail.com Update app Android gradle scripts to use flutter.versionName and flutter.versionCode (flutter/flutter#146604) 2024-04-11 tessertaha@gmail.com Fix out of sync templates files and add a check (flutter/flutter#145747) 2024-04-11 dacoharkes@google.com [tools] Fix `--template=plugin_ffi` formatting (flutter/flutter#146269) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40b0f81332cb to d8560d495d9f (1 revision) (flutter/flutter#146622) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from fef8499fb9bf to 40b0f81332cb (1 revision) (flutter/flutter#146621) 2024-04-11 tessertaha@gmail.com Fix `IconButton` theming in the `InputDecorator` (flutter/flutter#146567) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2a45bda45cb to fef8499fb9bf (1 revision) (flutter/flutter#146619) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from e47dc9a7a24d to e2a45bda45cb (1 revision) (flutter/flutter#146618) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 069e73eaef34 to e47dc9a7a24d (1 revision) (flutter/flutter#146617) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 28ed19073fcf to 069e73eaef34 (1 revision) (flutter/flutter#146613) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4e33a0b47e3d to 28ed19073fcf (1 revision) (flutter/flutter#146611) 2024-04-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6f8ee9ffd9fa to 4e33a0b47e3d (3 revisions) (flutter/flutter#146609) 2024-04-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 91ba23575c82 to 6f8ee9ffd9fa (4 revisions) (flutter/flutter#146607) 2024-04-10 jason-simmons@users.noreply.github.com Disable single character mode in the terminal when exiting flutter_tools (flutter/flutter#146534) 2024-04-10 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146606) 2024-04-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from beee02b5ba2d to 91ba23575c82 (4 revisions) (flutter/flutter#146605) 2024-04-10 34871572+gmackall@users.noreply.github.com Remove additional references to engine v1 android embedding (flutter/flutter#146523) 2024-04-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 077b742550ef to beee02b5ba2d (2 revisions) (flutter/flutter#146591) 2024-04-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d5412d4ee4b to 077b742550ef (1 revision) (flutter/flutter#146585) 2024-04-10 15619084+vashworth@users.noreply.github.com Convert ProjectMigration and ProjectMigrator to be async (flutter/flutter#146537) 2024-04-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from cee489a4e275 to 0d5412d4ee4b (7 revisions) (flutter/flutter#146577) 2024-04-10 engine-flutter-autoroll@skia.org Roll Packages from 17f55d3 to 2c15d86 (2 revisions) (flutter/flutter#146575) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

### Description - Add a check to verify template code in the Material library is synced with `gen_defaults` - Sync the changes to pass the new check.

TahaTesser marked this pull request as ready for review March 26, 2024 11:00

TahaTesser requested a review from guidezpl March 26, 2024 11:00

guidezpl approved these changes Mar 26, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

TahaTesser force-pushed the sync_templates branch 3 times, most recently from 5c710df to 7ecc6a1 Compare April 1, 2024 07:45

This comment was marked as resolved.

Sign in to view

guidezpl approved these changes Apr 2, 2024

View reviewed changes

goderbauer reviewed Apr 2, 2024

View reviewed changes

dev/bots/analyze.dart Outdated Show resolved Hide resolved

TahaTesser force-pushed the sync_templates branch from 7ecc6a1 to 0de5f40 Compare April 3, 2024 09:14

TahaTesser changed the title ~~Fix out of sync templates files for FAB switch expressions and unused tokens~~ Fix out of sync templates files and add a check Apr 3, 2024

This comment was marked as resolved.

Sign in to view

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 3, 2024

TahaTesser force-pushed the sync_templates branch 3 times, most recently from cf9f30b to 9cfe768 Compare April 3, 2024 14:52

TahaTesser requested a review from goderbauer April 3, 2024 15:20

goderbauer reviewed Apr 3, 2024

View reviewed changes

dev/bots/test/analyze-gen-defaults/dev/tools/gen_defaults/bin/gen_defaults.dart Outdated Show resolved Hide resolved

TahaTesser requested a review from QuncCccccc April 4, 2024 07:53

TahaTesser force-pushed the sync_templates branch from 10053ec to f7f68a5 Compare April 4, 2024 14:10

QuncCccccc reviewed Apr 9, 2024

View reviewed changes

bleroux mentioned this pull request Apr 10, 2024

Fix label text color is wrong for a focused and hovered TextField #146572

Merged

QuncCccccc reviewed Apr 10, 2024

View reviewed changes

QuncCccccc approved these changes Apr 10, 2024

View reviewed changes

TahaTesser force-pushed the sync_templates branch from f7f68a5 to b3511ac Compare April 10, 2024 19:28

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine repository. See also e: labels. f: integration_test The flutter/packages/integration_test plugin labels Apr 11, 2024

Fix out of sync templates files and add a check

fc9ebba

TahaTesser force-pushed the sync_templates branch from aaa489d to fc9ebba Compare April 11, 2024 07:41

github-actions bot removed a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine repository. See also e: labels. f: integration_test The flutter/packages/integration_test plugin labels Apr 11, 2024

TahaTesser added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. and removed tool Affects the "flutter" command-line tool. See also t: labels. labels Apr 11, 2024

guidezpl reviewed Apr 11, 2024

View reviewed changes

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 11, 2024

auto-submit bot merged commit 9436b3c into flutter:master Apr 11, 2024
142 checks passed

TahaTesser deleted the sync_templates branch April 11, 2024 13:40

engine-flutter-autoroll mentioned this pull request Apr 11, 2024

Roll Flutter from 97cd47a02e60 to 557fbf5167f6 (22 revisions) flutter/packages#6502

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 11, 2024

9436b3c1c Fix out of sync templates files and add a check (flutter/fl…

1d2c176

…utter#145747)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix out of sync templates files and add a check #145747

Fix out of sync templates files and add a check #145747

TahaTesser commented Mar 26, 2024 •

edited

flutter-dashboard bot commented Mar 26, 2024

This comment was marked as resolved.

This comment was marked as resolved.

guidezpl commented Apr 2, 2024

goderbauer left a comment

This comment was marked as resolved.

TahaTesser commented Apr 3, 2024

QuncCccccc commented Apr 3, 2024

goderbauer left a comment

QuncCccccc left a comment

QuncCccccc Apr 9, 2024

TahaTesser Apr 9, 2024

QuncCccccc Apr 9, 2024

bleroux Apr 9, 2024 •

edited

QuncCccccc Apr 9, 2024

guidezpl Apr 10, 2024

bleroux Apr 10, 2024

QuncCccccc Apr 10, 2024

TahaTesser Apr 10, 2024

QuncCccccc Apr 10, 2024

QuncCccccc left a comment

QuncCccccc Apr 10, 2024

guidezpl Apr 11, 2024

TahaTesser Apr 11, 2024

Fix out of sync templates files and add a check #145747

Fix out of sync templates files and add a check #145747

Conversation

TahaTesser commented Mar 26, 2024 • edited

Description

Pre-launch Checklist

flutter-dashboard bot commented Mar 26, 2024

This comment was marked as resolved.

This comment was marked as resolved.

guidezpl commented Apr 2, 2024

goderbauer left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

TahaTesser commented Apr 3, 2024

QuncCccccc commented Apr 3, 2024

goderbauer left a comment

Choose a reason for hiding this comment

QuncCccccc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleroux Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuncCccccc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TahaTesser commented Mar 26, 2024 •

edited

bleroux Apr 9, 2024 •

edited