Day picker should dispose created MaterialStatesController's.#133884
Day picker should dispose created MaterialStatesController's.#133884polina-c merged 25 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
framework
polina-c
added
the
autosubmit
| onTap: () => widget.onChanged(dayToBuild), | ||
| radius: _dayPickerRowHeight / 2 + 4, | ||
| statesController: MaterialStatesController(states), | ||
| statesController: _materialStatesController, |
There was a problem hiding this comment.
This doesn't look right, every date widget should have a separate states controller. cc @HansMuller
There was a problem hiding this comment.
That's right. each day widget should have its own selected/disabled/etc state.
There was a problem hiding this comment.
It is interesting that tests passed...
There was a problem hiding this comment.
Fixed. Looks better?
There was a problem hiding this comment.
Yes that looks better although factoring out a stateful day widget per @chunhtai's suggestion seems like a cleaner way to do this.
And, as you pointed out, it looks like need some additional tests.
There was a problem hiding this comment.
I expect separate widget for a day will take 2-3 iterations, and I do not want to block conversion of tests to testWidgetsWithLeakTracking, that will insure disposable are disposed. Created TODO: #134323.
polina-c
changed the title
polina-c
removed
the
autosubmit
polina-c
requested review from
HansMuller
and removed request for
goderbauer
| onTap: () => widget.onChanged(dayToBuild), | ||
| radius: _dayPickerRowHeight / 2 + 4, | ||
| statesController: MaterialStatesController(states), | ||
| statesController: statesController, |
There was a problem hiding this comment.
This may leak if this widget rebuild with different days.
One idea to make this easier to manage is to create a statefulwidget wrapper around the dayWidget that holds the statesController. that way you won't need to keep track of each statesController in this widget.
There was a problem hiding this comment.
It will not leak because the map's keys are from 1 to 31.
So' it will create maximum 31 controllers. Even if month changes, controllers for days will get new correct values in next build.
Anything I am missing?
There was a problem hiding this comment.
yes you are right that it will not grow forever. I am fine with this change then. But since @HansMuller is more familiar with this part of the code, we should get his LGTM before merge
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, but it is better to wait for @HansMuller's approval
| onTap: () => widget.onChanged(dayToBuild), | ||
| radius: _dayPickerRowHeight / 2 + 4, | ||
| statesController: MaterialStatesController(states), | ||
| statesController: statesController, |
There was a problem hiding this comment.
yes you are right that it will not grow forever. I am fine with this change then. But since @HansMuller is more familiar with this part of the code, we should get his LGTM before merge
|
I'm OK with the change as it is although I'd prefer to see a Day widget factored out and a regression test that would fail if didn't properly maintain state per day (#133884 (comment)). |
polina-c
changed the title
polina-c
changed the title
polina-c
added
the
autosubmit
|
auto label is removed for flutter/flutter/133884, due to - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
polina-c
added
the
autosubmit
flutter/flutter@da676f7...7c28e8e 2023-09-09 [email protected] Roll Flutter Engine from 4e3231af6efc to d1913cb6a276 (1 revision) (flutter/flutter#134355) 2023-09-09 [email protected] Roll Flutter Engine from 348e3a376807 to 4e3231af6efc (1 revision) (flutter/flutter#134353) 2023-09-09 [email protected] Day picker should dispose created MaterialStatesController's. (flutter/flutter#133884) 2023-09-09 [email protected] Roll Flutter Engine from 00ef109b845e to 348e3a376807 (1 revision) (flutter/flutter#134349) 2023-09-09 [email protected] Roll Flutter Engine from 7af8a5d8d556 to 00ef109b845e (1 revision) (flutter/flutter#134336) 2023-09-09 [email protected] Roll Flutter Engine from 1f2da3d69da7 to 7af8a5d8d556 (2 revisions) (flutter/flutter#134332) 2023-09-09 [email protected] Roll Flutter Engine from 3a5f3ad1d054 to 1f2da3d69da7 (1 revision) (flutter/flutter#134328) 2023-09-09 [email protected] Roll Flutter Engine from d6aa2d9061c1 to 3a5f3ad1d054 (3 revisions) (flutter/flutter#134327) 2023-09-09 [email protected] Roll Flutter Engine from 66bec85d5005 to d6aa2d9061c1 (1 revision) (flutter/flutter#134324) 2023-09-08 [email protected] Roll Flutter Engine from 8d2892211366 to 66bec85d5005 (3 revisions) (flutter/flutter#134321) 2023-09-08 [email protected] Roll Flutter Engine from 2da727e23518 to 8d2892211366 (1 revision) (flutter/flutter#134316) 2023-09-08 [email protected] Fix memory leak in _DraggableScrollableSheetState (flutter/flutter#134212) 2023-09-08 [email protected] RestorationManager should dispatch creation in constructor. (flutter/flutter#133911) 2023-09-08 [email protected] Remove TextPainter migration flag from the framework (flutter/flutter#134274) 2023-09-08 [email protected] InputDecoration.error should activate error state (flutter/flutter#134001) 2023-09-08 [email protected] Roll Flutter Engine from b2cb1d271a88 to 2da727e23518 (1 revision) (flutter/flutter#134314) 2023-09-08 [email protected] Update chip docs to clarify how to specify a shape with no border & explain default values for Material 3 (flutter/flutter#134298) 2023-09-08 [email protected] Add ios analyzer command for universal links (flutter/flutter#134155) 2023-09-08 [email protected] Roll Flutter Engine from 47a79306eed3 to b2cb1d271a88 (5 revisions) (flutter/flutter#134313) 2023-09-08 [email protected] Roll Flutter Engine from 6d6b44886175 to 47a79306eed3 (2 revisions) (flutter/flutter#134310) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Fixes #133862