Skip to content

Conversation

GeertJohan
Copy link

I was looking for wrong something.runtimeType is comparisons in our codebase and the search matched this line in the vendored flutter sdk.

I'm not 100% sure this MR results in correct behavior. But since .runtimeType always returns Type which can never match OverscrollNotification, the current code must be wrong?

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Oct 10, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@goderbauer
Copy link
Member

Using runtimeType here does indeed look like a bug. Thanks for finding it! I wonder why this has never caused any problems before (cc @Piinks)? Also, that this change is not failing any existing tests means we are lacking some test coverage here...

Ideally, we need a test case covering the bug that this is fixing. However, I am right now not sure how to write this one here... Maybe @Piinks has an idea?

@jonahwilliams
Copy link
Contributor

This should either be an == check on the types or an is/is! check on the type like you've changed it to. is check is probably better, unless the intention is that we don't match subtypes?

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hmm I recall copying a lot of the notification logic here out of the glowing overscroll indicator.
Some of it was cleaned up in #87839, (see https://github.com/flutter/flutter/pull/87839/files#r994796453) but it looks like some tidying was left out.

Glowing overscroll indicator still sets _lastNotification = notification.runtimeType in the glowing one instead of _lastNotification = notification like in the stretching , can you update that as well so this is more consistent across the two?

It should be possible to write a test that enters this if block currently when it shouldn't, and that test will confirm this fixes the issue.

@GeertJohan
Copy link
Author

GeertJohan commented Oct 21, 2022

I see a _lastNotificationType being set to a runtimeType, but that seems to be correct; it's explicitly declared with type Type?.

To be honest I don't have the time to dive into it the coming weeks. Maybe it's better if someone takes this change. I won't feel bad if this PR is closed in favor of a more complete fix.

@Piinks
Copy link
Contributor

Piinks commented Oct 21, 2022

No worries @GeertJohan, thanks for contributing and bringing this to our attention. I'll file an issue to follow up on this.

@Piinks
Copy link
Contributor

Piinks commented Oct 21, 2022

Filed #113840 for follow up. Once we can identify a case where this creates a bug, we can write test for the fix. :) Thanks @GeertJohan!

@Piinks Piinks closed this Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants