Move Tab Switcher mode handling into CommandPalette #8656

Don-Vito · 2020-12-26T02:39:31Z

A part of the #8415.
Includes:

Moving TabSwitcherMode related decisions into CommandPalette
(simplifying the logic of TerminalPage::SelectNextTab)
Fix a bug where the index of first tab switch is incorrect
(since bindings are not updated)
Removing redundant CommandPalette updates
Preparations for tabs binding

zadjii-msft

These are some smaller questions, nothing too major IMO

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2021-01-07T17:28:51Z

src/cascadia/TerminalApp/CommandPalette.cpp

-        // Some callers might actually want smooth updating, like when the list
-        // of tabs changes.


Does this mean that now, if a tab is removed which the switcher is open, that the entire list will re-animate into existence, rather than the smooth update it does now?

Now if the tab is removed our ephemeral palette gets closed immediately 😊

I'm okay with this

I believe that's because of the earlier change to dispatch bindings from TerminalPage::PreviewKeyDown, right? That makes sense to me.

But... does this also hold if a user (trying to break terminal :P) issues a sleep 5; exit and then opens up the palette and waits for it to blow up?

@DHowett - what a nasty user 😊 I believe that the problem you describe exists in the original code as well.

From my understanding, If the user triggers delayed sleep and then starts switching, what will happen right now is that we will still have TabPaletteItem populated in the switcher, but switching to it will have no action (as weak ref is empty). Not perfect, not terrible (or however they said this in Chernobyl). To resolve this we need to have the fully fledged binding that I planned for the next PR (where we do follow the changes in _tabs, _mruTabs that are observable collections).

@DHowett - ignore the previous comment. What happens now, if the tab is closed by sleep 5; exit we actually dismiss the switcher. This was introduced as a part of the ephemeral palette work (every change in tabs collection closes the palette) to ensure consistency. If the user tries to break terminal this way, the only impact that the palette gets closed.

zadjii-msft

Okay this is better than before, esp for TabSearch - it reanimates less, which is good!

DHowett

Totally gonna sign off, but had a couple q outstanding. 😄 Thanks for cleaning up our command palette, @Don-Vito!

DHowett · 2021-01-18T19:18:39Z

src/cascadia/TerminalApp/CommandPalette.cpp

+            for (uint32_t index = 0; index < commandsToFilter.Size(); index++)
+            {
+                actions.push_back(commandsToFilter.GetAt(index));
+            }


nit: is this transformable to a std::copy with a back_inserter? I remember Carlos and I had a similar discussion on the Settings editor UI

@DHowett - semantically, yes 😊 Technically, I am not sure how to do it because commandsToFilter is an IVector. Is there a way to get the underlying vector / iterator for it?

This works, but I have no idea why. VS won't tell me exactly where begin and end originate.

std::vector<winrt::hstring> vec{ L"One", L"Two", L"Three" }; auto f = winrt::single_threaded_vector(std::move(vec)); Collections::IVector<winrt::hstring> projectedVector{ f }; std::for_each(begin(projectedVector), end(projectedVector), [](auto&& val) { std::wcout << std::wstring_view{ val } << L"\n"; });

I'm not using namespace winrt::impl, but here we are in 2021 and all signs point to that I am using begin/end directly out of winrt::impl.

This is absolutely cool! Is it documented somewhere? 😄
So begin and end are a part of Windows.Foundations.Collections.h - nice!

As far as I can tell, it's a language feature (!) that in part is used to support range-based for loops. I've been digging for documentation (and trying to figure out how it works), but it has been difficult.

C:\Users...\Documents\GitHub\terminal\src\cascadia\TerminalApp\Generated Files\winrt\Windows.Foundation.Collections.h

template <typename T, std::enable_if_t<has_GetAt<T>::value, int> = 0> fast_iterator<T> begin(T const& collection) noexcept { return { collection, 0 }; } template <typename T, std::enable_if_t<has_GetAt<T>::value, int> = 0> fast_iterator<T> end(T const& collection) { return { collection, collection.Size() }; }

Right, but if nobody is using namespace winrt::impl (which they tell us is private 😉), those definitions shouldn't be found when they're not namespace-qualified. Yet this works.

Got it! This is clever. Argument-dependent lookup (ADL) specifies how free functions are resolved (based on their argument types). One of the steps in argument-dependent lookup is the collection of the class hierarchy and the namespaces of the classes in that hierarchy.

