Skip to content

Conversation

@polina-c
Copy link
Contributor

@polina-c polina-c commented Sep 3, 2023

No description provided.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 3, 2023
@polina-c polina-c marked this pull request as ready for review September 4, 2023 01:06
@polina-c polina-c assigned chunhtai and unassigned chunhtai Sep 4, 2023
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, fyi @HansMuller

@HansMuller
Copy link
Contributor

Shouldn't we be trying to dispose controllers for segments that no longer exist?

@polina-c
Copy link
Contributor Author

polina-c commented Sep 5, 2023

Shouldn't we be trying to dispose controllers for segments that no longer exist?

Yes, this may great next improvement for the code. I am not familiar with code enough to implement it. Can you create it?

@polina-c polina-c merged commit cb0a613 into flutter:master Sep 5, 2023
@polina-c polina-c deleted the SegmentedButton branch September 5, 2023 18:00
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
@HansMuller
Copy link
Contributor

Unfortunately this PR prevents a source of unnecessary allocations but creates a leak (#133949 (comment)). I suppose what needs to be done is to diff the old and new lists of segments in a https://api.flutter.dev/flutter/widgets/State/didUpdateWidget.html override and dispose the discarded segments.

@polina-c
Copy link
Contributor Author

polina-c commented Sep 5, 2023

Unfortunately this PR prevents a source of unnecessary allocations but creates a leak (#133949 (comment)). I suppose what needs to be done is to diff the old and new lists of segments in a https://api.flutter.dev/flutter/widgets/State/didUpdateWidget.html override and dispose the discarded segments.

Thanks. I will prepare update: #134061

@polina-c
Copy link
Contributor Author

polina-c commented Sep 5, 2023

Fix: #134064

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants