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 a more elaborate example #1

Merged
merged 1 commit into from Aug 11, 2020
Merged

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented Aug 11, 2020

Has animation, restoration, didUpdateWidget, didChangeDependencies, etc.

Has animation, restoration, didUpdateWidget, didChangeDependencies, etc.
@Hixie
Copy link
Contributor Author

Hixie commented Aug 11, 2020

I only added the StatefulWidget version, would love to see a Hooks version. I don't think the "late property" idea can really handle this yet, especially the performance-sensitive aspects.

super.initState();
_colorListenable = ValueNotifier<Color>(Colors.green[100]);
int color = 100;
_timer = Timer.periodic(const Duration(milliseconds: 900), (Timer timer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to instead use an AnimationController.repeat + tween, so that we benefit from TickerMode and are closer to common use-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because this only ticks every 900ms, not every 16ms. But it would be good to take TickerMode into account.

Comment on lines +87 to +92
if (_firstTime)
_animation = AnimationController(vsync: this);
_animation.duration = _duration.value;
if (_firstTime && widget.active)
_animation.repeat();
_firstTime = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these lines do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initialize the AnimationController and maintain its duration in the face of state restoration (e.g. hitting the back button on Web after clicking the button to change the duration).

@override
void didChangeDependencies() {
super.didChangeDependencies();
_height = _expensiveComputation(MediaQuery.of(context).size.height);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but I never understood the point of having didChangeDependencies
It's very brittle as adding any new dependency on a different InheritedWidget will cause the performance of this code to drop

So in the end we'd want to do:

final sizeHeight = MediaQuery.of(context).size.height;
if (previousSizeHeight != sizeHeight) {
  previousSizeHeight = sizeHeight;
  _height = _expensiveComputation(sizeHeight);
}

But that's very tedious.

}

class _StatefulAnimationState extends State<StatefulAnimation> {
ValueNotifier<Color> _colorListenable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this inside StatefulAnimation rather than Example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to show that the ValueListenable is coming via the widget rather than being local to the widget (in which case you wouldn't use a ValueListenable in the first place). Ideally it would be in the root widget in main.dart but I couldn't figure out how to make the hooks widget do it.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll merge it in as is, I can place the ValueNotifier higher in the tree to make more clear the purpose later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be fine to have them in the example as two different widgets just to show how it would be done in each mode. That keeps the weirdness out of the root.

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 still a bit weirded out by the presence of the ValueListenable<Color> color.
It feels like some unrelated architecture decisions are leaking in the example.

@TimWhiting TimWhiting merged commit 5136c60 into TimWhiting:master Aug 11, 2020
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.

None yet

3 participants