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

Adaptive Switch #130425

Merged
merged 5 commits into from
Nov 7, 2023
Merged

Adaptive Switch #130425

merged 5 commits into from
Nov 7, 2023

Conversation

QuncCccccc
Copy link
Contributor

@QuncCccccc QuncCccccc commented Jul 12, 2023

Currently, Switch.factory delegates to CupertinoSwitch when platform is iOS or macOS. This PR is to:

  • have the factory configure the Material Switch for the expected look and feel.
  • introduce Adaptation class to customize themes for the adaptive components.
Adaptive_switch_example.mov

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].
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Jul 12, 2023
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This looks great! If we can prove (with tests) that we're faithfully rendering the Cupertino version of the Switch, this an important first step in a big project.

light = value;
});
},
return Column(
Copy link
Contributor

Choose a reason for hiding this comment

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

This example should include some Switches that override some visual defaults, to demonstrate that we honor visual parameters on all platforms. A final variant could show how a developer could configure a switch differently, depending on the Theme.of(context).platform. Nice to encapsulate that version in a CustomSwitch custom widget.

}
}
}
return _buildMaterialSwitch(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

The final version of this can just inline _buildMaterialSwitch of course.

Also: see the ElevatedButton.icon factory for an example of a public constructor that just maps to a private one.

@@ -752,6 +725,18 @@ class _MaterialSwitchState extends State<_MaterialSwitch> with TickerProviderSta
@override
bool? get value => widget.value;

void updateCurve() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the curve and reverseCurve properties should be Switch and SwitchTheme parameters (could be a separate PR). They'll be a little complicated to explain, but they are clearly needed to configure the switch for different look-and-feels.

packages/flutter/lib/src/material/switch.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch.dart Show resolved Hide resolved
@@ -1294,6 +1304,17 @@ class _SwitchPainter extends ToggleablePainter {
notifyListeners();
}

bool get isCupertino => _isCupertino!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is OK although the implementation might eventually need an enum so that other platforms could be specified.

@@ -1966,3 +2030,176 @@ class _SwitchConfigM3 with _SwitchConfig {
}

// END GENERATED TOKEN PROPERTIES - Switch

// Hand coded defaults for iOS/macOS Switch
class _SwitchDefaultsCupertino extends SwitchThemeData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Assume that we need both dark and light theme colors here?

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

Just a few comments; didn't have time to review the whole PR

examples/api/lib/material/switch/switch.4.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch.dart Outdated Show resolved Hide resolved
final ThemeData theme = Theme.of(context);
final SwitchThemeData switchTheme = SwitchTheme.of(context);
ThemeData theme = Theme.of(context);
SwitchThemeData switchTheme = SwitchTheme.of(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. The default adaptation should handle the theme.platform case analysis and we shouldn't be using ThemeData.copyWith ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks for pointing this out! Just updated this part and moved the platform check to ThemeData.adaptive().

@QuncCccccc QuncCccccc marked this pull request as ready for review November 1, 2023 23:33
@flutter-dashboard
Copy link

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #130425 at sha b8eab57

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Nov 2, 2023
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
examples/api/lib/material/switch/switch.4.dart Outdated Show resolved Hide resolved
examples/api/lib/material/switch/switch.4.dart Outdated Show resolved Hide resolved
dev/tools/gen_defaults/lib/switch_template.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

final ThemeData theme = ThemeData(
platform: isMaterial ? TargetPlatform.android : TargetPlatform.iOS,
adaptations: <Adaptation<Object>>[
if (isCustomized) const _SwitchThemeAdaptation()
Copy link
Contributor

Choose a reason for hiding this comment

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

NICE

packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved
@QuncCccccc QuncCccccc force-pushed the switch_adaptive branch 2 times, most recently from 494b723 to 4cd0c6e Compare November 6, 2023 19:28
Comment on lines +116 to +123
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.windows:
return defaultValue;
case TargetPlatform.iOS:
case TargetPlatform.macOS:
return SwitchThemeData(
Copy link
Member

Choose a reason for hiding this comment

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

Could we theme it for the non-Apple platforms too?

The customization button only customizes for Apple platforms right now. It's not clearly why it doesn't customize the material style by reading

/// This sample shows how to create and use subclasses of [Adaptation] that
/// define adaptive [SwitchThemeData]s.

Screen.Recording.2023-11-06.at.21.28.23.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can! To theme a switch for a particular platform, we can just provide a custom SwitchThemeData for that platform. Like:

switch (theme.platform) {
  case TargetPlatform.android:
    return defaultValue;
  case TargetPlatform.fuchsia:
    return SwitchThemeDataFuchsia();
  case TargetPlatform.linux:
    return SwitchThemeDataLinux();
  case TargetPlatform.windows:
    return SwitchThemeDataWindows();
  case TargetPlatform.iOS:
  case TargetPlatform.macOS:
    return SwitchThemeDataCupertino();
}

Just updated the sample doc! Thanks for the suggestion:)!

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #130425 at sha 4cd0c6e

@TahaTesser
Copy link
Member

@QuncCccccc
This is an excellent foundational PR for Adaptations! Thank you!

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #130425 at sha a6694f1

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #130425 at sha b7d43da

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #130425 at sha bc941dd

@QuncCccccc QuncCccccc merged commit ed70f4e into master Nov 7, 2023
137 checks passed
@QuncCccccc QuncCccccc deleted the switch_adaptive branch November 7, 2023 18:26
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 8, 2023
flutter/flutter@5a6a322...4b4a1fe

2023-11-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 117d47aa3f88 to b0310da3254d (1 revision) (flutter/flutter#138096)
2023-11-08 15619084+vashworth@users.noreply.github.com Run a couple of iOS tests in presubmit (flutter/flutter#138089)
2023-11-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3e3be5e33bda to 117d47aa3f88 (20 revisions) (flutter/flutter#138091)
2023-11-08 engine-flutter-autoroll@skia.org Roll Packages from be18d28 to 94c7623 (6 revisions) (flutter/flutter#138086)
2023-11-08 sokolovskyi.konstantin@gmail.com Ticker should dispatch creation and disposal events. (flutter/flutter#137844)
2023-11-08 christopherfujino@gmail.com [flutter_tools] Fix local engine preview device (flutter/flutter#138046)
2023-11-07 dnfield@google.com Revert "Add no-shuffle to reorderable_list_test.dart" (flutter/flutter#137715)
2023-11-07 greg@zulip.com Document where `Curves` curves correspond to CSS easing functions (flutter/flutter#137318)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8961d203039 to 3e3be5e33bda (1 revision) (flutter/flutter#138039)
2023-11-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use no-response from cocoon." (flutter/flutter#138042)
2023-11-07 godofredoc@google.com Use no-response from cocoon. (flutter/flutter#138037)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b20752e2a63 to f8961d203039 (6 revisions) (flutter/flutter#138034)
2023-11-07 hans.muller@gmail.com Added an AnimationController API doc example (flutter/flutter#137975)
2023-11-07 christopherfujino@gmail.com [flutter_tools] toolexit when using plugins with preview device (flutter/flutter#136936)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 38895fbd9468 to 1b20752e2a63 (1 revision) (flutter/flutter#138020)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from a9e0f9514f78 to 38895fbd9468 (1 revision) (flutter/flutter#138016)
2023-11-07 zanderso@users.noreply.github.com Move Skia new_gallery_transition_perf on a02 from staging to prod (flutter/flutter#138013)
2023-11-07 36861262+QuncCccccc@users.noreply.github.com Adaptive `Switch` (flutter/flutter#130425)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2810f07abb5 to a9e0f9514f78 (1 revision) (flutter/flutter#138005)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b91400976b4a to e2810f07abb5 (1 revision) (flutter/flutter#138002)
2023-11-07 dacoharkes@google.com [native assets] Tool exit on build failure (flutter/flutter#137995)

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 bmparr@google.com,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
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
flutter/flutter@5a6a322...4b4a1fe

2023-11-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 117d47aa3f88 to b0310da3254d (1 revision) (flutter/flutter#138096)
2023-11-08 15619084+vashworth@users.noreply.github.com Run a couple of iOS tests in presubmit (flutter/flutter#138089)
2023-11-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3e3be5e33bda to 117d47aa3f88 (20 revisions) (flutter/flutter#138091)
2023-11-08 engine-flutter-autoroll@skia.org Roll Packages from be18d28 to 94c7623 (6 revisions) (flutter/flutter#138086)
2023-11-08 sokolovskyi.konstantin@gmail.com Ticker should dispatch creation and disposal events. (flutter/flutter#137844)
2023-11-08 christopherfujino@gmail.com [flutter_tools] Fix local engine preview device (flutter/flutter#138046)
2023-11-07 dnfield@google.com Revert "Add no-shuffle to reorderable_list_test.dart" (flutter/flutter#137715)
2023-11-07 greg@zulip.com Document where `Curves` curves correspond to CSS easing functions (flutter/flutter#137318)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8961d203039 to 3e3be5e33bda (1 revision) (flutter/flutter#138039)
2023-11-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use no-response from cocoon." (flutter/flutter#138042)
2023-11-07 godofredoc@google.com Use no-response from cocoon. (flutter/flutter#138037)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b20752e2a63 to f8961d203039 (6 revisions) (flutter/flutter#138034)
2023-11-07 hans.muller@gmail.com Added an AnimationController API doc example (flutter/flutter#137975)
2023-11-07 christopherfujino@gmail.com [flutter_tools] toolexit when using plugins with preview device (flutter/flutter#136936)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 38895fbd9468 to 1b20752e2a63 (1 revision) (flutter/flutter#138020)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from a9e0f9514f78 to 38895fbd9468 (1 revision) (flutter/flutter#138016)
2023-11-07 zanderso@users.noreply.github.com Move Skia new_gallery_transition_perf on a02 from staging to prod (flutter/flutter#138013)
2023-11-07 36861262+QuncCccccc@users.noreply.github.com Adaptive `Switch` (flutter/flutter#130425)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2810f07abb5 to a9e0f9514f78 (1 revision) (flutter/flutter#138005)
2023-11-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b91400976b4a to e2810f07abb5 (1 revision) (flutter/flutter#138002)
2023-11-07 dacoharkes@google.com [native assets] Tool exit on build failure (flutter/flutter#137995)

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 bmparr@google.com,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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 12, 2024
Brings over the changes from `Switch.adaptive` into `CupertinoSwitch`.

This change adds the following `Switch` parameters to `CupertinoSwitch`:
`inactiveThumbColor,` `activeThumbImage`, `onActiveThumbImageError`, `inactiveThumbImage`, `onInactiveThumbImageError`, `trackOutlineColor`, `trackOutlineWidth`, `thumbIcon`, `mouseCursor`. 

The following `Switch` parameters are ignored:

* `activeTrackColor`: because `activeColor` has the same function.
* `inactiveTrackColor`: because `trackColor` has the same function.
* `materialTapTargetSize`: because it is only applicable in `Material`.
* `hoverColor`, `overlayColor`, `splashRadius`: because these parameters configure the radial reaction overlay in `Material`, so are not applicable here.

The following `CupertinoSwitch` parameters which are absent from `Switch.adaptive` are retained:

* `onLabelColor`, 
* `offLabelColor`, 
* `applyTheme`

`trackColor` and `thumbColor` are of type `WidgetStateProperty` in Material `Switch`, but are currently of type `Color` in `CupertinoSwitch`. For backwards compatibility, both parameters are kept as `Color`s, but can be resolved in different `WidgetState`s using `WidgetStateColor.resolve()`.

This PR does not apply any fidelity updates to `CupertinoSwitch`.

Part of #149275

Related PRs: #130425 #148804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants