-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 for "matching" profiles in "New Tab Customization" #12584
Changes from 3 commits
5a1fe10
55dcf14
7f84a18
fa5ad0b
cd29610
26c3248
5bc2a90
b203c52
cd313e1
539edf8
2f2f329
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||||
--- | ||||||||
author: Mike Griese @zadjii-msft | ||||||||
created on: 2020-5-13 | ||||||||
last updated: 2022-25-02 | ||||||||
last updated: 2022-12-09 | ||||||||
issue id: 1571 | ||||||||
--- | ||||||||
|
||||||||
|
@@ -76,11 +76,20 @@ There are five `type`s of objects in this menu: | |||||||
- The `"entries"` property specifies a list of menu entries that will appear | ||||||||
nested under this entry. This can contain other `"type":"folder"` groups as | ||||||||
well! | ||||||||
- The `"expand"` property accepts two values | ||||||||
- `auto`: When the folder only has one entry in it, don't actually create a | ||||||||
nested layer to then menu. Just place the single entry in the layer that | ||||||||
folder would occupy. (Useful for dynamic profile sources with only a | ||||||||
single entry). | ||||||||
- `always`: (**default**) Always create a nested entry, even for a single | ||||||||
sub-item. | ||||||||
- The `allowEmpty` property will force this entry to show up in the menu, even | ||||||||
if it doesn't have any profiles in it. This defaults to `false`, meaning | ||||||||
that folders without any entries in them will just be ignored when | ||||||||
generating the menu. This will be more useful with the `matchProfile` entry, | ||||||||
below. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to specify how these two interact? For n>=2 and n=1 it's clear; but for n=0 would there be conflicting behaviour here? I don't think so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good point. I totally forgot about it. Huh. I'll add some commentary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to show a placeholder entry in the dropdown indicating that the folder is empty? Otherwise currently the dropdown is just a small rectangle that can easily be missed - we wouldn't want redundant bug reports since the menu is empty. |
||||||||
- _This setting is probably pretty niche, and not a requirement_. More of a | ||||||||
theoretical suggestion than anything. | ||||||||
* `"type":"action"`: This represents a menu entry that should execute a specific | ||||||||
`ShortcutAction`. | ||||||||
- the `id` property will specify the global action ID (see [#6899], [#7175]) | ||||||||
|
@@ -90,12 +99,6 @@ There are five `type`s of objects in this menu: | |||||||
either provided as the `"name"` in the global list of actions, or the | ||||||||
generated name if no `name` was provided) | ||||||||
- The icon for this entry will similarly re-use the action's `icon`. | ||||||||
* `"type": "matchProfile"`: Expands to all the profiles that match a given | ||||||||
regex. This lets the user easily specify a whole collection of profiles for a | ||||||||
folder, without needing to add them all manually. | ||||||||
- `"on"`: One of `"name"`, `"commandline"` or `"source"`, used to identify the | ||||||||
property of the profile to match against. | ||||||||
- `"match"`: a regex to string compare the given `on` with. | ||||||||
* `"type":"remainingProfiles"`: This is a special type of entry that will be | ||||||||
expanded to contain one `"type":"profile"` entry for every profile that was | ||||||||
not already listed in the menu. This will allow users to add one entry for | ||||||||
|
@@ -108,7 +111,15 @@ There are five `type`s of objects in this menu: | |||||||
enabling all other profiles to also be accessible. | ||||||||
- The "name" of these entries will simply be the name of the profile | ||||||||
- The "icon" of these entries will simply be the profile's icon | ||||||||
- This won't include any profiles that have been included via `match` entries. | ||||||||
- This won't include any profiles that have been included via `matchProfile` | ||||||||
entries (below) | ||||||||
* `"type": "matchProfile"`: Expands to all the profiles that match a given | ||||||||
regex, which haven't already been included. This lets the user easily specify | ||||||||
a whole collection of profiles for a folder, without needing to add them all | ||||||||
manually. | ||||||||
- `"name"`, `"commandline"` or `"source"`: These three properties are used to | ||||||||
filter the list of profiles, based on the matching property in the profile | ||||||||
itself. The value is a regex to string compare the profile's value with. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to allow simple (partial) string matching in addition to regexes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh. #12584 (comment) raises a good point. I figured that regex would just always be easier, but I forgot that matching on the I dunno, I'm on a real regex kick as of late, so I think I'm personally leaning towards regex always, but this is a point of contention that I'll let the team veto me on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well just make "source" an array and match the whole string. This should work practically just as well as regexes would, since the amount of distinct "source"s is fairly limited (they aren't really auto-generated after all) and would simplify diagnostics like "oh you specified the same source twice but they're supposed to be unique". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meh, I know that I'm using a weird personal edge case here but what about the following? I've got a bunch of profiles with commandline's like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after realizing https://github.com/microsoft/terminal/pull/12584/files#r1009785219, I'm inclined to leave this as-is |
||||||||
|
||||||||
The "default" new tab menu could be imagined as the following blob of json: | ||||||||
|
||||||||
|
@@ -129,16 +140,16 @@ nested entries for each subsequent dynamic profile generator. | |||||||
"newTabMenu": [ | ||||||||
{ "type":"profile", "profile": "cmd" }, | ||||||||
{ "type":"profile", "profile": "Windows PowerShell" }, | ||||||||
{ "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.PowerShellCore" } | ||||||||
{ "type": "matchProfile", "source": "Microsoft.Terminal.PowerShellCore" } | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's a regex, I would say
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ironically, this would still work without escaping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'VE BEEN THINKING ABOUT THAT FOR LIKE ALL OF LEAVE lol. We can totally just leave it as a regex that just so happens to work |
||||||||
{ | ||||||||
"type": "folder", | ||||||||
"name": "WSL", | ||||||||
"entries": [ { "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.Wsl" } ] | ||||||||
"entries": [ { "type": "matchProfile", "source": "Microsoft.Terminal.Wsl" } ] | ||||||||
}, | ||||||||
{ | ||||||||
"type": "folder", | ||||||||
"name": "Visual Studio", | ||||||||
"entries": [ { "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.VisualStudio" } ] | ||||||||
"entries": [ { "type": "matchProfile", "source": "Microsoft.Terminal.VisualStudio" } ] | ||||||||
}, | ||||||||
// ... etc for other profile generators | ||||||||
{ "type": "remainingProfiles" } | ||||||||
|
@@ -152,7 +163,7 @@ moving the user's cheese too much, if they're already using the Terminal and | |||||||
happy with their list as is. Especially consider someone who's default profile | ||||||||
is a WSL distro, which would now need two clicks to get to. | ||||||||
|
||||||||
> _note_: We will also want to support the same `{ "key": "SomeResoureString"}` | ||||||||
> _note_: We will also want to support the same `{ "key": "SomeResourceString"}` | ||||||||
> syntax used by the Command Palette commands | ||||||||
> for specifying localizable names, if we chose to pursue this route. | ||||||||
|
||||||||
|
@@ -202,6 +213,13 @@ The design chosen in this spec more cleanly separates the responsibilities of | |||||||
the list of profiles and the contents of the new tab menu. This way, each object | ||||||||
can be defined independent of the structure of the other. | ||||||||
|
||||||||
Regarding implementation of `matchProfile` entries: In order to build the menu, | ||||||||
we'll evaluate the entries in the following order: | ||||||||
|
||||||||
* all explicit `profile` entries | ||||||||
* then all `matchProfile` entries, using profiles not already specified | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also specify the order of evaluation of these entries, otherwise the behaviour of Or do we want to add a flag toggling the "remaining" feature? So we can have this entry act both as "searching all that match unconditionally", or "search all that match that have not yet been specified elsewhere". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm all for giving people more settings than they could all possibly use. At this point I think I'd love to chase down a sensible default first, in the case we never get around to the setting to toggle it. Maybe it should be a property after all? Maybe default to false, cause true seems harder to implement. When it's true, we'd do a DFS as we evaluate the rest of the (idle thought: we may want to consider the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm really in doubt whether And then, since indeed remainingProfiles is not "unique", we might just as well implement this flag immediately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think this should be clarified in the latest version |
||||||||
* then expand out `remainingProfiles` with anything not found above. | ||||||||
|
||||||||
## UI/UX Design | ||||||||
|
||||||||
See the above _figure 1_. | ||||||||
|
@@ -340,7 +358,7 @@ And assuming the user has bound: | |||||||
|
||||||||
## Updates | ||||||||
|
||||||||
_Feburary 2022_: Doc updated in response to some discussion in [#11326] and | ||||||||
_February 2022_: Doc updated in response to some discussion in [#11326] and | ||||||||
[#7774]. In those PRs, it became clear that there needs to be a simple way of | ||||||||
collecting up a whole group of profiles automatically for sorting in these | ||||||||
menus. Although discussion centered on how hard it would be for extensions to | ||||||||
|
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 love this idea. However: I read it the other way around. My brain wants to turn
expand: always
into "100% of the time, expand this folder into its parent". That is, it will insert 1..N profiles into the parent menu. Which sounds just like not having a folder at all.We may want to rename it to
inline
ordisplayInline
(always
becomesnever
) or swing the other direction and call itdisplayAsSubmenu
(very explicit,auto
andalways
still make sense)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 like this