Conversation
This comment was marked as resolved.
This comment was marked as resolved.
c1ffaa4 to
85f036c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c8b645c to
3f3d104
Compare
# Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp
# Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp
There was a problem hiding this comment.
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.
- key: an ID for the string (i.e.
- 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
Other feedback
- make sure to update the schema
- Idea: what if we got rid of
MatchModealtogether and just introduced a separate key binding? So we could havecolorSelectionandcolorAllMatchingText? Then when we want to add the regex, we can add that tocolorAllMatchingText?
| 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; |
There was a problem hiding this comment.
With regards to localization, we should use these resources:
terminal/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Lines 138 to 217 in 2f58711
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.
There was a problem hiding this comment.
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.
| #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) |
There was a problem hiding this comment.
Can we make this an enum instead of an int?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
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... :(
| fgStr = hasForeground ? _FormatColorString(fgBuf, Foreground().TextColor()) : L"(default)"; | ||
| bgStr = hasBackground ? _FormatColorString(bgBuf, Background().TextColor()) : L"(default)"; |
There was a problem hiding this comment.
- what do you mean by
(default)? - we should localize "default" if we're keeping it
There was a problem hiding this comment.
"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?
| // This will throw for something like "j0", but return 0 for something like | ||
| // "0j". |
There was a problem hiding this comment.
I forget. If this throws, what happens? Does the deserialization fail and the action is ignored?
There was a problem hiding this comment.
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 ".
| } | ||
| else if (matchMode == 1) | ||
| { | ||
| auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end); |
There was a problem hiding this comment.
| auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end); | |
| const auto text = _activeBuffer().GetPlainText(/*trimTrailingWhitespace*/ IsBlockSelection(), start, end); |
| { | ||
| if (matchMode == 0) | ||
| { | ||
| auto length = _activeBuffer().SpanLength(start, end); |
There was a problem hiding this comment.
| 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); |
There was a problem hiding this comment.
| 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); |
| // 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(); |
There was a problem hiding this comment.
Would TriggerSelection() work? We're only modifying the selected area.
Also what do you mean by the comment?
There was a problem hiding this 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.
src/buffer/out/textBuffer.cpp
Outdated
| // - 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 |
There was a problem hiding this comment.
nit: reword this. What do those coords represent? I think you're basically giving start/end for ranges/spans of text on the buffer?
|
@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. |
3f3d104 to
9971364
Compare
|
This was very helpful, thank you! In reply to: 1039294143 |
|
Okay @lhecker, I have an area that could use some expert help: @carlos-zamora requested that the 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 |
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?
d3f8162 to
b5a59c0
Compare
|
@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 |
323f646 to
3320c1a
Compare
|
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 " 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! 😊 |
DHowett
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
@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 |
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 But for the majority of the users, I think something like
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 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 (Unrelated to this PR...) |
|
Note from bug bash:
|
DHowett
left a comment
There was a problem hiding this comment.
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?
|
Hello @DHowett! Because this pull request has the 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 (
|
|
🎉 Handy links: |
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:
E_NOTIMPL, but is now wired up. Don't know what/if anything else uses this.boolin your settings JSON, to light up all the keybindings you would expect from the legacy "EnableColorSelection" feature:*!*But they work if you do them from the command palette.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.Xs red if you like."Soft" spots:
PR Checklist
EnableColorSelectionin Terminal #9583Validation Steps Performed
Just manual testing.