Day picker should dispose created MaterialStatesController's. #133884

polina-c · 2023-09-01T18:45:58Z

…PickerState-fix

chunhtai · 2023-09-06T18:57:27Z

packages/flutter/lib/src/material/calendar_date_picker.dart

            onTap: () => widget.onChanged(dayToBuild),
            radius: _dayPickerRowHeight / 2 + 4,
-            statesController: MaterialStatesController(states),
+            statesController: _materialStatesController,


This doesn't look right, every date widget should have a separate states controller. cc @HansMuller

That's right. each day widget should have its own selected/disabled/etc state.

It is interesting that tests passed...

Fixed. Looks better?

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.

Added test.

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.

…PickerState-fix

chunhtai · 2023-09-07T22:40:43Z

packages/flutter/lib/src/material/calendar_date_picker.dart

            onTap: () => widget.onChanged(dayToBuild),
            radius: _dayPickerRowHeight / 2 + 4,
-            statesController: MaterialStatesController(states),
+            statesController: statesController,


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.

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?

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

LGTM, but it is better to wait for @HansMuller's approval

chunhtai · 2023-09-08T18:58:55Z

packages/flutter/lib/src/material/calendar_date_picker.dart

            onTap: () => widget.onChanged(dayToBuild),
            radius: _dayPickerRowHeight / 2 + 4,
-            statesController: MaterialStatesController(states),
+            statesController: statesController,


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

HansMuller · 2023-09-08T19:38:17Z

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)).

…PickerState-fix

auto-submit · 2023-09-08T22:37:05Z

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.

…flutter/flutter#133884)

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

…flutter/flutter#133884)

polina-c and others added 6 commits September 1, 2023 07:51

Update date_picker_test.dart

faff29a

Update date_picker_test.dart

873176d

Merge branch '_DayPickerState' into _DayPickerState-fix

525c0cf

-

53ef827

Update date_picker_test.dart

6fbebdc

Merge branch 'master' into _DayPickerState-fix

072493b

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 1, 2023

polina-c mentioned this pull request Sep 1, 2023

_DayPickerState should not create MaterialStatesController on every build, without disposing #133862

Closed

polina-c added 2 commits September 1, 2023 12:40

Update date_picker_test.dart

53b1b86

Merge branch 'master' of https://github.com/flutter/flutter into _Day…

720369d

…PickerState-fix

polina-c requested review from chunhtai and goderbauer September 2, 2023 00:22

polina-c mentioned this pull request Sep 2, 2023

Clean up notDisposed leaks in Flutter Framework, phase 1. dart-lang/leak_tracker#134

Closed

polina-c and others added 2 commits September 5, 2023 09:27

Update date_picker_test.dart

a268ffe

Merge branch 'master' into _DayPickerState-fix

f959310

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2023

chunhtai reviewed Sep 6, 2023

View reviewed changes

polina-c changed the title ~~Day picker should not create MaterialStatesController on every build~~ Day picker should dispose created MaterialStatesController's. Sep 6, 2023

Merge branch 'master' into _DayPickerState-fix

8b6331f

polina-c removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2023

polina-c added 2 commits September 7, 2023 15:30

Update calendar_date_picker.dart

40783c1

Merge branch 'master' of https://github.com/flutter/flutter into _Day…

3118eaf

…PickerState-fix

polina-c requested review from HansMuller and removed request for goderbauer September 7, 2023 22:31

chunhtai reviewed Sep 7, 2023

View reviewed changes

polina-c requested a review from chunhtai September 7, 2023 22:47

polina-c added 3 commits September 7, 2023 15:59

Merge branch 'master' into _DayPickerState-fix

46ed5d3

Merge branch 'master' into _DayPickerState-fix

f6d6dec

Merge branch 'master' into _DayPickerState-fix

c400567

polina-c added 2 commits September 7, 2023 17:51

Merge branch 'master' into _DayPickerState-fix

e46630b

Merge branch 'master' into _DayPickerState-fix

92b5883

chunhtai approved these changes Sep 8, 2023

View reviewed changes

polina-c changed the title ~~Day picker should dispose created MaterialStatesController's.~~ Day picker should dispose created MaterialStatesController's (abandoned). Sep 8, 2023

polina-c changed the title ~~Day picker should dispose created MaterialStatesController's (abandoned).~~ Day picker should dispose created MaterialStatesController's. Sep 8, 2023

polina-c added 5 commits September 8, 2023 15:22

Update calendar_date_picker_test.dart

6d150fb

Update date_picker_test.dart

24396d1

Update date_picker_test.dart

6f627f0

Update calendar_date_picker.dart

ffc0fb0

Merge branch 'master' of https://github.com/flutter/flutter into _Day…

0efcb88

…PickerState-fix

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023

polina-c mentioned this pull request Sep 8, 2023

_DayPicker should build days using separate stetefull widget _Day. #134323

Closed

polina-c added 2 commits September 8, 2023 15:55

-

44607e9

Update calendar_date_picker.dart

f0bd43f

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023

polina-c merged commit 5555836 into flutter:master Sep 9, 2023

engine-flutter-autoroll mentioned this pull request Sep 9, 2023

Roll Flutter from da676f79aa37 to 55558367127f (18 revisions) flutter/packages#4877

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023

5555836 Day picker should dispose created MaterialStatesController's. (…

d4cfa61

…flutter/flutter#133884)

engine-flutter-autoroll mentioned this pull request Sep 9, 2023

Roll Flutter from da676f79aa37 to 6660cc981669 (19 revisions) flutter/packages#4878

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023

5555836 Day picker should dispose created MaterialStatesController's. (…

b5857d8

…flutter/flutter#133884)

engine-flutter-autoroll mentioned this pull request Sep 9, 2023

Roll Flutter from da676f79aa37 to 7c28e8eb0734 (20 revisions) flutter/packages#4879

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023

5555836 Day picker should dispose created MaterialStatesController's. (…

bd123cd

…flutter/flutter#133884)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

5555836 Day picker should dispose created MaterialStatesController's. (…

2b36806

…flutter/flutter#133884)

Day picker should dispose created MaterialStatesController's. #133884

Day picker should dispose created MaterialStatesController's. #133884

Uh oh!

Conversation

polina-c commented Sep 1, 2023

Uh oh!

chunhtai Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

HansMuller Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

HansMuller Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Sep 8, 2023

Uh oh!

auto-submit bot commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HansMuller Sep 7, 2023 •

edited

Loading

polina-c Sep 8, 2023 •

edited

Loading

polina-c Sep 8, 2023 •

edited

Loading

polina-c Sep 7, 2023 •

edited

Loading