Skip to content

Grdowns/inactive region opacity#2061

Merged
bobbrow merged 9 commits into
masterfrom
grdowns/inactive-region-opacity
Jun 6, 2018
Merged

Grdowns/inactive region opacity#2061
bobbrow merged 9 commits into
masterfrom
grdowns/inactive-region-opacity

Conversation

@grdowns

@grdowns grdowns commented May 30, 2018

Copy link
Copy Markdown
Contributor

Add opacity setting for inactive preprocessor blocks to replace text greying

Comment thread Extension/src/LanguageServer/client.ts Outdated
let settings: CppSettings = new CppSettings(this.RootUri);

let decoration: vscode.TextEditorDecorationType = vscode.window.createTextEditorDecorationType({
opacity: String(settings.inactiveRegionOpacity)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any error validation here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, should be a number between 0 and 100...although maybe 0 is too low if the text becomes invisible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do it elsewhere in the codebase. I believe the line of thought is CppSettings should always be constructible, as this.RootUri should always be valid. @bobbrow?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Andrew is referring to the inactiveRegionOpacity setting

Comment thread Extension/package.json Outdated
"C_Cpp.inactiveRegionOpacity": {
"type:": "number",
"default": 0.5,
"Description": "Controls the opacity of inactive preprocessor blocks. Scales between 0.0 and 1.0. This setting is overridden by dimInactiveRegions.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more clear if we said "This setting only applies when inactive region dimming is enabled."

Comment thread Extension/package.json Outdated
},
"C_Cpp.inactiveRegionOpacity": {
"type:": "number",
"default": 0.5,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add "minimum": 0 and "maximum": 1 to the schema so that VS Code will squiggle values outside the valid range?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

Comment thread Extension/src/LanguageServer/client.ts Outdated
let settings: CppSettings = new CppSettings(this.RootUri);

let decoration: vscode.TextEditorDecorationType = vscode.window.createTextEditorDecorationType({
opacity: String(settings.inactiveRegionOpacity)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't need to make this a string since you already enforced the inactiveRegionOpacity setting to be a string in settings.ts.

let renderOptions: vscode.DecorationRenderOptions = {
light: { color: "rgba(175,175,175,1.0)" },
dark: { color: "rgba(155,155,155,1.0)" },
rangeBehavior: vscode.DecorationRangeBehavior.ClosedOpen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to keep the ClosedOpen rangeBehavior?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-added!

Comment thread Extension/package.json Outdated
},
"C_Cpp.inactiveRegionOpacity": {
"type:": "number",
"default": 0.5,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be 0.55 to match Visual Studio's default opacity?

Comment thread Extension/src/LanguageServer/client.ts Outdated
let settings: CppSettings = new CppSettings(this.RootUri);

let decoration: vscode.TextEditorDecorationType = vscode.window.createTextEditorDecorationType({
opacity: String(settings.inactiveRegionOpacity)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, should be a number between 0 and 100...although maybe 0 is too low if the text becomes invisible.

let renderOptions: vscode.DecorationRenderOptions = {
light: { color: "rgba(175,175,175,1.0)" },
dark: { color: "rgba(155,155,155,1.0)" },
rangeBehavior: vscode.DecorationRangeBehavior.ClosedOpen

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need that.

@sean-mcmanus

Copy link
Copy Markdown
Contributor

@bobbrow Can you unblock this now?

@bobbrow bobbrow merged commit 3027e18 into master Jun 6, 2018
@bobbrow bobbrow deleted the grdowns/inactive-region-opacity branch June 6, 2018 19:14
@olr666

olr666 commented Jun 29, 2018

Copy link
Copy Markdown

Is it possible to restore the greyed text and leave users with the option to choose either grey inactive regions OR dimmed but coloured inactive regions? Some people do prefer greyed sections to dimmed-but-still-coloured.

@sean-mcmanus

Copy link
Copy Markdown
Contributor

@olr666 It's possible, but I don't think we were planning to implement that unless we got more user feedback. We could accept a pull request for some bool grayInactiveRegions setting that defaulted to false...maybe the inactiveRegionOpacity could be used to change the gray color as well?

@bobbrow

bobbrow commented Jun 29, 2018

Copy link
Copy Markdown
Member

I think adding an inactiveRegionColor setting would be better. When not set we leave the colors alone. When set, we override the color.

@github-actions github-actions Bot locked and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants