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.

Sign up for GitHub

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

New Tab Menu Customization #13763

Merged
29 commits merged into from
Dec 9, 2022
Merged

New Tab Menu Customization #13763

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7e922ab
Adding a basic NewTabMenuEntry and some scaffolding
FWest98 Aug 16, 2022
88a336b
Add SeparatorEntry but it breaks the build
FWest98 Aug 16, 2022
cec114e
Implemented FolderEntry and SeparatorEntry
FWest98 Aug 16, 2022
294c13a
Add Profile and RemainingProfiles entry definitions
FWest98 Aug 16, 2022
6676785
Full implementation of the feature
FWest98 Aug 16, 2022
ea62c28
Add ProfileCollection abstraction
FWest98 Aug 17, 2022
1d5f6d3
Code cleanup and documentation
FWest98 Aug 17, 2022
40f2a3e
Update schema
FWest98 Aug 17, 2022
b140c75
Code style fixes
FWest98 Aug 17, 2022
fc18f1f
Add words
FWest98 Aug 17, 2022
2e6f1ce
Fixing spelling
FWest98 Sep 10, 2022
bc628f6
Remove ProfilesSourceEntry for now
FWest98 Sep 10, 2022
7b7dc4f
Copyright headers and small code improvements
FWest98 Sep 10, 2022
46a09a4
Remove redundant override
FWest98 Sep 10, 2022
14f6ece
Merge branch 'main' into fw/profiles_dropdown
FWest98 Sep 10, 2022
af0515a
Review comment
FWest98 Sep 10, 2022
5425298
Merge branch 'main' into fw/profiles_dropdown
carlos-zamora Oct 7, 2022
6bceb8c
Apply suggestions from code review
FWest98 Oct 7, 2022
e5d81a1
Merge branch 'main' into fw/profiles_dropdown
FWest98 Nov 20, 2022
6239a7f
Fix merging mistake
FWest98 Nov 21, 2022
afc2f0b
Update resources file with feedback; reintroduce VS spaces
FWest98 Nov 22, 2022
77304b7
Bug bash, review feedback
FWest98 Nov 22, 2022
b7f915f
Implement auto-inlining and emtpy-handling for folders
FWest98 Nov 24, 2022
547b500
Fix code formatting
FWest98 Nov 24, 2022
7ee3602
Add support for matchProfiles entry
FWest98 Nov 24, 2022
86fb995
Update JSON Schema
FWest98 Nov 24, 2022
2473709
Incorporate review feedback
FWest98 Dec 1, 2022
ac703df
Merge remote-tracking branch 'origin/main' into fw/profiles_dropdown
zadjii-msft Dec 6, 2022
ec176b1
Adjust matchProfiles semantics to AND
FWest98 Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsModel/FolderEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ winrt::com_ptr<NewTabMenuEntry> FolderEntry::FromJson(const Json::Value& json)

// A FolderEntry should only expose the entries to actually render to WinRT,
// to keep the logic for collapsing/expanding more centralised.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit hesitant with this method, actually. Right now, we're giving two masters control over when entries are visible: TerminalPage makes its own decisions for each type of entry independently from this one, except when it asks a Folder and then Folder makes decisions and reports back to Page. Now, this doesn't hide any of the complexity from Page: it still needs to check whether a folder is empty, inlined, or forced to display as empty.

I'm not sure that this is more centralized. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't like this. In a way, it's just view logic. But then it's also partly functional logic. Putting all the filtering in the TerminalPage felt like a bad idea, since we also can't access the implementation types there, but putting view logic in the models also felt dirty. Any ideas on how to fix this?

using NTMEModel = winrt::Microsoft::Terminal::Settings::Model::NewTabMenuEntry;
IVector<NTMEModel> FolderEntry::Entries() const
using NewTabMenuEntryModel = winrt::Microsoft::Terminal::Settings::Model::NewTabMenuEntry;
IVector<NewTabMenuEntryModel> FolderEntry::Entries() const
{
// We filter the full list of entries from JSON to just include the
// non-empty ones.
IVector<NTMEModel> result{ winrt::single_threaded_vector<NTMEModel>() };
IVector<NewTabMenuEntryModel> result{ winrt::single_threaded_vector<NewTabMenuEntryModel>() };

for (const auto& entry : _Entries)
{
Expand Down
19 changes: 14 additions & 5 deletions src/cascadia/TerminalSettingsModel/MatchProfilesEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,31 @@ winrt::com_ptr<NewTabMenuEntry> MatchProfilesEntry::FromJson(const Json::Value&

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// sensible default value for the desired semantics: the first property we want
// to match on should always be applied (so one would set "true" as a default),
// but if none of the properties are set, the default return value should be false
// since this entry type is expected to behave like a positive match/whitelist.
//
// The easiest way to deal with this neatly is to use an optional, then for any
// property to match we consider a null value to be "true", and for the return
// value of the function we consider the null value to be "false".
auto isMatching = std::optional<bool>{};

if (!_Name.empty())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
isMatching = isMatching || _Name == profile.Name();
isMatching = { isMatching.value_or(true) && _Name == profile.Name() };
}

if (!_Source.empty())
{
isMatching = isMatching || _Source == profile.Source();
isMatching = { isMatching.value_or(true) && _Source == profile.Source() };
}

if (!_Commandline.empty())
{
isMatching = isMatching || _Commandline == profile.Commandline();
isMatching = { isMatching.value_or(true) && _Commandline == profile.Commandline() };
}
Copy link
Member

Choose a reason for hiding this comment

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

This could be collapsed to...

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

// etc...

return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


return isMatching;
return isMatching.value_or(false);
}