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

feat: add background highlight to LSP signature help #771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndreasNasman
Copy link

I used surface1 as the color, the same as those used by LspReferenceText, LspReferenceRead, and LspReferenceWrite.

This change helps to highlight the current parameter under the cursor when invoking vim.lsp.buf.signature_help.

Before the change

name: string has a peach foreground but the same background as the other parameters, making it hard to distinguish.
image

After the change

name: string has a peach foreground and a surface1 background, making distinguishing from the other parameters easier.
image

@igorlfs
Copy link
Contributor

igorlfs commented Oct 5, 2024

Hello! I was thinking about making a similar change for this group myself. I do agree that a background color would be nice, but I also think that by having a different background color, there's no need to set a foreground color at all. By not setting a foreground color, we can have a better context for the parameter using treesitter (that's specially nice when it comes to complex parameters). Another reason to not set a foreground at all is that there will always be some overlap between the foreground color and another highlight group for a given language (for instance, using the default peach as the fg, there's overlap with @constant.builtin, which shows up fairly often in parameters in typescript (as undefined)).

Currently, I'm using

                    LspSignatureActiveParameter = {
                        fg = mocha.none,
                        bg = mocha.surface2,
                        style = { "bold" },
                    },

Which shows up as

image

What do you think?

@AndreasNasman
Copy link
Author

Your change looks great, @igorlfs. Your version makes it even more apparent which parameter is active.

A couple of questions came to mind:

  1. Is the fg = mocha.none, part needed? I tested your version and removed it and couldn't spot a difference.
  2. Which background color is most suited? I chose to use surface1 since LspReferenceText, LspReferenceRead and LspReferenceWrite use that background color, but surface2 also works great.
  3. Could you explain this part in more detail?

    By not setting a foreground color, we can have a better context for the parameter using treesitter (that's specially nice when it comes to complex parameters).

@igorlfs
Copy link
Contributor

igorlfs commented Oct 7, 2024

Your change looks great, @igorlfs. Your version makes it even more apparent which parameter is active.

Thanks!

  1. Is the fg = mocha.none, part needed? I tested your version and removed it and couldn't spot a difference.

I don't think that's needed. I assumed it was necessary since I was overriding the default one, but that doesn't seem to be the case

  1. Which background color is most suited? I chose to use surface1 since LspReferenceText, LspReferenceRead and LspReferenceWrite use that background color, but surface2 also works great.

Either one is fine IMO

  1. Could you explain this part in more detail?
    > By not setting a foreground color, we can have a better context for the parameter using treesitter (that's specially nice when it comes to complex parameters).

Using the same example I gave, but defining a foreground color:

image

To me, it looks way messier. Since the color falls back to treesitter, I find it more appealing / clearer what type is expected for the parameter.

@AndreasNasman AndreasNasman force-pushed the add-background-lsp-signature-help branch from 01420be to 8f99df4 Compare October 7, 2024 18:53
@AndreasNasman
Copy link
Author

Thanks for the answers! I updated the PR accordingly. I kept the background as surface1 to align with the other Lsp values.

Copy link
Contributor

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

This is pretty embarassing but I reviewed the wrong repository, will look at this PR later!

@sgoudham sgoudham self-requested a review October 7, 2024 22:16
@AndreasNasman AndreasNasman force-pushed the add-background-lsp-signature-help branch from 58c4aa0 to 8f99df4 Compare October 8, 2024 16:46
I used surface1 as the color, the same as those used by
LspReferenceText, LspReferenceRead, and LspReferenceWrite.
@AndreasNasman AndreasNasman force-pushed the add-background-lsp-signature-help branch from 8f99df4 to 04b3589 Compare October 8, 2024 16:47
Copy link
Collaborator

@vollowx vollowx left a comment

Choose a reason for hiding this comment

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

LGTM!

@AndreasNasman
Copy link
Author

Two checks are failing, but the same errors happen on the main branch.

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.

4 participants