Can we keep this widget and just implement it in terms of SliverLayoutBuilder and SliverFixedExtendList? That would limit the scope of the API breakage and we'd only remove the RenderSliverFillViewport, I think?

Aye

why this change?

it looks like we changed the behavior when viewportFraction > 1
The page start at the very left edge of the page instead of after extra space.

Yeah it does change the behavior when viewportFraction > 1 and breaks the API (although I doubt it would really affect anyone, and starting from 0 makes more sense to me) . Context: #8539

What's the motivation for changing this?

Please add a description of the changed behavior to the PR description.

I think the original behavior make more sense to me. I would expect the viewport is center align with the page, that is why you have the padding when viewportfraction<1. If the same thing apply when viewportfraction>1, the original behavior is expected. and i am not quite sure how #8539 related to this behavior. Can you elaborate?

Ah I thought a weird viewportFraction like 1.3 would mess up the snapping behavior but that's not true. Thanks for pointing out. But SliverPaddings can't go negative. I'll find an alternative for it.

Can you just use constraints.axis?

Also, with enums instead of checking individual values use a switch/case. That way the analyzer tells us that we need to update this code if we ever add new enum values.

What's the motivation for changing this?

Please add a description of the changed behavior to the PR description.

nit: either place this at the end of the previous line (preferred) or indent this more.

Also below - I think that improves readability.

Declare this were the first value is assigned to it?

You could also just subtract a small offset, which may make the intend here clearer:

this feel dangerous to me. That is true when you change this property in didupdatewidget. If you are certain that is the case for the future as well, you should add an assert to verify that.

I am not sure if this is better but how about

if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks) {
     correctPixels(getPixelsFromPage(oldPage));
    } else {
      forcePixels(getPixelsFromPage(oldPage));
    }

This class itself is private, and the setter is only used in PageController.attach. I'm not sure if we should call notifyListeners in ScrollController.attach, I think it should be up to the caller?

I am more worry about we change the scroll offset but not markneedbuild, and yes this should be fine because this is private class and as of now it only called during the build stage. However, it is possible this might change. If you think that is unlikely, we should still add an assert here to guard it.

Yeah let me get rid of the setter altogether. The implementation should probably live in PageController.attach which has more context.

nit, extra line

let's revert this change

Why move this out of the setter for viewportFraction?

forcePixel (in this new version) and correctPixel also have slightly different semantics regarding notifications...

from ScrollPosition.forcePixel documentation:

/// This should not be called during layout (e.g. when setting the initial
/// scroll offset). Consider [correctPixels] if you find you need to adjust
/// the position during layout.

Also it doesn't sound right to notify the listeners in ScrollController.attach.

Why move it out of the setter though?

IMO since using either forcePixel or correctPixel needs to be decided on a case-by-case basis, it feels a bit error-prone to have a viewportFraction setter which sounds like a silver bullet for changing viewportFraction.
Also attach is the only caller of the setter right now, so I got rid of the setter and moved the implementation to attach.

this is build, not layout. should be fine to call during build.

It may be worthwhile to document what the "initial page offset" is.

Same nit as above here regarding formatting.

I double checked the indentation, line 607 does have 2 more spaces than 606, it's just ( takes much less space than a regular character.

according to documentation of correctPixels, this function should only be called during layout phase
can you add an assert something like
assert(SchedulerBinding.instance.schedulerPhase == SchedulerPhase.persistentCallbacks, 'attach can only be called during SchedulerPhase.persistentCallbacks')

If this assumption no longer held, would be better to update the documentation of correctPixels to include this case

should really only be called when debugDoingLayout is true for the viewport in question

This should document when it is valid to call this method (e.g. what the assert is asserting).

I think it has to be a good old function if the line is too long

Looking into this further, I'm not sure this deprecation makes sense. If someone is using RenderSliverFillViewport directly, then they're not using widgets, which means that using SliverLayoutBuilder isn't an option.

We should probably remove this render object if we're not supporting it any more (which I presume we're not, since we didn't try to fix the hit test bug in this render object).

cc @goderbauer @Piinks

we move the padding from rendering layer to widget layer. I can't think of a way to achieve the same behavior by using our current rendering library. I agree we should remove this given we have deprecated warning for a while.

Yes, this is no longer used and should just be removed: #44612