fix(app): Propogate color

[epage]

In #2851, we moved color from an AppSetting to function (with some
tweaks in #2907). When doing this, we documented App::color to be
equivalent of App::global_settings(Color...) but never actually
propagated it.

We are now propagating it. A test is added to ensure that no matter
how we store the color choice, we continue to propagate it. This
required exposing App::get_color.

[pksunkara]

While I agree with the need of the PR, I don't think this is the right approach. This is probably caused because we use the old settings. The right approach would actually be to propagate the stored color choice during building of the app like how we propagate other data too.

Because of trying to provide backward compatibility for these old settings:

• We are not string the color choice in the correct place.
• We are not using the correct approach for propagating this data which increases future risk.

[epage]

While I agree with the need of the PR, I don't think this is the right approach. This is probably caused because we use the old settings. The right approach would actually be to propagate the stored color choice during building of the app like how we propagate other data too.

Because of trying to provide backward compatibility for these old settings:

* We are not string the color choice in the correct place.

* We are not using the correct approach for propagating this data which increases future risk.

This is not caused by us using the "old" settings, propagating support was never implemented when the conversion first happened.

As long as we are still supporting the old settings API (which this PR is not the place to re-litigate that), we still need to use the settings for storage. This is the way to propagate for our currently selected storage backend for color.

Note that I put "old" in quotes for the settings because, independent of what the API expose, using the bit flags lets us take advantage of the packing which is the reason clap switched to bit flags.

The test will help ensure that if we do decide to change the storage, we update the propagation, as needed.

[pksunkara]

using the bit flags lets us take advantage of the packing which is the reason clap switched to bit flags.

I am not against that. What I am saying is this is just not the right approach for doing that. To decrease future risk, we need to have the correct approach here and then have the Color Settings added on top of that to provide backward compatibility. But what you are doing here is making the Color Settings the approach itself. This is how the correct approach I am referring to look like:

1. Have a new bitflags for just the color choices
2. Use those bitflags to store the color choice in the app. (Not using the settings bitflags)
3. Propagate those bitflags during building of the app.
4. Support backward compatibility for color settings by checking them during app building and then manipulating the stored bitflags accordingly.

This way, when we remove the deprecated settings, we just remove the last part instead of rewriting the whole thing which is much more risky.

[epage]

But what you are doing here is making the Color Settings the approach itself.

There is nothing fundamental about the approach being taken that cannot be changed in the future when we remove the deprecated settings.

I am not against that. What I am saying is this is just not the right approach for doing that. To decrease future risk, we need to have the correct approach here and then have the Color Settings added on top of that to provide backward compatibility. But what you are doing here is making the Color Settings the approach itself. This is how the correct approach I am referring to look like:

1. Have a new bitflags for just the color choices

2. Use those bitflags to store the color choice in the app. (Not using the settings bitflags)

3. Propagate those bitflags during building of the app.

4. Support backward compatibility for color settings by checking them during app building and then manipulating the stored bitflags accordingly.

This way, when we remove the deprecated settings, we just remove the last part instead of rewriting the whole thing which is much more risky.

Assuming the reverting of the deprecated color settings is out of scope for this PR, this would require the App to check color settings in two different places and then propagate that. We'd need to decide how those two different settings interact with each other. Merging from two different sources correctly adds risk.

The proposal also does not do memory packing because a bit field of just the color enum will use the same memory as the color enum. It has to be packed with other data to get benefits.

This also requires rewriting everything now, when we should be wrapping up the release, increasing the risk.

Overall, I think staying with this approach now and later migrating wholesale if we have a need to, is the lowest risk approach. Trying to get two different approaches to live by side and to do so now in the release is a higher risk decision.

[pksunkara]

bors r+

I would appreciate if there is an issue tracking the removal deprecated colored settings that points to the discussion here in that case.

[bors]

Build succeeded:

• CI

[epage]

I would appreciate if there is an issue tracking the removal deprecated colored settings that points to the discussion here in that case.

I created #3021 but I'm unsure what is actionable from this. If we intend to keep bit packing, there isn't anything we can really change besides dropping the user-facing parts. If we drop bit packing, then that does open us up for other aspects.