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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec: Appearance configuration objects for profiles #8345
Spec: Appearance configuration objects for profiles #8345
Changes from 13 commits
963e624
a24d166
d5f6ced
83b78ba
887af3b
7ddb322
5cf8216
0e7b578
6812b76
ad352df
df039d2
95ab38c
2f18add
7445282
8a62a44
5048f8f
df9627b
c50df8d
e00e259
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, when
"adminState"
gets added, we'll have a larger set of accepted parameters. For example...name
,tabTitle
,suppressApplicationTitle
--> what is my title?startingDirectory
andcommandline
--> startup-related settingsShould we just accept every profile setting, then silently ignore ones that don't apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that at least for now, these additional state configurations should be entirely appearance-based, so we don't accept things like
commandline
,startingDirectory
and probably nothing to do with the title either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so just to be sure, the ordering is
(where
profile.appearance
is just the appearance of the profile itself)and we just return whatever the first non-null one is? And if the
profile.unfocusedState
is{}
, we'll just useprofile.appearance
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though there is also the question of what we do about the undefined parameters within the unfocused config object, so I prefer to think of it as:
profile.unfocusedConfig
defined: undefined params taken fromprofile
(so the case whereprofile.unfocusedConfig
is{}
falls in here)profile.unfocusedConfig
not defined,profile.defaults.unfocusedConfig
defined: undefined params are default valuesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OKay I think case 2 is weird, but it's well defined at least. I guess I'd expect undefined params in that case to use the profile ones still, but I understand how (from a layering perspective), that's just impossible. I think it'd be weird to throw an unfocused config in the defaults...
okay though what if I wanted all my profiles to be gray when they're unfocused. I'd throw a
unfocusedConfig: { scheme: gray }
in the defaults, and be happy. But then the rest of the unfocused config is coming from the default settings, not the profile's settings.Like yea that makes sense, it's just weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would add a small comment somewhere explaining why we're doing this "all-or-nothing" approach as opposed to the "per-setting" approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but its a clean way given we don't want to define our own default
unfocusedConfig
and we don't want to mess up when nounfocusedConfig
is defined anywhere in the fileSounds good, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly. I guess a user could choose for themselves some unfocused states that would make the UI highly inaccessible to themselves (poor contrast ratio.)
What happens for high-contrast mode? Is the user allowed to still override high-contrast mode colors to the point where they might not be high-contrast anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user can override high-contrast mode colors already in their current profiles, then yes they can do so in the unfocused appearance as well. I.e. this change doesn't provide additional functionality with regards to a specific appearance, it just gives users a second appearance for their profiles, so any accessibility concerns with regards to users messing around with their unfocused appearance already exist because they could do the same with the one appearance they have currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a issue laying around about "make all the settings properties getters-only", and that touches on this subject. The settings should really only be read-only, but TermControl is kinda also using them as the bucket for it's runtime appearance as well. So it needs to kinda be applied as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, you're saying that if we get an OSC sequence to change some appearance value, we should apply it to the unfocused appearance if the control is not focused when the OSC is processed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh derp. No, sorry for confusing you. I meant
appearance = focused? default + runtime changes : unfocused
So I guess I'm imagining that if someone changes the BG color with an OSC< that only changes the focused BG color, never the unfocused one.
Plus, this way, when the settings reload, the runtime changes would still persist. This would apply for things like changing the font size too (though that's not as relevant for this spec)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll add that in!