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 support to colorize FileInfo file names #14403

Merged
merged 12 commits into from Jul 12, 2021

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Dec 12, 2020

PR Summary

Built on PSAnsiRendering experimental feature to enable coloring of specific file types. Example:

Screen Shot 2020-12-12 at 10 32 02 AM

Adds FileInfo member to $PSStyle automatic variable to allow customization. Directory, SymbolicLink, and Executable are built-in, but an Extension member which is a dictionary allows modification and addition of new extensions and custom styles. Leverages existing NameString extended member for coloring. Pre-included extensions for archives and PowerShell files. Default color choices are mostly consistent with ls --color.

PR Context

Fix #9270

PR Checklist

@SteveL-MSFT
Copy link
Member Author

Codefactor issues are existing ones

@iSazonov
Copy link
Collaborator

@SteveL-MSFT I am very concerned that we are colorizing many things by default. This is a very sensitive area for many people. I think we need themes support and the default theme needs to be approved by MSFT accessibility team.

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Dec 15, 2020

@iSazonov from an accessibility standpoint, the rule that applies here is sufficient contrast between colors (foreground and background) which is covered by the default selections. Can certainly add a [bool] $PSStyle.FileInfo.Color member to make it easy to turn on/off if you feel that is necessary. Themes should really be the domain of the terminal and not the shell.

Let's continue the discussion in the issue #9270

@iSazonov
Copy link
Collaborator

My eyes can hold up quite well all day with the classic Windows soothing color style, but they will definitely be against the checkered color scheme.

Themes should really be the domain of the terminal and not the shell.

As I mentioned early, $PSStyle should be evaluated based on TermInfo, and we need to map $PSStyle to TermInfo - the mapping I named theme and the themes should be switchable and customizable.

@Jaykul
Copy link
Contributor

Jaykul commented Dec 16, 2020

image
This was already done in the terminal-icons module, ipso facto it should be rejected? ¯\_(ツ)_/¯

@SteveL-MSFT
Copy link
Member Author

@Jaykul, this PR makes changes to FileSystemProvider to make it as fast as possible. Certainly it's feasible to call a native POSIX API to determine if a file on Linux/macOS is an executable or call a win32 API to check if an object is a reparse point and obtain the target within a format ps1xml, but it would be much more code.

@Jaykul
Copy link
Contributor

Jaykul commented Dec 16, 2020

@Jaykul, this PR makes changes to FileSystemProvider to make it as fast as possible. Certainly it's feasible to call a native POSIX API to determine if a file on Linux/macOS is an executable or call a win32 API to check if an object is a reparse point and obtain the target within a format ps1xml, but it would be much more code.

I missed that Platform.NonWindowsIsExecutable is a private method (sigh), but I don't think that matters.

Ignoring who wrote this code, the PowerShell team pledged that feature requests would be aggressively rejected if they can be done externally, and this has, in fact, already been done multiple times in modules. Just for example:

  • PSColor was written in 2014 and has over 8k downloads on the gallery (which it predates).
  • Terminal-Icons was released 18 months ago and has over 3k downloads.

Many of these modules do far more than just color files, and have spawned their own copy cats and even modules which depend on them.

The two I picked do things in a Windows-specific way (i.e. coloring files by extension) --they're the ones I've personally used in the past-- but a module can easily duplicate a PInvoke or two and implement this in more cross-platform way, and still ship externally so as to not end up doing the work twice (and breaking the other modules if the experimental flag is ever removed).

Building this into PowerShell also adds a lot of maintenance, because it will mean that this repo and team are going to get requests for every other type of coloring of Get-ChildItem output which you may have forgotten, including requests to add icons, to support the LS_COLOR environment variable, etc. 😣

In my opinion this should not be merged. I do think the functionality to check IsExecutable should be added to the FileInfo object as a CodeProperty (there's a lot of prescedent for that).

@ghost ghost added the Review - Needed The PR is being reviewed label Dec 24, 2020
@ghost
Copy link

ghost commented Dec 24, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 1, 2021
@SteveL-MSFT
Copy link
Member Author

@Jaykul I would agree that anything more than what is provided here would make sense to provide as a module. The intent here is only parity with ls --color.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 8, 2021
@SteveL-MSFT
Copy link
Member Author

Created discussion for partial format updates to support other PSColor and Terminal-Icon scenarios #15746

@joeyaiello
Copy link
Contributor

@PowerShell/powershell-committee discussed this at length today, and many of us agreed that you made some excellent points in this thread, @Jaykul. In particular, we empathize with your point about our stance to push innovation happening out of the PS repo vs. within the repo, and that's certainly the general case.

However, the original experimental feature--PsAnsiRendering, which includes the $PSStyle variable--is a feature that we felt is standard enough in all modern shells that this is a gap we need to fill in the mainline product. (As an additional data point, the lack of a new dependency here helps push this over the edge, as none of this work is significantly contributing to the size of the package.)

Now, to your point, there's very cool stuff happening in other modules like PSColor and Terminal-Icons. We definitely don't want to invalidate that work with anything we do, but a cursory analysis shows that modules like that should still work to override this work just fine. And in fact, we want to create a more extensible substrate for modules like that to easily implement colorization for a larger set of types. This assumption could be wrong, but that's exactly the sort of thing we'd like to learn by shipping this as an experimental feature.

Another reason for shipping this work is to validate that the $PSStyle work is the correct approach. We renamed the experimental feature so that it's a part of the same namespace as the rest of the PSAnsiRendering work. Based on the real world usage and issues, we're open to not necessarily shipping both of these as stable at the same time.

To all those ends, we're going to merge this as experimental, but we'll still evaluate it as any other experimental feature before we promote it to stable. I believe that @SteveL-MSFT also plans to write an RFC for all the $PSStyle work (based on #13071) before this happens.

As a personal side note, I'd like to do some further analysis of the default colors before shipping as stable as well. @SteveL-MSFT took these straight from ls, but it might be a better idea to fit with the default colorization of PSReadline.

@joeyaiello joeyaiello added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jul 8, 2021
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 9, 2021

However, the original experimental feature--PsAnsiRendering, which includes the $PSStyle variable--is a feature that we felt is standard enough in all modern shells that this is a gap we need to fill in the mainline product.

Standard? This standard is so old that you won't surprise anyone. PowerShell was approved and came into being only because it was a next generation shell with great innovations.
Why copy bash (you mean it, don't you?) if you already have it? Run bash and be happy!

Don't you have any breakthrough ideas at all? What about Jason's PSMore for next dynamic formatting? Jim's DSL? New modern remoting? There are others lost in the midst of several thousand issues. Some interesting ideas seem to have been started but forgotten.

PowerShell Core is celebrating its fifth anniversary and we have to ask where are the unparalleled innovations?
(I'm not writing this to diminish achievements, but to inspire new ones.)

(As an additional data point, the lack of a new dependency here helps push this over the edge, as none of this work is significantly contributing to the size of the package.)

You could open Notepad and try colorize - on the second entity you will feel sorry for your time and the bloated XML will horrify you :-)

Another reason for shipping this work is to validate that the $PSStyle work is the correct approach.

It makes no sense to spend time on coping old school semi-features (especially if there are already alternative modules).
Do real innovations!

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jul 9, 2021
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@rjmholt rjmholt merged commit bfb4a81 into PowerShell:master Jul 12, 2021
@musm
Copy link

musm commented Jul 12, 2021

Yay, many thanks for this @SteveL-MSFT !

@SteveL-MSFT SteveL-MSFT deleted the file-color branch July 13, 2021 02:30
@rjmholt rjmholt added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 21, 2021
@ghost
Copy link

ghost commented Jul 22, 2021

🎉v7.2.0-preview.8 has been released which incorporates this pull request.:tada:

Handy links:

@jantari
Copy link

jantari commented Jul 26, 2021

In case anyone else comes here after installing v7.2.0-preview.8 because you're looking to change the ... questionable ... default blue/white color for directories. I find this looks really nice - add it to your $PROFILE:

if ($PSStyle) {
    $PSStyle.FileInfo.Directory = "`e[4;1m"
}

It makes directories use standard colors but they are underlined and bold.

@jhoneill
Copy link

In case anyone else comes here after installing v7.2.0-preview.8 because you're looking to change the ... questionable ... default blue/white color for directories. I find this looks really nice - add it to your $PROFILE:

if ($PSStyle) {
    $PSStyle.FileInfo.Directory = "`e[4;1m"
}

It makes directories use standard colors but they are underlined and bold.

TBH I find a lot of the defaults questionable. When I get a file listing.
image

  1. The green header "Splits" the screen and "orphans" the directory name. Bold or brighter text, makes sense for a header, a slightly off white, but who puts the header rows in their Excel or Word documents in a totally different colour ? If we are going to have that then directory name is part of the header and should be in the accent colour, but $PSStyle.Formatting.TableHeader doesn't do that.
  2. The Directories are URGENT. I know they are directories because (a) they are sorted to the top and (b) there is a blank space where the length would go. It's pulling my eye to something, that I don't need to know.
  3. I also know the symbolic link is a link because it has the target next to it (now in a different colour)
  4. What is dangerous or erroneous about ZIP files? If there is a need to distinguish them at all (questionable) should they be shouting for attention ? . Red is a bad choice (c.f. Get-Error where the use of Red means the error text jumps out from everything else - which is useful)
  5. It may be my eyesight, but Green "recedes" so less.exe is harder to pick out, especially when sandwiched between emphasized PS1 files. (Although it says it is foreground 32 - Green, it appears to render as light green 92 so I couldn't make it more prominent. )
  6. Emphasizing PowerShell files has my brain asking "why are these different from those" and colouring .psd and .psxml files but not json and xml files compounds the problem.

The net effect of 1-6 is that it takes me longer to find what I'm looking for in a listing. I know all of these can be tuned but the defaults look a bit like the first thing someone thought of. The defaults need attention if the feature is going from experimental to on-by-default

When I might want colour to give me more information , e.g. intellisense with [ctrl]+[space] there is none.
image

Here, picking out directories with colour or underline would help, so might a warning I'm about to move an exe or a script whose path is hard coded somewhere. But it's not available, and if it were it would probably require a listing to use the same colours, making it all the more important to get good defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Color output of gci
9 participants