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

Make callbacks fire before future #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

caseycrogers
Copy link
Contributor

@caseycrogers caseycrogers commented Dec 25, 2021

Currently, the onShow and the onDismiss callbacks fire when their respective actions are completed, not when the actions start. This PR makes them fire on start.

This is preferable because developers often want to trigger behavior on start (eg to play an animation in-step with the tooltip's show animation). They can't do this with the current design because their callback runs too late.

If developers want to trigger behavior when the tooltip animations finish, they can simply add a delay of the same duration as their animations to their callback:

  onShow: () async {
    await Future.delayed(milliseconds: 150);
    doStuffAfterToolTipIsShown();
  }

@stargazing-dino
Copy link
Owner

I won't really get to this until after the holiday but what do you think about the onShow getting passed the state the tooltip is in?

Something like:

onShow: (state) {
  switch (state) {
    case TooltipState.start:
      //
    case TooltipState.end:
      //
  }
}

That or we could even pass through the controller state as it's a value notifier with the state anyways.

Another option would be to just add two more callbacks of onShowEnd or similar.

I'm open to the onShow being on the leading end like you say and just awaiting the same duration but I think both cases are common enough that a user wouldn't mind both cases being easily handled

Thank you for the PR too !

@caseycrogers
Copy link
Contributor Author

caseycrogers commented Dec 26, 2021

All these options seem good, a lot of the Flutter first party widgets will have a simple onShow for convenience and then a more complex change notifier and/or notification system for when you want more control. I'll update this PR to pass state to onShow and have it called both on start and end.

(also obviously no rush on this, I actually ended up cutting the feature in my app that required tool tips anyway)

P.S. happy holidays!


final completer = Completer<void>();
final future = completer.future.whenComplete(() {
_removeEntries();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was some really weird behavior around the completers in _hideTooltip and showTooltip. Basically, hide and show were only kicking off the animation when they were first called and then setting the controller's status when the animation finished. Then, a listener on the animation status would see that it had changed and would call _hide or _show respectively a second time, this time with immediately = true. The functions would then properly execute clean up logic under an if (immediately) check. This was incorrectly firing the onShow and onDismiss callbacks twice.

I've fixed this by putting all the clean up logic in completer.complete() so it's called by the immediate and non-immediate code paths and by adding a check at the top of each function to short circuit it if the tooltip is already in the desired state.

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