Add StrokeCap to CircularProgressIndicator#122664
Add StrokeCap to CircularProgressIndicator#122664auto-submit[bot] merged 15 commits intoflutter:masterfrom
StrokeCap to CircularProgressIndicator#122664Conversation
001a705 to
1327e51
Compare
ProgressIndicator to be round
0db3818 to
5f80ae7
Compare
|
Yes, I pushed and went to sleep. I only saw his comment after publishing, but he is welcome here. Could you give your feedback on the boolean and Linear indicator ideas? |
|
I did rebase my changes but since it'd be identical to this, I think I'll refrain from opening a duplicate PR. As for the discussions about boolean or StrokeCap, I think exposing it as a boolean makes sense, and then setting it to the correct strokecap inside it The original issue #71085 which got you to reopen #109137, mentions customizing the radius. From what I know the Thanks for taking my input @Piinks |
844f5f2 to
d2224ea
Compare
|
I just changed |
|
Since the end goal is to potentially add a border radius to both widgets, I think it would make a bit more sense if |
|
I don't think adding borderRadius to CircularProgress would ever be possible, but we can decide what to do with the Linear one because it is quite different. I'm just afraid of polluting the theme too much. |
There was a problem hiding this comment.
Since StrokeCap.round is only relevant to CircularProgressIndicator (If the Linear one chooses a radius instead), should this only be on the CircularProgressIndicator class?
There was a problem hiding this comment.
I am also not sure if the name is clear as a user, why not use StrokeCap directly?
There was a problem hiding this comment.
StrokeCap has 3 values and only 2 of them are relevant. Butt and Square collide with each other, because Circular uses Butt when it has value, Square when it doesn't (it is spinning).
There was a problem hiding this comment.
should this only be on the CircularProgressIndicator
Probably. How do you think the theme should be? CircularHasRoundIndicator and LinearIndicatorRadius?
There was a problem hiding this comment.
Renamed to preferRoundIndicator.
There was a problem hiding this comment.
only 2 of them are relevant
I disagree with this statement, why are there 3 then? :) It's not for us to decide that one is irrelevant, the uses cases have been pointed out in the discussion of this PR.
How do you think the theme should be? CircularHasRoundIndicator and LinearIndicatorRadius?
Are there not individual component themes for circular and linear? I would not put both in the super class ProgressIndicatorTheme.
There was a problem hiding this comment.
I disagree with this statement, why are there 3 then? :) It's not for us to decide that one is irrelevant, the uses cases have been pointed out in the discussion of this PR.
If it is square, the value is represented wrong: #109137 (comment).
If it is butt, it is represented correctly.
But if it is butt, on indeterminate, it "disappears", so whoever implemented it made it square so it is always visible.
Exposing this to the user is hard: on square they will have the wrong value, on butt they will have an "unintended" animation (which, if it is to be customizable, we could allow changing other things, such as speed).
Are there not individual component themes for circular and linear?
No, I could make one, I guess..
There was a problem hiding this comment.
No, I could make one, I guess..
Oh, I meant that if they don't exist then we should probably omit theming support for now. :) No worries, no need to create more themes right now for this change.
|
How do we call the The only "unusual" thing I made is that the track has Radius.zero in the start position (when it it is determined). This prevents the left side from being round, which doesn't make sense. Everything is working fine. |
95189e2 to
2822c0b
Compare
There was a problem hiding this comment.
| /// By default it is null, which produces a rectangular track. | |
| /// | |
| /// By default it is null, which produces a rectangular track. |
This is a non-nullable parameter. How does it default to null? :)
There was a problem hiding this comment.
Actually, I was playing with a few things and forgot to update the documentation. I was checking if I could "auto add a borderRadius" but my idea was bad and it is better to allow the user to break things. IndicatorBorderRadius = 100 + background = 0 result in a weird scenario I was trying to avoid, but I'm letting the user decide.
There was a problem hiding this comment.
square or round
The name/boolean/square-round is kind of confusing here.
I think StrokeCap is still just the straightforward solution.
There was a problem hiding this comment.
The issue with strokeCap, as I tried to explain before, is that butt/square are always wrong. If you want butt, indeterminate is wrong. If you want square, value is wrong.
There is no reason for square + value. That must be overridden.
There is no reason for butt + indeterminate. That must be overridden.
Would we have a fourth value (null?) that goes between butt and square? I think that's too much complexity for the user. Even introducing a new enum could be easier to understand.
enum CircularProgressIndicatorShape { square, round }
There was a problem hiding this comment.
Is StrokeCap not properly documented? Again, these are decisions I do not think are fair for us to make for the developer. I think this should be StrokeCap.
There was a problem hiding this comment.
How do you see the API working? Because either value (square/butt) would be breaking. The strokeCap is documented, but there is a hole from "square paints a bit more" to "it is going to paint the wrong progress value", which is what people won't want. It is hard to even find a default value, because the default depends on the state. Right now it mixes the two.
| indeterminate | value |
butt | ❌ | ✅ |
square | ✅ | ❌ |
round | ✅ | ✅ |
There was a problem hiding this comment.
How would it be breaking? Does exposing this value to developers break the default behavior?
There was a problem hiding this comment.
Oh, I totally agree. The weird thing for me is that there would be 4 values and only 2 would be really "correct". So even though we want to give users options they will shoot themselves with 2 of them (which seem the correct thing, you have circle, you want... square, bang). But considering one day they might add other strokeCap effects, it makes sense.
There was a problem hiding this comment.
I think where we are disagreeing here is that I do not feel we can be the arbiters of "correct" as you have been asserting. It could very well be the behavior the developer wants.
There was a problem hiding this comment.
If the progress value starts on -10 instead of 0, we have a "deterministic" problem. No one would want it to start before, and if they want, we could just expose that number. Same thing, if they want value: 40, they don't want value: 50 (else we would expose like an offset and let they play with it).
For indeterminate, it is less bad because the material animation would change (most of them wouldn't notice), so I believe, if we want the material animation to be customizable, there are other ways of doing this (like allowing to change the speed, curves and delay).
I had a hard time, for example, in the LinearProgressIndicator to see if my borderRadius was working with indeterminate because it is too fast, what if I wanted slower? I can't set this. This could be more useful than say, a value that starts before and ends after.
There was a problem hiding this comment.
Just my 2cts, I think it is fair to assume that a determinate CircularIndicator is "correct" if it starts right at 90 degrees and that it draws half of a circle when its value is 0.5. Both are not true when using StrokeCap.square (not to say it is not 'circular' because ‘StrokeCap.square’ really means there are two squares drawn at both ends of the arc, which might makes sense if you want to ‘connect’ lines and arcs).
Nonetheless, @Pinks point about the versatility of the public API is valid, especially considering @bernaferrari following argument: “one day they might add other strokeCap effects, it makes sense.” (maybe Impeller will).
There was a problem hiding this comment.
I doubt so because CSS is 20 years old and only supports the same 3 strokeCap. But I agree they could. I just don't know what it could be that would add benefit to the framework.
e88e684 to
e61a241
Compare
1d1edae to
7f86d40
Compare
|
auto label is removed for flutter/flutter, pr: 122664, due to This PR has not met approval requirements for merging. You have project association CONTRIBUTOR and need 1 more review(s) in order to merge this PR.
|
|
auto label is removed for flutter/flutter, pr: 122664, due to Validations Fail. |
|
Thanks a lot! 🎉 |
|
Ah! I thought you were a member of flutter hackers! I will sponsor you, then your PRs will not need a second review. |
|
Now I have super-powers. It was a fun day. Thanks a lot Piinks! |
|
Amazing, thank you @Piinks and @bernaferrari very much! Kate, can you shed some light on why this PR was accepted and the previous ones were rejected? |
|
The previous one didn't pass flutter tests. I used it as the base for this. |
Add `StrokeCap` to `CircularProgressIndicator`
Split from #122664 so it gets easier to review, as this is now unrelated to the `preferRound`. I'm one step from adding a width attribute, lol. <img width="474" alt="image" src="proxy.php?url=https://user-images.githubusercontent.com/351125/226083798-71e529e9-4ae9-41de-a500-0412b989a273.png"> cc @Piinks
|
merge, when will be this in stable ? |
|
Probably in 2 months |
|
May I ask which version this PR was released in? |
|
@fingerart It's out in 3.13 |


Sequel to #109111
Fix #109137
I can makeLinearProgressIndicatoraccept BorderRadius (instead of round/square), but I can't (and no one can) makeCircularProgressIndicatorwork with that. You would need someone from Dart/Skia team that is good at math to make drawArc more flexible.So I used strokeCap everywhere, even though borderRadius (LinearProgressIndicator) + strokeCap (CircularProgressIndicator) would be better IMO. In a future PR I think I could add borderRadius to the "background" of the LinearProgressIndicator, so a borderRadius to the foreground could make a lot of sense. But I think people would be happy already with the round option. What do you think? Option 1 or 2?Disclaimer: I don't think StrokeCap works that well for what we are doing. It has 3 options, butt, square and round, but 99% of the time you want two of them, and you never know which two you want.
CircularProgressIndicatoruses square for indeterminate (paints a bit more, so it is always visible, even on 0), butt for determinate (paints exactly).CircularProgressIndicatordoesn't use it.Something like
isIndicatorRoundwould be better, since all we want is true/false. Do you agree?or
cc @Piinks