terminal
terminal copied to clipboard
microsoft
Implement EnableColorSelection
Summary of the Pull Request
As described in #9583, this change implements the legacy conhost "EnableColorSelection" feature.
Detailed Description of the Pull Request / Additional comments
@zadjii-msft was super nice and provided the outline/plumbing (WinRT classes and such) as a hackathon-type project (thank you!)--a "SelectionColor" runtimeclass, a ColorSelection method on the ControlCore runtimeclass, associated plumbing through the layers; plus the action-and-args plumbing to allow hooking up a basic "ColorSelection" action, which allows you to put actions in your settings JSON like so:
{
"command":
{
"action": "experimental.colorSelection",
"foreground": "#0f3"
},
"keys": "alt+4"
},
On top of that foundation, I added a couple of things:
- The ability to specify indexes for colors, in addition to RGB and RRGGBB colors.
- It's a bit hacky, because there are some conversions that fight against sneaking an "I'm an index" flag in the alpha channel.
- A new "matchMode" parameter on the action, allowing you to say if you want to only color the current selection ("0") or all matches ("1").
- I made it an int, because I'd like to enable at least one other "match mode" later, but it requires me/someone to fix up search.cpp to handle regex first.
- Search used an old UIA "ColorSelection" method which was previously
E_NOTIMPL, but is now wired up. Don't know what/if anything else uses this.
- An uber-toggle setting, "EnableColorSelection", which allows you to set a single
boolin your settings JSON, to light up all the keybindings you would expect from the legacy "EnableColorSelection" feature:- alt+[0..9]: color foreground
- alt+shift+[0..9]: color foreground, all matches
- ctrl+[0..9]: color background
- ctrl+shift+[0..9]: color background, all matches
- A few of the actions cannot be properly invoked via their keybindings, due to #13124.
*!*But they work if you do them from the command palette. - If you have "
EnableColorSelection : true" in your settings JSON, but then specify a different action in your JSON that uses the same key binding as a color selection keybinding, your custom one wins, which I think is the right thing. - I fixed what I think was a bug in search.cpp, which also affected the legacy EnableColorSelection feature: due to a non-inclusive coordinate comparison, you were not allowed to color a single character; but I see no reason why that should be disallowed. Now you can make all your
Xs red if you like.
"Soft" spots:
- I was a bit surprised at some of the helpers I had to provide in textBuffer.cpp. Perhaps there are existing methods that I didn't find?
- Localization? Because there are so many (40!) actions, I went to some trouble to try to provide nice command/arg descriptions. But I don't know how localization works…
PR Checklist
- [x] Closes #9583
- [x] CLA signed. If not, go over here and sign the CLA
- [ ] Tests added/passed (what would be the right place to add tests for this?)
- [ ] Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
- [ ] Schema updated. (is this needed?)
- [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
Validation Steps Performed
Just manual testing.
@check-spelling-bot Report
Unrecognized words, please review:
- itow
- pbg
- tcbg
- tcfg
Previously acknowledged words that are now absent
azurewebsites Checkin condev Consolescreen css cxcy DCompile debolden deconstructed DECRST DECRSTS devicefamily dxp errno FARPROC GETKEYSTATE guardxfg HPAINTBUFFER HPROPSHEETPAGE iconify ipa LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL mspartners ned newcursor nlength NOWAIT PENDTASKMSG pgorepro pgort PGU Poli PPORT PSMALL redistributable SOURCESDIRECTORY Timeline timelines toolbars unintense UWA UWAs VKKEYSCAN wddmconrenderer wdx windev WResult xfgTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands
... in a clone of the [email protected]:jazzdelightsme/terminal.git repository
on the dev/danthom/enableColorSelection branch:
update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/478c2c3613dc7f626474369987ae16a684574c6e.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}
comment_json=$(mktemp)
curl -L -s -S \
--header "Content-Type: application/json" \
"https://api.github.com/repos/microsoft/terminal/issues/comments/1174692697" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json
patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
update_files
rm $comment_body
git add -u
:pencil2: Contributor please read this
By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
:warning: The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...
If the listed items are:
- ... misspelled, then please correct them instead of using the command.
- ... names, please add them to
.github/actions/spelling/allow/names.txt. - ... APIs, you can add them to a file in
.github/actions/spelling/allow/. - ... just things you're using, please add them to an appropriate file in
.github/actions/spelling/expect/. - ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in
.github/actions/spelling/patterns/.
See the README.md in each directory for more information.
:microscope: You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. :wink:
:clamp: If you see a bunch of garbage
If it relates to a ...
well-formed pattern
See if there's a pattern that would match it.
If not, try writing one and adding it to a patterns/{file}.txt.
Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
binary-ish string
Please add a file path to the excludes.txt file instead of just accepting the garbage.
File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@check-spelling-bot Report
Unrecognized words, please review:
- itow
- pbg
- tcbg
- tcfg
Previously acknowledged words that are now absent
azurewebsites Checkin condev Consolescreen css cxcy DCompile debolden deconstructed DECRST DECRSTS devicefamily dxp errno FARPROC GETKEYSTATE guardxfg HPAINTBUFFER HPROPSHEETPAGE iconify ipa LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL mspartners ned newcursor nlength NOWAIT PENDTASKMSG pgorepro pgort PGU Poli PPORT PSMALL redistributable SOURCESDIRECTORY Timeline timelines toolbars unintense UWA UWAs VKKEYSCAN wddmconrenderer wdx windev WResult xfgTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands
... in a clone of the [email protected]:jazzdelightsme/terminal.git repository
on the dev/danthom/enableColorSelection branch:
update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/478c2c3613dc7f626474369987ae16a684574c6e.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}
comment_json=$(mktemp)
curl -L -s -S \
--header "Content-Type: application/json" \
"https://api.github.com/repos/microsoft/terminal/issues/comments/1174696196" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json
patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
update_files
rm $comment_body
git add -u
:pencil2: Contributor please read this
By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
:warning: The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...
If the listed items are:
- ... misspelled, then please correct them instead of using the command.
- ... names, please add them to
.github/actions/spelling/allow/names.txt. - ... APIs, you can add them to a file in
.github/actions/spelling/allow/. - ... just things you're using, please add them to an appropriate file in
.github/actions/spelling/expect/. - ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in
.github/actions/spelling/patterns/.
See the README.md in each directory for more information.
:microscope: You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. :wink:
:clamp: If you see a bunch of garbage
If it relates to a ...
well-formed pattern
See if there's a pattern that would match it.
If not, try writing one and adding it to a patterns/{file}.txt.
Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
binary-ish string
Please add a file path to the excludes.txt file instead of just accepting the garbage.
File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
@jazzdelightsme BTW If you'd like to, I'd be happy to help you get this PR over the finishing line by making any necessary changes for you. 🙂
@lhecker, thank you for your very kind offer. I should have a bit of time in the next few days, so I will see what I can get done, and then we can see what could still use some help.
Okay @lhecker, I have an area that could use some expert help: @carlos-zamora requested that the matchMode parameter be made an enum. I took a shot at it, but it did not go well.
My attempt: https://github.com/jazzdelightsme/terminal/tree/dev/danthom/enableColorSelection_enum
The problem is that I defined the enum in ControlCore.idl, but then the definition wasn't available in terminal.hpp (although I tried to follow existing prior art to just pre-declare them). I tried defining it in a different place, but still fought with layering problems, and the extremely long build times make this pretty painstaking to just try a bunch of things to see what works--this is an area where having someone with expertise who actually knows how it should happen would be hugely advantageous.
In reply to: 1200704868
@jazzdelightsme I'm terribly sorry for leaving you on read for the last 2 weeks (it's been a busy two weeks!). I've just pushed a commit that fixes the compilation. I'm also going to go through your PR now to address anything that I believe might be simplified somehow (I hope you don't mind).
Oh right, I meant to ask: Why is MatchMode an integer or enum in the first place? Wouldn't a mere boolean work just as well and be just as descriptive (bool matchModeAll)?
I want a third mode: a "magical number matching" mode, similar to what is available in WinDbg, where it matches numbers in different bases (e.g. "0x8080" would also match with "32896").
To do this would require regex support, and I took a look at adding that, just using the STL regex stuff--we should just be able to supply iterators to the buffer et voila. The problem is that the Terminal code uses const and refs all over the place, which is great, and I like it, but it just does not play well with the STL's concepts of iterators--basically everything needs to be copy constructible/assignable, and default constructible and such, and there's basically no way to shoehorn a bunch of stuff with const and const & into that paradigm, so I left that for a separate change. I figured I would attempt prototyping some alternative buffer iterator code and make sure I could get it to work with the regex stuff before proposing it, but I have not had time, and probably will not for the next few months at least.
In reply to: 1217976489
@jazzdelightsme I've just pushed 4 commits + a merge with main.
- 2 commits fix some issues with this PR that I found.
- "Allow til::from_wchars to work with chars" is unrelated to this PR but can be cherry-picked into a another pR and merged in advance if we want to. It's a minor change to allow us to use this handy utility function in our json parsing code.
- The final commit contains a series of changes which either deduplicate some code / use existing functionality, or which I personally think fit better with the long term plans of this project.
There was a point that Carlos raised a while ago which I think may have got lost in the collapsed comments: could we please use the actual color names for the indexed colors (i.e. red, green, and blue) rather than numeric values like i1, i2, and i4.
I know that means you're limited to the 16 basic ANSI colors (and I guess the special colors like foreground and background), but there's really no practical use for the other indexed colors. They don't sync with the color scheme, so an RGB value would serve just as well, and is probably more readable.
I'm of two minds on this. I won't block either way, but... eventually I want to unify all of Terminal's color parsers, so that you can specify "gray30" or "rebeccapurple" or "rgb:ff/ee/dd" or even "#fea" and have it do roughly the same thing everywhere you can put a colorspec. red has two meanings there, if it refers to index 1.
At the same time, I am not gonna block on a YAGNI concern. We may never get there anyway.
Ok, finished investigating it. Only found one issue where the new actions pop up in the Settings UI, and if you delete one of the new actions, we crash. Fixed with that commit I added. Everything looks great!
Since all of the things I mentioned are minor, I'm gonna go ahead and fix them, approve, and merge to main. Just gonna head out and get lunch first. Thanks for everything @jazzdelightsme! 😊
@j4james Re: using the color names for indexed colors: I have no objection, but the thing that made me go "hm?" was the fact that can't people assign whatever colors they want to the indexes? So the index slot for red might be pink; the index slot for magenta might be brown; etc. So maybe we can just say "well if you assigned some other color to the index traditionally used for "red", then that's the color you get for "red"." But then what about the idea of using friendly names for full RGB colors--how does one differentiate between "the color associated with the index traditionally used for red" ("indexed red") versus the RGB value #ff0000?
the thing that made me go "hm?" was the fact that can't people assign whatever colors they want to the indexes?
Yeah, it's possible some users will assign weird colors to the standard 16 range, but I would think they're going to be in the minority, and they will at least know that they've done that. If you're literally selecting the color labelled Red in the settings and changing it to pink, you're not going to be hugely surprised if you get pink when you select red as your color here.
But for the majority of the users, I think something like i1 would be completely baffling. And is i1 red, or is it actually blue? The Windows color order and the ANSI color order are different, so that adds another layer of confusion if you're accustomed to the Windows way of doing things.
But then what about the idea of using friendly names for full RGB colors
If that's really what we want to do, then I can accept that. But I suspect there are very few people likely to use that functionality. Most will either want to pick a value from their color scheme (for which red will be some shade of red, 99% of the time), or otherwise they'll want to provide an explicit RGB value of some sort. How many people are really likely to know the X11 color names, and decide they want something like RebeccaPurple.
And it's worth noting that the X11 color names don't always match the names used by CSS, and if people have any knowledge of color names I'd expect it's the CSS names they'd be more familiar with. Admittedly there aren't that many differences, but again it's just introducing another potential source of confusion.
I don't know. I may be wrong, but I think this design is optimised for the least likely use case at the expense of the most common one. But it's not that big a deal.
Since we only support parsing indexed colors for this feature, which is marked as experimental, I think we can just use iNN for now. But I agree that named colors would be a better choice, because I also couldn't tell what i01 means, but I do know what "red" is. 😅
(Unrelated to this PR...)
Personally, I would love if we nudge our settings strings closer to CSS if anything. For instance I'd like to see support for rgba(r, g, b, a) in the future. Assuming we want that, and before we have ruled out that we never want to support it in its entirety, it's probably also for the best if we reserve named CSS colors and not immediately use them for ANSI colors. But we could still support something like ansi-red, ansi-brightblue, etc. (or similar) in the near term, which I currently think would be the best choice and trade-off. It's not immediately obvious that you have to prefix it with ansi-, but once you've seen it you can still immediately recognize what color ansi-red stands for. And it's unlikely to conflict with anything CSS will come up in the future.
Note from bug bash:
- [ ] ECS binds color selection to Ctrl+Shift+[0-9] but those are already taken; the "new tab with profile" bindings win.
Hello @DHowett!
Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.
p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.
:tada:Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:
Handy links: