mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
keymap_editor: Remove redundant parentheses when displaying keymap context (#50906)
Closes #34976 ## Root Cause The current implementation of `fmt::Display` in `KeyBindingContextPredicate` hardcodes a parentheses around `And` and `Or` cases. Causing the text displayed on the keymap editor to have nested parentheses. This PR refactors the display logic and make the parentheses added only when there's nested operator. ## As is/ To be As is | To be --- | --- <img width="800" height="1101" alt="Screenshot 2026-03-06 at 15 07 10" src="https://github.com/user-attachments/assets/33b4366d-4d16-48db-9cc6-b79d6cc7ddc7" /> | <img width="800" height="977" alt="Screenshot 2026-03-06 at 14 54 57" src="https://github.com/user-attachments/assets/3d5ed38d-949d-4265-8fad-e29250c3a285" /> ## Test coverage <img width="800" height="916" alt="Screenshot 2026-03-06 at 15 00 42" src="https://github.com/user-attachments/assets/5846b908-5c86-48a7-919e-a7ae198484a8" /> Before you mark this PR as ready for review, make sure that you have: - [x] Added a solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects - [ ] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - keymap_editor: Remove redundant parentheses when displaying keymap context --------- Co-authored-by: Ben Kunkle <ben@zed.dev>
This commit is contained in:
parent
5aa037e4a0
commit
74ad75843e
1 changed files with 138 additions and 7 deletions
|
|
@ -199,13 +199,20 @@ pub enum KeyBindingContextPredicate {
|
|||
impl fmt::Display for KeyBindingContextPredicate {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
match self {
|
||||
Self::Identifier(name) => write!(f, "{}", name),
|
||||
Self::Equal(left, right) => write!(f, "{} == {}", left, right),
|
||||
Self::NotEqual(left, right) => write!(f, "{} != {}", left, right),
|
||||
Self::Not(pred) => write!(f, "!{}", pred),
|
||||
Self::Descendant(parent, child) => write!(f, "{} > {}", parent, child),
|
||||
Self::And(left, right) => write!(f, "({} && {})", left, right),
|
||||
Self::Or(left, right) => write!(f, "({} || {})", left, right),
|
||||
Self::Identifier(name) => write!(f, "{name}"),
|
||||
Self::Equal(left, right) => write!(f, "{left} == {right}"),
|
||||
Self::NotEqual(left, right) => write!(f, "{left} != {right}"),
|
||||
Self::Descendant(parent, child) => write!(f, "{parent} > {child}"),
|
||||
Self::Not(pred) => match pred.as_ref() {
|
||||
Self::Identifier(name) => write!(f, "!{name}"),
|
||||
_ => write!(f, "!({pred})"),
|
||||
},
|
||||
Self::And(..) => self.fmt_joined(f, " && ", LogicalOperator::And, |node| {
|
||||
matches!(node, Self::Or(..))
|
||||
}),
|
||||
Self::Or(..) => self.fmt_joined(f, " || ", LogicalOperator::Or, |node| {
|
||||
matches!(node, Self::And(..))
|
||||
}),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -436,6 +443,52 @@ impl KeyBindingContextPredicate {
|
|||
anyhow::bail!("operands of != must be identifiers");
|
||||
}
|
||||
}
|
||||
|
||||
fn fmt_joined(
|
||||
&self,
|
||||
f: &mut fmt::Formatter<'_>,
|
||||
separator: &str,
|
||||
operator: LogicalOperator,
|
||||
needs_parens: impl Fn(&Self) -> bool + Copy,
|
||||
) -> fmt::Result {
|
||||
let mut first = true;
|
||||
self.fmt_joined_inner(f, separator, operator, needs_parens, &mut first)
|
||||
}
|
||||
|
||||
fn fmt_joined_inner(
|
||||
&self,
|
||||
f: &mut fmt::Formatter<'_>,
|
||||
separator: &str,
|
||||
operator: LogicalOperator,
|
||||
needs_parens: impl Fn(&Self) -> bool + Copy,
|
||||
first: &mut bool,
|
||||
) -> fmt::Result {
|
||||
match (operator, self) {
|
||||
(LogicalOperator::And, Self::And(left, right))
|
||||
| (LogicalOperator::Or, Self::Or(left, right)) => {
|
||||
left.fmt_joined_inner(f, separator, operator, needs_parens, first)?;
|
||||
right.fmt_joined_inner(f, separator, operator, needs_parens, first)
|
||||
}
|
||||
(_, node) => {
|
||||
if !*first {
|
||||
f.write_str(separator)?;
|
||||
}
|
||||
*first = false;
|
||||
|
||||
if needs_parens(node) {
|
||||
write!(f, "({node})")
|
||||
} else {
|
||||
write!(f, "{node}")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum LogicalOperator {
|
||||
And,
|
||||
Or,
|
||||
}
|
||||
|
||||
const PRECEDENCE_CHILD: u32 = 1;
|
||||
|
|
@ -757,4 +810,82 @@ mod tests {
|
|||
assert!(not_workspace.eval(slice::from_ref(&editor_context)));
|
||||
assert!(!not_workspace.eval(&workspace_pane_editor));
|
||||
}
|
||||
|
||||
// MARK: - Display
|
||||
|
||||
#[test]
|
||||
fn test_context_display() {
|
||||
fn ident(s: &str) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(Identifier(SharedString::new(s)))
|
||||
}
|
||||
fn eq(a: &str, b: &str) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(Equal(SharedString::new(a), SharedString::new(b)))
|
||||
}
|
||||
fn not_eq(a: &str, b: &str) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(NotEqual(SharedString::new(a), SharedString::new(b)))
|
||||
}
|
||||
fn and(
|
||||
a: Box<KeyBindingContextPredicate>,
|
||||
b: Box<KeyBindingContextPredicate>,
|
||||
) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(And(a, b))
|
||||
}
|
||||
fn or(
|
||||
a: Box<KeyBindingContextPredicate>,
|
||||
b: Box<KeyBindingContextPredicate>,
|
||||
) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(Or(a, b))
|
||||
}
|
||||
fn descendant(
|
||||
a: Box<KeyBindingContextPredicate>,
|
||||
b: Box<KeyBindingContextPredicate>,
|
||||
) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(Descendant(a, b))
|
||||
}
|
||||
fn not(a: Box<KeyBindingContextPredicate>) -> Box<KeyBindingContextPredicate> {
|
||||
Box::new(Not(a))
|
||||
}
|
||||
|
||||
let test_cases = [
|
||||
(ident("a"), "a"),
|
||||
(eq("a", "b"), "a == b"),
|
||||
(not_eq("a", "b"), "a != b"),
|
||||
(descendant(ident("a"), ident("b")), "a > b"),
|
||||
(not(ident("a")), "!a"),
|
||||
(not_eq("a", "b"), "a != b"),
|
||||
(descendant(ident("a"), ident("b")), "a > b"),
|
||||
(not(and(ident("a"), ident("b"))), "!(a && b)"),
|
||||
(not(or(ident("a"), ident("b"))), "!(a || b)"),
|
||||
(and(ident("a"), ident("b")), "a && b"),
|
||||
(and(and(ident("a"), ident("b")), ident("c")), "a && b && c"),
|
||||
(or(ident("a"), ident("b")), "a || b"),
|
||||
(or(or(ident("a"), ident("b")), ident("c")), "a || b || c"),
|
||||
(or(ident("a"), and(ident("b"), ident("c"))), "a || (b && c)"),
|
||||
(
|
||||
and(
|
||||
and(
|
||||
and(ident("a"), eq("b", "c")),
|
||||
not(descendant(ident("d"), ident("e"))),
|
||||
),
|
||||
eq("f", "g"),
|
||||
),
|
||||
"a && b == c && !(d > e) && f == g",
|
||||
),
|
||||
(
|
||||
and(and(ident("a"), or(ident("b"), ident("c"))), ident("d")),
|
||||
"a && (b || c) && d",
|
||||
),
|
||||
(
|
||||
or(or(ident("a"), and(ident("b"), ident("c"))), ident("d")),
|
||||
"a || (b && c) || d",
|
||||
),
|
||||
];
|
||||
|
||||
for (predicate, expected) in test_cases {
|
||||
let actual = predicate.to_string();
|
||||
assert_eq!(actual, expected);
|
||||
let parsed = KeyBindingContextPredicate::parse(&actual).unwrap();
|
||||
assert_eq!(parsed, *predicate);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue