Skip to content
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

Merged
merged 11 commits into from
Nov 18, 2022
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: 2020-08-04
last updated: 2022-11-01
issue id: 1571
---

Expand Down Expand Up @@ -76,6 +76,34 @@ 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
Copy link
Member

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 or displayInline (always becomes never) or swing the other direction and call it displayAsSubmenu (very explicit, auto and always still make sense)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this

- `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.
Comment on lines +89 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get why this is useful in combination with matchProfile. allowEmpty feels rather niche to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only scenario I can think of where this is useful is if you want to ensure that the indices don't change (i.e. newTab; index=3 when the third item is a folder that may be empty; allowEmpty=false would ensure that 3 doesn't open a profile).

That said, I feel like you generally mess with your settings in one sitting then don't touch them for a while. So, I agree with Leonard here, feels pretty niche and could probably be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagined this as

  • being useful for dynamic profiles, and shared settings files across machines. That would make the WSL entry still show up (with an entry) even when you're on a machine with no WSLs installed yet
  • being a totally optional follow-up task. P3, unimportant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the allowEmpty does indeed have some merit, not only for WSL but also for other generated profiles.


When this is true, and the folder is empty, we should add a
placeholder `<empty>` entry to the menu, to indicate that no profiles were
in that folder.
- _This setting is probably pretty niche, and not a requirement_. More of a
theoretical suggestion than anything.
- In the case of no entries for this folder, we should make sure to also
reflect the `expand` property:
- `allowEmpty:true`, `expand:auto`: just ignore the entry at all. Don't
add a placeholder to the parent list.
- `allowEmpty:true`, `expand:always`: Add a nested entry, with an
`<empty>` placeholder.
- `allowEmpty:false`, `expand:auto`: just ignore the entry at all. Don't
add a placeholder to the parent list.
- `allowEmpty:false`, `expand:always`: just ignore the entry at all. Don't
add a placeholder to the parent list.
* `"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])
Expand All @@ -97,6 +125,16 @@ 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 `matchProfile`
entries (below)
* `"type": "matchProfile"`: Expands to all the profiles that match a given
string. 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 string to compare with the corresponding property in
the profile. A full string comparison is done - not a regex or partial
string match.

The "default" new tab menu could be imagined as the following blob of json:

Expand All @@ -108,6 +146,42 @@ The "default" new tab menu could be imagined as the following blob of json:
}
```

Alternatively, we could consider something like the following. This would place
CMD, PowerShell, and all PowerShell cores in the root at the top, followed by
nested entries for each subsequent dynamic profile generator.

```jsonc
{
"newTabMenu": [
{ "type":"profile", "profile": "cmd" },
{ "type":"profile", "profile": "Windows PowerShell" },
{ "type": "matchProfile", "source": "Microsoft.Terminal.PowerShellCore" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a regex, I would say

Suggested change
{ "type": "matchProfile", "source": "Microsoft.Terminal.PowerShellCore" }
{ "type": "matchProfile", "source": "Microsoft\.Terminal\.PowerShellCore" }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ironically, this would still work without escaping the . haha. That's good though, because an unaware user should still get the expected result.

Copy link
Member Author

Choose a reason for hiding this comment

The 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", "source": "Microsoft.Terminal.Wsl" } ]
},
{
"type": "folder",
"name": "Visual Studio",
"entries": [ { "type": "matchProfile", "source": "Microsoft.Terminal.VisualStudio" } ]
},
// ... etc for other profile generators
{ "type": "remainingProfiles" }
]
}
```

I might only recommend that for `userDefaults.json`, which is the json files
used as a template for a user's new settings file. This would prevent us from
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": "SomeResourceString"}`
> syntax used by the Command Palette commands
> for specifying localizable names, if we chose to pursue this route.

### Other considerations

Also considered during the investigation for this feature was re-using the list
Expand Down Expand Up @@ -154,6 +228,42 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 matchProfile: name: .* in the main menu and matchProfile: source: Wsl in a folder would not be defined. (I think the only two options are depth-first or breadth-first of which I think depth-first is the optimal implementation logic)

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".

Copy link
Member Author

Choose a reason for hiding this comment

The 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 matchProfile and remainingProfiles entries.

(idle thought: we may want to consider the fact that matchProfile(name:".*", remaningOnly:true) === remainingProfiles)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really in doubt whether remainingOnly: true is a sensible default at all. Intuitively, matchProfile should always return all matches and should not differ depending on where I put the entry in my menu.

And then, since indeed remainingProfiles is not "unique", we might just as well implement this flag immediately.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

As an example:

```jsonc
{
"newTabMenu": [
{ "type": "matchProfile", "source": "Microsoft.Terminal.Wsl" }
{
"type": "folder",
"name": "WSLs",
"entries": [ { "type": "matchProfile", "source": "Microsoft.Terminal.Wsl" } ]
},
{ "type": "remainingProfiles" }
]
}
```

For profiles { "Profile A", "Profile B (WSL)", "Profile C (WSL)" }, This would
expand to:

```
New Tab Button ▽
├─ Profile A
├─ Profile B (WSL)
├─ Profile C (WSL)
└─ WSLs
└─ Profile B (WSL)
└─ Profile C (WSL)
```

## UI/UX Design

See the above _figure 1_.
Expand Down Expand Up @@ -289,7 +399,39 @@ And assuming the user has bound:
- Close Tab: `{ "action": "closeTab", "index": "${selectedTab.index}" }`
- Close Other Tabs: `{ "action": "closeTabs", "otherThan": "${selectedTab.index}" }`
- Close Tabs to the Right: `{ "action": "closeTabs", "after": "${selectedTab.index}" }`

* We may want to consider regex, tag-based, or some other type of matching for
`matchProfile` entries in the future. We originally considered using regex for
`matchProfile` by default, but decided instead on full string matches to leave
room for regex matching in the future. Should we chose to pursue something
like that, we should use a settings structure like:

```json
"type": "profileMatch",
"source": { "type": "regex", "value": ".*wsl.*" }
```
* We may want to expand `matchProfile` to match on other properties too. (`title`?)
* We may want to consider something like
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
```json
{
"type": "profileMatch",
"name": { "type": "regex", "value": "ssh: (.*)" }
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}
```
for matching to all your `ssh: ` profiles, but populate the name in the entry
with that first capture group. So, ["ssh: foo", "ssh: bar"] would just expand
to a "foo" and "bar" entry.

## Updates

_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
provide that customization themselves, the `match` statement was added as a way
to allow the user to easily filter those profiles themselves.

This was something we had originally considered as a "future consideration", but
ultimately deemed it to be out of scope for the initial spec review.

<!-- Footnotes -->
[#2046]: https://github.com/microsoft/terminal/issues/2046
Expand All @@ -298,3 +440,5 @@ And assuming the user has bound:
[#3337]: https://github.com/microsoft/terminal/issues/3337
[#6899]: https://github.com/microsoft/terminal/issues/6899
[#7175]: https://github.com/microsoft/terminal/issues/7175
[#11326]: https://github.com/microsoft/terminal/issues/11326
[#7774]: https://github.com/microsoft/terminal/issues/7774