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

Fix command-enter shortcut for Edit Code #1919

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Conversation

mkondratek
Copy link
Contributor

@mkondratek mkondratek commented Jul 18, 2024

Fixes https://linear.app/sourcegraph/issue/CODY-2868.
Fixes https://github.com/sourcegraph/cody-issues/issues/307

Test plan

  1. In particular: Having IC 2024.1 and com.intellij.lang.jsgraphql (241.14494.150) plugin installed
  2. Edit Code...
  3. Trigger the edit with the shortcut

@mkondratek mkondratek self-assigned this Jul 18, 2024
@mkondratek mkondratek marked this pull request as draft July 18, 2024 15:17
@mkondratek mkondratek force-pushed the mkondratek/fix/command-enter branch from 474cd0b to 51cb0c9 Compare July 18, 2024 15:19
@mkondratek mkondratek marked this pull request as ready for review July 18, 2024 15:19
@mkondratek mkondratek marked this pull request as draft July 20, 2024 09:22
@mkondratek mkondratek marked this pull request as ready for review July 20, 2024 10:46
@@ -52,6 +52,9 @@

<applicationService
serviceImplementation="com.sourcegraph.cody.config.CodyPersistentAccounts"/>

<actionPromoter implementation="com.sourcegraph.cody.edit.CodyActionPromoter"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the actual fix for this one - thank you JetBrains community Slack for help 😃

@mkondratek mkondratek enabled auto-merge (squash) July 22, 2024 08:41
<action id="cody.inlineEditEditCode"
text="Edit Code"
class="com.sourcegraph.cody.edit.InlineEditPromptEditCodeAction" >
<keyboard-shortcut replace-all="true" first-keystroke="ctrl ENTER" keymap="$default"/>
Copy link
Contributor

@pkukielka pkukielka Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see such comment from @steveyegge in this file:

    <!--
        N.B. You have to declare all the Mac keystrokes with ctrl or alt separately,
        even if their definition is identical to $default, because otherwise it will
        result in the actual keybindings switching to command/meta and/or shift.
    -->

@mkondratek could you please check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please add <override-text place="GoToAction" text="Cody: Confirm Code Edition"/>
or something along those lines (it is for search everywhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text="Cody: Confirm Code Edition" that would be different from the text in the button and hence confusing imho

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is that we have two "Edit Code" now. One to open edit code prompt and second for confirming the edit. they somehow need to have different names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed your second comment about ....
In this case: <override-text place="GoToAction" text="Cody: Edit Code"/>
Still we need to add Cody: part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -285,6 +288,12 @@
<override-text place="GoToAction" text="Cody: Dismiss"/>
</action>

<action id="cody.inlineEditEditCode"
text="Edit Code"
Copy link
Contributor

@pkukielka pkukielka Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think text should not be Edit Code. We already have other Edit Code and I think both will be displayed in the key bindings settings with the same name (for button on the edit dialog we can have custom name, if needed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have Edit Code... action (not the three dots!) - it's a common practice in JB that the actions that open dialogs or need some confirmation have ... suffix. IMHO it's a good enough to distinguish them (we clearly indicate what the shortcuts do and display them - it's super clear).

@mkondratek mkondratek force-pushed the mkondratek/fix/command-enter branch from b9a109b to c4cc72a Compare July 22, 2024 11:25
@mkondratek mkondratek force-pushed the mkondratek/fix/command-enter branch from c8a0446 to f30e6cc Compare July 22, 2024 12:09
Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkondratek mkondratek merged commit 31423f6 into main Jul 22, 2024
6 checks passed
@mkondratek mkondratek deleted the mkondratek/fix/command-enter branch July 22, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants