-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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 cupertino page transition leak #147133
fixes cupertino page transition leak #147133
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
cc: @polina-c These changes directly reduces i want know if this is good approach (converting it into stateful widgets and using Also while looking into curve animation leaks i've noticed few things that might help-out:-
|
_primaryShadowCurve?.dispose(); | ||
} | ||
|
||
void _init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we invoke this method not only in the beginning, but also when widget is updated, meay be better name will be something like _setupAnimation
?
Thanks. Yes, in general approach looks correct. This does not mean it will be applicable everywhere though :) |
1540 to 660 sounds impressive. Thank you! |
Hii @polina-c Updated PR as per suggestion thanks for the review :) |
|
||
@override | ||
void didUpdateWidget(covariant CupertinoPageTransition oldWidget) { | ||
super.didUpdateWidget(oldWidget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should linearTransition be here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually primaryRouteAnimation
will also change when linearTransition
gets switched so i thought not make things more complicated, but it seems adding it would be good fail-safe, so added in last commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on the same fixes in #146999. I believe there are some issues in those changes:
- You might dispose some
CurvedAnimation
twice - You are actually always recreating the
CurvedAnimation
in each build because ofoldWidget.child != widget.child
which will always be true. This is why you are not facing the failing tests I am facing in Fix janks and memory leaks inCupertinoPageTransition
andCupertinoFullscreenDialogTransition
#146999
Those issues are "fixed" in #146999 so maybe I can continue the work there
if (oldWidget.primaryRouteAnimation != widget.primaryRouteAnimation | ||
|| oldWidget.secondaryRouteAnimation != widget.secondaryRouteAnimation | ||
|| oldWidget.child != widget.child || oldWidget.linearTransition != widget.linearTransition) { | ||
_disposeCurve(); | ||
_setupAnimation(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to check the child is different here, right? It is not used in the curve
void _disposeCurve() { | ||
_primaryPositionCurve?.dispose(); | ||
_secondaryPositionCurve?.dispose(); | ||
_primaryShadowCurve?.dispose(); | ||
} | ||
|
||
void _setupAnimation() { | ||
_primaryPositionAnimation = | ||
(widget.linearTransition | ||
? widget.primaryRouteAnimation | ||
: _primaryPositionCurve = CurvedAnimation( | ||
parent: widget.primaryRouteAnimation, | ||
curve: Curves.fastEaseInToSlowEaseOut, | ||
reverseCurve: Curves.fastEaseInToSlowEaseOut.flipped, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, because here you might end up with disposing the curved animation twice. You are not resetting it to null
@ValentinVignal Oh, it seems we were working on same issue, thanks for your review :) |
@polina-c Would you like me to continue on this? i can modify it as per @ValentinVignal suggestion, but if he already almost finished, there is no point in continuing. |
flutter/flutter@140edb9...77043ba 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ffd3911b1ff7 to 79f49954cce8 (2 revisions) (flutter/flutter#147235) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7d9deb66bf7 to ffd3911b1ff7 (1 revision) (flutter/flutter#147227) 2024-04-23 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147220) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 075d834489d0 to c7d9deb66bf7 (2 revisions) (flutter/flutter#147215) 2024-04-23 leroux_bruno@yahoo.fr Mention visualDensity impact on ButtonStyle.padding documentation (flutter/flutter#147048) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c0d24ff1cb6 to 075d834489d0 (1 revision) (flutter/flutter#147214) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoTextMagnifier` (flutter/flutter#147208) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 004e98839ed7 to 9c0d24ff1cb6 (1 revision) (flutter/flutter#147211) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 79f753650c6e to 004e98839ed7 (1 revision) (flutter/flutter#147209) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8e373da5227 to 79f753650c6e (1 revision) (flutter/flutter#147206) 2024-04-23 102401667+Dimilkalathiya@users.noreply.github.com fixes cupertino page transition leak (flutter/flutter#147133) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `PopupMenu` (flutter/flutter#147174) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62c9f17169cf to f8e373da5227 (2 revisions) (flutter/flutter#147205) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 33c828fb3ff5 to 62c9f17169cf (4 revisions) (flutter/flutter#147203) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.2 to 4.3.3 (flutter/flutter#147192) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/flutter#147193) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) (flutter/flutter#147190) 2024-04-22 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147094) 2024-04-22 polinach@google.com Re-land fix for not disposed TabController (flutter/flutter#146745) 2024-04-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 75ca2195c936 to a4b71f02a1c7 (1 revision) (flutter/flutter#147175) 2024-04-22 lamnhandev@gmail.com Update `examples/api` for android platform (flutter/flutter#147102) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sorry, I did not catch duplicate. @ValentinVignal , thanks for good catches. |
@Dimilkalathiya, @polina-c I'll continue with my PR (#146999) so I guess we are good for You can try to fix the same issues you have in your other PR for |
…r#6599) flutter/flutter@140edb9...77043ba 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ffd3911b1ff7 to 79f49954cce8 (2 revisions) (flutter/flutter#147235) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7d9deb66bf7 to ffd3911b1ff7 (1 revision) (flutter/flutter#147227) 2024-04-23 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147220) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 075d834489d0 to c7d9deb66bf7 (2 revisions) (flutter/flutter#147215) 2024-04-23 leroux_bruno@yahoo.fr Mention visualDensity impact on ButtonStyle.padding documentation (flutter/flutter#147048) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c0d24ff1cb6 to 075d834489d0 (1 revision) (flutter/flutter#147214) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoTextMagnifier` (flutter/flutter#147208) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 004e98839ed7 to 9c0d24ff1cb6 (1 revision) (flutter/flutter#147211) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 79f753650c6e to 004e98839ed7 (1 revision) (flutter/flutter#147209) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8e373da5227 to 79f753650c6e (1 revision) (flutter/flutter#147206) 2024-04-23 102401667+Dimilkalathiya@users.noreply.github.com fixes cupertino page transition leak (flutter/flutter#147133) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `PopupMenu` (flutter/flutter#147174) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62c9f17169cf to f8e373da5227 (2 revisions) (flutter/flutter#147205) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 33c828fb3ff5 to 62c9f17169cf (4 revisions) (flutter/flutter#147203) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.2 to 4.3.3 (flutter/flutter#147192) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/flutter#147193) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) (flutter/flutter#147190) 2024-04-22 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147094) 2024-04-22 polinach@google.com Re-land fix for not disposed TabController (flutter/flutter#146745) 2024-04-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 75ca2195c936 to a4b71f02a1c7 (1 revision) (flutter/flutter#147175) 2024-04-22 lamnhandev@gmail.com Update `examples/api` for android platform (flutter/flutter#147102) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#6599) flutter/flutter@140edb9...77043ba 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ffd3911b1ff7 to 79f49954cce8 (2 revisions) (flutter/flutter#147235) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7d9deb66bf7 to ffd3911b1ff7 (1 revision) (flutter/flutter#147227) 2024-04-23 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147220) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 075d834489d0 to c7d9deb66bf7 (2 revisions) (flutter/flutter#147215) 2024-04-23 leroux_bruno@yahoo.fr Mention visualDensity impact on ButtonStyle.padding documentation (flutter/flutter#147048) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c0d24ff1cb6 to 075d834489d0 (1 revision) (flutter/flutter#147214) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoTextMagnifier` (flutter/flutter#147208) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 004e98839ed7 to 9c0d24ff1cb6 (1 revision) (flutter/flutter#147211) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 79f753650c6e to 004e98839ed7 (1 revision) (flutter/flutter#147209) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8e373da5227 to 79f753650c6e (1 revision) (flutter/flutter#147206) 2024-04-23 102401667+Dimilkalathiya@users.noreply.github.com fixes cupertino page transition leak (flutter/flutter#147133) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `PopupMenu` (flutter/flutter#147174) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62c9f17169cf to f8e373da5227 (2 revisions) (flutter/flutter#147205) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 33c828fb3ff5 to 62c9f17169cf (4 revisions) (flutter/flutter#147203) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.2 to 4.3.3 (flutter/flutter#147192) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/flutter#147193) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) (flutter/flutter#147190) 2024-04-22 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147094) 2024-04-22 polinach@google.com Re-land fix for not disposed TabController (flutter/flutter#146745) 2024-04-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 75ca2195c936 to a4b71f02a1c7 (1 revision) (flutter/flutter#147175) 2024-04-22 lamnhandev@gmail.com Update `examples/api` for android platform (flutter/flutter#147102) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Part of #141198
Fixes
CupertinoPageTransition
widget leaksPre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.