Add tooltip text to Color Buttons #6498
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a fast path to `til::bitmap::translate`: use bit shifts when the delta is vertical. Performance while printing the content of a big file, with the patch from microsoft#6492 which hasn't been merged yet, in Release mode: Before: * translate represents 13.08% of samples in InvalidateScroll After: * translate represents 0.32% of samples in InvalidateScroll ## Validation Tests passed.
"While re-reading the code, I found out that I forgot to do clear cached runs after translate_y in c360b75."
zadjii-msft
requested changes
Jun 15, 2020
There was a problem hiding this comment.
I love this, but I think we need to have the names be defined in our Resources.resw to make sure they're localizable.
Check out how it's done for the min/max/close buttons:
terminal/src/cascadia/TerminalApp/MinMaxCloseControl.xaml
Lines 186 to 191 in c0ffc9b
terminal/src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Lines 284 to 286 in c0ffc9b
Add a unique x:Uid for each color, then add a ColorUid.[using:Windows.UI.Xaml.Automation]AutomationProperties.Name entry to the Resources file
ghost
added
the
Needs-Author-Feedback
zadjii-msft
added
Area-User Interface
) This commit reverts the removal of the "SSH hack" in microsoft#5383. It was originally added as a solution to microsoft#4911, when we realized that SSH would request the SS3 cursor key encoding but we weren't equipped to handle it. A number of folks have filed issues that, in summary, say "when I use SSH, I can't select/copy/paste text". It turns out that SSH will _also_ pass through requests for mouse input. Terminal dutifully responds to those requests, of course, by disabling mouse selection/copy/paste. SSH is **NOT** actually in VT_INPUT_MODE, so it will never receive the mouse messages. It's important to note that even with microsoft#376 fixed, we are still required to keep this check. With the closure of microsoft#376, we'll be able to convert VT mouse input back into Win32 mouse input for Win32 applications . . . but SSH also doesn't know how to handle Win32 mouse input. Fixes microsoft#6476. Fixes microsoft#6196. Fixes microsoft#5704. Fixes microsoft#5608.
## Summary of the Pull Request Pulls the `ActionAndArgs` deserializing into its own class, separate from `AppKeyBindings`. Some 2.0 features are going to need to re-use these actions in their json, so we'll want one unified way of deserializing them. ## References * Done primarily as part of the work on microsoft#2046/microsoft#5400/microsoft#5674 * Also related: microsoft#1571/microsoft#5888 * Will aggressively conflict with any open PRs that introduced keybindings (looking at microsoft#6299) ## PR Checklist * [x] Closes nothing, this is code refactoring * [x] I work here * [x] Current tests passed * [n/a] Requires documentation to be updated
|
If you need help, ask me. It took me a while to figure out the resource thing too. |
## Summary of the Pull Request Make sure to set the scancode for the manual alt-up's we're sending. If you don't, then terminalInput in the conpty is going to treat that keypress as an actual NUL, and send that to the connected client. ## References * regressed in microsoft#6421 ## PR Checklist * [x] Closes microsoft#6513 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Tested `showkeys -a`
## Summary of the Pull Request Prior to microsoft#6309, we'd only snap on input for non-modifier key_down_ events. microsoft#6423 fixed this for modifier keys, but didn't fix this for keyups. ## References * microsoft#6423 was an incomplete fix to this problem, which caused this regression ## PR Checklist * [x] Closes microsoft#6481 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated
ghost
removed
the
Needs-Author-Feedback
zadjii-msft
approved these changes
Jun 16, 2020
Chips1234
approved these changes
Jun 16, 2020
| @@ -24,82 +24,82 @@ | |||
| <Setter Property="Margin" Value="2"/> | |||
| </Style> | |||
| </VariableSizedWrapGrid.Resources> | |||
| <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson"> | |||
| <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson" ToolTipService.ToolTip="Crimson"> | |||
There was a problem hiding this comment.
Add a line, with name CrimsonTooltip in Resources.Resw and Value as Crimson so that it can be localized into languages. (If you use VS, and open Resources, there is a nice table)
Suggested change
| <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson" ToolTipService.ToolTip="Crimson"> | |
| <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson" ToolTipService.ToolTip=RS_(L"CrimsonTooltip")> |
There was a problem hiding this comment.
Hi @Chips1234
I'm confused about your suggestion, Do I need to add a name like ToolTipService.ToolTip=RS_(L"CrimsonTooltip") or x:Uid="CrimsonColorButton" ?
Thanks
There was a problem hiding this comment.
@garciaolais I believe it all works.
There was a problem hiding this comment.
I'd be shocked if ToolTipService.ToolTip=RS_(L"CrimsonTooltip") compiled in XAML like that.
There was a problem hiding this comment.
This is weird. @Chips1234 I think you made the review on the old code, the commit 051d2fc already fixed the localization, no?
|
Oh sorry! I was confused. In XAML, you have to use “x:uid”
________________________________
From: greg904 <[email protected]>
Sent: Tuesday, June 16, 2020 12:29:54 PM
To: microsoft/terminal <[email protected]>
Cc: Eric Tian <[email protected]>; Mention <[email protected]>
Subject: Re: [microsoft/terminal] Add tooltip text to Color Buttons (#6498)
@greg904 commented on this pull request.
________________________________
In src/cascadia/TerminalApp/ColorPickupFlyout.xaml<#6498 (comment)>:
@@ -24,82 +24,82 @@
<Setter Property="Margin" Value="2"/>
</Style>
</VariableSizedWrapGrid.Resources>
- <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson">
+ <Button Click="ColorButton_Click" AutomationProperties.Name="Crimson" ToolTipService.ToolTip="Crimson">
This is weird. @Chips1234<https://github.com/Chips1234> I think you made the review on the old code, the commit 051d2fc<051d2fc> already fixed the localization, no?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#6498 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALKX3RTWYAP35HKSRAH4AS3RW7B3FANCNFSM4N5H6SXQ>.
|
Nobody was using it. Discussed in microsoft#2661.
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request This is the spec for the Advanced Tab Switcher. This would allow the user to navigate through a vertical list of tabs through a UI, similar to those found in VSCode and Visual Studio. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References microsoft#1502: Feature Request: Advanced Tab Switcher microsoft#973: Ctrl+Tab toggling between two tabs <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Spec for microsoft#1502 * [x] CLA signed.
WSLUser
reviewed
Jun 17, 2020
| <value>Steel Blue</value> | ||
| </data> | ||
| <data name="MediumSeaGreenColorButton.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve"> | ||
| <value>Medium Sea Green</value> |
There was a problem hiding this comment.
This is a dupe of the one above
Read the [JsonUtils Spec] for more details. This pull request introduces the next version of JsonUtils. It is in a separate file for ease of review and testing. JsonUtilsNew will be renamed in a subsequent commit that rewrites our JSON deserializers. ### Implementer's Notes I went with telescoping exceptions for the key parsing code, because it's totally possible that you can be five keys deep and encounter a type error. This lets us encode information about all failures in the chain instead of just the topmost one. The original JsonUtilsNew code changed to use `decay` everywhere because the tests wouldn't compile. We want to treat `GetValue<const guid>` _the same as_ `GetValue<guid>`, and this lets us do so. `decay` is awesome. I've been developing this with a shim that redirects `JsonUtils.h` to `JsonUtilsNew.h`. I am not comfortable deleting the original until we've moved off of it, and that _will_ be the subject of a followup PR. ## Validation Steps Performed So many tests. [JsonUtils Spec]: https://github.com/microsoft/terminal/blob/master/doc/cascadia/Json-Utility-API.md Refs microsoft#2550
ghost
requested review from
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request When the user double clicks on a tab, show the tab rename box as if they right clicked on the tab and clicked on "Rename". <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes microsoft#6600 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] 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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments I added a handler for the `DoubleTapped` event on the tab view item when we are constructing it for the tab (in `Tab::_MakeTabViewItem`). The code for that handler was copied the "rename tab menu item" click handler. I did not extract the code into a member function because it is very short (only 2 lines of code) and only used twice so it is not worth it IMO. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
…es (microsoft#6532) ## Summary of the Pull Request This is another iteration on the Command Palette spec, from microsoft#5674. These were some ideas that were tossed around by @DHowett, @cinnamon-msft and myself, formalized here. I proposed this as an addendum to the original spec, since I think the first made sense atomically, and this only makes sense as a set of changes to the original. I didn't want to go hacking up the original doc to add this set of changes. **There are two proposals in this spec - they should be viewed as two atomic units. They can be accepted or rejected independently. I'm suggesting we approve both. They work _together_. I'm realizing now that this is worded confusingly, and it's on me to fix that.** ## PR Checklist * [x] Another spec in the microsoft#2046 / microsoft#5400 saga * [x] I work here * [x] _is a doc_ > ## Abstract > > This document is intended to serve as an addition to the [Command Palette Spec]. > While that spec is complete in it's own right, subsequent discussion revealed > additional ways to improve the functionality and usability of the command > palette. This document builds largely on the topics already introduced in the > original spec, so readers should first familiarize themselves with that > document. > > One point of note from the original document was that the original specification > was entirely too verbose when defining both keybindings and commands for > actions. Consider, for instance, a user that wants to bind the action "duplicate > the current pane". In that spec, they need to add both a keybinding and a > command: > > ```json > { > "keybindings": [ > { "keys": [ "ctrl+alt+t" ], "command": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" } }, > ], > "commands": [ > { "name": "Duplicate Pane", "action": { "action": "splitPane", "split":"auto", "splitMode": "duplicate" }, "icon": null }, > ] > } > ``` > > These two entries are practically the same, except for two key differentiators: > * the keybinding has a `keys` property, indicating which key chord activates the > action. > * The command has a `name` property, indicating what name to display for the > command in the Command Palette. > > What if the user didn't have to duplicate this action? What if the user could > just add this action once, in their `keybindings` or `commands`, and have it > work both as a keybinding AND a command? >
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Many places in this codebase has an equality comparison to the boolean FALSE. This adds unneeded complexity as C and C++ has a NOT operand for use of these in if statements. This makes the code more readable in those areas. <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [X] Tests added/passed * [ ] Requires documentation to be updated * [ ] 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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments One boolean being compared to FALSE was only used once, with the boolean name being "b", so it is better off not existing at all. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed Unit Testing passed, compiler refactoring
Replace std::map with std::unordered_map when the order doesn't matter and hash functions are provided. Simple optimizations, but I expect the performance should be strictly better, especially for CodepointWidthDetector.hpp.
…#6492) * Update _TerminalCursorPositionChanged to use ThrottledFunc. * Rename previous ThrottledFunc to ThrottledArgFunc because now ThrottledFunc is for functions that do not take an argument. * Update ThrottledFunc and ThrottledArgFunc to accept a CoreDispatcher on which the function should be called for convenience. * Don't use coroutines/winrt::fire_and_forget in ThrottledFunc/ThrottledArgFunc because they are too slow (see PR). _AdjustCursorPosition went from 17% of samples to 3% in performance testing.
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Add keybinding for renaming a tab <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [X] Fulfills format requirements set by microsoft#6567 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [X] Tests passed * [X] Requires documentation to be updated * [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: microsoft#6567 and here (microsoft#6557) This no longer c loses microsoft#6256, as the spec changed. <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
…oft#6665) This reverts commit 94eab6e. We'll reintroduce this again after making sure it plays nicely with recycling and box drawing glyphs. Fixes microsoft#6488 Fixes microsoft#6664
This pull request implements shift+double/triple click. Proper behavior (as described in microsoft#4557) is to only expand one selection point, not both. Adding the `bool targetStart` was a bit weird. I decided on this being the cleanest approach though because I still want `PivotSelection` to be its own helper function. Otherwise, the concept of "pivoting" gets kinda messy. ## Validation Steps Performed Manual testing as described on attached issue. Tests were added for Shift+Click and pivoting the selection too. Closes microsoft#4557
## Summary of the Pull Request Adds a pair of `ShortcutAction`s for setting the tab color. * `setTabColor`: This changes the color of the current tab to the provided color, or can be used to clear the color. * `openTabColorPicker`: This keybinding immediately activates the tab color picker for the currently focused tab. ## References ## PR Checklist * [x] scratches my own itch * [x] I work here * [x] Tests added/passed * [x] MicrosoftDocs/terminal#69 ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed * hey look there are tests * Tested with the following: ```json // { "command": "setTabColor", "keys": [ "alt+c" ] }, { "keys": "ctrl+alt+c", "command": { "action": "setTabColor", "color": "#123456" } }, { "keys": "alt+shift+c", "command": { "action": "setTabColor", "color": null} }, { "keys": "alt+c", "command": "openTabColorPicker" }, ```
…soft#6670) This workaround was suggested by @chingucoding in microsoft/microsoft-ui-xaml#2711 Fixes microsoft#6570 ## References microsoft/microsoft-ui-xaml#2711 microsoft#6570 ## PR Checklist * [x] Closes an issue * [x] CLA * [x] Tested * [x] Docs not required * [x] Schema not required
Occasionally, we get users with corrupt PATH environment variables: they can't lauch PowerShell, because for some reason it's dropped off their PATH. We also get users who have stray applications named `powershell.exe` just lying around in random system directories. We can combat both of these issues by simply hardcoding where we expect PowerShell and CMD to live. %SystemRoot% was chosen over %WINDIR% because apparently (according to Stack Overflow), SystemPath is read-only and WINDIR isn't. Refs microsoft#6039, microsoft#4390, microsoft#4228 (powershell was not found) Refs microsoft#4682, Fixes microsoft#6082 (stray powershell.exe)
## Summary of the Pull Request  This adds a first iteration on the command palette. Notable missing features are: * Commandline mode: This will be a follow-up PR, following the merge of microsoft#6537 * nested and iterable commands: These will additionally be a follow-up PR. This is also additionally based off the addenda in microsoft#6532. This does not bind a key for the palette by default. That will be done when the above follow-ups are completed. ## References * microsoft#2046 - The original command palette thread * microsoft#5400 - This is the megathread for all command palette issues, which is tracking a bunch of additional follow up work * microsoft#5674 and microsoft#6532 - specs * microsoft#6537 - related ## PR Checklist * [x] Closes microsoft#2046 - incidentally also closes microsoft#6645 * [x] I work here * [x] Tests added/passed * [ ] Requires documentation to be updated - delaying this until it's more polished. ## Detailed Description of the Pull Request / Additional comments * There's a lot of code for autogenerating command names. That's all in `ActionArgs.cpp`, because each case is so _not_ boilerplate, unlike the rest of the code in `ActionArgs.h`. ## Validation Steps Performed * I've been playing with this for months. * Tests * Selfhost with the team
Restores the simple text run analysis and skipping of most of the shaping/layout steps. Corrects one of the fast-path steps to ensure that offsets and clusters are assigned. ## References - Bug microsoft#6488 - Bug microsoft#6664 - Simple run PR microsoft#6206 - Simple run revert PR microsoft#6665 - Recycle glyph runs PR microsoft#6483 The "correction" functions, by which box drawing analysis is one of them, is dependent on the steps coming before it properly assigning the four main vectors of the text layout glyphs: indices, advances, offsets, and clusters. When the fast path is identified by the code from microsoft#6206, only two of those are fully updated: indices and advances. The offsets doesn't tend to cause a problem because offsets are rarely used so they're pretty much always 0 already (but this PR enforces that they're zero for the simple/fast path.) The clusters, however, were not mapped for the fast path. This showed itself in one of two ways: 1. Before the recycled runs PR microsoft#6483, the cluster map had a 0 in every field for the stock initialized vector. 2. After the recycled runs PR microsoft#6483, the cluster map had the previous run's mapping in it. This meant that when we reached the steps where glyph runs were potentially split during the correction phase for box drawing characters, unexpected values were present to map the glyph indices to clusters and were corrected, adjusted, or split in an unexpected fashion. For instance, the index out of range bug could appear when the default 0 values appended to the end of the clusters vector were decremented down to a negative value during the run splitter as the true DWrite cluster mapper doesn't generate that sort of pattern in the slow path case without also breaking the run itself. The resolution here is therefore to ensure that all of the fields related to glyph layout are populated even in the fast path. This doesn't affect the slow path because that one always populated all fields by asking DWrite to do it. The fast path just skips a bunch of DWrite steps because it can implicitly identify patterns and save a bunch of time. I've also identified a few vectors that weren't cleared on reset/reuse of the layout. I'm clearing those now so the `.resize()` operations performed on them to get to the correct lengths will fill them with fresh and empty values instead of hanging on to ones that may have been from the previous. This should be OK memory perf wise because the act of `.clear()` on a vector shouldn't free anything, just mark it invalid. And doing `.resize()` from an empty one should just default construct them into already allocated space (which ought to be super quick). ## Validation * [x] Far.exe doesn't crash and looks fine * [x] "\e[30;47m\u{2500} What \u{2500}\e[m" from microsoft#6488 appears appropriately antialiased * [x] Validate the "\e[30;47m\u{2500} What \u{2500}\e[m" still works when `FillGeometry` is nerfed as a quick test that the runs are split correctly. * [x] Put `u{fffd} into Powershell Core to make a replacement char in the output. Then press enter a few times and see that shrunken initial characters on random rows. Verify this is gone. Closes microsoft#6668 Closes microsoft#6669 Co-Authored-By: Chester Liu <[email protected]>
With this commit, terminal will be able to copy text to the system clipboard by using OSC 52 MANIPULATE SELECTION DAATA. We chose not to implement the clipboard querying functionality offered by OSC 52, as sending the clipboard text to an application without the user's knowledge or consent is an immense security hole. We do not currently support the clipboard specifier Pc to specify which clipboard buffer should be filled # Base64 encoded `foo` $ echo -en "\e]52;;Zm9v\a" # Multiple lines # Base64 encoded `foo\r\nbar` $ echo -en "\e]52;;Zm9vDQpiYXI=\a" Closes microsoft#2946.
…6698) Essentially what this does is map the default legacy foreground and background attributes (typically white on black) to the `IsDefault` color type in the `TextColor` class. As a result, we can now initialize the buffer for "legacy" shells (like PowerShell and cmd.exe) with default colors, instead of white on black. This fixes the startup rendering in conpty clients, which expect an initial default background color. It also makes these colors update appropriately when the default palette values change. One complication in getting this to work, is that the console permits users to change which color indices are designated as defaults, so we can't assume they'll always be white on black. This means that the legacy-to-`TextAttribute` conversion will need access to those default values. Unfortunately the defaults are stored in the conhost `Settings` class (the `_wFillAttribute` field), which isn't easily accessible to all the code that needs to construct a `TextAttribute` from a legacy value. The `OutputCellIterator` is particularly problematic, because some iterator types need to generate a new `TextAttribute` on every iteration. So after trying a couple of different approaches, I decided that the least worst option would be to add a pair of static properties for the legacy defaults in the `TextAttribute` class itself, then refresh those values from the `Settings` class whenever the defaults changed (this only happens on startup, or when the conhost _Properties_ dialog is edited). And once the `TextAttribute` class had access to those defaults, it was fairly easy to adapt the constructor to handle the conversion of default values to the `IsDefault` color type. I could also then simplify the `TextAttribute::GetLegacyAttributes` method which does the reverse mapping, and which previously required the default values to be passed in as a parameter VALIDATION I had to make one small change to the `TestRoundtripExhaustive` unit test which assumed that all legacy attributes would convert to legacy color types, which is no longer the case, but otherwise all the existing tests passed as is. I added a new unit test verifying that the default legacy attributes correctly mapped to default color types, and the default color types were mapped back to the correct legacy attributes. I've manually confirmed that this fixed the issue raised in microsoft#5952, namely that the conhost screen is cleared with the correct default colors, and also that it is correctly refreshed when changing the palette from the properties dialog. And I've combined this PR with microsoft#6506, and confirmed that the PowerShell and the cmd shell renderings in Windows Terminal are at least improved, if not always perfect. This is a prerequisite for PR microsoft#6506 Closes microsoft#5952
This PR reimplements the VT rendering engines to do a better job of preserving the original color types when propagating attributes over ConPTY. For the 16-color renderers it provides better support for default colors and improves the efficiency of the color narrowing conversions. It also fixes problems with the ordering of character renditions that could result in attributes being dropped. Originally the base renderer would calculate the RGB color values and legacy/extended attributes up front, passing that data on to the active engine's `UpdateDrawingBrushes` method. With this new implementation, the renderer now just passes through the original `TextAttribute` along with an `IRenderData` interface, and leaves it to the engines to extract the information they need. The GDI and DirectX engines now have to lookup the RGB colors themselves (via simple `IRenderData` calls), but have no need for the other attributes. The VT engines extract the information that they need from the `TextAttribute`, instead of having to reverse engineer it from `COLORREF`s. The process for the 256-color Xterm engine starts with a check for default colors. If both foreground and background are default, it outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to make sure any reset state is reapplied. With that out the way, the foreground and background are updated (if changed) in one of 4 ways. They can either be a default value (SGR 39 and 49), a 16-color index (using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value (both using SGR 38 and 48 sequences). Then once the colors are accounted for, there is a separate step that handles the character rendition attributes (bold, italics, underline, etc.) This step must come _after_ the color sequences, in case a SGR reset is required, which would otherwise have cleared any character rendition attributes if it came last (which is what happened in the original implementation). The process for the 16-color engines is a little different. The target client in this case (Windows telnet) is incapable of setting default colors individually, so we need to output an SGR 0 reset if _either_ color has changed to default. With that out the way, we use the `TextColor::GetLegacyIndex` method to obtain an approximate 16-color index for each color, and apply the bold attribute by brightening the foreground index (setting bit 8) if the color type permits that. However, since Windows telnet only supports the 8 basic ANSI colors, the best we can do for bright colors is to output an SGR 1 attribute to get a bright foreground. There is nothing we can do about a bright background, so after that we just have to drop the high bit from the colors. If the resulting index values have changed from what they were before, we then output ANSI 8-color SGR sequences to update them. As with the 256-color engine, there is also a final step to handle the character rendition attributes. But in this case, the only supported attributes are underline and reversed video. Since the VT engines no longer depend on the active color table and default color values, there was quite a lot of code that could now be removed. This included the `IDefaultColorProvider` interface and implementations, the `Find(Nearest)TableIndex` functions, and also the associated HLS conversion and difference calculations. VALIDATION Other than simple API parameter changes, the majority of updates required in the unit tests were to correct assumptions about the way the colors should be rendered, which were the source of the narrowing bugs this PR was trying to fix. Like passing white on black to the `UpdateDrawingBrushes` API, and expecting it to output the default `SGR 0` sequence, or passing an RGB color and expecting an indexed SGR sequence. In addition to that, I've added some VT renderer tests to make sure the rendition attributes (bold, underline, etc) are correctly retained when a default color update causes an `SGR 0` sequence to be generated (the source of bug microsoft#3076). And I've extended the VT renderer color tests (both 256-color and 16-color) to make sure we're covering all of the different color types (default, RGB, and both forms of indexed colors). I've also tried to manually verify that all of the test cases in the linked bug reports (and their associated duplicates) are now fixed when this PR is applied. Closes microsoft#2661 Closes microsoft#3076 Closes microsoft#3717 Closes microsoft#5384 Closes microsoft#5864 This is only a partial fix for microsoft#293, but I suspect the remaining cases are unfixable.
…t#6728) This is essentially a rewrite of the `TerminalDispatch::SetGraphicsRendition` method, bringing it into closer alignment with the `AdaptDispatch` implementation, simplifying the `ITerminalApi` interface, and making the code easier to extend. It adds support for a number of attributes which weren't previously implemented. REFERENCES * This is a mirror of the `AdaptDispatch` refactoring in PR microsoft#5758. * The closer alignment with `AdaptDispatch` is a small step towards solving issue microsoft#3849. * The newly supported attributes should help a little with issues microsoft#5461 (italics) and microsoft#6205 (strike-through). DETAILS I've literally copied and pasted the `SetGraphicsRendition` implementation from `AdaptDispatch` into `TerminalDispatch`, with only few minor changes: * The `SetTextAttribute` and `GetTextAttribute` calls are slightly different in the `TerminalDispatch` version, since they don't return a pointless `success` value, and in the case of the getter, the `TextAttribute` is returned directly instead of by reference. Ultimately I'd like to move the `AdaptDispatch` code towards that way of doing things too, but I'd like to deal with that later as part of a wider refactoring of the `ConGetSet` interface. * The `SetIndexedForeground256` and `SetIndexedBackground256` calls required the color indices to be remapped in the `AdaptDispatch` implementation, because the conhost color table is in a different order to the XTerm standard. `TerminalDispatch` doesn't have that problem, so doesn't require the mapping. * The index color constants used in the 16-color `SetIndexedForeground` and `SetIndexedBackground` calls are also slightly different for the same reason. VALIDATION I cherry-picked this code on top of the microsoft#6506 and microsoft#6698 PRs, since that's only way to really get the different color formats passed-through to the terminal. I then ran a bunch of manual tests with various color coverage scripts that I have, and confirmed that all the different color formats were being rendered as expected. Closes microsoft#6725
DHowett
approved these changes
Jul 1, 2020
…rciaolais/terminal into garciaolais-dev/garciaolais/color-tooltip
|
Pushed a merge. I'll set this to automatically merge :) |
DHowett
added
the
AutoMerge
|
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 (
|
@DHowett Thank you and thanks everyone! this made my day 😃 |
DHowett
pushed a commit
that referenced
this pull request
Jul 7, 2020
This commit adds tooltip text to every color button in the tab color picker. (cherry picked from commit 44e80d4)
|
🎉 Handy links: |
1 similar comment
|
🎉 Handy links: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds tooltip text to every color button in the tab color
picker.