Teach CommandPalette model to natively support tabs and command lines #8420

Don-Vito · 2020-11-27T11:06:15Z

First step towards #8415:

Introduce PaletteItem and derive from it to provide native support
for tabs and command lines (ActionPaletteItem / TabPaletteItem,
CommandLinePaltteItem)
Remove business logic behind PaletteItem from palette (aka dispatch
commands and preview tabs externally)

github-actions · 2020-11-27T11:26:00Z

New misspellings found, please review:

Actionpalette

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"AAAAA Bopomofo CParams CSV GENERATEPROJECTPRIFILE hhhh Inlines MAKEINTRESOURCEA renamer
 Reserialize rgus SGRXY tcon UDK UDKs Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/9301d8f4320589cdabc457f61ce5bc6ad1946dec.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"Actionpalette csv inlines Renamer reserialize udk unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/9301d8f4320589cdabc457f61ce5bc6ad1946dec.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Don-Vito · 2020-11-27T11:27:10Z

@DHowett , @zadjii-msft - the first one in the saga. Probably the most interesting one since it affects interface:

SwitchToTabCommand is fully eliminated - palette works directly with tabs.
CommandPalette is responsible only for presentation, the business logic is moved to subscribers.

It also fixes a crash in terminal - I will extract it as a separate PR

DHowett

This looks very, very good. I love it. I have reviewed 27/31 (waiting to review CommandPalette.* just yet).

I have one question, though, about the overarching design.

Why'd you choose to dispatch three different types of event and make palette item management internal to the command palette? Is it just to hide the implementation details from the caller?

I wonder if there's a design that uses a single event, "I selected a thing, you can figure out what it is on your own," to decouple the palette even further form what it hosts?

In addition -- I know there'd be some code duplication, so it would be pretty annoying -- could PaletteItem become an interface? If it becomes an interface, TabBase itself could conform to the interface (it has all the same properties, and already does notifications for them(!)) (EDIT: it doesn't have KeyChordText, but... there actually are key chords that activate tabs!)

Perhaps that's too much coupling. I'm not sure how I feel, having suggested it.

This is an amazing start.

DHowett · 2020-12-03T19:42:57Z

src/cascadia/TerminalApp/PaletteItem.h

+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Name, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Icon, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, KeyChordText, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, FilterableValue, _PropertyChangedHandlers);


FilterableValue not mentioned in the idl!

Sorry.. It was a part of the idl at some moment.. but then I decided to postpone this feature (until we generalize the filterable list.. and realized it will be implemented differently).. unfortunately, I forgot to cleanup it from the header..

Don-Vito · 2020-12-03T19:59:30Z

This looks very, very good. I love it. I have reviewed 27/31 (waiting to review CommandPalette.* just yet).

I have one question, though, about the overarching design.

Why'd you choose to dispatch three different types of event and make palette item management internal to the command palette? Is it just to hide the implementation details from the caller?

I wonder if there's a design that uses a single event, "I selected a thing, you can figure out what it is on your own," to decouple the palette even further form what it hosts?

I was hesitating about that a lot.. the translation between the generic Item and the specific one should happen somewhere (either in the TerminalPage or in the palette). I decided to do it in the Palette.. not to complicate the Page even more... So the current idea is:

Extract a FilterablePaletteItemList (and later on generalized FilterableList) that
- Contains PaletteItems
- Triggers two events: "item selected" and "item activated"
Make CommandPalette to be the "glue" that connects between the FilterablePaletteItemList and the logic (like tabs).

In addition -- I know there'd be some code duplication, so it would be pretty annoying -- could PaletteItem become an interface? If it becomes an interface, TabBase itself could conform to the interface (it has all the same properties, and already does notifications for them(!)) (EDIT: it doesn't have KeyChordText, but... there actually are key chords that activate tabs!)

Perhaps that's too much coupling. I'm not sure how I feel, having suggested it.

Here I was hesitating as well - to allocate tons of objects or do some coupling. But then I assumed that since the number of objects is overall low, it's better to decouple (isn't the gang of four all about composition rather than inheritance? 😊)

This is an amazing start.

zadjii-msft

Okay, I understand it all, and it makes a lot of code WAY easier to read. I'm a go for this, but on the single condition of please keep going. I'm looking forward to where this is going.

zadjii-msft · 2020-12-03T18:35:10Z

src/cascadia/TerminalApp/TabBase.h

@@ -28,14 +28,12 @@ namespace winrt::TerminalApp::implementation
        WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);

        // The TabViewIndex is the index this Tab object resides in TerminalPage's _tabs vector.
-        // This is needed since Tab is going to be managing its own SwitchToTab command.
        GETSET_PROPERTY(uint32_t, TabViewIndex, 0);


Note to future self: TabViewIndex is still needed, just not for the SwitchToTab command

@zadjii-msft - exactly. This is why I removed the comment, as it was misleading 😊

zadjii-msft · 2020-12-03T18:36:26Z

src/cascadia/TerminalApp/PaletteItem.h

+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Name, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Icon, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, KeyChordText, _PropertyChangedHandlers);
+        OBSERVABLE_GETSET_PROPERTY(winrt::hstring, FilterableValue, _PropertyChangedHandlers);


is FilterableValue unused?

It is.. sorry... I will reintroduce it in some form, but later on - when extracting a generic filterable list view

zadjii-msft · 2020-12-03T22:12:01Z

src/cascadia/TerminalApp/CommandPalette.cpp

-                _dispatch.DoAction(actionAndArgs);
-            }
+            const auto filteredCommand{ selectedCommand.try_as<winrt::TerminalApp::FilteredCommand>() };
+            _switchToTab(filteredCommand);


okay so let me get this right - whenever the selection changes, we'll try to _switchToTab to that item, regardless if it's an action, commandline, or tab. Then, _switchToTab will ignore the things it doesn't care about.

I see where this is going in the rest of #8415, with raising separate events for selected vs dispatched items, but right now it feels a little weird

@zadjii-msft - are you addressing the readability or the runtime flow? From the code readability it is still far from perfect, but from the runtime perspective it will be applied only for tabs (as the function is invoked only in TabSwitch mode).

…-item

…e items

zadjii-msft · 2020-12-08T14:42:27Z

@Don-Vito Any reason this one's still marked a draft?

Don-Vito · 2020-12-09T20:31:26Z

@zadjii-msft - this is not a draft anymore 😊
@DHowett - a week has passed, so bumping 👼

DHowett · 2020-12-09T20:38:14Z

Well, not since you marked it as reviewable 😉

I'll put this one on my list!

Don-Vito · 2020-12-09T20:45:15Z

Well, not since you marked it as reviewable 😉

I'll put this one on my list!

I am not sure if this one reviewable even if it is marked as one 😄
Thanks!
I want to move to the next chapters, but afraid to get too far from the main.

DHowett

I love it. It's excellent!

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2020-12-09T21:59:24Z

@msftbot merge this in 1 minute

ghost · 2020-12-09T21:59:29Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 09 Dec 2020 22:00:26 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2020-12-09T21:59:31Z

Thanks so much :D

Don-Vito · 2020-12-09T22:05:19Z

Thanks so much :D

Glad to work on this 😊

And I guess that more joy is ahead - as now I seem to conflict myself 😄
Fixing it.

…-item

ghost · 2020-12-09T22:27:50Z

Hello @DHowett!

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about -1642 seconds. No worries though, I will be back when the time is right! 😉

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.

Don-Vito · 2020-12-09T23:08:52Z

Something bad is happening to this build:

DHowett · 2020-12-09T23:10:47Z

kicked

Don-Vito · 2020-12-09T23:26:11Z

kicked

@DHowett - what are the odds that #8536 could affect the CI? Seems that NuGet restore is not happy

DHowett · 2020-12-09T23:27:40Z

Very minimal. Those files are used by a build task that isn't even included in the CI build 😄

Don-Vito · 2020-12-09T23:31:00Z

Very minimal. Those files are used by a build task that isn't even included in the CI build 😄

Lol. Sorry.
The day that Mike fixed the tests CI surprises us with something new 😄

#8420 removed `SwitchToTab()` as a responsibility of `TabBase` and replaces `_mruTabActions` with `_mruTabs` (conceptually). This PR fixes the build break by... - replacing `TerminalPage`'s reference to the SettingsTab's SwitchToTab command, with a reference to the tab itself - using that reference to maintain existing tab switching behavior ## References #1564 - Settings UI #8420 - Command Palette + SwitchToTab refactoring ## PR Checklist * [X] Closes #8538 ## Validation Steps Performed ✅ Open SUI --> switch to a different tab --> try opening SUI again --> switches to existing SUI ✅ Open SUI --> switch to a different tab --> reorder tabs --> try opening SUI again --> switches to existing SUI

ghost · 2021-01-28T18:03:52Z

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

Handy links:

…microsoft#8420) First step towards microsoft#8415: * Introduce `PaletteItem` and derive from it to provide native support for tabs and command lines (`ActionPaletteItem` / `TabPaletteItem`, `CommandLinePaltteItem`) * Remove business logic behind PaletteItem from palette (aka dispatch commands and preview tabs externally)

Don-Vito added 4 commits November 27, 2020 10:01

Introduce PaletteItem - not in use yet

7484513

Make CommandPalette to use PaletteItem

46567a8

Nuke SwitchToTabCommand

9a8ac59

Fix previous attempt to handle index underflow

9301d8f

Don-Vito added 2 commits November 27, 2020 13:30

Fix spelling error

c761ef9

Remove diff file

d8f3943

Don-Vito mentioned this pull request Nov 27, 2020

Terminal crashes when navigating in empty palette #8421

Closed

Replace command palette callbacks with event handlers

c7fd0ad

This was referenced Nov 27, 2020

Bind CommandPalette to Tabs and MRU Tabs collections #8427

Closed

Teach CommandPalette to natively support tabs and command lines #8415

Open

zadjii-msft self-assigned this Nov 30, 2020

DHowett reviewed Dec 3, 2020

View reviewed changes

microsoft deleted a comment Dec 3, 2020

zadjii-msft approved these changes Dec 4, 2020

View reviewed changes

Don-Vito added 3 commits December 5, 2020 14:37

Merge remote-tracking branch 'upstream/main' into 8145-phase0-palette…

a058ed8

…-item

Fix case sensitive sorting tests by teaching them to work with palett…

b921cd6

…e items

Remove FilterableValue from PaletteItem header

b2ebd15

zadjii-msft assigned DHowett and leonMSFT and unassigned zadjii-msft Dec 8, 2020

zadjii-msft added Area-CmdPal Command Palette issues and features Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal. labels Dec 8, 2020

Don-Vito marked this pull request as ready for review December 8, 2020 15:09

Don-Vito requested a review from DHowett December 9, 2020 13:50

DHowett approved these changes Dec 9, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

Do not increase tabs reference + initialize tab index

d9c68f9

Don-Vito requested a review from DHowett December 9, 2020 21:54

DHowett approved these changes Dec 9, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2020

Don-Vito added 2 commits December 10, 2020 00:11

Merge remote-tracking branch 'upstream/main' into 8145-phase0-palette…

2527eaf

…-item

Fix merge inaccuracy

f6225af

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2020

Don-Vito requested a review from DHowett December 9, 2020 22:27

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 9, 2020

DHowett approved these changes Dec 9, 2020

View reviewed changes

ghost merged commit 89d82ff into microsoft:main Dec 10, 2020

This was referenced Dec 10, 2020

[feature/settings-ui] command palette build break on SettingsTab #8538

Closed

Fix SwitchToTab build break #8545

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teach CommandPalette model to natively support tabs and command lines #8420

Teach CommandPalette model to natively support tabs and command lines #8420

Don-Vito commented Nov 27, 2020 •

edited by DHowett

Loading

github-actions bot commented Nov 27, 2020

Don-Vito commented Nov 27, 2020 •

edited

Loading

DHowett left a comment •

edited

Loading

DHowett Dec 3, 2020

Don-Vito Dec 5, 2020

Don-Vito commented Dec 3, 2020 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Dec 3, 2020

Don-Vito Dec 5, 2020

zadjii-msft Dec 3, 2020

Don-Vito Dec 5, 2020

zadjii-msft Dec 3, 2020

Don-Vito Dec 5, 2020 •

edited

Loading

zadjii-msft commented Dec 8, 2020

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

DHowett left a comment

DHowett commented Dec 9, 2020

ghost commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

ghost commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

ghost commented Jan 28, 2021

Teach CommandPalette model to natively support tabs and command lines #8420

Teach CommandPalette model to natively support tabs and command lines #8420

Conversation

Don-Vito commented Nov 27, 2020 • edited by DHowett Loading

github-actions bot commented Nov 27, 2020

New misspellings found, please review:

⚠️ Reviewers

Don-Vito commented Nov 27, 2020 • edited Loading

DHowett left a comment • edited Loading

Choose a reason for hiding this comment

DHowett Dec 3, 2020

Choose a reason for hiding this comment

Don-Vito Dec 5, 2020

Choose a reason for hiding this comment

Don-Vito commented Dec 3, 2020 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Dec 3, 2020

Choose a reason for hiding this comment

Don-Vito Dec 5, 2020

Choose a reason for hiding this comment

zadjii-msft Dec 3, 2020

Choose a reason for hiding this comment

Don-Vito Dec 5, 2020

Choose a reason for hiding this comment

zadjii-msft Dec 3, 2020

Choose a reason for hiding this comment

Don-Vito Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

zadjii-msft commented Dec 8, 2020

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Dec 9, 2020

ghost commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

ghost commented Dec 9, 2020

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.

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

DHowett commented Dec 9, 2020

Don-Vito commented Dec 9, 2020

ghost commented Jan 28, 2021

Don-Vito commented Nov 27, 2020 •

edited by DHowett

Loading

Don-Vito commented Nov 27, 2020 •

edited

Loading

DHowett left a comment •

edited

Loading

Don-Vito commented Dec 3, 2020 •

edited

Loading

Don-Vito Dec 5, 2020 •

edited

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.