Because each C++/WinRT projection type inherits from something in winrt::impl, ADL on projection types finds any free functions inside the impl namespace. If they didn't inherit from winrt::impl, winrt::impl::begin would not be found.

This is really clever.. but it also makes the modern c++ extremely challenging to code review IMHO.

DHowett

Since the command palette doesn't observe the vector passed in to SetTabs, we could weaken the contract to IVector<TabBase> from IObservableVector<TabBase> and undo the changes in TerminalPage that make _mruTab observable. I'm not blocking on this, but if you want to do it I'll be holding off the merge 😄

DHowett · 2021-01-18T22:17:28Z

@msftbot merge this in 24 hours

ghost · 2021-01-18T22:17:32Z

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 Tue, 19 Jan 2021 22:17:30 GMT, which is in 1 day

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

Don-Vito · 2021-01-19T12:09:44Z

Since the command palette doesn't observe the vector passed in to SetTabs, we could weaken the contract to IVector<TabBase> from IObservableVector<TabBase> and undo the changes in TerminalPage that make _mruTab observable. I'm not blocking on this, but if you want to do it I'll be holding off the merge 😄

@DHowett - this is actually a preparation. I am going to submit a PR, where cmdpal observes these collections (and we will set them only once). This will be a next step towards filterable list view

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

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

Handy links:

A part of the microsoft#8415. Includes: * Moving `TabSwitcherMode` related decisions into `CommandPalette` (simplifying the logic of `TerminalPage::SelectNextTab`) * Fix a bug where the index of first tab switch is incorrect (since bindings are not updated) * Removing redundant `CommandPalette` updates * Preparations for tabs binding

Move Tab Switcher mode handling into CommandPalette

61b47b2

zadjii-msft self-assigned this Jan 4, 2021

Merge branch 'main' into 8415-move-tabswitcher-mode

4476cdd

zadjii-msft reviewed Jan 7, 2021

View reviewed changes

zadjii-msft removed their assignment Jan 7, 2021

Merge branch 'main' into 8415-move-tabswitcher-mode

cb04967

Don-Vito requested a review from zadjii-msft January 13, 2021 11:49

zadjii-msft assigned zadjii-msft, DHowett and leonMSFT Jan 15, 2021

zadjii-msft approved these changes Jan 15, 2021

View reviewed changes

zadjii-msft removed their assignment Jan 15, 2021

zadjii-msft added Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Jan 15, 2021

DHowett reviewed Jan 18, 2021

View reviewed changes

Use std::copy for better readability

988caa0

Don-Vito requested a review from DHowett January 18, 2021 21:01

DHowett approved these changes Jan 18, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 18, 2021

ghost merged commit f196285 into microsoft:main Jan 19, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Tab Switcher mode handling into CommandPalette #8656

Move Tab Switcher mode handling into CommandPalette #8656

Don-Vito commented Dec 26, 2020

zadjii-msft left a comment

zadjii-msft Jan 7, 2021

Don-Vito Jan 7, 2021

zadjii-msft Jan 15, 2021

DHowett Jan 18, 2021

Don-Vito Jan 18, 2021 •

edited

Don-Vito Jan 18, 2021

zadjii-msft left a comment

DHowett left a comment

DHowett Jan 18, 2021

Don-Vito Jan 18, 2021

DHowett Jan 18, 2021

Don-Vito Jan 18, 2021 •

edited

DHowett Jan 18, 2021

Don-Vito Jan 18, 2021 •

edited

DHowett Jan 18, 2021

DHowett Jan 18, 2021

Don-Vito Jan 18, 2021

DHowett left a comment

DHowett commented Jan 18, 2021

ghost commented Jan 18, 2021

Don-Vito commented Jan 19, 2021 •

edited

ghost commented Jan 28, 2021

		// Some callers might actually want smooth updating, like when the list
		// of tabs changes.

Move Tab Switcher mode handling into CommandPalette #8656

Move Tab Switcher mode handling into CommandPalette #8656

Conversation

Don-Vito commented Dec 26, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-Vito Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jan 18, 2021

ghost commented Jan 18, 2021

Don-Vito commented Jan 19, 2021 • edited

ghost commented Jan 28, 2021

Don-Vito Jan 18, 2021 •

edited

Don-Vito Jan 18, 2021 •

edited

Don-Vito Jan 18, 2021 •

edited

Don-Vito commented Jan 19, 2021 •

edited