Preview tab switching with the ATS #7796

zadjii-msft · 2020-10-01T11:11:46Z

Summary of the Pull Request

This PR enables the ATS to display the active tab as the user navigates the tab switcher. We do this by dispatching the tab switch actions as the user navigates the menu, and manually not focusing the new tab when the tab switcher is open.

References

Advanced Tab Switcher #6732 - original tab switcher PR
Allow commands to be previewed in the command palette #6689 - That's a more involved, generic version of this, but this PR will be enough to stop most of the complaints hopefully

PR Checklist

Closes When selecting a new tab with the tab switcher, preview the selected tab in the Terminal #7409
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Validation Steps Performed

Opened tabs, tabbed through the menu, verified that it did what I'd expect

leonMSFT

This is awesome, thanks for taking it up! 😄

Possibly unrelated to this PR - but I wonder if those same people complaining would also want tab preview when using Tab Search? 🤔 The argument about tab titles being useless still applies there, but perhaps less so.

zadjii-msft · 2020-10-01T18:52:24Z

I wonder if those same people complaining would also want tab preview when using Tab Search? 🤔 The argument about tab titles being useless still applies there, but perhaps less so.

Oh they very well might, good catch

DHowett · 2020-10-07T17:10:40Z

src/cascadia/TerminalApp/CommandPalette.cpp

+    void CommandPalette::_selectedCommandChanged(const IInspectable& /*sender*/,
+                                                 const Windows::UI::Xaml::RoutedEventArgs& /*args*/)
+    {
+        if (_currentMode == CommandPaletteMode::TabSwitchMode)
+        {
+            const auto& selectedCommand = _filteredActionsView().SelectedItem();
+            if (const auto& command = selectedCommand.try_as<TerminalApp::Command>())
+            {
+                const auto& actionAndArgs = command.Action();
+                _dispatch.DoAction(actionAndArgs);
+            }
+        }
+    }


This seems circuitous. TerminalPage is already driving the selection of tabs in the tab switcher, so should we reduce coupling by having TerminalPage do the active terminal change in addition to driving the switching UI?

Maybe having it be in MRU order changes things.

(I'm averse to taking on too many more "are we in mode A, B, C, or D" checks :/)

DHowett

block for chat

zadjii-msft · 2020-10-07T20:41:14Z

Chatted with Dustin - yes this seems like extra hoops to jump through right now, but in the future, the Tab Switcher won't just have the list of tabs in the same order as the tabs themselves. The tab switcher might have them in MRU order, or it might be displaying the panes in a tab. In those cases, the page will need to route the action through the switcher, so it makes sense to leave it like this for now.

Further: yes this is a little convoluted for now, but this is a "fix the problem now" kind of changelist, not a "perfect solution" fix. We can work on architectural purity in the future, but let's relieve the user dissatisfaction now.

ghost · 2020-10-07T21:04:07Z

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.

zadjii-msft · 2020-10-07T21:06:31Z

/azp run

azure-pipelines · 2020-10-07T21:06:36Z

No pipelines are associated with this pull request.

mpela81 · 2020-10-08T12:11:40Z

Hi - I'm getting this when building master after this merge:

4>C:\code\windows-terminal\terminal-original\src\cascadia\TerminalApp\CommandPalette.cpp(135,75): error C2039: 'Command': is not a member of 'winrt::TerminalApp'
4>C:\code\windows-terminal\terminal-original\src\cascadia\TerminalApp\CommandPalette.cpp(20): message : see declaration of 'winrt::TerminalApp'

carlos-zamora · 2020-10-08T16:38:41Z

Hi - I'm getting this when building master after this merge:

4>C:\code\windows-terminal\terminal-original\src\cascadia\TerminalApp\CommandPalette.cpp(135,75): error C2039: 'Command': is not a member of 'winrt::TerminalApp'
4>C:\code\windows-terminal\terminal-original\src\cascadia\TerminalApp\CommandPalette.cpp(20): message : see declaration of 'winrt::TerminalApp'

😲 oh no. This seems to have conflicted with my TerminalSettingsModel change. I'll go ahead and fix that right now. Thanks!

#7796 and #7667 were being implemented concurrently. As a part of #7667, Command was moved from TermApp to TSM. This just applies that change to a line we missed in #7796 and fixes the build break.

microsoft#7796 and microsoft#7667 were being implemented concurrently. As a part of microsoft#7667, Command was moved from TermApp to TSM. This just applies that change to a line we missed in microsoft#7796 and fixes the build break.

## Summary of the Pull Request ![preview-ats-000](https://user-images.githubusercontent.com/18356694/94801728-18302a00-03ac-11eb-851d-760b92ebb46f.gif) This PR enables the ATS to display the active tab as the user navigates the tab switcher. We do this by dispatching the tab switch actions as the user navigates the menu, and manually _not_ focusing the new tab when the tab switcher is open. ## References * #6732 - original tab switcher PR * #6689 - That's a more involved, generic version of this, but this PR will be enough to stop most of the complaints hopefully ## PR Checklist * [x] Closes #7409 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Opened tabs, tabbed through the menu, verified that it did what I'd expect (cherry picked from commit 22887d7)

ghost · 2020-11-11T18:07:41Z

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

Handy links:

ghost · 2020-11-11T18:13:38Z

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

Handy links:

Preview tab switching with the ATS

fd8cf4a

zadjii-msft requested a review from leonMSFT October 1, 2020 11:11

zadjii-msft assigned leonMSFT Oct 1, 2020

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Oct 1, 2020

leonMSFT approved these changes Oct 1, 2020

View reviewed changes

zadjii-msft unassigned leonMSFT Oct 1, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Oct 1, 2020

ghost requested review from miniksa, carlos-zamora, DHowett and PankajBhojwani October 1, 2020 18:56

This comment has been minimized.

Sign in to view

zadjii-msft assigned DHowett Oct 7, 2020

PankajBhojwani approved these changes Oct 7, 2020

View reviewed changes

DHowett reviewed Oct 7, 2020

View reviewed changes

DHowett requested changes Oct 7, 2020

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 Oct 7, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 7, 2020

DHowett approved these changes Oct 7, 2020

View reviewed changes

zadjii-msft unassigned DHowett Oct 7, 2020

zadjii-msft added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Oct 7, 2020

ghost merged commit 22887d7 into master Oct 7, 2020

ghost deleted the dev/migrie/b/7409-preview-tab-switching branch October 7, 2020 22:49

carlos-zamora mentioned this pull request Oct 8, 2020

Fix old reference to TermApp::Command #7863

Merged

This was referenced Oct 22, 2020

Feedback on the Tab switcher UI #8006

Closed

Change current terminal behavior for "nextTab" and "prevTab" commands #8011

Closed

ghost mentioned this pull request Nov 11, 2020

When selecting a new tab with the tab switcher, preview the selected tab in the Terminal #7409

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preview tab switching with the ATS #7796

Preview tab switching with the ATS #7796

zadjii-msft commented Oct 1, 2020

leonMSFT left a comment

zadjii-msft commented Oct 1, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DHowett Oct 7, 2020

DHowett Oct 7, 2020

DHowett left a comment

zadjii-msft commented Oct 7, 2020 •

edited

Loading

ghost commented Oct 7, 2020

zadjii-msft commented Oct 7, 2020

azure-pipelines bot commented Oct 7, 2020

mpela81 commented Oct 8, 2020

carlos-zamora commented Oct 8, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

Preview tab switching with the ATS #7796

Preview tab switching with the ATS #7796

Conversation

zadjii-msft commented Oct 1, 2020

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

leonMSFT left a comment

Choose a reason for hiding this comment

zadjii-msft commented Oct 1, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DHowett Oct 7, 2020

Choose a reason for hiding this comment

DHowett Oct 7, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

zadjii-msft commented Oct 7, 2020 • edited Loading

ghost commented Oct 7, 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.

zadjii-msft commented Oct 7, 2020

azure-pipelines bot commented Oct 7, 2020

mpela81 commented Oct 8, 2020

carlos-zamora commented Oct 8, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

zadjii-msft commented Oct 7, 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.