Teach CommandPalette to persist recent command lines #11030
Conversation
ghost
added
Area-CmdPal
|
Going to hit some merge conflicts with #10972 but I suspect you'll win the race to be on main first. |
Not sure I will get there earlier, but if required would love to assist with the merge 😊 |
Thanks a lot for the comments! They are great as usual! |
There was a problem hiding this comment.
I still disagree about the implementation of CommandPalette::_updateRecentCommands. It's rather long and complex whereas the previous solution was only 3 LOC and certainly faster.
But it's not incorrect code, so I'll let others chime in and approve it in the meantime.
|
My approval doesn't mean anything but it looks good to me 👍 |
Thanks for helping with this one! |
There was a problem hiding this comment.
Okay so on load, we'll load the recent commands from state.json. When a command is run, we'll update state.json with our list of recent commands. So
- Window 1 loads ["foo", "bar"]
- Window 1 runs "baz" (state is now ["foo", "bar", "baz"])
- Window 2 loads ["foo", "bar", "baz"]
- Window 1 runs "boo" (state is now ["foo", "bar", "baz", "boo"])
- Window 2 runs "asdf" (state is now ["foo", "bar", "baz", "asdf"])
- Window 1 runs "qwer" (state is now ["foo", "bar", "baz", "boo", "qwer"])
that seems right to me. Thanks for doing this! This is a cool feature to have land.
| @@ -55,6 +55,48 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils | |||
| return fmt::format("{}[]", ConversionTrait<GUID>{}.TypeDescription()); | |||
| } | |||
| }; | |||
|
|
|||
| template<typename T> | |||
| struct ConversionTrait<winrt::Windows::Foundation::Collections::IVector<T>> | |||
There was a problem hiding this comment.
everybody's writing this exact same converter this week, aren't they 😝
|
You know, I think it's different still: state is part of the reload trigger, so Window 2 will pick up |
AH okay, didn't know that |
|
Does that mean all of settings is reloaded each time the command palette is opened/closed? |
From my understanding no. It is reloaded when the state file changes (with a throttling of 1 sec). |
|
That makes more sense. For a moment I was thinking that if state was changed, then settings was also refreshed. I knew about the throttling on loading the ApplicationState. |
| @@ -744,15 +739,14 @@ namespace winrt::TerminalApp::implementation | |||
| } | |||
| } | |||
|
|
|||
| std::optional<winrt::TerminalApp::FilteredCommand> CommandPalette::_buildCommandLineCommand(std::wstring const& commandLine) | |||
| std::optional<TerminalApp::FilteredCommand> CommandPalette::_buildCommandLineCommand(const hstring& commandLine) | |||
| { | |||
| if (commandLine.empty()) | |||
| { | |||
| return std::nullopt; | |||
There was a problem hiding this comment.
Huh, I just realized this. A FilteredCommand is already nullable, because it is a smart pointer type. Is there value that we get out of using nullopt (which would mean "not null, and also not populated" to provide a third state?
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
ghost
merged commit 
|
I wish there was a way to let other instances know that the state changed without actually writing the state file to disk. We currently write the state to disk up to once a second. It's fine compared to other applications, but I wish there was another way, like telling the OS "no you really don't need to flush this file to disk right away". |
The first time you open commandline mode, `recentCommands` doesn't exist yet. However, we immediately try to read the `Size()` in a couple places. This'll A/V and we'll crash 😨 The fix is easy - don't try and read the size of the non-existent `recentCommands` Found this while playing with #11069 Regressed in #11030 Didn't bother filing an issue for it when I have the fix in hand
|
🎉 Handy links: |
ghost
mentioned this pull request
Closes #11026