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.

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

Add missing parameter to TableBorder.symmetric, and improve class constructors #144279

Merged
merged 8 commits into from Mar 4, 2024

Conversation

nate-thegrate
Copy link
Member

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

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

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: gestures flutter/packages/flutter/gestures repository. labels Feb 28, 2024
Comment on lines -425 to -435
BlinkTraceEvent._({
required this.args,
required this.cat,
required this.name,
required this.ph,
this.pid,
this.tid,
this.ts,
this.tts,
this.tdur,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines -324 to -328
/// 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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

class AutofillHints {
// This class is not meant to be instantiated or extended; this constructor
// prevents instantiation and extension.
AutofillHints._();

abstract final class AutofillHints {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@nate-thegrate nate-thegrate marked this pull request as ready for review February 28, 2024 01:21
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

@@ -190,7 +190,7 @@ class MediaQueryData {
'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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 dart-lang/linter#4909, and I also made #144501, so hopefully information on factory constructors will be more easily accessible moving forward.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

some questions

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

Choose a reason for hiding this comment

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

why did you add a fallback to false here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

Copy link
Member

Choose a reason for hiding this comment

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

ok, makes sense.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

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

Choose a reason for hiding this comment

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

ok, makes sense.

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 4, 2024
@auto-submit auto-submit bot merged commit 1a0dc8f into flutter:master Mar 4, 2024
132 checks passed
@nate-thegrate nate-thegrate deleted the class-constructors branch March 4, 2024 20:20
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Mar 5, 2024
flutter/flutter@65cd84b...3b5a2ec

2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17a4b66e0729 to 8916bb32b7b8 (1 revision) (flutter/flutter#144615)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from d514a302cdff to 17a4b66e0729 (8 revisions) (flutter/flutter#144607)
2024-03-05 matej.knopp@gmail.com Do not shorten native assets framework names (flutter/flutter#144568)
2024-03-05 leroux_bruno@yahoo.fr Restorable CupertinoTextFormFieldRow (flutter/flutter#144541)
2024-03-05 hans.muller@gmail.com Updated the TextButton image button example artwork (flutter/flutter#144583)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 728aa4ef05d4 to d514a302cdff (2 revisions) (flutter/flutter#144581)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62cf93be60b2 to 728aa4ef05d4 (3 revisions) (flutter/flutter#144578)
2024-03-05 jonahwilliams@google.com Disable super flakey impeller test. (flutter/flutter#144573)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40018811ec0e to 62cf93be60b2 (1 revision) (flutter/flutter#144572)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) (flutter/flutter#144570)
2024-03-04 magder@google.com Print warning and exit when iOS device is unpaired (flutter/flutter#144551)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from a31209171949 to 0d8588b1de7a (1 revision) (flutter/flutter#144558)
2024-03-04 31859944+LongCatIsLooong@users.noreply.github.com Remove unnecessary (and the only) `RenderObject.markParentNeedsLayout` override (flutter/flutter#144466)
2024-03-04 leroux_bruno@yahoo.fr Fix text color for default CupertinoContextMenuAction (flutter/flutter#144542)
2024-03-04 nate.w5687@gmail.com Add missing parameter to `TableBorder.symmetric`, and improve class constructors (flutter/flutter#144279)
2024-03-04 matej.knopp@gmail.com Fix build mode not propagated in android native asset build (flutter/flutter#144550)
2024-03-04 engine-flutter-autoroll@skia.org Roll Packages from a9c68b8 to 0625827 (3 revisions) (flutter/flutter#144556)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from afd2d9f9421d to a31209171949 (1 revision) (flutter/flutter#144554)
2024-03-04 36861262+QuncCccccc@users.noreply.github.com Doc fix for `DropdownButtonFormField.value` (flutter/flutter#144427)
2024-03-04 pateltirth454@gmail.com Fix Small Typo in Skia_Client Doc Comment (flutter/flutter#144490)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2338a557e018 to afd2d9f9421d (1 revision) (flutter/flutter#144549)
2024-03-04 737941+loic-sharma@users.noreply.github.com [Windows] Update keyboard modifiers link (flutter/flutter#144426)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9cbca80e2196 to 2338a557e018 (2 revisions) (flutter/flutter#144545)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c52b3033eca6 to 9cbca80e2196 (1 revision) (flutter/flutter#144523)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 024ea680ceed to c52b3033eca6 (1 revision) (flutter/flutter#144522)
2024-03-03 engine-flutter-autoroll@skia.org 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 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
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
…r#6269)

flutter/flutter@65cd84b...3b5a2ec

2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17a4b66e0729 to 8916bb32b7b8 (1 revision) (flutter/flutter#144615)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from d514a302cdff to 17a4b66e0729 (8 revisions) (flutter/flutter#144607)
2024-03-05 matej.knopp@gmail.com Do not shorten native assets framework names (flutter/flutter#144568)
2024-03-05 leroux_bruno@yahoo.fr Restorable CupertinoTextFormFieldRow (flutter/flutter#144541)
2024-03-05 hans.muller@gmail.com Updated the TextButton image button example artwork (flutter/flutter#144583)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 728aa4ef05d4 to d514a302cdff (2 revisions) (flutter/flutter#144581)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62cf93be60b2 to 728aa4ef05d4 (3 revisions) (flutter/flutter#144578)
2024-03-05 jonahwilliams@google.com Disable super flakey impeller test. (flutter/flutter#144573)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40018811ec0e to 62cf93be60b2 (1 revision) (flutter/flutter#144572)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) (flutter/flutter#144570)
2024-03-04 magder@google.com Print warning and exit when iOS device is unpaired (flutter/flutter#144551)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from a31209171949 to 0d8588b1de7a (1 revision) (flutter/flutter#144558)
2024-03-04 31859944+LongCatIsLooong@users.noreply.github.com Remove unnecessary (and the only) `RenderObject.markParentNeedsLayout` override (flutter/flutter#144466)
2024-03-04 leroux_bruno@yahoo.fr Fix text color for default CupertinoContextMenuAction (flutter/flutter#144542)
2024-03-04 nate.w5687@gmail.com Add missing parameter to `TableBorder.symmetric`, and improve class constructors (flutter/flutter#144279)
2024-03-04 matej.knopp@gmail.com Fix build mode not propagated in android native asset build (flutter/flutter#144550)
2024-03-04 engine-flutter-autoroll@skia.org Roll Packages from a9c68b8 to 0625827 (3 revisions) (flutter/flutter#144556)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from afd2d9f9421d to a31209171949 (1 revision) (flutter/flutter#144554)
2024-03-04 36861262+QuncCccccc@users.noreply.github.com Doc fix for `DropdownButtonFormField.value` (flutter/flutter#144427)
2024-03-04 pateltirth454@gmail.com Fix Small Typo in Skia_Client Doc Comment (flutter/flutter#144490)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2338a557e018 to afd2d9f9421d (1 revision) (flutter/flutter#144549)
2024-03-04 737941+loic-sharma@users.noreply.github.com [Windows] Update keyboard modifiers link (flutter/flutter#144426)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9cbca80e2196 to 2338a557e018 (2 revisions) (flutter/flutter#144545)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c52b3033eca6 to 9cbca80e2196 (1 revision) (flutter/flutter#144523)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 024ea680ceed to c52b3033eca6 (1 revision) (flutter/flutter#144522)
2024-03-03 engine-flutter-autoroll@skia.org 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 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
auto-submit bot pushed a commit that referenced this pull request Apr 16, 2024
[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.
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
[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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: gestures flutter/packages/flutter/gestures repository. f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TableBorder.symmetric is missing the borderRadius parameter
3 participants