It's confusing that activeColor above also resolves to CupertinoColors.activeBlue. What parameters are prioritized when it comes to setting the color of the checkbox?

If any one is giving null, it resolves to bottom one

1. fillColor
2. activeColor
3. CupertinoColors.activeBlue

Yes, but that is not clear to someone reading the documentation. The documentation for fillColor and activeColor should both clearly specify this order of precedence.

I'd recommend appending this to activeColor's docs:

  /// If [fillColor] returns a non-null color in the [WidgetState.selected]
  /// state, it will be used instead of this color.

Then, since activeColor already defaults to CupertinoColors.activeBlue, fillColor only needs to default to activeColor in the selected state, so that there is a single source of truth for what the final default color is.

I have updated code to make it simple and in sync with docs. Please review and let me know if it makes more sense now.

It might be a little confusing to have an activeColor here when the expectation is activeColor should be widget.activeColor. I'd recommend consolidating this activeColor into effectiveActiveColor, so it's clear what all variables are referring to.

We are already using effectiveActiveColor, will it be bad if we call it resolvedActiveColor ?

Reason we are having activeColor and effectiveActiveColor is fillColor and _widgetFillColor might resolve to null.
So, either we can rename activeColor to resolvedActiveColor
or
Just keep effectiveActiveColor that resolves everything

Either is fine, but I would prefer the latter because I don't see a need for the extra variable/step, and I can also find that pattern elsewhere in the framework:

I was a little confused at first glance. Maybe this reduces ambiguity a bit.

I suggest the following way:

The differences are:

• It made it clearer on what is null.
• It removes the duplicate mention of the default values of [activeColor] and [inactiveColor].
• It is clearer on what is "selected/unselected state".

I thought the other comment mentioned that these lines are changed into using union and difference. But they're now using ..add and ..remove. Doesn't the current way modify states? For example, if the current state has WidgetState.selected, won't state become unselected after the build?

This is true. I wonder why this has not caused problems in Material Checkbox though, because it does seem bad.

Yeah, this is what I don't like about ToggleableStateMixin: it has a states getter that re-constructs a Set each time it's accessed.

So the above code works fine, but it gives the appearance that activeStates and inactiveStates are modifying the same object.

Ah I see. Yeah... the naming is bad here.

Maybe add a comment:

// The `states` getter constructs a new set every time, which is therefore safe to edit in place.

Yeah, although we should do it in a separate PR since it's potentially breaking.

    final Set<WidgetState> selectedState = <WidgetState>{WidgetState.selected};
    final Set<WidgetState> activeStates = states.union(selectedState);
    final Set<WidgetState> inactiveStates = states.difference(selectedState);

How does this looks like, clean and easy to understand.

That'd be good, but my guess is that .union() and .difference() introduce a small performance penalty when compared with .add() and .remove().

If we end up going this route, then const Set<WidgetState> selectedState would be ideal.

Yeah, I think the current way is ok as long as it's commented.

// The `states` getter constructs a new set every time, which is therefore safe to edit in place.

Okay, I have added this comment.

Shall we combine the two lines?

Also for the inactive color below.

The indentation here feels weird, does this follow the style guide?

I'm not sure if the style guide has an enforcing rule but usually we use 2 spaces or 4 spaces in this kind of cases.

True. All the indentation stipulations in the style guide seem to suggest two spaces.

Not really. For example, https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#prefer-a-maximum-line-length-of-80-characters provided an example of

final int c =
    a.very.very....

Although it's in the BAD example section, it still shows that if we want a new line, the new line can use 4 spaces. I don't think this is really something that we really enforce.

Also in https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#align-expressions there is an example that shows what happens if we want to indent at the middle of an expression:

// BAD:
return foo.x == x &&
    foo.y == y &&
    foo.z == z;