mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
Fix git panel context menu keybinding jitter (#52217)
Context The git panel's context menu caused visual jitter (flickering/jumping) when opened via right-click on a tracked file. The root cause was in `dispatch_context()`: it used `self.focus_handle == focused` to check if the panel itself was directly focused, but when a context menu opened, focus moved to the menu (a child element), causing the `"menu"` and `"ChangesList"` key contexts to be dropped. This triggered a re-render with different keybindings, which re-added them, creating a loop of jitter. The fix replaces the direct focus equality check with `self.focus_handle.contains_focused(window, cx)`, which returns `true` when any child element (including the context menu) holds focus within the panel's focus tree. This is consistent with how other panels (project panel, outline panel, collab panel) handle focus-based dispatch contexts. Closes #51813 ## Demo **Before fix:** https://github.com/user-attachments/assets/e18d49b2-72a6-4411-8ec5-519e36628f29 **After fix:** https://github.com/user-attachments/assets/94c936d2-1e81-4d28-a86a-8b1ed76ddde1 ## How to Review This is a small, focused change in a single file: `crates/git_ui/src/git_panel.rs`. 1. **The fix** (~line 974): `dispatch_context()` method — the old code checked direct focus equality (`self.focus_handle == focused`), the new code uses `self.focus_handle.contains_focused(window, cx)` and restructures the conditionals so `CommitEditor` is checked first via `if/else if`. 2. **The test** (~line 7871): `test_dispatch_context_with_focus_states` — verifies 4 focus state transitions: commit editor focused, changes list focused, back to commit editor, and back to changes list. Each case asserts the correct key contexts are present/absent. ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed git panel context menu jitter caused by keybinding dispatch context flickering when right-clicking on tracked files (#51813)
This commit is contained in:
parent
ce7cd7c163
commit
45be23c56c
1 changed files with 133 additions and 9 deletions
|
|
@ -975,16 +975,11 @@ impl GitPanel {
|
|||
let mut dispatch_context = KeyContext::new_with_defaults();
|
||||
dispatch_context.add("GitPanel");
|
||||
|
||||
if window
|
||||
.focused(cx)
|
||||
.is_some_and(|focused| self.focus_handle == focused)
|
||||
{
|
||||
dispatch_context.add("menu");
|
||||
dispatch_context.add("ChangesList");
|
||||
}
|
||||
|
||||
if self.commit_editor.read(cx).is_focused(window) {
|
||||
dispatch_context.add("CommitEditor");
|
||||
} else if self.focus_handle.contains_focused(window, cx) {
|
||||
dispatch_context.add("menu");
|
||||
dispatch_context.add("ChangesList");
|
||||
}
|
||||
|
||||
dispatch_context
|
||||
|
|
@ -6488,7 +6483,7 @@ mod tests {
|
|||
repository::repo_path,
|
||||
status::{StatusCode, UnmergedStatus, UnmergedStatusCode},
|
||||
};
|
||||
use gpui::{TestAppContext, UpdateGlobal, VisualTestContext};
|
||||
use gpui::{TestAppContext, UpdateGlobal, VisualTestContext, px};
|
||||
use indoc::indoc;
|
||||
use project::FakeFs;
|
||||
use serde_json::json;
|
||||
|
|
@ -7874,4 +7869,133 @@ mod tests {
|
|||
let message = panel.update(cx, |panel, cx| panel.suggest_commit_message(cx));
|
||||
assert_eq!(message, Some("Update tracked".to_string()));
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_dispatch_context_with_focus_states(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
let fs = FakeFs::new(cx.background_executor.clone());
|
||||
fs.insert_tree(
|
||||
path!("/project"),
|
||||
json!({
|
||||
".git": {},
|
||||
"tracked": "tracked\n",
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
fs.set_head_and_index_for_repo(
|
||||
path!("/project/.git").as_ref(),
|
||||
&[("tracked", "old tracked\n".into())],
|
||||
);
|
||||
|
||||
let project = Project::test(fs.clone(), [Path::new(path!("/project"))], cx).await;
|
||||
let window_handle =
|
||||
cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
let workspace = window_handle
|
||||
.read_with(cx, |mw, _| mw.workspace().clone())
|
||||
.unwrap();
|
||||
let cx = &mut VisualTestContext::from_window(window_handle.into(), cx);
|
||||
let panel = workspace.update_in(cx, GitPanel::new);
|
||||
|
||||
let handle = cx.update_window_entity(&panel, |panel, _, _| {
|
||||
std::mem::replace(&mut panel.update_visible_entries_task, Task::ready(()))
|
||||
});
|
||||
cx.executor().advance_clock(2 * UPDATE_DEBOUNCE);
|
||||
handle.await;
|
||||
|
||||
// Case 1: Focus the commit editor — should have "CommitEditor" but NOT "menu"/"ChangesList"
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.focus_editor(&FocusEditor, window, cx);
|
||||
let editor_is_focused = panel.commit_editor.read(cx).is_focused(window);
|
||||
assert!(
|
||||
editor_is_focused,
|
||||
"commit editor should be focused after focus_editor action"
|
||||
);
|
||||
let context = panel.dispatch_context(window, cx);
|
||||
assert!(
|
||||
context.contains("GitPanel"),
|
||||
"should always have GitPanel context"
|
||||
);
|
||||
assert!(
|
||||
context.contains("CommitEditor"),
|
||||
"should have CommitEditor context when commit editor is focused"
|
||||
);
|
||||
assert!(
|
||||
!context.contains("menu"),
|
||||
"should not have menu context when commit editor is focused"
|
||||
);
|
||||
assert!(
|
||||
!context.contains("ChangesList"),
|
||||
"should not have ChangesList context when commit editor is focused"
|
||||
);
|
||||
});
|
||||
|
||||
// Case 2: Focus the panel's focus handle directly — should have "menu" and "ChangesList".
|
||||
// We force a draw via simulate_resize to ensure the dispatch tree is populated,
|
||||
// since contains_focused() depends on the rendered dispatch tree.
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.focus_handle.focus(window, cx);
|
||||
});
|
||||
cx.simulate_resize(gpui::size(px(800.), px(600.)));
|
||||
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
let context = panel.dispatch_context(window, cx);
|
||||
assert!(
|
||||
context.contains("GitPanel"),
|
||||
"should always have GitPanel context"
|
||||
);
|
||||
assert!(
|
||||
context.contains("menu"),
|
||||
"should have menu context when changes list is focused"
|
||||
);
|
||||
assert!(
|
||||
context.contains("ChangesList"),
|
||||
"should have ChangesList context when changes list is focused"
|
||||
);
|
||||
assert!(
|
||||
!context.contains("CommitEditor"),
|
||||
"should not have CommitEditor context when changes list is focused"
|
||||
);
|
||||
});
|
||||
|
||||
// Case 3: Switch back to commit editor and verify context switches correctly
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.focus_editor(&FocusEditor, window, cx);
|
||||
});
|
||||
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
let context = panel.dispatch_context(window, cx);
|
||||
assert!(
|
||||
context.contains("CommitEditor"),
|
||||
"should have CommitEditor after switching focus back to editor"
|
||||
);
|
||||
assert!(
|
||||
!context.contains("menu"),
|
||||
"should not have menu after switching focus back to editor"
|
||||
);
|
||||
});
|
||||
|
||||
// Case 4: Re-focus changes list and verify it transitions back correctly
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.focus_handle.focus(window, cx);
|
||||
});
|
||||
cx.simulate_resize(gpui::size(px(800.), px(600.)));
|
||||
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
assert!(
|
||||
panel.focus_handle.contains_focused(window, cx),
|
||||
"panel focus handle should report contains_focused when directly focused"
|
||||
);
|
||||
let context = panel.dispatch_context(window, cx);
|
||||
assert!(
|
||||
context.contains("menu"),
|
||||
"should have menu context after re-focusing changes list"
|
||||
);
|
||||
assert!(
|
||||
context.contains("ChangesList"),
|
||||
"should have ChangesList context after re-focusing changes list"
|
||||
);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue