New Tab Menu Customization by FWest98 · Pull Request #13763 · microsoft/terminal

FWest98 · 2022-08-17T01:59:28Z

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

Added better rendering logic in TerminalPage Better JSON parsing with inheritance And fixed the tricky vcxproj definition....

Add ProfilesSourceEntry to select all profiles from a given source

FWest98 · 2022-08-17T02:25:04Z

On second thought, I only saw #12584 after I implemented my barebones ProfilesSourceEntry. I guess you prefer not to be stuck to this temporary API forever, so maybe it's better to remove that part for now.
Or, if #12584 is finished, it should only take me a few hours to implement it as it stands now, so then we would have a more complete MR here.

Please let me know what you think is best!

zadjii-msft · 2022-08-17T14:24:20Z

Thoughts from the dome: I'm fucking jazzed to see this implemented. Definitely one of the biggest feature requests we've had, and I can't wait to see it land.

That being said - we're kinda putting the final wraps on 1.16 at the moment. This PR might take a little while to get merged while we focus on locking 1.16 down. Thanks for bearing with us!

carlos-zamora

I couldn't help myself and went ahead and reviewed (most of) it haha. Great work! I just have to review the serialization and TerminalApp changes, but so far it's been really straightforward.

Some feedback:

all the stuff in that .github text file should go into .github/actions/spelling/expect/expect.txt
you should be able to revert the dep/wil change (I think)
honestly, just combine all of the smaller NewTabMenuEntry stuff into the same NewTabMenuEntry files. They're all small enough and like one-liners. Maybe even create a macro like DEFINE_NEW_TAB_MENU_ENTRY that implements the ToJson/FromJson for you.

src/cascadia/TerminalSettingsModel/SeparatorEntry.cpp

src/cascadia/TerminalSettingsModel/RemainingProfilesEntry.cpp

src/cascadia/TerminalSettingsModel/ProfilesSourceEntry.cpp

src/cascadia/TerminalSettingsModel/ProfileCollectionEntry.h

src/cascadia/TerminalSettingsModel/ProfileCollectionEntry.cpp

src/cascadia/TerminalSettingsModel/NewTabMenuEntry.cpp

src/cascadia/TerminalSettingsModel/FolderEntry.cpp

zadjii-msft

I'm not through CascadiaSettingsSerialization.cpp quite yet, but figured I'd give feedback sooner than later.

Definitely revert the wil change. We moved that dependency to nuget recently.
I'd revert the ProfileSource change for now, or break into a separate PR, just so we can definitely merge this ASAP and start kicking the tires.
We'll bump #12584 up in discussion priority
Meh, spelling nits.

Most of the code so far I'm super cool with. I'm VERY excited to get this merged here at the start of 1.17 ☺️

(The team should feel free to dismiss my block if the checkboxes above get addressed, if my leave starts before I can unblock it myself)

src/cascadia/TerminalSettingsModel/RemainingProfilesEntry.h

src/cascadia/TerminalSettingsModel/ProfilesSourceEntry.h

src/cascadia/TerminalSettingsModel/ProfileEntry.h

src/cascadia/TerminalApp/TerminalPage.cpp

.github/actions/spelling/expect/8cf56971f61ef4c0d9d6fedd478821ea33f2d6c4.txt

src/cascadia/TerminalSettingsModel/ProfileEntry.cpp

src/cascadia/TerminalSettingsModel/NewTabMenuEntry.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

.github/actions/spelling/expect/8cf56971f61ef4c0d9d6fedd478821ea33f2d6c4.txt

PankajBhojwani

Excited for this :)

src/cascadia/TerminalSettingsModel/ProfileEntry.h

src/cascadia/TerminalSettingsModel/ProfileEntry.cpp

Co-authored-by: Mike Griese <[email protected]>

FWest98 · 2022-09-10T23:04:44Z

Incorporated all your feedback! If there's one reason to upgrade my PC, it would be this project... Easily takes 15-20min for a build after a change in .idl files.

I indeed removed the source item for now, it's easy to get it back by reverting bc628f6 and adapting it later. Hopefully it's nearly done now!

Appease the spellbot, hopefully

zadjii-msft · 2022-12-06T19:25:32Z

The semantics of having a matchProfiles entry with multiple parameters set is not defined in the spec. As a consequence of the implementation, it now behaves as a disjunction; but I can imagine a conjunction might be preferable from a semantic standpoint.

Whoops, you're right, I didn't ever move #12584 (comment) into the spec itself.

on command line or name, since there will typically be only one match. Only the source matching seems to actually be useful. So we might want to consider supporting regexes straightaway.

That's a great point. I was probably the biggest proponent of regexes for matching on all my "commandline": "cmd /k #work ...." profiles. We've probably got a monthish before we ship 1.17 so we can noodle on that and ship it in a followup if we want. FWIW, I think the source matching is probably what most people were looking for, or at least, will be the most immediately valuable.

DHowett · 2022-12-06T19:28:12Z

FWIW on the topic of regex, we should get this one in first and then iterate 😄

zadjii-msft · 2022-12-06T19:29:14Z

FWIW on the topic of regex, we should get this one in first and then iterate 😄

ABSOLUTELY. I want to get this in this week for sure

zadjii-msft

I really don't want to block over this - I might just push the fix directly to your branch if that's cool, and Dustin signs off soon enough. I want to get these 1.17 features all cleared out of the queue asap.

zadjii-msft · 2022-12-06T19:33:12Z

src/cascadia/TerminalSettingsModel/MatchProfilesEntry.cpp

+{
+    auto isMatching = false;
+
+    if (!_Name.empty())


So this is now a union of all the properties, not the intersection. I feel like we should probably make it the conjunction. If it's a disjunction, there's no way to filter on multiple properties at once. If it's an intersection instead, then you can still get a union by just adding one entry per property

Since it's now a separate function, I'll write a quick fix for it tonight.

Luckily for you, European tonight is already now ;) It should now have conjunction semantics.

DHowett · 2022-12-07T00:30:49Z

src/cascadia/TerminalSettingsModel/MatchProfilesEntry.cpp

 bool MatchProfilesEntry::MatchesProfile(const Model::Profile& profile)
 {
-    auto isMatching = false;
+    // We use an optional here instead of a simple bool directly, since there is no


I kinda wonder if we want the opposite behavior! Matching with no conditions means everything passes... that would let us simplify the logic as in my farther-down comment

@zadjii-msft Does that make sense to you, or did I countermand you?

I'm not sure, to me it wouldn't feel intuitive that matchProfiles (which I would interpret as a positive match) without criteria would match everything, I would find it intuitive to match nothing. (which is harder to implement indeed)

Huh. I've been thinking about it all night, and honestly both seem alright, so I think we should just pick one and go with it.

Having no conditions match everything kinda makes sense - you start with everything, and each property filters that list down more.

Having it match nothing also makes sense - there's no properties, so that doesn't really make sense as an entry, and then it just gets ignored.

(also, I've got the plague now too so I might need @DHowett to finish driving this one down)

If we do decide for default everything, then naming it filterProfiles makes more sense. So then, matchProfiles to me is default nothing.

DHowett · 2022-12-07T00:32:22Z

src/cascadia/TerminalSettingsModel/MatchProfilesEntry.cpp

    if (!_Commandline.empty())
    {
-        isMatching = isMatching || _Commandline == profile.Commandline();
+        isMatching = { isMatching.value_or(true) && _Commandline == profile.Commandline() };
    }


This could be collapsed to...

if (_Commandline && _Commandline != profile.Commandline()) return false; // etc... return true;

Only if we want the no-filter case to return all profiles; otherwise this would not work properly.

Okay, I find that I am only really reacting to the optional<bool>. Feel free to dismiss me out-of-hand.

This is an even sillier (but I don't know, more straightforward?) way to handle it.

int conditions = 0, matched = 0; conditions += _Commandline.has_value(); matched += _Commandline == profile.Commandline(); // nullopt == anything is FALSE // ... return conditions == matched;

0 conditions, 0 matches (thanks to all the nullopts) => true.

do I love it? i dunno, not really any more than .value_or(true).

I do generally agree about match vs filter, and I accept that we want match! Sorry, I didn't make that clear. :D

Hmm I dislike the other setup a bit. Neither way is amazing though, so feel free to pick what you think is best.

dismissing myself on account of the plague

zadjii-msft

I'm gonna say I'm fine with this as is. matchProfiles with nothing returns nothing, that makes sense to me. Additional parameters restrict which things we match on. That makes sense to me.

I'm not locked in to that though - if @DHowett feels passionate the other way, idgaf.

[3:03 PM] Dustin Howett you can dismiss me and merge it

ghost · 2022-12-09T22:40:17Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

`_Entries` was getting default constructed to `nullptr`. We should be careful about that. Adds a test too, and fixes a regression in the local tests introduced in #13763. Closes #14557

ghost · 2023-01-24T18:23:22Z

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

As tracked in microsoft/terminal#1571 As added in microsoft/terminal#13763 This shipped in 1.17 😬 Closes #667 _big props to copilot for somehow writing 99% of this for me_

FWest98 added 8 commits August 16, 2022 02:18

Adding a basic NewTabMenuEntry and some scaffolding

7e922ab

Add SeparatorEntry but it breaks the build

88a336b

Implemented FolderEntry and SeparatorEntry

cec114e

Added better rendering logic in TerminalPage Better JSON parsing with inheritance And fixed the tricky vcxproj definition....

Add Profile and RemainingProfiles entry definitions

294c13a

Full implementation of the feature

6676785

Add ProfileCollection abstraction

ea62c28

Add ProfilesSourceEntry to select all profiles from a given source

Code cleanup and documentation

1d5f6d3

Update schema

40f2a3e

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Aug 17, 2022

FWest98 changed the title ~~New Tab Menu Customization #1571~~ New Tab Menu Customization Aug 17, 2022

This comment has been minimized.

Sign in to view

FWest98 added 2 commits August 17, 2022 15:50

Code style fixes

b140c75

Add words

fc18f1f

zadjii-msft added this to the Terminal v1.17 milestone Aug 18, 2022

carlos-zamora reviewed Aug 20, 2022

View reviewed changes

zadjii-msft requested changes Sep 9, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 9, 2022

PankajBhojwani reviewed Sep 9, 2022

View reviewed changes

src/cascadia/TerminalSettingsModel/ProfileEntry.h Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/ProfileEntry.cpp Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 9, 2022

FWest98 and others added 6 commits September 10, 2022 23:53

Fixing spelling

2e6f1ce

Remove ProfilesSourceEntry for now

bc628f6

Copyright headers and small code improvements

7b7dc4f

Remove redundant override

46a09a4

Merge branch 'main' into fw/profiles_dropdown

14f6ece

Review comment

af0515a

Co-authored-by: Mike Griese <[email protected]>

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 1, 2022

This comment has been minimized.

Sign in to view

zadjii-msft mentioned this pull request Dec 5, 2022

Importing PuTTY sessions #10943

Open

zadjii-msft added the zBugBash-Consider label Dec 6, 2022

Merge remote-tracking branch 'origin/main' into fw/profiles_dropdown

ac703df

Appease the spellbot, hopefully

This comment has been minimized.

Sign in to view

zadjii-msft previously requested changes Dec 6, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 6, 2022

zadjii-msft removed their assignment Dec 6, 2022

Adjust matchProfiles semantics to AND

ec176b1

DHowett reviewed Dec 7, 2022

View reviewed changes

zadjii-msft approved these changes Dec 7, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2022

ghost merged commit 2668273 into microsoft:main Dec 9, 2022

zadjii-msft mentioned this pull request Jan 4, 2023

Fix a crash on startup with a folder entry without entries #14629

Merged

ghost mentioned this pull request Jan 24, 2023

Allow dropdown menu customization in profiles.json #1571

Closed

DennisGaida mentioned this pull request Jan 30, 2023

Allow for adjustable delay when opening New Tab Menu Customizations #14756

Closed

zadjii-msft mentioned this pull request Jul 19, 2023

Docs for newTabMenu MicrosoftDocs/terminal#692

Merged

carlos-zamora mentioned this pull request Sep 28, 2023

[Schema] Fix incorrect default value for 'allowEmpty' #16040

Merged

zadjii-msft mentioned this pull request Oct 29, 2023

Ability to Create Profiles Groups in 'New Tab' Drop-Down Profile Menu #7672

Closed

This pull request was closed.

Conversation

FWest98 commented Aug 17, 2022 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

This comment has been minimized.

FWest98 commented Aug 17, 2022

Uh oh!

zadjii-msft commented Aug 17, 2022

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PankajBhojwani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FWest98 commented Sep 10, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

zadjii-msft commented Dec 6, 2022

Uh oh!

DHowett commented Dec 6, 2022

Uh oh!

zadjii-msft commented Dec 6, 2022

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FWest98 commented Aug 17, 2022 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.