Fix ACP permission dialog keyboard shortcuts not working - #58365
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @wazer24 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
We require contributors to sign our Contributor License Agreement, and we don't have @wazer24 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
'@cla-bot check'. |
|
We require contributors to sign our Contributor License Agreement, and we don't have @wazer24 on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
'@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
2 similar comments
|
The cla-bot has been summoned, and re-checked this pull request! |
|
The cla-bot has been summoned, and re-checked this pull request! |
dinocosta
left a comment
There was a problem hiding this comment.
Hey @wazer24 ! Thank you for taking the time to contribute to Zed 🙂
Before reviewing these changes, I'd just like to double-check that you've actually be able to reproduce the issue reported in #57546 , as it seems I'm unable to reproduce it using the latest stable release (1.4.4).
Lastly, there's some formatting changes to assets/keymaps/default-windows.json that should be cleaned up, as they're not related to the issue being solved.
Thanks!
d723f75 to
79435a6
Compare
Hi @dinocosta, thanks for the review! I've cleaned up the formatting changes — the diff now only contains the actual keybinding additions (13 lines across 3 files). Regarding reproducibility: I looked at the GPUI dispatch code and you're right that bindings in the parent AcpThread context should also match when focus is in AcpThread > Editor, since the context stack includes the full path from root to focused node. So the shortcuts may indeed work on 1.4.4. However, this PR still adds value: It follows the existing pattern — other bindings like scroll actions are explicitly declared in both AcpThread and AcpThread > Editor contexts. |
The bindings being added for the `AcpThread > Editor` context already exist on the `AcpThread` context, so it'd be redundant to add these.
I believe that, for the scroll actions, we need that duplication in As such, I've gone ahead and removed the duplicate permissions-related actions being added and kept the addition ot macOS' default keymap. Thank you for taking another look! 🙂 |
…ies#58365) ## Summary Add keyboard shortcut for macOS, in the ACP Permission dialog, for "Always Allow", following what is available in both Linux and Windows. ### 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) - [ ] Tests cover the new/changed behavior - [ ] Performance impact has been considered and is acceptable Release Notes: - Added missing `AllowAlways` keybinding on macOS (`cmd-alt-y`). --------- Co-authored-by: dino <dinojoaocosta@gmail.com>
…ies#58365) ## Summary Add keyboard shortcut for macOS, in the ACP Permission dialog, for "Always Allow", following what is available in both Linux and Windows. ### 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) - [ ] Tests cover the new/changed behavior - [ ] Performance impact has been considered and is acceptable Release Notes: - Added missing `AllowAlways` keybinding on macOS (`cmd-alt-y`). --------- Co-authored-by: dino <dinojoaocosta@gmail.com>
Summary
Add keyboard shortcut for macOS, in the ACP Permission dialog, for "Always Allow", following what is available in both Linux and Windows.
Self-Review Checklist
Closes #57546
Release Notes:
AllowAlwayskeybinding on macOS (cmd-alt-y).