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: further customise highlight colors #31

Merged
merged 5 commits into from
May 23, 2024

Conversation

josh-nz
Copy link
Contributor

@josh-nz josh-nz commented May 23, 2024

This adds the ability to define custom colors for the precognition highlight. This allows you to not have to rely in exisiting highlight values. It does this by creating a new highlight if the config for highlightColor is a table, otherwise it falls back to using the string specified as the highlight.

This is a very quick implementation based on some poking around and the code diff from #28 so it might not be considered good enough as it currently stands. In particular:

  • I'm unsure on how to set the correct @field type spec for the highlightColor now, and as a result the LSP complains that the call to nvim_set_hl doesn't accept a string, even though we know it is a table.
  • It may not be desirable for the entire set of highlight values to be configured. This may wish to be restricted to a subset.
  • I'm unsure if I'm creating custom highlights in the correct manner. There is an option to create in the global namespace by passing the value 0 instead of ns as the first argument to nvim_set_hl and this might be preferable.
  • I might have broken the tests introduced in feat: custom highlight colors #28 or at the very least, now added a feature not covered by the tests. This part is outside my knowledge and experience.
  • I might have selected the wrong branch to merge to. Apologies if I've done something incorrect in creating this pull request, I'm new to this.

@@ -355,6 +355,13 @@ function M.setup(opts)

ns = vim.api.nvim_create_namespace("precognition")
au = vim.api.nvim_create_augroup("precognition", { clear = true })

if type(config.highlightColor) == "table" then
vim.api.nvim_set_hl(ns, "precognition", config.highlightColor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're right, this should probably use the global namespace.

Also, mind capitalizing the hlgroup name? I don't think it actually matters, but pascal-case is generally used for hlgroups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use the global namespace, it doesn't seem to apply the colors provided. I can see the lightlight is defined, :filter precog highlight but just with a grey color instead of my defined yellow. However, if I reload the plugin via :Lazy reload precog (where precog is the name I gave to the Lazy spec for this plugin), it then uses the correct colors. I can't find anything further on the correct way to do this, after a bit of searching. Any ideas or pointers?

Copy link
Owner

Choose a reason for hiding this comment

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

Can you test the latest refactors i've done. It seems to work for me good now

@willothy
Copy link
Collaborator

Thanks for the PR!

Copy link
Owner

@tris203 tris203 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

lua/precognition/init.lua Outdated Show resolved Hide resolved
@@ -355,6 +355,13 @@ function M.setup(opts)

ns = vim.api.nvim_create_namespace("precognition")
au = vim.api.nvim_create_augroup("precognition", { clear = true })

if type(config.highlightColor) == "table" then
Copy link
Owner

Choose a reason for hiding this comment

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

Should we drop the if here, and create the highlight group anyways and use it?

Copy link
Contributor Author

@josh-nz josh-nz May 23, 2024

Choose a reason for hiding this comment

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

Possibly. I was trying to support your original implementation of just a highlight string as well as a custom highlight. Really what I wanted was a way to configure some basic colours or styles, and not have to try and find an existing highlight that could change when I changed my colorscheme. Perhaps we could drop support for the string highlight and just take a table and always make a custom highlight group. Internally we can still default to the Comment styles by just copying them across to the custom highlight.

Copy link
Owner

Choose a reason for hiding this comment

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

I did some refactoring and dropped the string input

if you want to test it and give any other thoughts i think this is good now!

@tris203 tris203 requested a review from willothy May 23, 2024 16:28
Copy link
Collaborator

@willothy willothy left a comment

Choose a reason for hiding this comment

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

Just some small nits, then looks good to me!

README.md Outdated Show resolved Hide resolved
lua/precognition/init.lua Show resolved Hide resolved
Copy link
Collaborator

@willothy willothy left a comment

Choose a reason for hiding this comment

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

Awesome, lgtm!

@tris203 tris203 merged commit 83d452d into tris203:main May 23, 2024
9 checks passed
@tris203
Copy link
Owner

tris203 commented May 23, 2024

Thanks for your help @josh-nz

Great idea and only a few tweaks were needed. Brilliant job if you are new

@josh-nz
Copy link
Contributor Author

josh-nz commented May 24, 2024

Is this working for anyone? I'm running into the issue I mentioned here:

#31 (comment)

Not working: Using the global namespace (as per code on main), it doesn't apply my config:

    vim.api.nvim_set_hl(0, hl_name, config.highlightColor)

Working: When using the specified namespace, it does apply my config:

    vim.api.nvim_set_hl(ns, hl_name, config.highlightColor)
    vim.api.nvim_set_hl_ns(ns)

I'm running Neovim 0.10.0 if that has any impact.

@tris203
Copy link
Owner

tris203 commented May 24, 2024

Is this working for anyone? I'm running into the issue I mentioned here:

#31 (comment)

Not working: Using the global namespace (as per code on main), it doesn't apply my config:

    vim.api.nvim_set_hl(0, hl_name, config.highlightColor)

Working: When using the specified namespace, it does apply my config:

    vim.api.nvim_set_hl(ns, hl_name, config.highlightColor)
    vim.api.nvim_set_hl_ns(ns)

I'm running Neovim 0.10.0 if that has any impact.

Does it not apply any config, eg its plain white text?
Or it applies the default config, eg its a comment text?

I can't produce this

@josh-nz
Copy link
Contributor Author

josh-nz commented May 24, 2024

I'm not sure if it's plain white, but it's not the comment highlight, at least not in a lua file.
Screen Shot 2024-05-24 at 17 57 28

My config:

return {
    "tris203/precognition.nvim",
    enabled = true,
    config = {
      highlightColor = {
        fg = "yellow",
        underline = true
      },
    },
}

If I add some print statements:

    print(config.highlightColor.fg)
    print(config.highlightColor.underline)
    vim.api.nvim_set_hl(0, hl_name, config.highlightColor)

I get yellow and true as expected, but then it results in the screenshot above as white(-ish).

@tris203
Copy link
Owner

tris203 commented May 24, 2024

Can you try a hex code. Say #FFFF00

That's what I do in the test which seems to pass

Also try foreground as a property. Not fg

@josh-nz
Copy link
Contributor Author

josh-nz commented May 24, 2024

{ foreground = "red"} does not work
{ foreground = "#FF0000"} does not work

:lua print(vim.api.nvim_get_hl(0, { name = "PrecognitionHighlight"})) returns vim.empty_dict(), assuming I'm using that API correctly.

(:lua print(vim.api.nvim_get_hl(0, { name = "Comment"})) returns table... as expected).

@tris203
Copy link
Owner

tris203 commented May 24, 2024

image
i am having no issues at all which is odd...

@josh-nz
Copy link
Contributor Author

josh-nz commented May 24, 2024

Using some of your config options, like the VeryLazy event and the toggle keymap, I can make it work. So this has given me some things to play with to try and isolate the issue. I'll let you know what I find when I have more time to play with it. It's looking like something to do with the way the Lazy plugin manager initializes things.

If you try with my exact config as here:

#31 (comment)

Does it work for you?

@tris203
Copy link
Owner

tris203 commented May 24, 2024

Your exact config doesnt, but changing fg to foreground does

image

@josh-nz
Copy link
Contributor Author

josh-nz commented May 24, 2024

Using foreground with the above config still doesn't work for me, as per my previous attempt. What does work is adding the VeryLazy event back in. I've had a quick look at your dotfiles repo and can't immediately see anything obvious that would be forcing your above config to be implicitly lazy as opposed to eager.

Also, when I use the VeryLazy event, both fg and foreground work.

This is a strange intersection of different keys working/not working, and different loading events.

I'm not sure I have the time/energy/excitement/whatever to try and debug what appears to be some intersection between Nvim loading lifecycle and Lazy loading lifecycle (even though this doesn't explain why your above config works with foreground but not the offical docs fg key). I think I'll just leave the VeryLazy event in and add a comment about it.

You may wish to add a note to the readme that if others are having issues with getting the custom highlight config to work, to refer to this conversation.

If I do ever figure it out, I'll comment back here. Otherwise, thanks to you both @tris203 and @willothy for your help and assistance with this PR.

@josh-nz josh-nz deleted the feature/custom-highlights branch May 31, 2024 23:11
@givensuman
Copy link

I'm having a similar issue with highlight color not being changed

Screenshot from 2024-06-03 12-12-58

@tris203
Copy link
Owner

tris203 commented Jun 3, 2024

I'm having a similar issue with highlight color not being changed

Screenshot from 2024-06-03 12-12-58

Interesting. Can you open an issue please @givensuman with neovim version info?

@givensuman
Copy link

I got it working, but I'll document the issue here for posterity.

$ nvim --version

NVIM v0.10.0
Build type: Release
LuaJIT 2.1.1713484068
Run "nvim -V1 -v" for more info

I'm a bit of a Neovim newbie and am using the AstroNvim framework which includes a packaged version of this plugin in it's Astrocommunity repo which was how I was using this plugin when I made the above comment.

That addition looks like this in my configuration (specifically, the community.lua file):

  { import = "astrocommunity.workflow.precognition-nvim" },
  {
    "tris203/precognition.nvim",
    event = "VeryLazy",
    config = {
      highlightColor = {
        foreground = "#FF4499",
      },
    },
  },

I scrubbed that out and manually imported the plugin in a LazySpec file like this (specifically, the plugins/user.lua file:

  {
    "tris203/precognition.nvim",
    event = "VeryLazy",
    opts = {},
    config = function()
      require("precognition").setup {
        highlightColor = {
          foreground = "#FF4499",
        },
      }
    end,
  },

This seems more likely an AstroNvim issue than a precognition.nvim one, but I'm actually having trouble recreating it to open an issue there.

Anyways, thanks for the sweet plugin!

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