Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IDs to Commands #16904

Merged
merged 32 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
90627b3
add origin tag
PankajBhojwani Mar 5, 2024
9dff28f
update calls in tests
PankajBhojwani Mar 5, 2024
8bcbd0b
fix tests
PankajBhojwani Mar 5, 2024
052d063
ah one of the tests uses this
PankajBhojwani Mar 5, 2024
8cc82de
generated
PankajBhojwani Mar 5, 2024
642d0ab
inbox makes more sense
PankajBhojwani Mar 5, 2024
66fe08f
default ids
PankajBhojwani Mar 7, 2024
2bb1b6c
conflict
PankajBhojwani Mar 19, 2024
be193b2
merge origin
PankajBhojwani Mar 19, 2024
db528c9
generate IDs for user commands
PankajBhojwani Mar 26, 2024
7c907fe
nits
PankajBhojwani Mar 26, 2024
b43191d
spacing
PankajBhojwani Mar 26, 2024
2093660
line
PankajBhojwani Mar 26, 2024
6c32539
string of numbers is unsightly but it works
PankajBhojwani Mar 27, 2024
eccd87f
update comment
PankajBhojwani Mar 27, 2024
44510dc
move id generation to fixupusersettings
PankajBhojwani Mar 28, 2024
10d1fc8
this way is better
PankajBhojwani Mar 28, 2024
71bf90f
even better, also get the ID from json
PankajBhojwani Mar 28, 2024
d57c7a1
move this
PankajBhojwani Mar 28, 2024
5c2307c
fix test
PankajBhojwani Mar 28, 2024
9fc6972
add tests
PankajBhojwani Mar 29, 2024
dca7df5
excess line
PankajBhojwani Mar 29, 2024
dd25ed7
change tests
PankajBhojwani Apr 1, 2024
6e293a5
Everytime
PankajBhojwani Apr 1, 2024
af2d22f
defaults conflict
PankajBhojwani Apr 11, 2024
bdf42c2
first round of nits
PankajBhojwani Apr 11, 2024
12f3aa9
truncate and hex, debug assert
PankajBhojwani Apr 12, 2024
aa49212
null check
PankajBhojwani Apr 12, 2024
5ee630e
fmt is smart
PankajBhojwani Apr 12, 2024
360b92e
fmt_compile, fix test
PankajBhojwani Apr 17, 2024
5e70911
remove 0
PankajBhojwani Apr 17, 2024
ca3eb87
rename and comment
PankajBhojwani Apr 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,36 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return found != GeneratedActionNames.end() ? found->second : L"";
}

// Function Description:
// - This will generate an ID for this ActionAndArgs, based on the ShortcutAction and the Args
// - It will always create the same ID if the ShortcutAction and the Args are the same
// - Note: this should only be called for User-created actions
// - Example: The "SendInput 'abc'" action will have the generated ID "User.sendInput.<hash of 'abc'>"
// Return Value:
// - The ID, based on the ShortcutAction and the Args
winrt::hstring ActionAndArgs::GenerateID() const
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These IDs are very similar to our InternalActionID except instead of also hashing the ShortcutAction, we only hash the Args and we leave the ShortcutAction as a string. Since these will end up in the user file, I prefer this as it will be more readable

Copy link
Member

Choose a reason for hiding this comment

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

this is clever, thanks!

{
if (_Action != ShortcutAction::Invalid)
{
auto actionKeyString = ActionToStringMap.find(_Action)->second;
auto result = fmt::format(FMT_COMPILE(L"User.{}"), actionKeyString);
if (_Args)
{
// If there are args, we need to append the hash of the args
// However, to make it a little more presentable we
// 1. truncate the hash to 32 bits
// 2. convert it to a hex string
// there is a _tiny_ chance of collision because of the truncate but unlikely for
// the number of commands a user is expected to have
const auto argsHash32 = static_cast<uint32_t>(_Args.Hash() & 0xFFFFFFFF);
// {0:X} formats the truncated hash to an uppercase hex string
fmt::format_to(std::back_inserter(result), FMT_COMPILE(L".{:X}"), argsHash32);
}
return winrt::hstring{ result };
}
return L"";
}

winrt::hstring ActionAndArgs::Serialize(const winrt::Windows::Foundation::Collections::IVector<Model::ActionAndArgs>& args)
{
Json::Value json{ Json::objectValue };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
com_ptr<ActionAndArgs> Copy() const;

hstring GenerateName() const;
hstring GenerateID() const;

WINRT_PROPERTY(ShortcutAction, Action, ShortcutAction::Invalid);
WINRT_PROPERTY(IActionArgs, Args, nullptr);
Expand Down
19 changes: 19 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,25 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return nullptr;
}

bool ActionMap::GenerateIDsForActions()
{
bool fixedUp{ false };
for (auto actionPair : _ActionMap)
{
auto cmdImpl{ winrt::get_self<Command>(actionPair.second) };

// Note: this function should ONLY be called for the action map in the user's settings file
// this debug assert should verify that for debug builds
assert(cmdImpl->Origin() == OriginTag::User);

if (cmdImpl->ID().empty())
{
fixedUp = cmdImpl->GenerateID() || fixedUp;
}
}
return fixedUp;
}

// Method Description:
// - Rebinds a key binding to a new key chord
// Arguments:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Json::Value ToJson() const;

// modification
bool GenerateIDsForActions();
bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys);
void DeleteKeyBinding(const Control::KeyChord& keys);
void RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,10 @@ bool SettingsLoader::FixupUserSettings()
fixedUp = true;
}

// we need to generate an ID for a command in the user settings if it doesn't already have one
auto actionMap{ winrt::get_self<ActionMap>(userSettings.globals->ActionMap()) };
fixedUp = actionMap->GenerateIDsForActions() || fixedUp;
Comment on lines +507 to +509
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we aren't actually using these IDs yet, this could definitely be moved into the follow up PR that is actually going to refactor our maps to use the new IDs. However then that leaves this PR as just "adding an ID field to Command and barely populating them anywhere". I'm okay with either if anyone feels strongly about it

Copy link
Member

Choose a reason for hiding this comment

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

nah this is great


return fixedUp;
}

Expand Down
32 changes: 31 additions & 1 deletion src/cascadia/TerminalSettingsModel/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace winrt
}

static constexpr std::string_view NameKey{ "name" };
static constexpr std::string_view IDKey{ "id" };
static constexpr std::string_view IconKey{ "icon" };
static constexpr std::string_view ActionKey{ "command" };
static constexpr std::string_view IterateOnKey{ "iterateOn" };
Expand All @@ -39,7 +40,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
auto command{ winrt::make_self<Command>() };
command->_name = _name;
command->_Origin = OriginTag::User;
command->_Origin = _Origin;
command->_ID = _ID;
command->_ActionAndArgs = *get_self<implementation::ActionAndArgs>(_ActionAndArgs)->Copy();
command->_keyMappings = _keyMappings;
command->_iconPath = _iconPath;
Expand Down Expand Up @@ -114,6 +116,25 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
}
}

hstring Command::ID() const noexcept
{
return hstring{ _ID };
}

bool Command::GenerateID()
{
if (_ActionAndArgs)
{
auto actionAndArgsImpl{ winrt::get_self<implementation::ActionAndArgs>(_ActionAndArgs) };
if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty())
{
_ID = generatedID;
return true;
}
}
return false;
}

void Command::Name(const hstring& value)
{
if (!_name.has_value() || _name.value() != value)
Expand Down Expand Up @@ -264,6 +285,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
auto result = winrt::make_self<Command>();
result->_Origin = origin;
JsonUtils::GetValueForKey(json, IDKey, result->_ID);

auto nested = false;
JsonUtils::GetValueForKey(json, IterateOnKey, result->_IterateOn);
Expand Down Expand Up @@ -423,6 +445,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Json::Value cmdJson{ Json::ValueType::objectValue };
JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath);
JsonUtils::SetValueForKey(cmdJson, NameKey, _name);
if (!_ID.empty())
{
JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
}

if (_ActionAndArgs)
{
Expand All @@ -443,6 +469,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// First iteration also writes icon and name
JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath);
JsonUtils::SetValueForKey(cmdJson, NameKey, _name);
if (!_ID.empty())
{
JsonUtils::SetValueForKey(cmdJson, IDKey, _ID);
}
}

if (_ActionAndArgs)
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalSettingsModel/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
hstring Name() const noexcept;
void Name(const hstring& name);

hstring ID() const noexcept;
bool GenerateID();

Control::KeyChord Keys() const noexcept;
hstring KeyChordText() const noexcept;
std::vector<Control::KeyChord> KeyMappings() const noexcept;
Expand All @@ -84,6 +87,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command> _subcommands{ nullptr };
std::vector<Control::KeyChord> _keyMappings;
std::optional<std::wstring> _name;
std::wstring _ID;
std::optional<std::wstring> _iconPath;
bool _nestedCommand{ false };

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/Command.idl
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Model
Command();

String Name { get; };
String ID { get; };
ActionAndArgs ActionAndArgs { get; };
Microsoft.Terminal.Control.KeyChord Keys { get; };
void RegisterKey(Microsoft.Terminal.Control.KeyChord keys);
Expand Down
Loading
Loading