Fix stale pending keybinding indicators on focus change#44678
Fix stale pending keybinding indicators on focus change#44678ConradIrwin merged 13 commits intozed-industries:mainfrom
Conversation
|
Hi @ConradIrwin , we surface this issue since we introduced the timeout logic in this PR #42659. The issue was existing before, but we will have 1 second timeout to flush it anyway. We should address this before adding the which-key menu. |
|
Please let me know if we need to add any additional integration or UI tests for this one. Thanks |
|
Makes sense. Can we do this without modifying |
I explored using |
|
Makes sense, it just feels like it's the wrong layer of abstraction. Could the component listen to the window's focus event and check if the pending keybindings have changed? |
Good question. But I am afraid of there are not better choice to handle this, listening to the focus event is not enough. Apparently, how we should handle the platform hooks (lets say you open a OS menu on the top bar), the focus path doesn't change for this case |
|
Another thing is I think even for window focus changes, we’d still need glue code somewhere to detect “pending keystrokes were cleared because a focus change happened” and notify every listener manually, which effectively duplicates what the existing pending-input observer already does. What do you think? I am not 100% sure |
|
Maybe we should pass a cx into |
|
Hi @ConradIrwin , thank you for following up. I do have some thoughts over the last weekend and I totally agree we should approach this differently. It is not good to have that change in the draw() function which is in the rendering time and also it is not aligned with overall GPUI architecture.
That is one alternative, and it is an ideal and concrete approach. However, it will have some risks for breaking changes on API interface. Currently there are lots of references for this window.focus() function. Personally, I am more a bit hesitate to touch that part yet only for fixing this particular issue. There are some other alternatives I am still exploring. I will post those comments later today. Thank you. |
|
Makes sense, hopefully |
Updated all focus() and focus_next/focus_prev calls to match the new API signature that requires &mut App as a second parameter.
Resolved conflicts in terminal_panel.rs by combining both changes: - Added cx parameter to PaneGrid::split() calls (from main) - Added cx parameter to window.focus() calls (from branch) Also fixed a new focus() call in branch_picker.rs that came from main.
|
Hi @ConradIrwin , it seems like now it has some merge conflicts. Would you be able to address and merge it? Or I can look into it though. Thank you |
Resolved modify/delete conflict: - crates/onboarding/src/welcome.rs was deleted in main (as part of launchpad page changes) and modified in PR. Accepted deletion since the file no longer exists in main.
|
blergh, I can't do it fast enough. Let me try one more time |
Closes #ISSUE Problem: - The status bar’s pending keystroke indicator (shown next to --NORMAL-- in Vim mode) didn’t clear when focus moved to another context, e.g. hitting g in the editor then clicking the Git panel. The keymap state correctly canceled the prefix, but observers that render the indicator never received a “pending input changed” notification, so the UI kept showing stale prefixes until a new keystroke occurred. Fix: - The change introduces a `pending_input_changed_queued` flag and a new helper `notify_pending_input_if_needed` which will flushes the queued notification as soon as we have an App context. The `pending_input_changed` now resets the flag after notifying subscribers. Before: https://github.com/user-attachments/assets/7bec4c34-acbf-42bd-b0d1-88df5ff099aa After: https://github.com/user-attachments/assets/2264dc93-3405-4d63-ad8f-50ada6733ae7 Release Notes: - Fixed: pending keybinding prefixes on the status bar now clear immediately when focus moves to another panel or UI context. --------- Co-authored-by: Nathan Sobo <nathan@zed.dev> Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
Closes #ISSUE Problem: - The status bar’s pending keystroke indicator (shown next to --NORMAL-- in Vim mode) didn’t clear when focus moved to another context, e.g. hitting g in the editor then clicking the Git panel. The keymap state correctly canceled the prefix, but observers that render the indicator never received a “pending input changed” notification, so the UI kept showing stale prefixes until a new keystroke occurred. Fix: - The change introduces a `pending_input_changed_queued` flag and a new helper `notify_pending_input_if_needed` which will flushes the queued notification as soon as we have an App context. The `pending_input_changed` now resets the flag after notifying subscribers. Before: https://github.com/user-attachments/assets/7bec4c34-acbf-42bd-b0d1-88df5ff099aa After: https://github.com/user-attachments/assets/2264dc93-3405-4d63-ad8f-50ada6733ae7 Release Notes: - Fixed: pending keybinding prefixes on the status bar now clear immediately when focus moves to another panel or UI context. --------- Co-authored-by: Nathan Sobo <nathan@zed.dev> Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
…es#44678) Closes #ISSUE Problem: - The status bar’s pending keystroke indicator (shown next to --NORMAL-- in Vim mode) didn’t clear when focus moved to another context, e.g. hitting g in the editor then clicking the Git panel. The keymap state correctly canceled the prefix, but observers that render the indicator never received a “pending input changed” notification, so the UI kept showing stale prefixes until a new keystroke occurred. Fix: - The change introduces a `pending_input_changed_queued` flag and a new helper `notify_pending_input_if_needed` which will flushes the queued notification as soon as we have an App context. The `pending_input_changed` now resets the flag after notifying subscribers. Before: https://github.com/user-attachments/assets/7bec4c34-acbf-42bd-b0d1-88df5ff099aa After: https://github.com/user-attachments/assets/2264dc93-3405-4d63-ad8f-50ada6733ae7 Release Notes: - Fixed: pending keybinding prefixes on the status bar now clear immediately when focus moves to another panel or UI context. --------- Co-authored-by: Nathan Sobo <nathan@zed.dev> Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
…es#44678) Closes #ISSUE Problem: - The status bar’s pending keystroke indicator (shown next to --NORMAL-- in Vim mode) didn’t clear when focus moved to another context, e.g. hitting g in the editor then clicking the Git panel. The keymap state correctly canceled the prefix, but observers that render the indicator never received a “pending input changed” notification, so the UI kept showing stale prefixes until a new keystroke occurred. Fix: - The change introduces a `pending_input_changed_queued` flag and a new helper `notify_pending_input_if_needed` which will flushes the queued notification as soon as we have an App context. The `pending_input_changed` now resets the flag after notifying subscribers. Before: https://github.com/user-attachments/assets/7bec4c34-acbf-42bd-b0d1-88df5ff099aa After: https://github.com/user-attachments/assets/2264dc93-3405-4d63-ad8f-50ada6733ae7 Release Notes: - Fixed: pending keybinding prefixes on the status bar now clear immediately when focus moves to another panel or UI context. --------- Co-authored-by: Nathan Sobo <nathan@zed.dev> Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
Closes #ISSUE
Problem:
Fix:
pending_input_changed_queuedflag and a new helpernotify_pending_input_if_neededwhich will flushes the queued notification as soon as we have an App context. Thepending_input_changednow resets the flag after notifying subscribers.Before:
Before.mov
After:
After.mov
Release Notes: