Skip to content

fix/pending-timer #151

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix/pending-timer #151

wants to merge 1 commit into from

Conversation

nank1ro
Copy link

@nank1ro nank1ro commented Feb 13, 2025

Hi,

I'm opening a PR to fix a bug I have on shadcn_ui, related issue nank1ro/flutter-shadcn-ui#294

The problem is that ShadApp uses ShadToaster which uses Animate from your package.
When the user runs tests with it, it gets the following error

Pending timers:
Timer (duration: 0:00:00.000000, periodic: false), created:
#0      new FakeTimer._ (package:fake_async/fake_async.dart:308:62)
#1      FakeAsync._createTimer (package:fake_async/fake_async.dart:252:27)
#2      FakeAsync.run.<anonymous closure> (package:fake_async/fake_async.dart:185:19)
#6      _AnimateState._restart (package:flutter_animate/src/animate.dart:318:23)
#7      _AnimateState.initState (package:flutter_animate/src/animate.dart:290:5)

The test failing is the basic counter test:

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';

import 'package:shadcn_counter/main.dart';

void main() {
  testWidgets('Counter increments smoke test', (WidgetTester tester) async {
    // Build our app and trigger a frame.
    await tester.pumpWidget(const MyApp());

    // Verify that our counter starts at 0.
    expect(find.text('0'), findsOneWidget);
    expect(find.text('1'), findsNothing);

    // Tap the '+' icon and trigger a frame.
    await tester.tap(find.byIcon(Icons.add));
    await tester.pump();

    // Verify that our counter has incremented.
    expect(find.text('0'), findsNothing);
    expect(find.text('1'), findsOneWidget);
  });
}

Code for main app

import 'package:flutter/material.dart';
import 'package:shadcn_ui/shadcn_ui.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return ShadApp.material(
      title: 'Flutter Demo',
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  int _counter = 0;

  void _incrementCounter() {
    setState(() {
      _counter++;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'You have pushed the button this many times:',
            ),
            Text(
              '$_counter',
              style: Theme.of(context).textTheme.headlineMedium,
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ),
    );
  }
}

Pubspec

dependencies:
  flutter:
    sdk: flutter
  cupertino_icons: ^1.0.8
  shadcn_ui: ^0.19.3

The ShadToaster doesn't do nothing too special, just a basic usage with controller and effects

                  return Animate(
                    controller: _controller,
                    effects: visible ? effectiveAnimateIn : effectiveAnimateOut,
                    child: Align(
                      alignment: effectiveAlignment,
                      child: Padding(
                        padding: EdgeInsets.symmetric(
                          horizontal: effectiveOffset.dx,
                          vertical: effectiveOffset.dy,
                        ),
                        child: toast,
                      ),
                    ),
                  );

For this reason, when the Duration is zero (which is the default) I don't schedule a timer.


I tried the fix locally, using the path directive and it works with the changes in the following PR

@gskinner
Copy link
Owner

gskinner commented Feb 13, 2025

Hi @nank1ro . Thanks for the PR!

It's been a long time since I wrote that code, and I'm almost certain there was an important reason it introduces the zero length delay. I believe I had this optimization in place early on, but had to remove it.

I'm not certain, but it may have to do with ensuring callbacks and events fire in a predictable manner. Perhaps due to it causing race conditions when there are callbacks at 0 that make calls that can impact the animation.

Sorry for the vague response. I'll try to look into this in more detail soon — including writing a solid callback test if there isn't one already. Feel free to do some investigation on your end if you'd like though.

@nank1ro
Copy link
Author

nank1ro commented Feb 26, 2025

Hi @gskinner, do you have any updates about it?
i'm writing widget tests on shadcn_ui and doing some bad workarounds to handle this issue.
In case you want to keep it as it is, are you willing to make duration optional, so I can override the default duration of zero with null? In case it's null do not trigger the scheduler.

@gskinner
Copy link
Owner

gskinner commented Mar 4, 2025

Still digging into this.

Up until 02aa06d I had an inline comment // TODO: bypass if delay=0?, but I pulled that out during a documentation pass (6f20fdc).

Unfortunately, I don't have any documentation on why, just a vague memory of testing it and finding issues with race conditions in some situations. I'm hoping to spend a bit more time with it today to see if I can remember the rationale (or determine it isn't an issue).

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.

2 participants