Skip to content
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

Allow changing a clipper's type (ShapeBorderClipper,_BottomAppBarClipper). #14937

Merged
merged 2 commits into from
Feb 28, 2018

Conversation

amirh
Copy link
Contributor

@amirh amirh commented Feb 28, 2018

The downcast was crashing when toggling hasNotch for BottomAppBar.

…per).

The downcast was crashing when toggling hasNotch for BottomAppBar.
@@ -86,6 +86,37 @@ void main() {
expect(physicalShape.color, const Color(0xff0000ff));
});

testWidgets('toggle hasNotch', (WidgetTester tester) async {
await tester.pumpWidget(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining why we're toggling hasNotch...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@HansMuller
Copy link
Contributor

LGTM

@amirh amirh merged commit 6993b20 into flutter:master Feb 28, 2018
@amirh amirh deleted the clipper-crash branch February 28, 2018 01:39
bool shouldReclip(covariant _BottomAppBarClipper oldClipper) {
return oldClipper.geometry != geometry;
bool shouldReclip(CustomClipper<Path> oldClipper) {
if (oldClipper.runtimeType != _BottomAppBarClipper)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is impossible. oldClipped is guaranteed to have the same runtimeType as this
(unless there's a bug in the code that calls shouldReclip, which is possible)

@Hixie
Copy link
Contributor

Hixie commented Mar 8, 2018

I think this fix is incorrect. The shouldRefoo(oldThing) pattern is supposed to guarantee that oldThing is the same type, otherwise it's guaranteed that you should refoo.

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
…per). (flutter#14937)

The downcast was crashing when toggling hasNotch for BottomAppBar.
@Hixie
Copy link
Contributor

Hixie commented Jun 6, 2018

cc @amirh see comment above

amirh pushed a commit to amirh/flutter that referenced this pull request Jun 6, 2018
A bug in _RenderCustomClip was compaeing the type of oldClipper to
itself instead to the type of newClipper.

This was the root cause for the crash flutter#14937 worked around.
This also reverts the workaround introduced in flutter#14937.
@amirh
Copy link
Contributor Author

amirh commented Jun 6, 2018

Looks like there was indeed a bug in _RenderCustomClip, sent a fix in #18248.

amirh added a commit that referenced this pull request Jun 6, 2018
A bug in _RenderCustomClip was compaeing the type of oldClipper to
itself instead to the type of newClipper.

This was the root cause for the crash #14937 worked around.
This also reverts the workaround introduced in #14937.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants