Bold matching text in the command palette by Don-Vito · Pull Request #7977 · microsoft/terminal

Don-Vito · 2020-10-20T15:51:23Z

Summary of the Pull Request

References

PR Checklist

Closes Command palette search results should bold the matching text #6646
CLA signed.
Tests added/passed - added UT for filter matching
Documentation updated - irrelevant
Schema updated - irrelevant
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Please be gentle - didn't touch XAML and C++ for 8 years 👼

Created a ViewModel class in the Command Palette called FilteredCommand, aggregating the Command, the filter and the highlighted presentation of the command name
This ListView of the filtered commands is bound to the vector of FilteredCommands
Introduced HihglightedTextControl user control with HighlightedText view model
Added this control to the ListView Item's grid
Bound the FilteredCommand's highlighted command name to the user control

Validation Steps Performed

UT for matching algorithm
Only manual tests
Searching in CommandLine, SwitchTab and Nested Command modes
Checking for bot matching an non matching filters
Dogfooding 😄 like this

…palette-bold-match

Don-Vito · 2020-10-20T17:08:23Z

Dustin L. Howett (@DHowett), Mike Griese (@zadjii-msft) - this is not done yet, but I wanted to share this one to see that the direction is OK.

My next steps are:

Make sure that view model is allocated only once
Prevent leaks (smart pointers, callback unregistration, etc.)
UTs

…lette-bold-match

Don-Vito · 2020-10-22T16:49:29Z

Mike Griese (@zadjii-msft) - can you please take a look? I believe it is starting to take a shape.

…typos

Mike Griese (zadjii-msft)

Sorry for the delay - I'm still technically on paternity so I'm only working 50% time ☺️

Overall, I think this looks like a fine approach, and it certainly gets the job done. I don't think any single thing I have called out is more than a nit, but I definitely do think there needs to be more comments throughout.

Thanks for jumping on this!

Mike Griese (zadjii-msft) · 2020-10-23T02:33:39Z

+        FilteredCommand(Microsoft.Terminal.Settings.Model.Command command);
+
+        Microsoft.Terminal.Settings.Model.Command Command { get; };
+        String Filter;


Does Filter really need to be exposed in the idl here? I think that could probably just stay as a private member of FilteredCommand

It is not required for the current implementation. I left this as this is "FilteredCommand", and I assumed that the interface consumers might be interested in both the Command and the Filter. I suggest to leave it, but please let me know if you think otherwise.

Mike Griese (zadjii-msft) · 2020-10-23T17:43:24Z

+    //
+    // E.g., ("CL", true) ("ose ", false), ("T", true), ("ab", false), ("S", true), ("after this", false)
+    //
+    // TODO: we probably need to merge this logic with _getWeight computation?


I'm okay with leaving this as a future TODO. I know Dustin L. Howett (@DHowett) has some ideas about _getWeight returning a list of match ranges, as well as the weight. I think for now, this is probably okay as-is though - it's not like iterating twice (O(m*n) for m strings of lengths n) over the strings in the command palette is going to be a bottleneck.

Mind filing a GitHub issue to track consolidating this logic, and linking it here in the code like so?

Suggested change

// TODO: we probably need to merge this logic with _getWeight computation?

// TODO GH#12345: we probably need to merge this logic with _getWeight computation

Mike Griese (@zadjii-msft) - it is actually O(m+n) - we either advance in the filter or in the name. But we definitely should not do it twice, not because of the performance, but rather because of consistency. I will try to unify the logic. If it somehow becomes complex I will open a followup task.

Mike Griese (@zadjii-msft) - I modified the weight computation logic to use the result of segments computation and moved everything related to filtering and comparison out of CommandPalette into FilteredCommand. Hope this is aligned with Dustin L. Howett (@DHowett) 's vision.

Actually I tried to replace everything related to sorting and filtering out of code-behind, using CollectionViewSource, to discover it is only partially supported in WinRT. So I parked there. But I believe we can create an advanced CollectionViewSource of our own or to use one from the toolkits (if this is considered appropriate) for further cleanup of the CommandPalette.

Don-Vito · 2020-11-04T19:47:59Z

Dustin L. Howett (@DHowett) , Mike Griese (@zadjii-msft) - I don't want to bother here, and somehow "toggle your focus" from the great stuff you are pushing into the product. I just want to raise a concern that since the change set is relatively large, there are conflicts from time to time (until now there were only few but I am worried the number might grow 😄) I will definitely continue back-merging, just bumping this one 😄

Dustin L. Howett (DHowett)

I really, really love this. Especially the fact that it's now tested. This is really excellent work, I've just got a couple questions. 😄

Dustin L. Howett (DHowett) · 2020-11-04T22:37:32Z

+            segments.Append(*highlightedSegment);
+        }
+
+        return *winrt::make_self<HighlightedText>(segments);


Suggested change

return *winrt::make_self<HighlightedText>(segments);

return winrt::make<HighlightedText>(segments);

make_self returns a com_ptr that you need to convert to a projected type; make just returns a projected type directly!

My bad. There are some more places, will fix everywhere. Thanks!

It’s subtle and likely doesn’t cause any trouble 😄 you just generally use the self version when you need direct access to the implementation namespace type

Dustin L. Howett (DHowett) · 2020-11-04T23:58:23Z

            {
-                if (const auto selectedItem = _filteredActionsView().SelectedItem())
+                const auto& selectedCommand = _filteredActionsView().SelectedItem();
+                if (const auto& filteredCommand = selectedCommand.try_as<winrt::TerminalApp::FilteredCommand>())


i think these have to be const auto instead of const auto&. We can't promise that the projected type reference returned from try_as is going to be "alive" in any meaningful sense of the word. (here and below)

This might work today, but I'm not sure about its legality in C++. Usually you only see const auto& in a ranged for or a parameter.

If there's a C++ person about, they may know better.

I am changing this in all other places (that were there before my change) to be consistent.

Dustin L. Howett (DHowett) · 2020-11-04T23:59:14Z

+                    auto nestedFilteredCommand = winrt::make_self<FilteredCommand>(action);
+                    _currentNestedCommands.Append(*nestedFilteredCommand);


you can likely use make<FilteredCommand> instead of make_self and avoid the dereference on 511

Dustin L. Howett (DHowett) · 2020-11-04T23:59:35Z

+            auto filteredCommand = winrt::make_self<FilteredCommand>(action);
+            vectorToPopulate.Append(*filteredCommand);


Dustin L. Howett (DHowett) · 2020-11-04T23:59:47Z

+    void CommandPalette::_populateFilteredActions(Collections::IVector<winrt::TerminalApp::FilteredCommand> const& vectorToPopulate, Collections::IVector<Command> const& actions)
+    {
+        vectorToPopulate.Clear();
+        for (const auto action : actions)


Suggested change

for (const auto action : actions)

for (const auto& action : actions)

this one is safe 😄

Dustin L. Howett (DHowett) · 2020-11-05T00:04:35Z

+    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
+    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
+    mc:Ignorable="d"
+    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">


Hmm. This one gets me -- should we force the color here or make it transparent? Should we actually template this control and offer a Background binding?

(Easiest is to make it transparent)

Absolutely! Thanks!

Dustin L. Howett (DHowett)

Upgrading my status to Red so it comes back around for review 😄

…lette-bold-match

Don-Vito · 2020-11-05T10:49:34Z

Dustin L. Howett (@DHowett) - thanks for the feedback! I hopefully addressed the comments and added some more tests. Please review.

In additional my local TabTests are currently failing with access violation, which seems to be related to #8153:

(8780.7df0): Access violation - code c0000005 (!!! second chance !!!)
TerminalApp_LocalTests!std::_Ptr_base<Pane>::get+0x2f:
00007ffb`aaf98eaf 488b00          mov     rax,qword ptr [rax] ds:00000000`000000f8=????????????????
0:007> kp
 # Child-SP          RetAddr           Call Site
00 000000cb`2cafef10 00007ffb`aaf7cf54 TerminalApp_LocalTests!std::_Ptr_base<Pane>::get(void)+0x2f [C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\memory @ 706] 
01 000000cb`2cafef40 00007ffb`aaf76aed TerminalApp_LocalTests!std::shared_ptr<Pane>::operator-><Pane,0>(void)+0x34 [C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\memory @ 1104] 
02 000000cb`2cafef70 00007ffb`aad788fe TerminalApp_LocalTests!winrt::TerminalApp::implementation::TerminalTab::GetLeafPaneCount(void)+0x3d [C:\Users\secret\Documents\GitHub\terminal\src\cascadia\TerminalApp\TerminalTab.cpp @ 953] 
03 000000cb`2cafefa0 00007ffb`aad77ac7 TerminalApp_LocalTests!<lambda_34b881531eb07f9f038955b9df762b3d>::operator()(void)+0x16e [C:\Users\secret\Documents\GitHub\terminal\src\cascadia\LocalTests_TerminalApp\TabTests.cpp @ 567] 
04 000000cb`2caff0d0 00007ffb`aad5bfae TerminalApp_LocalTests!<lambda_055f1a3d9bcd99dfbfd1ab0295dcba31>::operator()(void)+0x37 [C:\Users\secret\Documents\GitHub\terminal\src\cascadia\LocalTests_TerminalApp\CppWinrtTailored.h @ 88] 
05 000000cb`2caff100 00007ffb`aad5a7b9 TerminalApp_LocalTests!WEX::Private::SafeInvoke_Impl<<lambda_055f1a3d9bcd99dfbfd1ab0295dcba31> >(class RunOnUIThread::__l2::<lambda_3d1896f77de76b6c9a150531af511fbb>::()::__l2::<lambda_055f1a3d9bcd99dfbfd1ab0295dcba31> * functor = 0x000000cb`2caff260, bool * isCPlusPlusException = 0x000000cb`2caff184)+0x3e [C:\Users\secret\Documents\GitHub\terminal\packages\Taef.Redist.Wlk.10.57.200731005-develop\build\Include\WexTestClass.h @ 248] 
06 000000cb`2caff160 00007ffb`aad78e55 TerminalApp_LocalTests!WEX::SafeInvoke<<lambda_055f1a3d9bcd99dfbfd1ab0295dcba31> >(class RunOnUIThread::__l2::<lambda_3d1896f77de76b6c9a150531af511fbb>::()::__l2::<lambda_055f1a3d9bcd99dfbfd1ab0295dcba31> * functor = 0x000000cb`2caff260)+0x59 [C:\Users\secret\Documents\GitHub\terminal\packages\Taef.Redist.Wlk.10.57.200731005-develop\build\Include\WexTestClass.h @ 347] 
07 000000cb`2caff240 00007ffb`aad8102c TerminalApp_LocalTests!<lambda_3d1896f77de76b6c9a150531af511fbb>::operator()(void)+0x45 [C:\Users\secret\Documents\GitHub\terminal\src\cascadia\LocalTests_TerminalApp\CppWinrtTailored.h @ 88] 
08 000000cb`2caff280 00007ffc`3127b940 TerminalApp_LocalTests!winrt::impl::delegate<winrt::Windows::UI::Core::DispatchedHandler,<lambda_3d1896f77de76b6c9a150531af511fbb> >::Invoke(void)+0x3c [C:\Users\secret\Documents\GitHub\terminal\src\cascadia\TerminalApp\Generated Files\winrt\windows.ui.core.h @ 1277] 
09 000000cb`2caff2c0 00007ffc`3127b721 Windows_UI!Windows::UI::Core::CDispatcher::ProcessInvokeItem(bool * pbInvokeItemProcessed = 0x000000cb`2caff471)+0x1c0 [onecoreuap\windows\advcore\winrt\onecoreiwindow\corewindow\common\dispatcher.cpp @ 1263] 
0a 000000cb`2caff380 00007ffc`3127b390 Windows_UI!Windows::UI::Core::CDispatcher::ProcessMessage(bool bDrainQueue = <Value unavailable error>, bool * pbWindowMessagesProcessed = 0x000000cb`2caff470, bool * pbInvokeItemProcessed = 0x000000cb`2caff471)+0x211 [onecoreuap\windows\advcore\winrt\onecoreiwindow\corewindow\common\dispatcher.cpp @ 267] 
0b 000000cb`2caff430 00007ffc`3127b21b Windows_UI!Windows::UI::Core::CDispatcher::WaitAndProcessMessagesInternal(bool bRunAlwaysOnce = false, void * hEventWait = 0x00000000`00000000)+0xc0 [onecoreuap\windows\advcore\winrt\onecoreiwindow\corewindow\common\dispatcher.cpp @ 1950] 
0c 000000cb`2caff540 00007ffc`300d1a50 Windows_UI!Windows::UI::Core::CDispatcher::ProcessEvents(Windows::UI::Core::CoreProcessEventsOption options = CoreProcessEventsOption_ProcessUntilQuit (0n2))+0x7b [onecoreuap\windows\advcore\winrt\onecoreiwindow\corewindow\common\dispatcher.cpp @ 596] 
0d 000000cb`2caff5f0 00007ffc`300d19ef Windows_UI_Xaml!CJupiterWindow::RunCoreWindowMessageLoop(void)+0x4c [onecoreuap\windows\dxaml\xcp\dxaml\lib\jupiterwindow.cpp @ 1234] 
0e (Inline Function) --------`-------- Windows_UI_Xaml!CJupiterControl::RunMessageLoop(void)+0x1f [onecoreuap\windows\dxaml\xcp\dxaml\lib\jupitercontrol.cpp @ 1065] 
0f 000000cb`2caff620 00007ffc`3fc86d4a Windows_UI_Xaml!DirectUI::DXamlCore::RunMessageLoop(void)+0x47 [onecoreuap\windows\dxaml\xcp\dxaml\lib\dxamlcore.cpp @ 2463] 
10 000000cb`2caff690 00007ffc`3fc3efaf twinapi_appcore!Windows::ApplicationModel::Core::CoreApplicationView::Run+0x3a
11 000000cb`2caff6c0 00007ffc`47bcd8e9 twinapi_appcore!<lambda_643db08282a766b00cec20194396f531>::operator()+0x1ef
12 000000cb`2caff760 00007ffc`47c67034 shcore!_WrapperThreadProc+0xe9
13 000000cb`2caff840 00007ffc`4939cec1 KERNEL32!BaseThreadInitThunk+0x14
14 000000cb`2caff870 00000000`00000000 ntdll!RtlUserThreadStart+0x21

I guess

auto firstTab = page->_GetTerminalTabImpl(0);

works not really good with

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

I create a PR fixing this: #8168

…lette-bold-match

Dustin L. Howett (DHowett)

I love this. Thanks so much.

Deleted user (ghost) · 2020-11-11T18:12:20Z

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

Handy links:

Don-Vito added 3 commits October 20, 2020 18:02

6646: implement highlighted text control

2ea4085

Merge remote-tracking branch 'upstream/master' into fet/6646-command-…

9b95a29

…palette-bold-match

6646: fix profanity

34a0fcb