Skip to content

Conversation

@SimonSiefke
Copy link
Contributor

@SimonSiefke SimonSiefke commented May 26, 2024

fixes #128062, fixes #170576.

unset-color.mp4
@SimonSiefke SimonSiefke marked this pull request as ready for review May 26, 2024 20:00
@aeschli
Copy link
Contributor

aeschli commented May 27, 2024

The PR would allow to set colors to null that could not be null before.
Currently a color can only be null if there's no default defined for it and nother the current theme sets not the color customizations define a color.
Other colors can, in practice, not be null, as the color defaults define a value
Themes currently can not set colors to null, and of course also not color customizations.

Although in the code we check for undefined, but we have never tested how the UI would look like if certain CSS rules are not present.

So in essence, this PR opens a fleabag of new configuration possibilities we have not tested.

Therefore I think we should not go in the suggested direction. Instead we should look at case-by-case where we want to allow that colors can be unset.

Note: it's already possible to set a color to transparent.

@aeschli aeschli closed this May 27, 2024
@SimonSiefke
Copy link
Contributor Author

Thanks for the feedback! I agree it would not be good to set colors to null, potentially causing unexpected/undefined behaviour.

What do you think about instead of setting it to null, removing the property from colorMap so that customizations would only affect theme colors, but not the default colors.

E.g. with this color theme:

{
  "$schema": "vscode://schemas/color-theme",
  "type": "dark",
  "colors": {
    "activityBar.activeBackground": "#1f2727",
	"activityBar.activeBorder": "#00000000"
  }
}

and these settings:

{
	"workbench.colorCustomizations": {
		"activityBar.activeBackground": null
	}
}

the resulting colorMap would be

{
  "activityBar.activeBorder": "#00000000"
}

the same as if the color hasn't been defined in the color theme in the first place.

Thanks again!

@aeschli
Copy link
Contributor

aeschli commented May 27, 2024

Yes, using null to reset a color to the default would be fine. But it's unclear to me how many problems this solves. It would only be useful for the colorCustomization settings.

@SimonSiefke
Copy link
Contributor Author

Due to the PR being closed, I made some changes here: https://github.com/SimonSiefke/vscode/pull/14/files. What do you think?

@aeschli aeschli reopened this Jun 5, 2024
@aeschli aeschli added this to the June 2024 milestone Jun 5, 2024
@aeschli
Copy link
Contributor

aeschli commented Jun 5, 2024

I simplified the PR by allowing default also in color theme files. It's not useful, but also not harmful and easy to handle. That eliminates the schema split.

@aeschli aeschli enabled auto-merge (squash) June 5, 2024 14:31
@aeschli aeschli merged commit 28cbfce into microsoft:main Jun 5, 2024
@aeschli
Copy link
Contributor

aeschli commented Jun 5, 2024

Thanks @SimonSiefke !

@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants