Why has this test changed?

The old test used to check for the presence of an icon (the down-arrow) and make sure that the colour of the icon was correct. The change introduced in this PR (which matches iOS implementation) doesn't have an icon any more, so now the test makes sure that the in-progress calculations are correct.

Why the name change? I think we avoid using 'Apple' explicitly in the framework.

The old name was buildSimpleRefreshIndicator which, to me, was accurate in that it was a simple implementation that didn't deal with the complexities of how iOS renders its indicator. Having the full implementation labelled "simple" didn't feel quite right. Your point is taken about referencing Apple, though, so I've renamed it to simply buildRefreshIndicator

Nit: We usually include the .0 for doubles, just to clarify that it is a double.

Fixed. Also updated the radius variable above.

Needs a period at the end of the sentence. If you want you could probably even simplify this to:

// Rotate so that the top tick is the first to be drawn.

Fixed.

Should use the American spelling "behavior" per the style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#spell-words-in-identifiers-and-comments-correctly

Fixed.

"animated" => "animates"

Fixed.

Nit: Would it help to include references to the states like [RefreshIndicatorMode.armed] here?

I had a couple of goes at trying this, but each attempt resulted in an unwieldy mouthful.

At the moment, the function description is more like an overview, rather than an explanation of the internal mechanics. As a consumer of this function, I'm not sure if I would really need the gory details, which seem reasonably clear in the switch statement further down, but I'm happy to try further if you feel it is warranted.

No worries, I'm happy with leaving it as-is. Thanks for taking a look.

Nit: I've been called out on my PRs before for using static methods that return widgets and told to just make a new StatelessWidget instead. In this case I realize that buildSimpleRefreshIndicator was already guilty of this though :) Just something to keep in mind if you see a better way to do this, otherwise it's fine as-is.

Period at the end of the sentence here and two more comments below.

Fixed.

"let's" => "lets"

Fixed.

Should this still be tested in dark mode one way or another?

Excellent question. We don't do anything specific depending on what mode we're in... not sure if the standard practice is to always do a light and dark mode test, or only if the widget actually does something different. Do you know if there's any specific guidance one way or another?

No, it's not necessary if the change doesn't have any special dark mode behavior. But why was this test replaced? Could you keep the original test and add your new one, or does the original test conflict with the new behavior?

Ah I see your response to @Piinks above asking about the same thing. This is fine as-is since the dark mode specific behavior was removed. 👍

nit: it would make the paint logic slightly easier to understand if tickFundamentalRRect starts off from a vertical position, instead of making the adjustment later using canvas.rotate(math.pi / 2);

Fixed.

It seems when progress is under 1, setting animating to true or false makes no difference visually? Maybe we should assert(!animating) if progress < 1 , since progress basically takes control from the state's animation controller and it doesn't really make too much sense to have the animation running when progress is manually set?

+1. This is implicitly controlling whether it's static or animating. It should definitely be pointed out in the docs. Perhaps allow null and make it mutually exclusive with animating? Since it's a bit harder to explain that 1 specific value of this double has a secondary meaning.

I also strongly encourage consistency with https://api.flutter.dev/flutter/material/ProgressIndicator/value.html

I wonder if it is worth creating an explicit constructor for the "progressive release" version? That might help with trying to keep the mutually exclusive values separate... not sure if there's any specific guidelines around when we should/shouldn't create a separate constructor?

I think that's a great idea! Many widgets have more than one constructor so it doesn't look like a problem to me.

