Skip to content
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

Load all key bindings that parse and use markdown in error notifications #23113

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Jan 14, 2025

  • Collects and reports all parse errors

  • Shares parsed KeyBindingContextPredicate among the actions.

  • Updates gpui keybinding and action parsing to return structured errors.

  • Renames "block" to "section" to match the docs, as types like KeymapSection are shown in json-language-server hovers.

  • Removes wrapping of context and use_key_equivalents fields so that json-language-server auto-inserts "" and false instead of null.

  • Updates add_to_cx to take &self, so that the user keymap doesn't get unnecessarily cloned.

In retrospect I wish I'd just switched to using TreeSitter to do the parsing and provide proper diagnostics. This is tracked in #23333

Release Notes:

  • Improved handling of errors within the user keymap file. Parse errors within context, keystrokes, or actions no longer prevent loading the key bindings that do parse.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 14, 2025
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch 5 times, most recently from 3c1b6be to 2cc12d5 Compare January 14, 2025 10:22
@mgsloan mgsloan enabled auto-merge January 14, 2025 10:22
mgsloan added a commit that referenced this pull request Jan 16, 2025
Motivation for this change is #23113, but this will also be the state of it after the gpui refactor.
mgsloan added a commit that referenced this pull request Jan 16, 2025
Motivation for this is using markdown for keymap error notifications in #23113
mgsloan added a commit that referenced this pull request Jan 16, 2025
Falls back on notifying all workspaces if there isn't an active one.

This is to support notifying the user about keymap file errors in #23113. It will also be useful for notifying about settings file errors.
mgsloan added a commit that referenced this pull request Jan 16, 2025
…23220)

Motivation for this change is #23113, but this will also be the state of
it after the gpui refactor.

Release Notes:

- N/A
mgsloan added a commit that referenced this pull request Jan 16, 2025
Motivation for this is using markdown for keymap error notifications in
#23113

Release Notes:

- N/A
mgsloan added a commit that referenced this pull request Jan 16, 2025
…ext (#23226)

Falls back on notifying all workspaces if there isn't an active one.

This is to support notifying the user about keymap file errors in
#23113. It will also be useful for notifying about settings file errors.

Release Notes:

- N/A
mgsloan added a commit that referenced this pull request Jan 16, 2025
Motivation for this is using markdown for keymap error notifications in
#23113, but it also benefits the copied text of repl tables.

Release Notes:

- N/A
mgsloan added a commit that referenced this pull request Jan 16, 2025
* Collects and reports all parse errors

* Shares parsed `KeyBindingContextPredicate` among the actions.

* Renames "block" to "section" to match the docs, as types like `KeymapSection` are shown in `json-language-server` hovers.

* Removes wrapping of `context` and `use_key_equivalents` fields so that `json-language-server` auto-inserts `""` and `false` instead of `null`.

* Updates `add_to_cx` to take `&self`, so that the user keymap doesn't get unnecessarily cloned.

Use structured error types for keybinding load

Motivation for this is so that #23113 can instead render these as markdown for display.
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch 2 times, most recently from 048a826 to 773ea9e Compare January 16, 2025 13:04
@mgsloan mgsloan changed the title Load all keybindings that parse instead of omitting them all on error Load all key bindings that parse and use markdown in error notifications Jan 16, 2025
@mgsloan mgsloan disabled auto-merge January 16, 2025 13:06
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch 6 times, most recently from 66fe0f9 to 4f3c863 Compare January 17, 2025 23:46
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch from 4f3c863 to 3feb51b Compare January 18, 2025 20:14
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch from 3feb51b to 21410b8 Compare January 18, 2025 20:44
@mgsloan mgsloan enabled auto-merge (squash) January 18, 2025 20:44
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch 2 times, most recently from 2a78114 to 6b51a1b Compare January 18, 2025 21:49
* Collects and reports all parse errors

* Shares parsed `KeyBindingContextPredicate` among the actions.

* Removes use of callback for `handle_keymap_file_changes` to send notification, instead uses `show_app_notification` added in #23226

* Renames "block" to "section" to match the docs, as types like `KeymapSection` are shown in `json-language-server` hovers.

* Removes wrapping of `context` and `use_key_equivalents` fields so that `json-language-server` auto-inserts `""` and `false` instead of `null`.

* Updates `add_to_cx` to take `&self`, so that the user keymap doesn't get unnecessarily cloned.
Good suggestion from Conrad!
@mgsloan mgsloan force-pushed the load-all-keybindings-that-parse branch from 6b51a1b to e7a07fa Compare January 18, 2025 22:17
@mgsloan mgsloan merged commit 711dc21 into main Jan 18, 2025
12 checks passed
@mgsloan mgsloan deleted the load-all-keybindings-that-parse branch January 18, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant