-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make sure that a CheckboxListTile doesn't crash in 0x0 environment #173151
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
Make sure that a CheckboxListTile doesn't crash in 0x0 environment #173151
Conversation
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.
Code Review
This PR adds a regression test for a crash in CheckboxListTile when rendered in a zero-sized area. The test is a good addition, but the PR seems to be missing the actual code fix for the crash it's testing against. Merging this as-is could introduce a failing test. I've suggested a more descriptive name for the test to improve clarity, in line with our style guide's emphasis on readability. The test's widget tree can be simplified by removing a redundant Center widget, following the 'write what you need and no more' principle. I've referenced the Flutter Style Guide where applicable in my comments.
dkwingsmt
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
MitchellGoodwin
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 as well!
dkwingsmt
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.
Some changes per the latest discussion
| expect(secondaryOffset.dy - tileOffset.dy, bottomPositionSecondary); | ||
| }); | ||
|
|
||
| testWidgets('CheckboxListTile renders at zero area', (WidgetTester tester) async { |
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.
| testWidgets('CheckboxListTile renders at zero area', (WidgetTester tester) async { | |
| testWidgets('CheckboxListTile does not crash at zero area', (WidgetTester tester) async { |
| final Finder xText = find.text('X'); | ||
| expect(tester.getSize(xText).isEmpty, isTrue); |
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.
| final Finder xText = find.text('X'); | |
| expect(tester.getSize(xText).isEmpty, isTrue); | |
| expect(tester.getSize(find.byType(CheckboxListTile)), Size.zero); |
8f7dac0 to
5635d14
Compare
dkwingsmt
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.
Checklist:
- The test is in the correct file
- The test name goes “does not crash at zero area”
- The target widget is wrapped by
Center(or is fullscreen) - The target widget does not have an overlay, or the overlay is tested
- The target widget is expected to have a size of exactly
Size.zero
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Mac_x64 tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
82e7400 to
7d83d25
Compare
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
b0400a6 to
0481794
Compare
0481794 to
5859547
Compare
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/173151, because - The status or check suite Linux web_long_running_tests_3_5 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…lutter#173151) This is my attempt to handle flutter#6537 for the CheckboxListTile UI control. Co-authored-by: Tong Mu <[email protected]>
This is my attempt to handle #6537 for the CheckboxListTile UI control.