Spec for "matching" profiles in "New Tab Customization" #12584

zadjii-msft · 2022-02-25T20:05:40Z

This is a fever dream I had in response to the GDK PR we just got. I had to write it down immediately. I'm in no rush to get this in.

References:

This is a fever dream I had in response to the GDK PR we just got. I had to write it down immediately. I'm in no rush to get this in. References: * #1571 * #11326 * #7774

lhecker · 2022-03-03T18:47:42Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

+    generating the menu. This will be more useful with the `matchProfile` entry,
+    below.


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

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.

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.

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

lhecker · 2022-03-03T18:54:01Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

+        {
+            "type": "folder",
+            "name": "WSL",
+            "entries": [ { "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.Wsl" } ]
+        },


"matchProfile" would work well with something like an "autofolder" type.
Something that is only a folder if entries.length > 1. Otherwise it's just a regular entry. That way, if you only have a single WSL or pwsh entry, you save yourself a lot of patience in that context menu. 🙂
I feel like this would be a worthwhile addition to this spec.

heck what if we just did that implicitly - a folder with a single match in it is treated as this autofolder.

Okay no, but maybe there's definitely room for a property here:

"expand": {"auto"|"always"}

on folders.

I am in favour of the expand property, and it is simple to implement in the current setup!

…ching-nested-entries

carlos-zamora

The things I mentioned are minor enough, I feel.

carlos-zamora · 2022-05-16T23:40:46Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

+    generating the menu. This will be more useful with the `matchProfile` entry,
+    below.


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.

carlos-zamora · 2022-05-16T23:55:19Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

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


(devil's advocate) what if I want a combination of name=*VS* and/or commandline=*powershell*?

"Or" seems to be handled with something like this...

"entries": [ { "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.Wsl" }, { "type": "matchProfile", "on": "source", "match": "Microsoft.Terminal.PowerShell" } ]

"And" could be covered by making a slight modification and replacing "on" and "match" with...

string name = *

string source = *

string commandline = *
where each is already a regex and having multiple creates a conjunction.

okay lets toss out or and and for now and focus on union and intersection, for clarity's sake

Nesting complicated logic here is trick.

A union of a couple statements? easy enough. entries takes an array, just put a couple different matchProfile statements.

An intersection of properties? harder, but maybe if we did
{ "type": "matchProfile", "source": "Microsoft.Terminal.VS", "commandline": ".*powershell.*" }
That kinda works

an intersection of unions? (A ∩ (B ∪ C)) - can we just say no?

carlos-zamora · 2022-05-17T00:16:07Z

For any reviewers, here's a link to the rendered view: https://github.com/microsoft/terminal/blob/dev/migrie/s/1571-matching-nested-entries/doc/specs/%231571%20-%20New%20Tab%20Menu%20Customization/%231571%20-%20New%20Tab%20Menu%20Customization.md

FWest98 · 2022-08-17T02:10:38Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

@@ -97,6 +108,7 @@ 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.


What I was thinking is that it would be nice to have some combination of matchProfile and remainingProfiles: for example, you want to put your 2 most-used WSL profiles at the top, then a separator, and then "all remaining WSL profiles". Tricky to implement in code, but I think there is a good usecase for it.

Interesting, maybe we want to just always treat "match" entries as basically "remainingProfiles that match that filter". I'm not sure how much use case we get for having multiple entries for the same profile in the menu.

So we'd have to evaluate:

all explicit profile entries

then all match entries, using profiles not already specified

then expand out remainingProfiles with anything not found above.

…ching-nested-entries

FWest98 · 2022-09-12T21:08:21Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

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


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.

Oh good point. I totally forgot about it. Huh.

I'll add some commentary.

FWest98 · 2022-09-12T21:09:13Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

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


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.

FWest98 · 2022-09-12T21:10:59Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

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


Do we want to allow simple (partial) string matching in addition to regexes?

Huh. #12584 (comment) raises a good point. I figured that regex would just always be easier, but I forgot that matching on the .'s would be a common scenario that would be arguably worse.

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.

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

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 cmd.exe /k #work 15 that open up a workspace. I want to stick all those in a folder. So I add a { "type": "matchProfile", "commandline": "#work" } entry

I think we can make source an array, while doing regex/(partial) string match for name and commandline

after realizing https://github.com/microsoft/terminal/pull/12584/files#r1009785219, I'm inclined to leave this as-is

FWest98 · 2022-09-12T21:11:32Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

@@ -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" }


If it's a regex, I would say

Suggested change

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

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

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

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

FWest98 · 2022-09-12T21:16:32Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

+we'll evaluate the entries in the following order:
+
+* all explicit `profile` entries
+* then all `matchProfile` entries, using profiles not already specified


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

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)

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.

Okay, I think this should be clarified in the latest version

…ching-nested-entries

PankajBhojwani · 2022-11-01T18:07:12Z

After team discussion:

Let's drop regex for now, use whole string matching instead
In the future, we can think about how we want to do profile matching (labels based on sources? an entire object that contains several fields for matching?)

DHowett

Small notes, but signing off anyway. The notes may change the body text, so seek differential review if you change them!

DHowett · 2022-11-16T22:03:58Z

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

@@ -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


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)

I like this

doc/specs/#1571 - New Tab Menu Customization/#1571 - New Tab Menu Customization.md

…ching-nested-entries

@zadjii-msft

Implements an initial version of #1571 as it has been specified, the only big thing missing now is the possibility to add actions, which depends on #6899. Further upcoming spec tracked in #12584 Implemented according to [instructions by @zadjii-msft]. Mostly relatively straightforward, but some notable details: - In accordance with the spec, the counting/indexing of profiles is based on their index in the json (so the index of the profile, not of the entry in the menu). - Resolving a profile name to an actual profile is done in a similar fashion as how currently the `DefaultProfile` field is populated: the `CascadiaSettings` constructor now has an extra `_resolve` function that will iterate over all entries and resolve the names to instances; this same function will compute all profile sets (the set of all profiles from source "x", and the set of all remaining profiles) - ~Fun~ fact: I spent two whole afternoons and evenings trying to add 2 classes (which turned out to be a trivial `.vcxproj` error), and then managed to finish the entire rest of it in another afternoon and evening... ## Validation Steps Performed A lot of manual testing; as mentioned above I was not able to run any tests so I could not add them for now. However, the logic is not too tricky so it should be relatively safe. Closes #1571 [instructions by @zadjii-msft]: #1571 (comment)

Spec for "matching" profiles in "New Tab Customization"

5a1fe10

This is a fever dream I had in response to the GDK PR we just got. I had to write it down immediately. I'm in no rush to get this in. References: * #1571 * #11326 * #7774

zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Feb 25, 2022

This comment was marked as resolved.

Sign in to view

lhecker reviewed Mar 3, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/migrie/s/1571-mat…

55dcf14

…ching-nested-entries

carlos-zamora approved these changes May 17, 2022

View reviewed changes

FWest98 mentioned this pull request Aug 17, 2022

New Tab Menu Customization #13763

Merged

FWest98 reviewed Aug 17, 2022

View reviewed changes

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 9, 2022

zadjii-msft added 2 commits September 12, 2022 14:29

Merge remote-tracking branch 'origin/main' into dev/migrie/s/1571-mat…

7f84a18

…ching-nested-entries

feedback

fa5ad0b

FWest98 reviewed Sep 12, 2022

View reviewed changes

zadjii-msft added 3 commits September 15, 2022 07:03

stashing, mostly

cd29610

Merge remote-tracking branch 'origin/main' into dev/migrie/s/1571-mat…

26c3248

…ching-nested-entries

notes from review

5bc2a90

PankajBhojwani removed the Needs-Discussion Something that requires a team discussion before we can proceed label Nov 1, 2022

zadjii-msft added 2 commits November 1, 2022 13:59

as discussed

b203c52

ctrl+f regex

cd313e1

zadjii-msft assigned DHowett, lhecker and PankajBhojwani Nov 1, 2022

lhecker approved these changes Nov 4, 2022

View reviewed changes

DHowett approved these changes Nov 16, 2022

View reviewed changes

zadjii-msft added 2 commits November 18, 2022 14:07

Merge remote-tracking branch 'origin/main' into dev/migrie/s/1571-mat…

539edf8

…ching-nested-entries

nits

2f2f329

zadjii-msft merged commit 937cadc into main Nov 18, 2022

zadjii-msft deleted the dev/migrie/s/1571-matching-nested-entries branch November 18, 2022 20:13

		generating the menu. This will be more useful with the `matchProfile` entry,
		below.

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

Spec for "matching" profiles in "New Tab Customization" #12584

Spec for "matching" profiles in "New Tab Customization" #12584

Conversation

zadjii-msft commented Feb 25, 2022

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani commented Nov 1, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Mar 3, 2022 •

edited

Loading

lhecker Sep 13, 2022 •

edited

Loading