-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Change ClipboardStatusNofifier parameter in buildToolbar to ValueLise… #108917
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
Conversation
bd66959 to
ccf2d46
Compare
justinmc
left a comment
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.
LGTM but I left a few comments with questions so I understand the context a bit better.
| @override | ||
| void removeListener(VoidCallback listener) { | ||
| super.removeListener(listener); | ||
| if (!_disposed && !hasListeners) { |
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.
Why aren't these disposed checks needed anymore, was this fixed somewhere in the framework?
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.
yes #108931
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.
Thanks that's right 👍
| List<TextSelectionPoint> endpoints, | ||
| TextSelectionDelegate delegate, | ||
| ClipboardStatusNotifier? clipboardStatus, | ||
| ValueListenable<ClipboardStatus>? clipboardStatus, |
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.
Is the idea here just to simplify things since no methods from ClipboardStatusNotifier are needed here?
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.
Nevermind the answer is yes. I answered my own question by reading the original PR #98951.
…alueLise… (flutter#108917)" This reverts commit 98ce4bf.
…tnable
fixes #99360
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.