Deduplicate identical inbox color schemes to heal user settings#12800
Deduplicate identical inbox color schemes to heal user settings#12800
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
| { | ||
| const auto scheme{ winrt::get_self<ColorScheme>(entry.Value()) }; | ||
| schemes.append(scheme->ToJson()); | ||
| if (scheme->Origin() == OriginTag::User) |
There was a problem hiding this comment.
is this bodgy i can't tell
|
I'll include this in Leonard's new "Fix User Settings" code :) |
DHowett
force-pushed
the
dev/duhowett/fhl-202203/color-scheme-healing
branch
from
073540b to
a31b089
Compare
This comment has been minimized.
This comment has been minimized.
…03/color-scheme-healing
This comment has been minimized.
This comment has been minimized.
…03/color-scheme-healing
|
Notes from team sync today (post hackathon)
|
This comment has been minimized.
This comment has been minimized.
…03/color-scheme-healing
…03/color-scheme-healing
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
|
I am noodling on what the UI looks like when you try to edit one of these. We don't need to fun disclaimer text any more (woo), but we do need a call to action.
|
|
I'm gonna say |
. I'd maybe make it an InfoBar but that's basically the same UX
|
Okay, this works pretty well. |
This comment has been minimized.
This comment has been minimized.
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
lhecker
left a comment
lhecker
left a comment
There was a problem hiding this comment.
The fmt::format calls throughout the PR could use FMT_COMPILE IMO.
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
|
The one risk I am aware of is that we will delete user color schemes that ONLY change the cursor or selection colors. I can add checks to avoid this, but the risk seems fairly low overall. |
|
I think that's an acceptable risk. |
|
@lhecker any blockers left? |
|
Body updated |
|
I still don't quite understand how |
|
So! Because This line in CascadiaSettingsSerialization ensures that we write out the settings if we renamed a user color scheme. Because renaming adds a remapping entry, the condition will always reflect whether there have been any renames. The one case this does not cover is when a user had an identical color scheme which we deleted, but no other settings changes. Honestly, I don't mind that terribly. If they go save their settings again it will disappear, and it is not harming anyone much by being there. I can plumb through a "made changes" flag if we want to force a flush to disk when we delete schemes. |
DHowett
changed the title
|
|
||
| using namespace Microsoft::Console; | ||
| using namespace winrt::Microsoft::Terminal; | ||
| using namespace winrt::Microsoft::Terminal::Settings; |
5 participants
Up until now, we have treated inbox, fragment and user color schemes the
same: we load them all into one big map and when we save the settings
file we write them all out. It's been a big annoyance pretty much
forever.
In addition to cluttering the user's settings file, it prevents us from
making changes to the stock color schemes (like to change the cursor
color, or to adjust the colors in Tango Dark, or what have you) because
they're already copied in full in the user settings. It also means that
we need some special UI affordances for color schemes that you are
allowed to view but not to delete or rename.
We also have a funny hardcoded list of color scheme names and we use
that to determine whether they're "inbox" for UI purposes.
Because of all that, we are hesitant to add more color schemes to the
default set.
This pull request resolves all of those issues at once.
It:
(Inbox, Fragment, User, ...)
has a "duplicate this color scheme to start editing it" button
this allows us to finally disentangle the user's preferences from the
terminal's.
modified (even implicitly!) to their modified versions.
The equivalence check intentionally leaves out the cursor and selection
colors, so that we have the freedom to change them in the future.
The Origin is part of a new interface,
ISettingsModelObject, which wecan use in the future for things like Themes and Actions.