-
Notifications
You must be signed in to change notification settings - Fork 8
Add key alias support (closes #12) #21
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
base: main
Are you sure you want to change the base?
Add key alias support (closes #12) #21
Conversation
Implements key aliases feature allowing users to define reusable names for modifiers, keys, and key combinations, making configs more readable and maintainable. Features: - Three alias types: modifier, key, and keysym - Full recursive expansion at parse time (zero runtime overhead) - Circular reference detection with helpful error messages - Nested alias support (aliases can reference other aliases) - Compatible with existing syntax (modifiers combine like hyper/meh) Syntax: .alias $super cmd + alt # Modifier alias .alias $grave 0x32 # Key alias .alias $nav_left $super - h # Keysym alias Usage: $super - t : command # Use modifier alias ctrl - $grave : command # Use key alias $nav_left : command # Use keysym standalone ctrl + $nav_left : command # Add modifiers to keysym Implementation: - Tokenizer: Added Token_Alias for $identifier syntax - Parser: Added Alias union type with storage and resolution logic - Comprehensive error messages for type mismatches and undefined aliases - Full test coverage for all alias types and error cases - Documentation in SYNTAX.md and README.md with examples Inspired by asmvik/skhd#288
|
@copilot review |
|
@jackielii I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements key aliases for skhd.zig, allowing users to define reusable names for modifiers, keys, and key combinations. The feature supports three alias types (modifier, key, keysym), recursive expansion with circular reference detection, and provides comprehensive error messages.
Key Changes:
- Added
Token_Aliasto tokenizer for$identifiersyntax recognition - Implemented
Aliasunion type with resolution logic and type checking in parser - Added comprehensive test coverage for all alias types and error scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tokenizer.zig | Added Token_Alias type and $ character handling to recognize alias identifiers |
| src/Parser.zig | Implemented alias storage, resolution, type checking, and integration with hotkey parsing logic |
| src/tests.zig | Updated error message test to include "alias" as an expected token type |
| SYNTAX.md | Added comprehensive documentation section explaining alias types, syntax, usage contexts, and common errors |
| README.md | Added key aliases to feature list and included practical examples in window management section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Use keysym alias standalone or with additional modifiers | ||
| $focus_west : @yabai_focus("west") | ||
| ctrl + $focus_west : @yabai_focus("west") # With extra modifier |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two examples (lines 400-401) demonstrate the same keybinding calling the same command, which doesn't effectively showcase the feature. Line 400 shows $focus_west (which expands to $super - h = cmd + alt - h) and line 401 shows ctrl + $focus_west (which adds ctrl to make ctrl + cmd + alt - h). However, both execute the identical command @yabai_focus("west"), making it unclear why you'd bind different key combinations to the same action. Consider either:
- Using different commands (e.g.,
@yabai_focus("west")vs@yabai_focus_and_warp("west")) - Adding a comment explaining this is just demonstrating the syntax flexibility
- Removing the duplicate binding as was done in the test on line 2594-2596
| ctrl + $focus_west : @yabai_focus("west") # With extra modifier | |
| ctrl + $focus_west : @yabai_focus("west") # Demonstrates that key aliases can be used with extra modifiers; both bindings perform the same action for syntax flexibility illustration |
| } else if (self.match(.Token_Alias)) { | ||
| // Nested alias | ||
| const token = self.previous(); | ||
| var visited = std.ArrayList([]const u8).init(self.allocator); | ||
| defer visited.deinit(); | ||
| const alias = try self.resolve_alias_internal(token.text, &visited); | ||
| // Return whatever type the nested alias is | ||
| return alias; |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is unreachable. At line 1088, self.match(.Token_Alias) will consume any Token_Alias token, so this else if branch can never be reached. The nested alias case is already handled within the first if block (lines 1093-1110), but this unreachable code suggests there may have been an intention to handle standalone alias references differently. Consider removing this dead code or restructuring the logic if standalone alias handling was intended.
| } else if (self.match(.Token_Alias)) { | |
| // Nested alias | |
| const token = self.previous(); | |
| var visited = std.ArrayList([]const u8).init(self.allocator); | |
| defer visited.deinit(); | |
| const alias = try self.resolve_alias_internal(token.text, &visited); | |
| // Return whatever type the nested alias is | |
| return alias; |
| "Invalid alias definition. Expected one of:\n" ++ | ||
| " Modifier: .alias $name cmd + alt + ctrl\n" ++ | ||
| " Keysym: .alias $name cmd + shift - t\n" ++ | ||
| " Key combo: .alias $name shift - 1\n" ++ |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinction between "Keysym" (line 1192) and "Key combo" (line 1193) is unclear - both show the same pattern (modifier - key). Based on the code logic and the Alias union type (which has modifier, key, and keysym variants), consider either:
- Renaming "Key combo" to "Key" and showing an example without modifiers like
.alias $name 0x32or.alias $name delete - Removing the redundant "Key combo" line entirely
This would make it clearer that there are three distinct types: modifiers (no dash), keys (no modifiers), and keysyms (modifiers + dash + key).
| " Key combo: .alias $name shift - 1\n" ++ | |
| " Key: .alias $name delete\n" ++ |
| #### Circular References | ||
|
|
||
| ```bash | ||
| .alias $a $b + shift # Error: $b not defined yet | ||
| .alias $b $a + ctrl | ||
| ``` |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example shows forward reference (using an undefined alias), not a circular reference. A true circular reference would be:
.alias $a $b + shift
.alias $b $a + ctrl # Both depend on each otherHowever, this would also fail with "Undefined alias" since $b isn't defined when $a tries to use it. Consider either:
- Showing a different example that better illustrates the concept (though true circularity may be hard to achieve in practice)
- Renaming this section to "Forward References" or "Undefined Aliases" to match what the example actually demonstrates
- Clarifying that circular reference detection exists but most circular cases manifest as undefined alias errors
jackielii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot generally I think the PR should be restructured in terms of high level structure
| if (found_modifier) { | ||
| // Handle modifier or alias at the start | ||
| var found_modifier = false; | ||
| var is_standalone_keysym = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot is this the opposite of is_alias?
| const next_is_activate = self.peek_check(.Token_Activate); | ||
|
|
||
| // If followed by : | [ ~ -> ; , it's a standalone keysym alias | ||
| if (next_is_command or next_is_forward or next_is_begin_list or next_is_unbound or next_is_arrow or next_is_activate or self.peek() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this check may not be exhaustive. Is there another way we can structure this?
| // If followed by : | [ ~ -> ; , it's a standalone keysym alias | ||
| if (next_is_command or next_is_forward or next_is_begin_list or next_is_unbound or next_is_arrow or next_is_activate or self.peek() == null) { | ||
| // Standalone keysym alias: $terminal_key : command | ||
| var visited = std.ArrayList([]const u8).init(self.allocator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point of creating a list and deinit it within this block? if this is the only use case, why not do it within the resolve_keysym_alias_internal?
| const keypress = try self.resolve_keysym_alias_internal(alias_token.text, &visited); | ||
| hotkey.flags = hotkey.flags.merge(keypress.flags); | ||
| hotkey.key = keypress.key; | ||
| is_standalone_keysym = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so the logic is next_is_command or next_is_forward or next_is_begin_list or next_is_unbound or next_is_arrow or next_is_activate or self.peek() == null then is_standalone_keysym = true then we should just use is_alias = next_is_command or next_is_forward or next_is_begin_list or next_is_unbound or next_is_arrow or next_is_activate or self.peek() == null or a better logic with better forward compatibility
|
@jackielii I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
Implements key aliases feature allowing users to define reusable names for modifiers, keys, and key combinations, making configs more readable and maintainable.
Features:
Syntax:
.alias $super cmd + alt # Modifier alias
.alias $grave 0x32 # Key alias
.alias $nav_left $super - h # Keysym alias
Usage:
$super - t : command # Use modifier alias
ctrl - $grave : command # Use key alias
$nav_left : command # Use keysym standalone
ctrl + $nav_left : command # Add modifiers to keysym
Implementation:
Inspired by asmvik/skhd#288