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

fixes RawDialogRoute memory leak #147817

Merged
merged 1 commit into from May 7, 2024

Conversation

Dimilkalathiya
Copy link
Contributor

part of #141198

  • Fixes memory leak on RawDialogRoute
  • Adds opt-in test

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels May 4, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing CurvedAnimation here since default animation is linear anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Can you explain it in more details? It is not obvious to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Animation<double> we get from buildTransitions function is by default linear which is generated in abstract class TransitionRoute using AnimationController

Screenshot 2024-05-05 at 7 46 10 AM

So from my perspective it would be useless to add CurvedAnimation with linear curve since it is how Animation<double> will behave same by default, it would have been viable if it was anything except linear

does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Piinks, can you review this or help to find someone who can confirm there is no hidden footgun there?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, Curve.linear is no-op

@Dimilkalathiya
Copy link
Contributor Author

cc: @polina-c

@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label May 5, 2024
@polina-c polina-c requested a review from Piinks May 5, 2024 03:08
@goderbauer goderbauer requested a review from chunhtai May 7, 2024 22:07
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, Curve.linear is no-op

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2024
@auto-submit auto-submit bot merged commit 4abd735 into flutter:master May 7, 2024
73 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker autosubmit Merge PR when tree becomes green via auto submit App f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants