OXY2DEV / markview.nvim

An experimental markdown previewer for Neovim
Apache License 2.0
649 stars 25 forks source link

Adopt highlight group "namespace" and conventions #32

Closed scottmckendry closed 1 week ago

scottmckendry commented 1 week ago

Hello @OXY2DEV!

First off, this plugin is very impressive. What you've been able to achieve in a short space of time (and on a phone!?) is excellent.

I want to add native support for the plugin directly to my colorscheme (cyberdream.nvim) and notice that the highlight group naming convention seems a bit... off?

Names like green_fg and gradient_4 are great for local variables, but for highlight groups that all share a single namespace it doesn't really make a whole lot of sense.

What I'm suggesting it to adapt the existing highlight groups with a prefix. For example:

This makes identifying groups that belong to the plugin amongst the hundreds of others much easier.

If you want to see more examples of naming conventions, I would suggest checking out a few of the resources below:

If you have any questions around this, please let me know!

OXY2DEV commented 1 week ago

Actually the names are Markview_something.

The add_hl function automatically adds the Markview_ prefix.

OXY2DEV commented 1 week ago

Also, that's not why the highlight_groups option exists.

It's there for when you are happy with the defaults(or don't want them to be dependent on the color scheme).

In my case, I have made a simple plugin that sets them to whatever I chose for the current color scheme. So that I don't have to set them up for each color scheme.

scottmckendry commented 1 week ago

Actually the names are Markview_something.

The add_hl function automatically adds the Markview_ prefix.

Ahhh, yes! I can see that now 🙂 Ignore my ramblings then. I can see all the highlight groups are prefixed as you've pointed out:

image

mahmoudk1000 commented 1 week ago

@OXY2DEV that would be nice if combined with the marview plugin itself,wouldn't it?

I have edited the highlight group as example Markview_red through my color scheme. But they doesn't change the appearance at all, default one is still in use. Why that?

OXY2DEV commented 1 week ago

@OXY2DEV that would be nice if combined with the marview plugin itself,wouldn't it?

I have edited the highlight group as example Markview_red through my color scheme. But they doesn't change the appearance at all, default one is still in use. Why that?

Because add_hl() runs when neovim starts(in your case after your color scheme).

Just set highlight_grops = false(basically anything that's not a list) and it should work as intended.

scottmckendry commented 1 week ago

Thanks @OXY2DEV, I think I might need to reopen this one since I've not been able to get these highlight groups working at all.

My configuration looks like this:

return {
    "OXY2DEV/markview.nvim",
    ft = "markdown",
    dependencies = {
        "nvim-tree/nvim-web-devicons",
    },

    config = function()
        require("markview").setup({
            highlight_groups = false,
        })
    end,
}

But I still can't override the defaults.

OXY2DEV commented 1 week ago

Thanks @OXY2DEV, I think I might need to reopen this one since I've not been able to get these highlight groups working at all.

My configuration looks like this:

return {
    "OXY2DEV/markview.nvim",
    ft = "markdown",
    dependencies = {
        "nvim-tree/nvim-web-devicons",
    },

    config = function()
        require("markview").setup({
            highlight_groups = false,
        })
    end,
}

But I still can't override the defaults.

Yeah, I just realized it doesn't work. My bad.

You will probably have to wait for me to change how the highlights are applied.

scottmckendry commented 1 week ago

No worries @OXY2DEV! It would be good to be able to mix and match colorscheme highlights with the defaults. Rather than it being an on/off thing in the config. Most plugins support overrides in such a way that the user's config doesn't have to be modified at all.

I think the reason this particular option isn't working is because this block inside markview.lua executes before setup() is called:

-- line 544-546
if vim.islist(markview.configuration.highlight_groups) then
    markview.add_hls(markview.configuration.highlight_groups);
end

This is before the config includes any user options. But again, all highlight groups should be created regardless if the user wants to use them or not.

It would be good to remove this option altogether, and allow users to override individual groups either through a colorsheme or manually with something like :hi Markview_red guifg=#ff0000.

OXY2DEV commented 1 week ago

I think the reason this particular option isn't working is because this block inside markview.lua executes before setup() is called:

-- line 544-546
if vim.islist(markview.configuration.highlight_groups) then
  markview.add_hls(markview.configuration.highlight_groups);
end

This is before the config includes any user options. But again, all highlight groups should be created regardless if the user wants to use them or not.

Yes, that's the root of the issue.

It will be fixed with all the other color related things in the next update

scottmckendry commented 1 week ago

I would also suggest updating the markview.add_hls function to append default = true to the highlight group value. This will preserve any predefined groups:

markview.add_hls = function(usr_hls)
    local hl_list = usr_hls or renderer.hls

    for _, tbl in ipairs(hl_list) do
        tbl.value.default = true
        vim.api.nvim_set_hl(0, "Markview_" .. tbl.group_name, tbl.value)
    end
end

I've tested this locally and it works with the highlight group definitions in my colorscheme - so no changes required to the default options in markview's setup(). This will make it a lot easier for other colorshemes to add support as this plugin (inevitably) grows in popularity 🙂

OXY2DEV commented 1 week ago

I would also suggest updating the markview.add_hls function to append default = true to the highlight group value. This will preserve any predefined groups:

markview.add_hls = function(usr_hls)
    local hl_list = usr_hls or renderer.hls

    for _, tbl in ipairs(hl_list) do
        tbl.value.default = true
        vim.api.nvim_set_hl(0, "Markview_" .. tbl.group_name, tbl.value)
    end
end

I've tested this locally and it works with the highlight group definitions in my colorscheme - so no changes required to the default options in markview's setup(). This will make it a lot easier for other colorshemes to add support as this plugin (inevitably) grows in popularity 🙂

I was actually going to use the default highlight groups mentioned in highlight-default.

For your colorscheme's case it looks like this.

Screenshot_2024-07-08-08-12-37-879_com.termux-edit.jpg

Of course, this doesn't work with some of the older colorscheme's(as they don't define all of the necessary highlight groups). But you can always switch them out with something else.

scottmckendry commented 1 week ago

This is excellent! I've checked out the dev branch with your changes and it looks great! Except when I enable transparency the backgrounds look off - I think this is because bg is being used in the mix() function to calculate the highlight when its value is NONE. This may be an issue for other themes too:

image

I'm not sure what the best solution is for this. But I'll leave that with you 😉

It appears that overrides are working now too! Great stuff!

OXY2DEV commented 1 week ago

@scottmckendry It appears that you can't have semi-transparent colors in neovim(at least not for normal texts).

So, I can't really find a clear way to make the background color look nice for all colorschemes.

And it doesn't help that I can't get the default background color when the transparency is used.

scottmckendry commented 1 week ago

Final implementation that was recently merged into main looks good. Thanks for working on this @OXY2DEV.

Would you be open to converting highlight groups from snake_case to PascalCase? It will effectively be a breaking change, so the earlier you do it the better.

The reason I ask is because this is the only plugin I've come across so far that uses underscores in highlight group names. This is also consistent across all of the default highlight groups in Neovim. Such as StorageClass and SpecialChar.

Also, for my own selfish reasons, I have regex that looks for PascalCase highlight groups and attempts to apply the highlights to the text, This falls apart with the snake_case naming convention since the code was written with the assumption that these are lua variables.

OXY2DEV commented 1 week ago

Would you be open to converting highlight groups from snake_case to PascalCase? It will effectively be a breaking change, so the earlier you do it the better.

I honestly don't care about what is used for naming as long it doesn't affect readablity.

The reason I ask is because this is the only plugin I've come across so far that uses underscores in highlight group names. This is also consistent across all of the default highlight groups in Neovim. Such as StorageClass and SpecialChar.

Seems like a valid reason.

Also, for my own selfish reasons, I have regex that looks for PascalCase highlight groups and attempts to apply the highlights to the text, This falls apart with the snake_case naming convention since the code was written with the assumption that these are lua variables.

Is it for coloring highlight group names?


Anyway, I probably will switch the naming of highlight groups in the next commit.

OXY2DEV commented 1 week ago

So, I was looking at other highlight names and realized that there are highlight groups that don't use PascalCase.

Screenshot_2024-07-12-20-13-17-671_com.termux-edit.jpg

Plus, catppuccin seem to use camelCase which is strange.