Add one half color schemes #466
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| oneHalfDarkTable[11] = RGB(229, 192, 123); // yellow | ||
| oneHalfDarkTable[12] = RGB( 97, 175, 239); // blue | ||
| oneHalfDarkTable[13] = RGB(198, 120, 221); // magenta | ||
| oneHalfDarkTable[14] = RGB( 86, 182, 194); // cyan |
There was a problem hiding this comment.
I know this is probably in the original scheme code, but would you mind replacing these tabs with spaces (here and below?)
There was a problem hiding this comment.
Yeah, I'll make that change.
| @@ -358,7 +414,7 @@ std::wstring CascadiaSettings::ExpandEnvironmentVariableString(std::wstring_view | |||
| do | |||
| { | |||
| result.resize(requiredSize); | |||
| requiredSize = ::ExpandEnvironmentStringsW(source.data(), result.data(), result.size()); | |||
| requiredSize = ::ExpandEnvironmentStringsW(source.data(), result.data(), static_cast<DWORD>(result.size())); | |||
There was a problem hiding this comment.
Should this be a static_cast or a gsl::narrow? One throws if it doesn't fit; perhaps that's behaviour we want?
There was a problem hiding this comment.
static_cast<> is only a compile time check - not runtime. If we want a runtime error, then gsl_narrow(). Although given the size of a DWORD (32-bit unsigned) that should not be possible given that the max length of an env var value is ~32K. Perhaps we should use gsl_narrow_cast() instead?
gsl::narrow_cast clearly states that conversion can lose data and it is acceptable
|
@DHowett-MSFT Any thoughts on whether I should follow the ColorTool One Half Light scheme "bright" colors which differ from the normal colors? This seems to be a bit different than the schemes I've seen online for One Half Light where they just give a single set of "normal" colors. |
|
I'm close to having this ready to be merged (pending PR approval) but I've made some changes and darned if I can't the terminal to save out the new settings. It always seems to find the first one I've customized even though I've uinstalled, deleted the WindowsTerminal package dir and gone into settings after a reinstall and Reset the app data. How the heck is it finding my original profiles.json file? |
Added in customized colortool scheme colors for last 8 colors
rkeithhill
force-pushed
the
add-one-half-color-schemes
branch
from
2fa4c23 to
dd628a3
Compare
rkeithhill
changed the title
|
Figured out the saved state issue. This PR is ready to merge. |
There was a problem hiding this comment.
Looks like you've resolved all open issues and I don't see anything I'm personally concerned with.
In lieu of dragging @DHowett-MSFT off his current task, I'll approve and merge in his stead.
|
@miniksa Thanks! |
|
Just a heads up, I pulled this down and if there was an existing |
|
If we need a migration for this sort of thing, then please open a new issue. I'm not inclined to give that too much weight at this point, however, because this is an alpha product and blowing away and recreating your config at this early stage should be somewhat expected. |
|
Agreed, just calling it out here since there was specifically a comment about it being a problem and then being resolved and I don't believe it was. |
|
This will be helped by #754 :) |
|
Alright, good enough then. |
|
@amweiss Yeah, that behavior made it a bit hard to test this PR during development. :-) |
* Add One Half Dark & Light color schemes * WIP: Add One Half Dark/Lite schemes to settings * Address PR feedback - use gsl::narrow() * Fix reversed OneHalfLight fg/bg colors Added in customized colortool scheme colors for last 8 colors
This is close I think but what I see in the One Half Light theme as described here duplicates the dark colors for the light colors. On Windows w/colortool, I see that duplication for One Half Dark but not One Half Light. Should I emulate what colortool is using for the schemes? Also there are some "off-by-one" issues with the colortool scheme RGB values - at least as compared to the link above. Not sure which I should follow.