Skip to content

[beta branch] Migrate animation sample #701

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

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

RedBrogdon
Copy link
Contributor

  • Ups the SDK minimum for /animations to 2.12.0-0
  • Migrates all the code
  • Changes the animated list sample from using indexes to identify records to using IDs.
  • Updates pubspec.(yaml|lock)

Comments especially welcome on the changes for migration.

AnimationController controller;
Animation<Color> animation;
late AnimationController controller;
late Animation<Color?> animation;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious how nulls could be inserted this animation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While doing the conversion, this was a weird part for me as well. ColorTween is a glorified version of Tween<Color?>, which does allow for a null Color value:

https://github.com/flutter/flutter/blob/7c0300a3588d91542366d9efadda26fa2c9d9224/packages/flutter/lib/src/animation/tween.dart#L355

I thought about switching to Tween<Color>, but that would cause us to lose the Color-specific lerp function provided by ColorTween.

In the case of this example, I don't expect the tween to ever produce a null value, but it looks like MaterialButton is perfectly happy to accept one and just use the default color in response:

https://github.com/flutter/flutter/blob/88809aa24720705d1cd8de0bfc46faf932fe0b6e/packages/flutter/lib/src/material/material_button.dart#L180

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, the docs for Tween call this out:

  /// See the constructor for details about whether this property may be null
  /// (it varies from subclass to subclass).
  T? begin;

ColorTween and most subclasses allows null intentionally:

  /// We recommend that you do not pass [Colors.transparent] as [begin]
  /// or [end] if you want the effect of fading in or out of transparent.
  /// Instead prefer null. [Colors.transparent] refers to black transparent and
  /// thus will fade out of or into black which is likely unwanted.

To me, this seems weird for users, but at least it's explicit now.

AnimationController controller;
Animation<Color> animation;
late AnimationController controller;
late Animation<Color?> animation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, the docs for Tween call this out:

  /// See the constructor for details about whether this property may be null
  /// (it varies from subclass to subclass).
  T? begin;

ColorTween and most subclasses allows null intentionally:

  /// We recommend that you do not pass [Colors.transparent] as [begin]
  /// or [end] if you want the effect of fading in or out of transparent.
  /// Instead prefer null. [Colors.transparent] refers to black transparent and
  /// thus will fade out of or into black which is likely unwanted.

To me, this seems weird for users, but at least it's explicit now.

@@ -5,13 +5,13 @@
import 'package:flutter/material.dart';

class TypewriterTween extends Tween<String> {
TypewriterTween({String begin = '', String end})
TypewriterTween({String begin = '', String end = ''})
Copy link
Contributor

Choose a reason for hiding this comment

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

What's weird about this is that it doesn't seem to be possible to implement a Tween with a non-nullable T:

class TypewriterTween extends Tween<String> {
  late String _begin;
  late String _end;

  TypewriterTween({String begin = '', String end = ''}) : _begin = begin, _end = end;

  @override
  String get begin => _begin;
  @override
  String get end => _end;
  @override
  set begin(String s) => _begin = s;
  @override
  set end(String s) => _end = s;

  @override
  String lerp(double t) {
    var cutoff = (end!.length * t).round();
    return end!.substring(0, cutoff);
  }
}

Screen Shot 2021-02-09 at 10 59 30 AM

But I can't seem to reproduce this in DartPad. This sample works just fine:

void main() {
  var b = B('foo');
  print(b.s);
}

class A<T> {
  T? s;
}

class B extends A {
  String _s;
  B(String s) : _s = s;

  String get s => _s;
  set(String s) => _s = s;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this threw me a bit, as you probably saw. Tween takes a single type param, and then defines begin and end with the nullable version of that type parameter.

Copy link
Contributor

@johnpryan johnpryan Feb 11, 2021

Choose a reason for hiding this comment

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

Yeah T? is a subtype of T so it's not clear to me why this isn't working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the subtype relationship was the reverse (String is a subtype of String?). I'm going to go read the null safety guide again. 😄

@RedBrogdon
Copy link
Contributor Author

@johnpryan Thanks, and PTAL! I believe I've addressed all the actionable comments.

@RedBrogdon RedBrogdon merged commit 7438e87 into flutter:beta Feb 12, 2021
@RedBrogdon RedBrogdon deleted the migrate-animations branch February 12, 2021 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants