Fix RangeMaintainingScrollPhysics #65135
Conversation
flutter-dashboard
bot
added
the
framework
|
cc @goderbauer Would you mind reviewing this? It's the same patch as before plus an extra commit to fix review comments from the last PR. |
|
So did you figure out what causes the internal test failed at the first land? |
|
@xu-baolin We're pretty sure those failures were because tests were assuming the incorrect behavior that has been in the tree for a while (they were all new tests). |
There was a problem hiding this comment.
Ah yes, test names. One of the other features I'd just remove entirely if I were to create a new test framework. :-)
Fixed
There was a problem hiding this comment.
I seriously considered leaving the original or having it say "funny and fully" or something but...
It now uses the scroll metrics as they stood at the end of the last frame. It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.
|
This pull request is not suitable for automatic merging in its current state.
|
|
Hi all, hi @Hixie, You can find a stackTrace / bug here: The issue is in applyContentDimensions within scroll_position.dart. If I revert the function to the commit before this PR, the issue disappears. Is there any chance to revert this change? |
@Rodiii I think we don't revert a commit for a package, but you can try to fix it by making a PR to the package. |
|
@AlexV525 Thanks for answering. Sorry, there has been a misunderstanding. I was not talking about the whole commit, I just meant the affected function. Sure, I could just hand in a PR which reverts the function to before this commit. Anyhow, I guess this would brake the feature(s)/bugfixes @Hixie introduced with his PRs. Anyhow, if I don't hear anything until tonight, I'll hand in a PR, as this issue just affects too many people. |
Because those commits you saw were squashed and merged as a single commit, there are no handy way to partially revert a commit inside them.
I suggest you land the PR in the package repo, because it did fix something but might be a breaking change for some packages. That require packages update too. |
|
Yeah, the right way to fix this is to fix the package. But in the future, the best thing is to have the package's tests submitted to the flutter/tests repository (https://github.com/flutter/tests), since we guarantee that we won't break tests submitted to that repo without first working with the package or application owner to migrate their code. |
|
Hi all, Thanks for your responses. There are good news, the maintainer of pull_to_refresh just updated the library to support the most recent flutter revision. So there's no issue here anymore :-). |
8 participants
It now uses the scroll metrics as they stood at the end of the last frame.
It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.
Fixes #60288 again.