Implement EnableColorSelection #13429

jazzdelightsme · 2022-07-05T06:59:08Z

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 bool in 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 InternalActionID is a hash, which cannot be depended on for equality comparison #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

Closes Support EnableColorSelection in Terminal #9583
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?)
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.

# Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp

carlos-zamora

How localization works

In VS (you can use a text editor too but VS makes it easier), open the Resources.resw file for the project you're working in (i.e TerminalSettingsModel)
this should give you a nice big table with the key, value, and comment...
- key: an ID for the string (i.e. MarkModeCommandKey)
- value: what the string will be displayed as (i.e. Toggle mark mode)
- comment: a description for the localization team to make sure they localize it properly (i.e. A command that will toggle "mark mode". This is a mode in the application where the user can mark the text by selecting portions of the text.).
  - NOTE: we've been bad about this, but we should generally put a comment in. The localization team doesn't necessarily have the knowledge of how terminals work, so they might misinterpret the meaning. A simple example is the word "bat" can mean many different things, so they may localize it as the animal when we meant the baseball tool. Try to explain the context a bit like "this is a header" or "color".
  - NOTE 2: sometimes, we don't want to localize something or we want to add more complex logic like string concatenation/injection. In those cases, say {Locked="JSON"} meaning you don't want to localize the word JSON, or explain that {0} will be replaced with something specifically (i.e. a file extension). This helps ensure the localized string is (more likely) grammatically correct.
actually use the string by using the macro RS_(<key>) (i.e. RS_(CopyTextAsSingleLineCommandKey))

(bonus) switching between the resw and the code can be cumbersome. If possible, in the code, just put what the string would look like. Example:

terminal/src/cascadia/TerminalSettingsModel/ActionArgs.cpp

Lines 528 to 536 in 2f58711

    
           // "Rename tab to \"{_Title}\"" 
        
           // "Reset tab title" 
        
           if (!Title().empty()) 
        
           { 
        
               return winrt::hstring{ 
        
                   fmt::format(std::wstring_view(RS_(L"RenameTabCommandKey")), 
        
                               Title().c_str()) 
        
               }; 
        
           }

Other feedback

make sure to update the schema
Idea: what if we got rid of MatchMode altogether and just introduced a separate key binding? So we could have colorSelection and colorAllMatchingText? Then when we want to add the regex, we can add that to colorAllMatchingText?

carlos-zamora · 2022-07-14T18:10:00Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+            case 0:
+                colorStr = L"black";
+                break;
+
+            case 1:
+                colorStr = L"dark red";
+                break;
+
+            case 2:
+                colorStr = L"dark green";
+                break;
+
+            case 3:
+                colorStr = L"dark yellow";
+                break;
+
+            case 4:
+                colorStr = L"dark blue";
+                break;
+
+            case 5:
+                colorStr = L"dark magenta";
+                break;
+
+            case 6:
+                colorStr = L"dark cyan";
+                break;
+
+            case 7:
+                colorStr = L"gray"; // "dark white"?
+                break;
+
+            case 8:
+                colorStr = L"dark gray"; // "bright black"?
+                break;
+
+            case 9:
+                colorStr = L"red";
+                break;
+
+            case 10:
+                colorStr = L"green";
+                break;
+
+            case 11:
+                colorStr = L"yellow";
+                break;
+
+            case 12:
+                colorStr = L"blue";
+                break;
+
+            case 13:
+                colorStr = L"magenta";
+                break;
+
+            case 14:
+                colorStr = L"cyan";
+                break;
+
+            case 15:
+                colorStr = L"white";
+                break;


With regards to localization, we should use these resources:

terminal/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

Lines 138 to 217 in 2f58711

<data name="ColorScheme_Background.Text" xml:space="preserve">

<value>Background</value>

<comment>This is the header for a control that lets the user select the background color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Black.Header" xml:space="preserve">

<value>Black</value>

<comment>This is the header for a control that lets the user select the black color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Blue.Header" xml:space="preserve">

<value>Blue</value>

<comment>This is the header for a control that lets the user select the blue color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightBlack.Header" xml:space="preserve">

<value>Bright black</value>

<comment>This is the header for a control that lets the user select the bright black color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightBlue.Header" xml:space="preserve">

<value>Bright blue</value>

<comment>This is the header for a control that lets the user select the bright blue color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightCyan.Header" xml:space="preserve">

<value>Bright cyan</value>

<comment>This is the header for a control that lets the user select the bright cyan color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightGreen.Header" xml:space="preserve">

<value>Bright green</value>

<comment>This is the header for a control that lets the user select the bright green color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightPurple.Header" xml:space="preserve">

<value>Bright purple</value>

<comment>This is the header for a control that lets the user select the bright purple color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightRed.Header" xml:space="preserve">

<value>Bright red</value>

<comment>This is the header for a control that lets the user select the bright red color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightWhite.Header" xml:space="preserve">

<value>Bright white</value>

<comment>This is the header for a control that lets the user select the bright white color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_BrightYellow.Header" xml:space="preserve">

<value>Bright yellow</value>

<comment>This is the header for a control that lets the user select the bright yellow color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_CursorColor.Text" xml:space="preserve">

<value>Cursor color</value>

<comment>This is the header for a control that lets the user select the text cursor's color displayed on the screen.</comment>

</data>

<data name="ColorScheme_Cyan.Header" xml:space="preserve">

<value>Cyan</value>

<comment>This is the header for a control that lets the user select the cyan color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Foreground.Text" xml:space="preserve">

<value>Foreground</value>

<comment>This is the header for a control that lets the user select the foreground color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Green.Header" xml:space="preserve">

<value>Green</value>

<comment>This is the header for a control that lets the user select the green color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Purple.Header" xml:space="preserve">

<value>Purple</value>

<comment>This is the header for a control that lets the user select the purple color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_SelectionBackground.Text" xml:space="preserve">

<value>Selection background</value>

<comment>This is the header for a control that lets the user select the background color for selected text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Red.Header" xml:space="preserve">

<value>Red</value>

<comment>This is the header for a control that lets the user select the red color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_White.Header" xml:space="preserve">

<value>White</value>

<comment>This is the header for a control that lets the user select the white color for text displayed on the screen.</comment>

</data>

<data name="ColorScheme_Yellow.Header" xml:space="preserve">

<value>Yellow</value>

<comment>This is the header for a control that lets the user select the yellow color for text displayed on the screen.</comment>

</data>

Unfortunately, they're in a different resw so we can't do that (at least not easily). There's two options we have here:

Move these resources down to the settings model and expose them as a function to the settings editor

the good: less redundant

the bad: we would need to expose a function for each (or really just one that takes an index) that retrieves the localized name of the color

the ugly: we still need to keep the ones in Editor around because older versions of Terminal still point to them

copy/paste them into the resources for the settings model, add a comment in the resw to have them match the one in the settings editor, and use them here

the good: easy

the bad: if the localization team messes up, this can look really bad

We should do option 1. Just don't delete the Editor resw entries. I'm ok with it being a follow-up though and just doing option 2 for now. Just make sure this is filed as an issue if that's the case.

I went with option 2. Something I realized while doing so is that, while the color names ought to match, they are actually different--capitalized versus not.

carlos-zamora · 2022-07-14T18:13:42Z

src/cascadia/TerminalSettingsModel/ActionArgs.h

+#define COLOR_SELECTION_ARGS(X)                                                                      \
+    X(winrt::Microsoft::Terminal::Control::SelectionColor, Foreground, "foreground", false, nullptr) \
+    X(winrt::Microsoft::Terminal::Control::SelectionColor, Background, "background", false, nullptr) \
+    X(uint32_t, MatchMode, "matchMode", false, 0u)


Can we make this an enum instead of an int?

I'd ultimately prefer a bool but since we want to add other modes, I understand. Curious though, what other modes are you thinking of? If we add regex support, wouldn't that still be "all matches" but with a separate param for regex, potentially?

Curious though, what other modes are you thinking of?

I want 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").

I attempted to turn this into an enum, but it did not go well, despite the fact that I'd found a good change to imitate.

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... :(

src/cascadia/TerminalSettingsModel/ActionMap.cpp

carlos-zamora · 2022-07-14T18:18:48Z

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

+        fgStr = hasForeground ? _FormatColorString(fgBuf, Foreground().TextColor()) : L"(default)";
+        bgStr = hasBackground ? _FormatColorString(bgBuf, Background().TextColor()) : L"(default)";


what do you mean by (default)?

we should localize "default" if we're keeping it

"Default" here just means the default color--default foreground color, or default background color. So when a value is not set, you get the default color for that thing (fg or bg). Does that make sense?

carlos-zamora · 2022-07-14T19:02:15Z

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

+            // This will throw for something like "j0", but return 0 for something like
+            // "0j".


I forget. If this throws, what happens? Does the deserialization fail and the action is ignored?

Yes--deserialization fails, the exception is handled somewhere up the stack, and you get that handy error dialog that says "I was expecting <string from TypeDescription>, but got ".

carlos-zamora · 2022-07-14T20:32:40Z

src/cascadia/TerminalCore/Terminal.cpp

+            }
+            else if (matchMode == 1)
+            {
+                auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end);


Suggested change

auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end);

const auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end);

carlos-zamora · 2022-07-14T20:32:52Z

src/cascadia/TerminalCore/Terminal.cpp

+        {
+            if (matchMode == 0)
+            {
+                auto length = _activeBuffer().SpanLength(start, end);


Suggested change

auto length = _activeBuffer().SpanLength(start, end);

const auto length = _activeBuffer().SpanLength(start, end);

carlos-zamora · 2022-07-14T20:33:42Z

src/cascadia/TerminalControl/ControlCore.cpp

+            auto pForeground = winrt::get_self<implementation::SelectionColor>(fg);
+            auto pBackground = winrt::get_self<implementation::SelectionColor>(bg);


Suggested change

auto pForeground = winrt::get_self<implementation::SelectionColor>(fg);

auto pBackground = winrt::get_self<implementation::SelectionColor>(bg);

const auto pForeground = winrt::get_self<implementation::SelectionColor>(fg);

const auto pBackground = winrt::get_self<implementation::SelectionColor>(bg);

carlos-zamora · 2022-07-14T20:36:04Z

src/cascadia/TerminalControl/ControlCore.cpp

+                // TODO: can this be scoped down further?
+                // one problem is that at this point on the stack, we don't know what changed
+                _renderer->TriggerRedrawAll();


Would TriggerSelection() work? We're only modifying the selected area.

Also what do you mean by the comment?

If matchMode is > 0, then we colored the selection and all matches. I was kind of stumbling around trying to figure out how the graphical invalidation works, and left that TODO for myself, but I probably should have just removed it--if we potentially changed a bunch of places, I don't think it's worth trying to keep track of all such places and then invalidating just those. This is a rare enough, and user-triggered, action, so I think it's okay to just invalidate everything.

carlos-zamora · 2022-07-14T20:37:09Z

src/buffer/out/textBuffer.cpp

+// - bufferCoordinates: when enabled, treat the coordinates as relative to
+//                      the buffer rather than the screen.
+// Return Value:
+// - one or more sets of start-end coordinates


nit: reword this. What do those coords represent? I think you're basically giving start/end for ranges/spans of text on the buffer?

lhecker · 2022-07-30T13:17:42Z

@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. 🙂

jazzdelightsme · 2022-08-01T04:54:03Z

@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.

jazzdelightsme · 2022-08-04T22:36:23Z

This was very helpful, thank you!

In reply to: 1039294143

jazzdelightsme · 2022-08-04T22:52:51Z

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

@zadjii-msft

As described in microsoft#9583, this change implements the legacy conhost "EnableColorSelection" feature. @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: ```json { "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 `bool` in 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 microsoft#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 `X`s 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?

lhecker · 2022-08-17T12:56:32Z

@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).

lhecker · 2022-08-17T12:58:20Z

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)?

…orSelection

DHowett · 2022-08-19T00:44:20Z

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.

carlos-zamora · 2022-08-19T18:40:38Z

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!

carlos-zamora · 2022-08-19T18:42:31Z

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! 😊

DHowett

I'm blocking not on philosophical grounds, but because I want to write up some of the long-term architectural tendrils that this will either grow or sever. Sorry to delay :)

carlos-zamora

All of my stuff has been addressed. Excited to see this land! Approving.

Regarding the various color types, I think that's a new problem (or at least, more apparent) now that Theming and ColorSelection are a thing. If possible, we should probably address it sooner than later. Probably not in time for 1.16, but I think it'd be a good candidate to take on for 1.17. Just not looking forward to bugs saying "why does 'accent'/'i00' work in some places but not others". As a bonus, we should figure out how that fits into the settings UI and tab color picker, but I'm definitely digressing at this point haha.

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

src/buffer/out/textBuffer.cpp

jazzdelightsme · 2022-08-20T05:06:10Z

@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?

j4james · 2022-08-20T11:53:56Z

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.

lhecker · 2022-08-20T12:39:56Z

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.

carlos-zamora · 2022-08-26T00:08:44Z

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.

DHowett

Let's do it. @jazzdelightsme, thank you as always for your patience and your careful stewardship of the features you love. @carlos-zamora, can you check this out and resolve the conflicts?

ghost · 2022-08-31T22:35:00Z

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.

ghost · 2022-09-13T17:05:44Z

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

This comment was marked as resolved.

Sign in to view

jazzdelightsme force-pushed the dev/danthom/enableColorSelection branch from c1ffaa4 to 85f036c Compare July 5, 2022 07:03

This comment was marked as resolved.

Sign in to view

jazzdelightsme force-pushed the dev/danthom/enableColorSelection branch 2 times, most recently from c8b645c to 3f3d104 Compare July 9, 2022 17:09

DHowett added a commit that referenced this pull request Jul 12, 2022

PRE-MERGE #13429 Implement EnableColorSelection

a0ed8fe

# Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp

DHowett added a commit that referenced this pull request Jul 12, 2022

PRE-MERGE #13429 Implement EnableColorSelection

5a25f3f

# Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp

carlos-zamora requested changes Jul 14, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 14, 2022

jazzdelightsme force-pushed the dev/danthom/enableColorSelection branch from 3f3d104 to 9971364 Compare August 1, 2022 04:54

jazzdelightsme force-pushed the dev/danthom/enableColorSelection branch from d3f8162 to b5a59c0 Compare August 4, 2022 22:56

jazzdelightsme requested a review from carlos-zamora August 13, 2022 19:43

first (failed) attempt: put it in ControlCore.idl (Control::MatchMode)

e4271f1

Fix compilation with new MatchMode enum

3320c1a

lhecker force-pushed the dev/danthom/enableColorSelection branch from 323f646 to 3320c1a Compare August 17, 2022 14:25

lhecker added 3 commits August 17, 2022 21:16

Merge remote-tracking branch 'origin/main' into dev/danthom/enableCol…

a08339a

…orSelection

Fix invalid exec command in vcxproj

c7c8668

Fix color selection settings error

84fb3a7

[ActionMap] remove FAIL_FAST on >1 parent

1915ca9

DHowett requested changes Aug 19, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 19, 2022

address carlos-zamora's comments

cf66289

carlos-zamora approved these changes Aug 19, 2022

View reviewed changes

carlos-zamora assigned DHowett Aug 19, 2022

DHowett reviewed Aug 20, 2022

View reviewed changes

src/cascadia/TerminalSettingsModel/ActionArgs.cpp Outdated Show resolved Hide resolved

jazzdelightsme commented Aug 20, 2022

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 20, 2022

carlos-zamora added 2 commits August 22, 2022 11:38

revert changes to textBuffer.cpp

26e948c

Merge branch 'main' into dev/danthom/enableColorSelection

eb30108

DHowett added a commit that referenced this pull request Aug 23, 2022

PRE-MERGE #13429 Implement EnableColorSelection

6252504

DHowett approved these changes Aug 31, 2022

View reviewed changes

Merge branch 'main' into dev/danthom/enableColorSelection

7be2cf4

carlos-zamora unassigned DHowett Aug 31, 2022

format

faa62b9

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 31, 2022

why do I keep breaking this PR

d22aea2

ghost merged commit d11ea72 into microsoft:main Aug 31, 2022

ghost mentioned this pull request Sep 13, 2022

Support EnableColorSelection in Terminal #9583

Closed

jazzdelightsme deleted the dev/danthom/enableColorSelection branch September 21, 2022 20:59

This pull request was closed.

	// "Rename tab to \"{_Title}\""
	// "Reset tab title"
	if (!Title().empty())
	{
	return winrt::hstring{
	fmt::format(std::wstring_view(RS_(L"RenameTabCommandKey")),
	Title().c_str())
	};
	}

	<data name="ColorScheme_Background.Text" xml:space="preserve">
	<value>Background</value>
	<comment>This is the header for a control that lets the user select the background color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Black.Header" xml:space="preserve">
	<value>Black</value>
	<comment>This is the header for a control that lets the user select the black color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Blue.Header" xml:space="preserve">
	<value>Blue</value>
	<comment>This is the header for a control that lets the user select the blue color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightBlack.Header" xml:space="preserve">
	<value>Bright black</value>
	<comment>This is the header for a control that lets the user select the bright black color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightBlue.Header" xml:space="preserve">
	<value>Bright blue</value>
	<comment>This is the header for a control that lets the user select the bright blue color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightCyan.Header" xml:space="preserve">
	<value>Bright cyan</value>
	<comment>This is the header for a control that lets the user select the bright cyan color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightGreen.Header" xml:space="preserve">
	<value>Bright green</value>
	<comment>This is the header for a control that lets the user select the bright green color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightPurple.Header" xml:space="preserve">
	<value>Bright purple</value>
	<comment>This is the header for a control that lets the user select the bright purple color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightRed.Header" xml:space="preserve">
	<value>Bright red</value>
	<comment>This is the header for a control that lets the user select the bright red color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightWhite.Header" xml:space="preserve">
	<value>Bright white</value>
	<comment>This is the header for a control that lets the user select the bright white color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_BrightYellow.Header" xml:space="preserve">
	<value>Bright yellow</value>
	<comment>This is the header for a control that lets the user select the bright yellow color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_CursorColor.Text" xml:space="preserve">
	<value>Cursor color</value>
	<comment>This is the header for a control that lets the user select the text cursor's color displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Cyan.Header" xml:space="preserve">
	<value>Cyan</value>
	<comment>This is the header for a control that lets the user select the cyan color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Foreground.Text" xml:space="preserve">
	<value>Foreground</value>
	<comment>This is the header for a control that lets the user select the foreground color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Green.Header" xml:space="preserve">
	<value>Green</value>
	<comment>This is the header for a control that lets the user select the green color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Purple.Header" xml:space="preserve">
	<value>Purple</value>
	<comment>This is the header for a control that lets the user select the purple color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_SelectionBackground.Text" xml:space="preserve">
	<value>Selection background</value>
	<comment>This is the header for a control that lets the user select the background color for selected text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Red.Header" xml:space="preserve">
	<value>Red</value>
	<comment>This is the header for a control that lets the user select the red color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_White.Header" xml:space="preserve">
	<value>White</value>
	<comment>This is the header for a control that lets the user select the white color for text displayed on the screen.</comment>
	</data>
	<data name="ColorScheme_Yellow.Header" xml:space="preserve">
	<value>Yellow</value>
	<comment>This is the header for a control that lets the user select the yellow color for text displayed on the screen.</comment>
	</data>

		fgStr = hasForeground ? _FormatColorString(fgBuf, Foreground().TextColor()) : L"(default)";
		bgStr = hasBackground ? _FormatColorString(bgBuf, Background().TextColor()) : L"(default)";

		// This will throw for something like "j0", but return 0 for something like
		// "0j".

	auto text = _activeBuffer().GetPlainText(/trimTrailingWhitespace/ IsBlockSelection(), start, end);
	const auto text = _activeBuffer().GetPlainText(/trimTrailingWhitespace/ IsBlockSelection(), start, end);

	auto length = _activeBuffer().SpanLength(start, end);
	const auto length = _activeBuffer().SpanLength(start, end);

		auto pForeground = winrt::get_self<implementation::SelectionColor>(fg);
		auto pBackground = winrt::get_self<implementation::SelectionColor>(bg);

Implement EnableColorSelection #13429

Implement EnableColorSelection #13429

Conversation

jazzdelightsme commented Jul 5, 2022

Summary of the Pull Request

Detailed Description of the Pull Request / Additional comments

PR Checklist

Validation Steps Performed

This comment was marked as resolved.

This comment was marked as resolved.

carlos-zamora left a comment • edited Loading

Choose a reason for hiding this comment

How localization works

Other feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Jul 30, 2022

jazzdelightsme commented Aug 1, 2022

jazzdelightsme commented Aug 4, 2022

jazzdelightsme commented Aug 4, 2022

lhecker commented Aug 17, 2022

lhecker commented Aug 17, 2022

DHowett commented Aug 19, 2022

carlos-zamora commented Aug 19, 2022

carlos-zamora commented Aug 19, 2022

DHowett left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

jazzdelightsme commented Aug 20, 2022

j4james commented Aug 20, 2022

lhecker commented Aug 20, 2022 • edited Loading

carlos-zamora commented Aug 26, 2022

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Aug 31, 2022

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.

ghost commented Sep 13, 2022

carlos-zamora left a comment •

edited

Loading

lhecker commented Aug 20, 2022 •

edited

Loading

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.