- Cleanup many HTML special cases and skips in framework tests.
- Update some dartdocs that referred to the HTML renderer.
For reviewers: it may help if you set `Hide whitespace` to true in
Github. It will help you skip through all the formatting/indentation
changes.
This auto-formats all *.dart files in the repository outside of the
`engine` subdirectory and enforces that these files stay formatted with
a presubmit check.
**Reviewers:** Please carefully review all the commits except for the
one titled "formatted". The "formatted" commit was auto-generated by
running `dev/tools/format.sh -a -f`. The other commits were hand-crafted
to prepare the repo for the formatting change. I recommend reviewing the
commits one-by-one via the "Commits" tab and avoiding Github's "Files
changed" tab as it will likely slow down your browser because of the
size of this PR.
---------
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com>
This pull request aims to improve code readability, based on feedback gathered in a recent design doc.
<br>
There are two factors that hugely impact how easy it is to understand a piece of code: **verbosity** and **complexity**.
Reducing **verbosity** is important, because boilerplate makes a project more difficult to navigate. It also has a tendency to make one's eyes gloss over, and subtle typos/bugs become more likely to slip through.
Reducing **complexity** makes the code more accessible to more people. This is especially important for open-source projects like Flutter, where the code is read by those who make contributions, as well as others who read through source code as they debug their own projects.
<hr>
<br>
The following examples show how pattern-matching might affect these two factors:
<details> <summary><h3>Example 1 (GOOD)</h3> [click to expand]</summary>
```dart
if (ancestor case InheritedElement(:final InheritedTheme widget)) {
themes.add(widget);
}
```
Without using patterns, this might expand to
```dart
if (ancestor is InheritedElement) {
final InheritedWidget widget = ancestor.widget;
if (widget is InheritedTheme) {
themes.add(widget);
}
}
```
Had `ancestor` been a non-local variable, it would need to be "converted" as well:
```dart
final Element ancestor = this.ancestor;
if (ancestor is InheritedElement) {
final InheritedWidget inheritedWidget = ancestor.widget;
if (widget is InheritedTheme) {
themes.add(theme);
}
}
```
</details>
<details> <summary><h3>Example 2 (BAD) </h3> [click to expand]</summary>
```dart
if (widget case PreferredSizeWidget(preferredSize: Size(:final double height))) {
return height;
}
```
Assuming `widget` is a non-local variable, this would expand to:
```dart
final Widget widget = this.widget;
if (widget is PreferredSizeWidget) {
return widget.preferredSize.height;
}
```
<br>
</details>
In both of the examples above, an `if-case` statement simultaneously verifies that an object meets the specified criteria and performs a variable assignment accordingly.
But there are some differences: Example 2 uses a more deeply-nested pattern than Example 1 but makes fewer useful checks.
**Example 1:**
- checks that `ancestor` is an `InheritedElement`
- checks that the inherited element's `widget` is an `InheritedTheme`
**Example 2:**
- checks that `widget` is a `PreferredSizeWidget`
(every `PreferredSizeWidget` has a `size` field, and every `Size` has a `height` field)
<br>
<hr>
I feel hesitant to try presenting a set of cut-and-dry rules as to which scenarios should/shouldn't use pattern-matching, since there are an abundance of different types of patterns, and an abundance of different places where they might be used.
But hopefully the conversations we've had recently will help us converge toward a common intuition of how pattern-matching can best be utilized for improved readability.
<br><br>
- resolves https://github.com/flutter/flutter/issues/152313
- Design Doc: [flutter.dev/go/dart-patterns](https://flutter.dev/go/dart-patterns)
The SkiaGoldHttpOverrides don't have any effect since we never build our own HttpClient, it's always passed in.
This is part 18 of a broken down version of the #140101 refactor.
This particular change is more risky than other changes in this series. While by inspection and some instrumentation testing I'm reasonably sure that my assumptions here are correct, it would behoove us to make sure Skia Gold testing still works post-commit after this lands. To this end, I've included a change to the "inconsequential" test. It should fail the tests. I recommend we land this _with this failure_ to make sure it also fails post-commit, then immediately flag the image as passing and rerun the relevant shard.
We have fiddled with this a bunch, and there is definitively no way to reliably include this, so I am removing it.
Since CI does not cover all possible values, we can't consistently look up images for local testing. We thought removing it from the look up would work fine, and it did for most tests, but there were still some that could not find an image without it.
A brief history on the abi key
- added in https://github.com/flutter/flutter/pull/143621
- disabled in https://github.com/flutter/flutter/pull/148023
- added back in https://github.com/flutter/flutter/pull/148072
- we thought there was only an issue with alphabetizing keys
- removed from local image look up in https://github.com/flutter/flutter/pull/149696
I updated the docs page to also discuss what makes a good key and what does not based on what we learned here.
Fixes https://github.com/flutter/flutter/issues/149435
CI does not comprehensively cover all of the possible abi keys, so some folks can't test locally if their machine's abi does not match CI. This just removes it from the local testing look up. The right image will be summoned, and in CI we can still keep track of it.
This is part 17 of a broken down version of the #140101 refactor (though this particular dependency didn't actually exist back then).
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
I also snuck in a minor stylistic change, using interpolation instead of explicit `toString`, to make the code more idiomatic. (I'm actually surprised that that didn't trigger a lint, I thought we had a lint to catch that. I must be thinking of something else though.)
This PR makes no effort to keep the order of parameters reasonable. There is an existing TODO to do a refactor sweep later that does nothing but reorder arguments/parameters/fields to be consistent.
This is part 16 of a broken down version of the #140101 refactor.
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
This PR makes no effort to keep the order of parameters reasonable. There is an existing TODO to do a refactor sweep later that does nothing but reorder arguments/parameters/fields to be consistent.
This is part 15 of a broken down version of the #140101 refactor.
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
This PR makes no effort to keep the order of parameters reasonable. There is an existing TODO to do a refactor sweep later that does nothing but reorder arguments/parameters/fields to be consistent.
This is part 14 of a broken down version of the #140101 refactor.
This is an extension of part 8 (#146008), which had omitted removing the filesystem dependency in the SkiaGoldClient class.
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
This particular change attempts to be minimal. I made no effort to keep the order of parameters reasonable here. I have a TODO to do a refactor sweep later that does nothing but reorder arguments/parameters/fields to be consistent.
This is part 13 of a broken down version of the #140101 refactor.
This only makes one dependency explicit. Further PRs will do the same
for other dependencies, until these APIs have no hidden dependencies.
This is part 12 of a broken down version of the #140101 refactor.
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
Here's another PR with a couple of typos fixed. As you can see there was a typo in _fileReferenceI**n**dentifiers_, in class _ParsedProjectInfo._ Maybe we should do some check on that since I'm not sure if that property is used somewhere outside Flutter?
This is part 8 of a broken down version of the #140101 refactor.
This only makes one dependency explicit. Further PRs will do the same for other dependencies, until these APIs have no hidden dependencies.
This particular change attempts to be minimal. A future change will apply some formatting cleanup (newlines, reordering parameters and arguments) for clarity.
Fixes https://github.com/flutter/flutter/issues/145618
The local file comparator was being used in post submit for the Linux coverage shard. This corrects it to choose the skipping comparator.
This is part 4 of a broken down version of the #140101 refactor.
This PR renames isAvailableForEnvironment to isForEnvironment and replaces a regular expression with a simple function. (The latter will change the behaviour for people with branch names like `mainly_refactors` or `chess_master_experiment` or whatever, but I'm pretty sure the old behaviour was not intended.)
----
This is a reland of https://github.com/flutter/flutter/pull/143176 which was speculatively reverted in https://github.com/flutter/flutter/pull/144855 but turned out not to be the cause of the tree redness.
Reverts: flutter/flutter#143176
Initiated by: QuncCccccc
Reason for reverting: made tree red.
Original PR Author: Hixie
Reviewed By: {Piinks}
This change reverts the following previous change:
This is part 4 of a broken down version of the #140101 refactor.
This PR renames isAvailableForEnvironment to isForEnvironment and replaces a regular expression with a simple function. (The latter will change the behaviour for people with branch names like `mainly_refactors` or `chess_master_experiment` or whatever, but I'm pretty sure the old behaviour was not intended.)
This is part 4 of a broken down version of the #140101 refactor.
This PR renames isAvailableForEnvironment to isForEnvironment and replaces a regular expression with a simple function. (The latter will change the behaviour for people with branch names like `mainly_refactors` or `chess_master_experiment` or whatever, but I'm pretty sure the old behaviour was not intended.)
This is part 2 of a broken down version of the #140101 refactor.
This particular change wasn't in that original refactor but is a note to myself so that I remember how to test each of these changes in the future.
Originally landed in https://github.com/flutter/flutter/pull/139549
Originally reverted in https://github.com/flutter/flutter/pull/140085
- Remove all use of global variables.
- Always pass in all dependencies, only create them in main or in tests.
- Pass in the "print" primitive.
- Make all network traffic retry (except when run locally, when it just auto-passes).
- Enable tests to be run in random order.
- Better error messages
Reverts flutter/flutter#139549
Initiated by: Piinks
This change reverts the following previous change:
Original Description:
* Remove all use of global variables.
* Always pass in all dependencies, only create them in main or in tests.
* Pass in the "print" primitive.
* Make all network traffic retry (except when run locally, when it just auto-passes).
* Enable tests to be run in random order.
* Remove all use of global variables.
* Always pass in all dependencies, only create them in main or in tests.
* Pass in the "print" primitive.
* Make all network traffic retry (except when run locally, when it just auto-passes).
* Enable tests to be run in random order.
Part of fixing https://github.com/flutter/flutter/issues/139673, it will need to be cherry picked into the stable branch to fully resolve.
Originally I tried to come at this from `ci.yaml`, but the syntax does not exist to either conditionally include a dependency based on the branch we are on, or to disable a shard for a given branch (as opposed to enabling it which is supported). I could double every CI shard that uses Gold to try to serve my purpose, but it is already a very large and cumbersome file to keep up to date. Doubling it does not feel like the best solution. Using a RegEx is not my favorite, but I am using the same one we use in our CI, and left a note there to update it if it should ever change. Since there is already a whole infra built around it, I feel it is pretty safe so we can fix the stable tree.
We already had mitigated Gold affecting release branches in the past through flutter/cocoon (https://github.com/flutter/flutter/issues/58814), but https://github.com/flutter/flutter/issues/139673 exposed a rather rare edge case. A change was CP'd into the stable branch that introduced golden file image changes. Typically this would not cause an issue since any change that has landed on the master branch has golden files accounted for. In this case, the CP'd change on master has generated a different image on canvaskit due to another change that was not on stable. So when the CP landed, it generated a new image Gold had never seen before. Gold only tracks the master branch, so we cannot approve the image, and so cannot fix the stable tree.
This would disable the failing check on release branches and fix the tree.
## Description
This removes all of the comments that are of the form "so-and-so (must not be null|can ?not be null|must be non-null)" from the cases where those values are defines as non-nullable values.
This PR removes them from the animation, cupertino, foundation, gestures, semantics, and services libraries. Each of them only had a few, so I lumped them together.
This was done by hand, since it really didn't lend itself to scripting, so it needs to be more than just spot-checked, I think. I was careful to leave any comment that referred to parameters that were nullable, but I may have missed some.
In addition to being no longer relevant after null safety has been made the default, these comments were largely fragile, in that it was easy for them to get out of date, and not be accurate anymore anyhow.
This did create a number of constructor comments which basically say "Creates a [Foo].", but I don't really know how to avoid that in a large scale change, since there's not much you can really say in a lot of cases. I think we might consider some leniency for constructors to the "Comment must be meaningful" style guidance (which we de facto have already, since there are a bunch of these).
## Related PRs
- https://github.com/flutter/flutter/pull/134991
- https://github.com/flutter/flutter/pull/134992
- https://github.com/flutter/flutter/pull/134993
- https://github.com/flutter/flutter/pull/134994
## Tests
- Documentation only change.
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_driver`
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_goldens`
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_goldens_client`
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_localizations`
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_test`
* Use `curly_braces_in_flow_control_structures` for `packages/flutter_web_plugins`
* fix comments
* Use `curly_braces_in_flow_control_structures` for `packages/integration_test`
* fix indentation
