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

Introduce InlineCompletionProvider #9777

Merged
merged 8 commits into from
Mar 26, 2024
Merged

Introduce InlineCompletionProvider #9777

merged 8 commits into from
Mar 26, 2024

Conversation

as-cii
Copy link
Member

@as-cii as-cii commented Mar 25, 2024

This pull request introduces a new InlineCompletionProvider trait, which enables making Editor copilot-agnostic and lets us push all the copilot functionality into the copilot_ui module. Long-term, I would like to merge copilot and copilot_ui, but right now project depends on copilot, which makes this impossible.

The reason for adding this new trait is so that we can experiment with other inline completion providers and swap them at runtime using config settings.

Please, note also that we renamed some of the existing copilot actions to be more agnostic (see release notes below). We still kept the old actions bound for backwards-compatibility, but we should probably remove them at some later version.

Also, as a drive-by, we added new methods to the Global trait that let you read or mutate a global directly, e.g.:

MyGlobal::update(cx, |global, cx| {
});

Release Notes:

  • Renamed the copilot::Suggest action to editor::ShowInlineCompletion
  • Renamed the copilot::NextSuggestion action to editor::NextInlineCompletion
  • Renamed the copilot::PreviousSuggestion action to editor::PreviousInlineCompletion
  • Renamed the editor::AcceptPartialCopilotSuggestion action to editor::AcceptPartialInlineCompletion

as-cii and others added 3 commits March 25, 2024 15:57
Co-Authored-By: Nathan <nathan@zed.dev>
Co-Authored-By: Kyle <kylek@zed.dev>
Co-Authored-By: Kyle <kylek@zed.dev>
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Mar 25, 2024
@maxbrunsfeld
Copy link
Collaborator

/cc @pjlast @beyang - this might be relevant to your Cody branch.

@rgbkrk
Copy link
Member

rgbkrk commented Mar 25, 2024

This branch is failing for two keymap tests, where it tests modifying the base and retaining the users keympa

cargo test --bin Zed keymap
    Finished test [unoptimized + debuginfo] target(s) in 0.48s
     Running unittests src/main.rs (target/debug/deps/Zed-a25a0cca1ed3c648)

running 2 tests
test zed::tests::test_base_keymap ... FAILED
test zed::tests::test_disabled_keymap_binding ... FAILED

failures:

---- zed::tests::test_base_keymap stdout ----
thread 'zed::tests::test_base_keymap' panicked at crates/zed/src/zed.rs:2873:9:
On 2877 Failed to find pane::ActivatePrevItem with key binding [

---- zed::tests::test_disabled_keymap_binding stdout ----
thread 'zed::tests::test_disabled_keymap_binding' panicked at crates/zed/src/zed.rs:3007:9:
On 3011 Failed to find pane::ActivatePrevItem with key binding [
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    zed::tests::test_base_keymap
    zed::tests::test_disabled_keymap_binding

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 17 filtered out; finished in 0.59s

error: test failed, to rerun pass `--bin Zed`

The assertion that's failing in both of these tests (near the end of each test):

        // Test modifying the base, while retaining the users keymap
        app_state
            .fs
            .save(
                "/settings.json".as_ref(),
                &r#"
                {
                    "base_keymap": "JetBrains"
                }
                "#
                .into(),
                Default::default(),
            )
            .await
            .unwrap();

        cx.background_executor.run_until_parked();

        assert_key_bindings_for(
            workspace.into(),
            cx,
            vec![("cmd-shift-[", &ActivatePrevItem)],
            line!(),
        )

I couldn't figure out how the changes here might have caused this. After looking into it with @mikayla-maki, we did not find the root cause but maybe found something odd. In handle_keymap_file_changes, we do an unbounded send on base_keymap_tx that never seems to get polled in the base_keymap_rx.next in the select_biased! below it.

@as-cii as-cii merged commit fb6cff8 into main Mar 26, 2024
9 checks passed
@as-cii as-cii deleted the inline-completion-provider branch March 26, 2024 12:28
maxdeviant added a commit that referenced this pull request Mar 26, 2024
…disabled (#9826)

This PR fixes some noisy error logs from the `CopilotCompletionProvider`
when Copilot is disabled entirely via the settings.

I have the following in my settings file:

```json
{
  "features": {
    "copilot": false
  },
}
```

After #9777 I started seeing my Zed logs getting filled up with messages
like this:

```
[2024-03-26T14:33:09-04:00 ERROR util] crates/copilot_ui/src/copilot_completion_provider.rs:206: copilot is disabled
```

Release Notes:

- N/A
maxdeviant added a commit that referenced this pull request Apr 29, 2024
This PR restores the `Global` trait's status as a marker trait.

This was the original intent from #7095, when it was added, that had
been lost in #9777.

The purpose of the `Global` trait is to statically convey what types can
and can't be accessed as `Global` state, as well as provide a way of
restricting access to said globals.

For example, in the case of the `ThemeRegistry` we have a private
`GlobalThemeRegistry` that is marked as `Global`:
https://github.com/zed-industries/zed/blob/91b3c24ed35d58438ae33970f07d1ff01d3acfc7/crates/theme/src/registry.rs#L25-L34

We're then able to permit reading the `ThemeRegistry` from the
`GlobalThemeRegistry` via a custom getter, while still restricting which
callers are able to mutate the global:
https://github.com/zed-industries/zed/blob/91b3c24ed35d58438ae33970f07d1ff01d3acfc7/crates/theme/src/registry.rs#L46-L61

Release Notes:

- N/A
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.

3 participants