Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add color options for Attributes, command arguments and loop labels. #1724

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MartinGC94
Copy link

@MartinGC94 MartinGC94 commented Aug 2, 2020

PR Summary

Add 3 new color options for tokens that currently use the default token color.
More color options will make it easier to replicate themes from editors like VS code and the ISE into the normal console.
The default colors for these new tokens are:

  • Attribute: Cyan
  • CommandArgument: Gray
  • LoopLabel: DarkGray

PR Checklist

Microsoft Reviewers: Open in CodeFlow

Copy link
Member

@daxian-dbw daxian-dbw left a comment

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 useful to have the attribute color configurable, but making it cyan by default collides with the default emphasis color, which might make it confusing when doing back/forward history searching (e.g. ctrl+r).

For the command argument color, it may be useful, but I'm not sure if we can accurately identify only the arguments, especially when it comes to native commands.

For the loop label, I doubt whether it's worth to make its color configurable, given how often it gets used.

@lzybkr Can you please share your thoughts?

if ((token.Kind == TokenKind.Identifier | token.Kind == TokenKind.Generic) && token.TokenFlags == TokenFlags.None)
{
return _options._commandArgumentColor;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is accurate to reflect command arguments. Could the condition be true for something that is not a command argument?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so, I've pasted a lot of code into the console to test the colors and I haven't found any unexpected results regarding command arguments missing color or affecting something that wasn't a command argument.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using logical or instead of bitwise or.

That said, with these changes, which tokens get the default now? I can't think of any, in which case, it seems unnecessary to introduce a new option.

@lzybkr
Copy link
Member

lzybkr commented Sep 2, 2020

I'm not opposed to adding the loop label, but obviously it's rarely used.

I'm not excited with using a different color for attributes and types by default, but it seems fine to have the option.

I'm also not excited by the change to command argument coloring by default, I think it shouldn't change from the current setting.

If these changes are accepted, what tokens would fall under the default color? With this change, all I can think of are delimiters like parenthesis and braces.

Thinking about this a little more, I think coloring command arguments not using the default color might be a surprising and annoying change for anyone who customizes the default.

@MartinGC94
Copy link
Author

I understand not wanting to change the defaults.
I'm not sure how to implement this in a nice way without affecting customized profiles though.
I guess we could change: https://github.com/PowerShell/PSReadLine/blob/master/PSReadLine/Cmdlets.cs#L514-L529

Into something like:

                        {"ContinuationPrompt", (o, v) => o.ContinuationPromptColor = v},
                        {"Emphasis", (o, v) => o.EmphasisColor = v},
                        {"Error", (o, v) => o.ErrorColor = v},
                        {"Default", (o, v) =>
                        {
                            o.DefaultTokenColor = v;
                            o.CommandArgumentColor = v;
                            o.LoopLabelColor = v;
                        }
                        },
                        {"Comment", (o, v) => o.CommentColor = v},
                        {"Keyword", (o, v) => o.KeywordColor = v},
                        {"String", (o, v) => o.StringColor = v},
                        {"Operator", (o, v) => o.OperatorColor = v},
                        {"Variable", (o, v) => o.VariableColor = v},
                        {"Command", (o, v) => o.CommandColor = v},
                        {"Parameter", (o, v) => o.ParameterColor = v},
                        {"Type", (o, v) =>
                        {
                            o.TypeColor = v;
                            o.AttributeColor = v;
                        }
                        },
                        {"Number", (o, v) => o.NumberColor = v},
                        {"Member", (o, v) => o.MemberColor = v},
                        {"Selection", (o, v) => o.SelectionColor = v},
                        {"Prediction", (o, v) => o.PredictionColor = v},
                        {"Attribute", (o, v) => o.AttributeColor = v},
                        {"CommandArgument", (o, v) => o.CommandArgumentColor = v},
                        {"LoopLabel", (o, v) => o.LoopLabelColor = v},

So if you run: Set-PSReadLineOption -Colors @{Default="Red";Type="Green"} you'll get the same results you get today but you also have the option of setting these new custom colors by running Set-PSReadLineOption again with the new color options. The problem with this is that the only way to get reliable results is to make 2 different calls to Set-PSReadLineOption because of the random order inside hashtables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants