Add missing parameter to `TableBorder.symmetric`, and improve class constructors by nate-thegrate · Pull Request #144279 · flutter/flutter

nate-thegrate · 2024-02-28T00:30:57Z

Originally, my aim was just to refactor (as per usual), but while messing around with the TableBorder.symmetric constructor, I realized that borderRadius was missing!

This pull request makes a few class constructors more efficient, and it fixes #144277 by adding the missing parameter.

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.

… into class-constructors

nate-thegrate · 2024-02-28T00:34:01Z

dev/devicelab/lib/framework/browser.dart

-  BlinkTraceEvent._({
-    required this.args,
-    required this.cat,
-    required this.name,
-    required this.ph,
-    this.pid,
-    this.tid,
-    this.ts,
-    this.tts,
-    this.tdur,
-  });


This constructor was only used in BlinkTraceEvent.fromJson, so it's no longer needed.

nate-thegrate · 2024-02-28T00:35:02Z

packages/flutter/lib/src/material/material_state.dart

-  /// Returns a [BorderSide] that's to be used when a Material component is
-  /// in the specified state. Return null to defer to the default value of the
-  /// widget or theme.
-  @override
-  BorderSide? resolve(Set<MaterialState> states);


Some methods needed to be moved down due to the "constructors should come before other stuff" rule.

nate-thegrate · 2024-02-28T00:38:12Z

packages/flutter/lib/src/services/autofill.dart

-  // prevents instantiation and extension.
-  AutofillHints._();
-
+abstract final class AutofillHints {


Most of these private constructors were removed once the final class modifier was added; I finished the rest in this PR.

goderbauer

LGTM

goderbauer · 2024-03-01T00:47:27Z

packages/flutter/lib/src/widgets/media_query.dart

    'This constructor was deprecated in preparation for the upcoming multi-window support. '
    'This feature was deprecated after v3.7.0-32.0.pre.'
  )
-  factory MediaQueryData.fromWindow(ui.FlutterView window) => MediaQueryData.fromView(window);


Maybe there should be a lint for this given how often we forgot that "assigning" a factory constructor is a thing. You could file a request for that on the linter repo and see if others would find that useful as well: https://github.com/dart-lang/linter

I went down a "class constructor rabbit hole" today—apparently the act of assigning a factory constructor is barely documented at all!

I proposed a linter rule over at https://github.com/dart-lang/linter/issues/4909, and I also made #144501, so hopefully information on factory constructors will be more easily accessible moving forward.

… into class-constructors

christopherfujino

some questions

packages/flutter_tools/lib/src/base/utils.dart

christopherfujino · 2024-03-04T18:34:41Z

packages/flutter_tools/lib/src/platform_plugins.dart

+        classPrefix = '',
+        pluginClass = yaml[kPluginClass] as String?,
+        dartPluginClass = yaml[kDartPluginClass] as String?,
+        ffiPlugin = yaml[kFfiPlugin] as bool? ?? false,


why did you add a fallback to false here?

Originally, IOSPlugin.fromYaml was a factory constructor that created an object using the default constructor. I added the fallback here (and in other places further down) to match the default constructor's fallback:

}) : ffiPlugin = ffiPlugin ?? false,

ok, makes sense.

packages/flutter_tools/lib/src/platform_plugins.dart

christopherfujino

LGTM

christopherfujino · 2024-03-04T20:19:02Z

packages/flutter_tools/lib/src/platform_plugins.dart

+        classPrefix = '',
+        pluginClass = yaml[kPluginClass] as String?,
+        dartPluginClass = yaml[kDartPluginClass] as String?,
+        ffiPlugin = yaml[kFfiPlugin] as bool? ?? false,


ok, makes sense.

… class constructors (flutter/flutter#144279)

flutter/flutter@65cd84b...3b5a2ec 2024-03-05 [email protected] Roll Flutter Engine from 17a4b66e0729 to 8916bb32b7b8 (1 revision) (flutter/flutter#144615) 2024-03-05 [email protected] Roll Flutter Engine from d514a302cdff to 17a4b66e0729 (8 revisions) (flutter/flutter#144607) 2024-03-05 [email protected] Do not shorten native assets framework names (flutter/flutter#144568) 2024-03-05 [email protected] Restorable CupertinoTextFormFieldRow (flutter/flutter#144541) 2024-03-05 [email protected] Updated the TextButton image button example artwork (flutter/flutter#144583) 2024-03-05 [email protected] Roll Flutter Engine from 728aa4ef05d4 to d514a302cdff (2 revisions) (flutter/flutter#144581) 2024-03-05 [email protected] Roll Flutter Engine from 62cf93be60b2 to 728aa4ef05d4 (3 revisions) (flutter/flutter#144578) 2024-03-05 [email protected] Disable super flakey impeller test. (flutter/flutter#144573) 2024-03-04 [email protected] Roll Flutter Engine from 40018811ec0e to 62cf93be60b2 (1 revision) (flutter/flutter#144572) 2024-03-04 [email protected] Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) (flutter/flutter#144570) 2024-03-04 [email protected] Print warning and exit when iOS device is unpaired (flutter/flutter#144551) 2024-03-04 [email protected] Roll Flutter Engine from a31209171949 to 0d8588b1de7a (1 revision) (flutter/flutter#144558) 2024-03-04 [email protected] Remove unnecessary (and the only) `RenderObject.markParentNeedsLayout` override (flutter/flutter#144466) 2024-03-04 [email protected] Fix text color for default CupertinoContextMenuAction (flutter/flutter#144542) 2024-03-04 [email protected] Add missing parameter to `TableBorder.symmetric`, and improve class constructors (flutter/flutter#144279) 2024-03-04 [email protected] Fix build mode not propagated in android native asset build (flutter/flutter#144550) 2024-03-04 [email protected] Roll Packages from a9c68b8 to 0625827 (3 revisions) (flutter/flutter#144556) 2024-03-04 [email protected] Roll Flutter Engine from afd2d9f9421d to a31209171949 (1 revision) (flutter/flutter#144554) 2024-03-04 [email protected] Doc fix for `DropdownButtonFormField.value` (flutter/flutter#144427) 2024-03-04 [email protected] Fix Small Typo in Skia_Client Doc Comment (flutter/flutter#144490) 2024-03-04 [email protected] Roll Flutter Engine from 2338a557e018 to afd2d9f9421d (1 revision) (flutter/flutter#144549) 2024-03-04 [email protected] [Windows] Update keyboard modifiers link (flutter/flutter#144426) 2024-03-04 [email protected] Roll Flutter Engine from 9cbca80e2196 to 2338a557e018 (2 revisions) (flutter/flutter#144545) 2024-03-04 [email protected] Roll Flutter Engine from c52b3033eca6 to 9cbca80e2196 (1 revision) (flutter/flutter#144523) 2024-03-04 [email protected] Roll Flutter Engine from 024ea680ceed to c52b3033eca6 (1 revision) (flutter/flutter#144522) 2024-03-03 [email protected] Roll Flutter Engine from 2706c732b22a to 024ea680ceed (1 revision) (flutter/flutter#144519) 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 [email protected],[email protected] 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

[A previous PR](#144279) implemented `const` constructors in the `Remote` class, so I wanted to follow up and use those constructors in the appropriate places.

[A previous PR](flutter#144279) implemented `const` constructors in the `Remote` class, so I wanted to follow up and use those constructors in the appropriate places.

… class constructors (flutter/flutter#144279)

nate-thegrate added 2 commits February 27, 2024 17:05

improve class constructors

1bff28e

Merge branch 'class-constructors' of github.com:nate-thegrate/flutter…

bc0a68c

… into class-constructors

nate-thegrate commented Feb 28, 2024

View reviewed changes

nate-thegrate added 2 commits February 27, 2024 17:44

add const constructors

1126385

lightThemeStyle/darkThemeStyle → const values

edd33bc

nate-thegrate marked this pull request as ready for review February 28, 2024 01:21

goderbauer approved these changes Mar 1, 2024

View reviewed changes

nate-thegrate added 4 commits February 29, 2024 19:17

Merge branch 'master' into class-constructors

525a2bb

a few constructor improvements that I missed earlier

9e3f87e

Merge branch 'class-constructors' of github.com:nate-thegrate/flutter…

8184971

… into class-constructors

add const constructor

d6fcb16

goderbauer requested a review from christopherfujino March 4, 2024 17:30

christopherfujino suggested changes Mar 4, 2024

View reviewed changes

christopherfujino approved these changes Mar 4, 2024

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 4, 2024

auto-submit bot merged commit 1a0dc8f into flutter:master Mar 4, 2024

nate-thegrate deleted the class-constructors branch March 4, 2024 20:20

engine-flutter-autoroll mentioned this pull request Mar 5, 2024

Roll Flutter from 65cd84b58885 to df2b36045302 (24 revisions) flutter/packages#6266

Closed

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

1a0dc8f Add missing parameter to TableBorder.symmetric, and improve…

2500539

… class constructors (flutter/flutter#144279)

engine-flutter-autoroll mentioned this pull request Mar 5, 2024

Roll Flutter from 65cd84b58885 to a47af8911920 (25 revisions) flutter/packages#6268

Closed

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

1a0dc8f Add missing parameter to TableBorder.symmetric, and improve…

c9d40e0

… class constructors (flutter/flutter#144279)

engine-flutter-autoroll mentioned this pull request Mar 5, 2024

Roll Flutter from 65cd84b58885 to 3b5a2ecf992d (26 revisions) flutter/packages#6269

Merged

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

1a0dc8f Add missing parameter to TableBorder.symmetric, and improve…

77661d8

… class constructors (flutter/flutter#144279)

HansMuller mentioned this pull request Mar 7, 2024

Flutter Framework Team: Notable Commits #121415

Closed

This was referenced Apr 9, 2024

enum cleanup nate-thegrate/flutter#3

Closed

Switch to relevant Remote constructors #146773

Merged

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

1a0dc8f Add missing parameter to TableBorder.symmetric, and improve…

ea3b811

… class constructors (flutter/flutter#144279)

nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jul 12, 2024

Conversation

nate-thegrate commented Feb 28, 2024

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants