Bind CommandPalette to Tabs and MRU Tabs collections #8427

Don-Vito · 2020-11-27T20:10:39Z

Second step towards #8415.
Closes: #8374

Introduces:

Biding of CommandPalette to Tabs and MRU tabs. Simplifies the terminal page and improves performance.
Fix possible race when focusing by index
Improves readability by introducing _GetFocusedTabImpl

github-actions · 2020-11-27T20:28:00Z

New misspellings found, please review:

Viewitem

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/9c68cf6eb166d8fbe65a1d33c2a90f2452ee1890.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"csv inlines Renamer reserialize udk unfocus Viewitem "');
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/9c68cf6eb166d8fbe65a1d33c2a90f2452ee1890.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.

zadjii-msft · 2020-12-04T04:19:32Z

Hey do you mind if I hold off reviewing this PR till after #8420 merges? It's just a little hard to tease out what's new in this PR the way it's currently set up 😬

Don-Vito · 2020-12-04T14:11:36Z

Hey do you mind if I hold off reviewing this PR till after #8420 merges? It's just a little hard to tease out what's new in this PR the way it's currently set up 😬

@zadjii-msft - absolutely! I created a PR for the diff between the branches, but the idea is in any case to merge them one by one.

…-item

…e items

DHowett · 2020-12-16T20:26:46Z

QQ: This is going to be unmarked draft when it is retargeted to your merged work in main, right? 😄

Don-Vito · 2020-12-16T20:29:10Z

QQ: This is going to be unmarked draft when it is retargeted to your merged work in main, right? 😄

@DHowett - yes. Absolutely. I need to merge and improve few things. Hope to get to it on the weekend (to get some more focus time) 😊

Don-Vito · 2020-12-17T16:48:14Z

@DHowett - btw.. not sure what is the best approach here.. but if the merge of the previous phase wasn't squashed the merge/rebase could be much simpler and safer.

DHowett · 2020-12-17T16:50:37Z

Yeah, our squash workflow can be a pain for stacked branches. A merge should work, though.

Worst case, a rebase?

Don-Vito · 2020-12-17T16:54:31Z

Yeah, our squash workflow can be a pain for stacked branches. A merge should work, though.

Worst case, a rebase?

It will work.. no worries - I was just surprised with the amount of conflicts 😄

github-actions · 2020-12-17T17:47:41Z

New misspellings found, please review:

reseting

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 Checkboxes CParams cso CSV DOWNSCALE GENERATEPROJECTPRIFILE hhhh Horiz Inlines INPATHROOT intersectors Kode MAKEINTRESOURCEA mousemode msixbundle psin Reserialize reso RETROII rgus sbri scanbri scol SGRXY spe sph spherefunctions tcon UDK UDKs Unfocus Viginetting wz xe xlang XWV xyw yzx "');
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/6d42f8166e4d8d54e478d4fe6695869836e27565.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"checkboxes csv horiz inlines reserialize reseting 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/6d42f8166e4d8d54e478d4fe6695869836e27565.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-12-17T19:23:45Z

@DHowett, @zadjii-msft - this was meant to be a minimal step of binding the tabs manually (later on they will be bound to filterable list control). However during the testing I met few bugs.. and it ended up more code than required (it fixes the race from #8374).
I can try to split this into several PRs.. but it won't be fun 😢

github-actions · 2020-12-17T19:31:35Z

New misspellings found, please review:

reseting

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 Checkboxes CParams cso CSV DOWNSCALE GENERATEPROJECTPRIFILE hhhh Horiz Inlines INPATHROOT intersectors Kode MAKEINTRESOURCEA mousemode msixbundle psin Reserialize reso RETROII rgus sbri scanbri scol SGRXY spe sph spherefunctions tcon UDK UDKs Unfocus Viginetting wz xe xlang XWV xyw yzx "');
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/3fa4b5318647c518d20d972f59a109a08aba0e66.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"checkboxes csv horiz inlines reserialize reseting 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/3fa4b5318647c518d20d972f59a109a08aba0e66.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.

github-actions · 2020-12-17T20:32:53Z

New misspellings found, please review:

reseting

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 Checkboxes CParams cso CSV DOWNSCALE GENERATEPROJECTPRIFILE hhhh Horiz Inlines INPATHROOT intersectors Kode MAKEINTRESOURCEA mousemode msixbundle psin Reserialize reso RETROII rgus sbri scanbri scol SGRXY spe sph spherefunctions tcon UDK UDKs Unfocus Viginetting wz xe xlang XWV xyw yzx "');
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/1b3f20ea27ddd3caaafce719e890d6b7aece1238.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"checkboxes csv horiz inlines reserialize reseting 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/1b3f20ea27ddd3caaafce719e890d6b7aece1238.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.

carlos-zamora · 2020-12-24T00:23:58Z

src/cascadia/TerminalApp/TerminalTab.cpp

+                else
+                {
+                    // we need to reset the path so the CommandPalette will be updated
+                    // TODO: this could be avoided if were not resetting the Icon property upon hide.


carlos-zamora · 2020-12-24T00:31:46Z

src/cascadia/TerminalApp/TerminalPage.cpp

-    winrt::com_ptr<TerminalTab> TerminalPage::_GetTerminalTabImpl(const TerminalApp::TabBase& tab) const
+    winrt::com_ptr<TerminalTab> TerminalPage::_GetTerminalTabImpl(const TerminalApp::TabBase& tab)


Shouldn't we keep the const?

carlos-zamora · 2020-12-24T00:32:32Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -2891,10 +2865,9 @@ namespace winrt::TerminalApp::implementation
    // - index: the in-order index of the tab to bump.


Suggested change

// - index: the in-order index of the tab to bump.

// - tab: the tab to bump.

Don-Vito · 2020-12-25T17:32:29Z

@carlos-zamora - thanks for the review! Due to #8653 it seems that I need to add some more logic here. So I guess that at this point the right thing to do is to split this PR into several stacked ones:

One for introducing GetFocusedTabImpl
One for maximizing the usage of tabs
One for binding tabs and ensuring (still don't know how) that the titles and the icons keep getting updated

A part of the #8415. Very technical commit to simplify the terminal page code towards additional steps of simplifying tab management. No business logic should change. A firs step in splitting #8427

Fix `TabPaletteItem` to hold only a weak reference to a tab. This way we guarantee that the refcount of the closed tab gets to 0 immediately (and that command palette cannot "raise it from the dead"). While this seems a correct thing to do, it is still not clear why the `FilteredCommand` itself (the one holding the `TabPaletteItem`) doesn't get released until the UI is refreshed. There is an impact of not registering to PropertyChanged event: if the tab title changes during Tab Switcher navigation the Tab Switcher item won't be updated immediately (the change will apply next time the Tab Switcher is open). Due to this change we need to make sure that the tabs binding in #8427 doesn't break the title / icon update. ## Validation Steps Performed * Manual testing Closes #8651

A part of the microsoft#8415. Very technical commit to simplify the terminal page code towards additional steps of simplifying tab management. No business logic should change. A firs step in splitting microsoft#8427

Fix `TabPaletteItem` to hold only a weak reference to a tab. This way we guarantee that the refcount of the closed tab gets to 0 immediately (and that command palette cannot "raise it from the dead"). While this seems a correct thing to do, it is still not clear why the `FilteredCommand` itself (the one holding the `TabPaletteItem`) doesn't get released until the UI is refreshed. There is an impact of not registering to PropertyChanged event: if the tab title changes during Tab Switcher navigation the Tab Switcher item won't be updated immediately (the change will apply next time the Tab Switcher is open). Due to this change we need to make sure that the tabs binding in microsoft#8427 doesn't break the title / icon update. ## Validation Steps Performed * Manual testing Closes microsoft#8651

Don-Vito added 11 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

Fix spelling error

c761ef9

Remove diff file

d8f3943

Replace command palette callbacks with event handlers

c7fd0ad

Bind CommandPalette to tabs and MRU tabs collections

06d4914

Use tab rather than index to select a tab

d303c32

Make -SetFocusedTab async again

2dcf7bc

Teach tab switcher selection to call _SetFocusTab directly

9c68cf6

Don-Vito mentioned this pull request Nov 27, 2020

Teach CommandPalette to natively support tabs and command lines #8415

Open

5 tasks

Don-Vito added 4 commits November 28, 2020 09:58

Fix spelling error

8aaefe7

Prevent double updates when enabling another mode

6259374

Reduce the number of updates in the action mode

12d22e6

Prevent updating filtered commands upon visibility

4c864ae

zadjii-msft self-assigned this Nov 30, 2020

Don-Vito added 4 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

Merge branch '8145-phase0-palette-item' into 8145-phase1-tab-management

df02a07

zadjii-msft mentioned this pull request Dec 9, 2020

Implement the Settings UI #8048

Merged

Merge branch 'main' into 8145-phase1-tab-management

6d42f81

Don-Vito added 2 commits December 17, 2020 20:51

Fix hiding logic to update icon only if hiding state chagnes

754d427

Introduce GetTerminalTabImpl

3fa4b53

ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Dec 17, 2020

Don-Vito marked this pull request as ready for review December 17, 2020 19:21

Fix typo

1b3f20e

Delete temp file

222800d

carlos-zamora approved these changes Dec 24, 2020

View reviewed changes

Don-Vito mentioned this pull request Dec 25, 2020

Prevent Tab Switcher from increasing Tab refcount #8653

Merged

Don-Vito closed this Dec 25, 2020

Don-Vito mentioned this pull request Dec 25, 2020

Simplify TerminalPage by introducing _GetFocusedTabImpl #8655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bind CommandPalette to Tabs and MRU Tabs collections #8427

Bind CommandPalette to Tabs and MRU Tabs collections #8427

Don-Vito commented Nov 27, 2020 •

edited

Loading

github-actions bot commented Nov 27, 2020

zadjii-msft commented Dec 4, 2020

Don-Vito commented Dec 4, 2020 •

edited

Loading

DHowett commented Dec 16, 2020

Don-Vito commented Dec 16, 2020

Don-Vito commented Dec 17, 2020

DHowett commented Dec 17, 2020

Don-Vito commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

Don-Vito commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

carlos-zamora Dec 24, 2020

carlos-zamora Dec 24, 2020

carlos-zamora Dec 24, 2020

Don-Vito commented Dec 25, 2020 •

edited

Loading

		winrt::com_ptr<TerminalTab> TerminalPage::_GetTerminalTabImpl(const TerminalApp::TabBase& tab) const
		winrt::com_ptr<TerminalTab> TerminalPage::_GetTerminalTabImpl(const TerminalApp::TabBase& tab)

		@@ -2891,10 +2865,9 @@ namespace winrt::TerminalApp::implementation
		// - index: the in-order index of the tab to bump.

	// - index: the in-order index of the tab to bump.
	// - tab: the tab to bump.

Bind CommandPalette to Tabs and MRU Tabs collections #8427

Bind CommandPalette to Tabs and MRU Tabs collections #8427

Conversation

Don-Vito commented Nov 27, 2020 • edited Loading

github-actions bot commented Nov 27, 2020

New misspellings found, please review:

⚠️ Reviewers

zadjii-msft commented Dec 4, 2020

Don-Vito commented Dec 4, 2020 • edited Loading

DHowett commented Dec 16, 2020

Don-Vito commented Dec 16, 2020

Don-Vito commented Dec 17, 2020

DHowett commented Dec 17, 2020

Don-Vito commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

New misspellings found, please review:

⚠️ Reviewers

Don-Vito commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

New misspellings found, please review:

⚠️ Reviewers

github-actions bot commented Dec 17, 2020

New misspellings found, please review:

⚠️ Reviewers

carlos-zamora Dec 24, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 24, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 24, 2020

Choose a reason for hiding this comment

Don-Vito commented Dec 25, 2020 • edited Loading

Don-Vito commented Nov 27, 2020 •

edited

Loading

Don-Vito commented Dec 4, 2020 •

edited

Loading

Don-Vito commented Dec 25, 2020 •

edited

Loading