Have added a new constructor called CupertinoActivityIndicator.partiallyRevealed() but am open to suggestions about better naming (I'm not super-thrilled about what I have now). Also updated some of the internal comments to be more accurate.

Maybe the percentage of spinner segments? The value ranges from 0 to 1. Also I think most people would expect the segments to distribute evenly when progress < 1, or even mistake this parameter for something that can be used to configure the total number of segments.

Fixed.

shouldRepaint should return true when progress changes. Not sure why it's repainting, maybe it's because the indicator is inside a LayoutBuilder.

Fixed

If you change the rotate direction here, doesn't it counteract the iteration direction change?

There's three things all going on here at once: 1) looping through rendering each tick when spinning, 2) pulling out an opacity for each tick, and 3) incrementally revealing each tick when progress < 1.

The first two don't really matter what sequence you do them in (or rotate) as long as you get the indexes correct for the colours (this is because in the finish, all 12 of them need to be drawn). However, the last case /must/ reveal the ticks in specific order (clockwise).

Thank for cleaning this up! Now the _alphaValues array is a lot easier to reason about.

nit: Should this be
final int t = (i - activeTick) % _kTickCount;
so the segment sitting at i == activeTick becomes the most visible one, when progress == 1 (But watch out this changes the direction of the opacity gradient too)?

I've adjusted most of the maths to be simpler (well, as simple as it can be)

Maybe mention this color change in the API documentation.

I've extracted that out into a private constant to be clearer

nit: native UIRefreshIndicators dismiss with a 0.3s shrink + fadeout animation (independent of percentageComplete)

optional: but this function would look cleaner if we just drew from the top first (adjust the tickFundamentalRRect if needed) instead of having to rotate twice in the paint.

Fixed

Note that I had to update a testcase to reflect this change because the testcase assumes that the 9 o'clock tick is the one that gets drawn first. I added a comment in the testcase for clarity.

I don't want to answer for @xster, but I would say it's definitely valuable to document that. "These values were obtained by stepping through a screen recording of <native widget> on <device> running <iOS version>."

I have lots of comments floating around stating that I just loosely eyeballed some value. It's valuable because if someone comes along with a value they got with a more precise method, they can be confident that their value is better than mine and they should update it.

With

const List<int> _alphaValues = <int>[47, 47, 47, 47, 64, 81, 97, 114, 131, 147, 47, 47];

the starting position of the activity indicator remains the same (as the current behavior on master, the most visible segment appears at 9 o'clock initially). Since the native activity indicator also starts at 9 o'clock, I think we should keep it as is to avoid introducing breaking changes.

I've modified the order of the values in the list so that the most prominent tick starts at 9 o'clock, and have added some comments explaining the meaning of the values.

Regarding the "breaking" test case... there's a bit of subtlety here. This test case is designed to confirm that the rounded rectangle has been generated correctly and it happens to do so by inspecting the /first/ rrect that is painted.

My implementation draws the first rrect at 12 o'clock, whereas the old implementation used to be at 9 o'clock. However, both implementations still draw all 12 ticks... the only thing that has changed is the sequence that they get drawn in.

There are no user-facing breakages at all. This test case just happens to be very much a white-box test, and we've now changed the internal implementation details.

Hope that makes sense.

Ah I see. I don't think having to change this specific test makes the PR a breaking change, it was meant to ensure the corner radius scales with the activity indicator.

What I meant by "breaking change" is the angle of the most prominent segment when animating is false. It used to be 9 o'clock and now its 11 o'clock. If we shift the alpha array to [47, 47, 47, 47, 64, 81, 97, 114, 131, 147, 47, 47] then it can go back to 9 o'clock. I doubt it will break anyone for real but it would be best if the appearance can remain exactly the same imo.

👍 I think I shuffled those entries in an earlier commit to reflect the original behaviour. All good.

Is the end value (0.2) correct? From what I'm seeing in Xcode the opacity seems to go up to 1.0 based on the drag distance.

This moves the center of the spinner to the top center from the center of the canvas, if the canvas has a big enough size. Is it because the activity indicator needs to be pinned to the top when it's in done state?

Yes, precisely. Once the drag is fully complete, size.height / 2.0 should be the same as radius, however where we were coming unstuck was that during the initial stages of the drag, size.height is less than the diameter of the spinner and changes as the user drags. This has the undesirable effect of moving the spinner as the user drags down.

By using radius, we ensure that the translation is constant and the spinner doesn't move. I've added some comments to this effect.

Activity indicator is a public widget so we probably shouldn't change its behavior just to suit our needs. I think we might want to change the SliverGeometry the _RenderCupertinoSliverRefresh reports to its viewport (or we can add it to the list of things we want to improve later).

Ah, of course ... that makes total sense. I've reverted that change and pushed a change that manipulates the sliver geometry.

While I was in there, I realised that the standard iOS pull-to-refresh interaction has a super-subtle effect where the first few pixels of the top tick get drawn out of the clipping bounds over the top of the content being dragged down. By adding the margin to the paintOrigin I achieve the same effect... however, it has the knock-on effect of impacting a couple of testcases.

At the time those test cases were written, the widget bounds that it was checking were correct, but I've had to adjust them slightly to accommodate these sliver geometry changes.

Let me know if I'm on the wrong path, or if there's a better way.

Thank for cleaning this up! Now the _alphaValues array is a lot easier to reason about.

With

const List<int> _alphaValues = <int>[47, 47, 47, 47, 64, 81, 97, 114, 131, 147, 47, 47];

the starting position of the activity indicator remains the same (as the current behavior on master, the most visible segment appears at 9 o'clock initially). Since the native activity indicator also starts at 9 o'clock, I think we should keep it as is to avoid introducing breaking changes.

Sorry for the delay. Thank you for putting detailed comments in the code!

Is it possible to add a padding to the activity indicator, instead of applying the paintOrigin change, and achieve the same thing? My concern is it might hurt the customizability of this widget, if the sliver has a hard-coded top margin.