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(highlights): add fzf-lua #79

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

strash
Copy link
Contributor

@strash strash commented Nov 26, 2024

https://github.com/ibhagwan/fzf-lua

Before
Screenshot 2024-11-26 at 04 10 08

After
Screenshot 2024-11-26 at 04 11 44

Before
Screenshot 2024-11-26 at 04 11 00

After
Screenshot 2024-11-26 at 04 12 34

@strash
Copy link
Contributor Author

strash commented Nov 26, 2024

We should probably mention in the README about this setting for generating highlights for fzf.

require("fzf-lua").setup({ fzf_colors = true })

I've been using this plugin for a while now, but I just learned about this setting recently. 😀

@strash
Copy link
Contributor Author

strash commented Nov 26, 2024

Some updates.

Before
Screenshot 2024-11-26 at 14 28 43

After
Screenshot 2024-11-26 at 14 27 36

Before
Screenshot 2024-11-26 at 14 29 25

After
Screenshot 2024-11-26 at 14 28 07

@strash
Copy link
Contributor Author

strash commented Dec 1, 2024

Hey @ramojus, this PR has all the changes from the PR with blink.

@ramojus ramojus changed the title Plugin fzf lua feat(highlights): add fzf.lua Dec 4, 2024
@ramojus ramojus changed the title feat(highlights): add fzf.lua feat(highlights): add fzf-lua Dec 4, 2024
@ramojus
Copy link
Owner

ramojus commented Dec 6, 2024

Hey, sorry for the delays, I'm quite busy lately.

I'm just wondering if we really need to support this plugin? We have telescope, which is much more popular. Does this plugin have anything that telescope doesn't?
Otherwise the PR looks good, I would have a few minor comments and it can be merged.

@ramojus
Copy link
Owner

ramojus commented Dec 6, 2024

Also just saw that this plugin has a telescope profile. In terms of highlight groups, with your changes, does it look similar to telescope when that profile is applied?

@strash
Copy link
Contributor Author

strash commented Dec 6, 2024

I'm just wondering if we really need to support this plugin?

Why Fzf-Lua
Personally, I think it's a matter of personal preference, but I find it slightly faster. I'm an Fzf user, and I would greatly appreciate it if this plugin were supported by this beautiful colorscheme. There's no real reason not to support it. By doing so, we can potentially attract more users.


Also just saw that this plugin has a telescope profile. In terms of highlight groups, with your changes, does it look similar to telescope when that profile is applied?

It looks like this.

Screenshot 2024-12-07 at 02 06 19 Screenshot 2024-12-07 at 02 07 56

@emretuna
Copy link

emretuna commented Dec 7, 2024

I was waiting for that one great!

@strash
Copy link
Contributor Author

strash commented Dec 8, 2024

@ramojus I've removed variables

@ramojus
Copy link
Owner

ramojus commented Dec 14, 2024

Thanks for the screenshots, that looks surprisingly good! We should make telescope look more like this :D
But of course, it's not viable unless telescope supports darker backdrop.

This is a difficult plugin as there are a lot of scenarios. I've pulled your changes to take a deeper look. There are some things I don't like about this plugin that I haven't found a way to fix:

  • Current result being bold (seems unnecessary - distracting).
    image
  • Backdrop removed when searching colorschemes. I get it, it's for colorscheme preview, but ideally fzf background would then be lighter when searching colorschemes.
    image
  • Line numbers in preview having their default dark background. (This is probably fine as long as the preview has Normal background)
    image
    I was thinking of making the backgrounds lighter, but with this issue, probably better to leave it as is.

As you know this plugin better than I do, I would appreciate any help with these issues. Otherwise, not a deal breaker.

I will also submit some review comments.

local M = {}

function M.set(hl, colors)
hl.set("FzfLuaNormal", { bg = colors.dark_bg, fg = colors.fg3 })
Copy link
Owner

Choose a reason for hiding this comment

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

Currently there is no easy way to change normal bg or preview bg without changing a whole lot of highlight groups manually.

I think it's okay to use some variables for highlights as long as it offers readability and/or maintainability improvements. Here we could have a variable normal for FzfLuaNormal, this variable could then be reused in other highlight groups. Similarly we could have a variable preview_normal.

A fine alternative would be to reuse FzfLuaNormal and FzfLuaPreviewNormal with linking and hl.get function.

hl.set("FzfLuaPreviewNormal", { bg = hl.get("Normal").bg, fg = hl.get("Normal").fg })
hl.set("FzfLuaPreviewBorder", { bg = hl.get("Normal").bg, fg = hl.get("Normal").bg })
hl.set("FzfLuaPreviewTitle", { link = "FzfLuaTitle" })
hl.set("FzfLuaCursorLine", { bg = colors.bg2, fg = colors.fg2 })
Copy link
Owner

Choose a reason for hiding this comment

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

Here I would suggest: (normal being FzfLuaNormal)

    local groups = require("mellifluous.highlights.custom_groups").get(colors)

    hl.set("FzfLuaCursorLine", { bg = groups.MenuButtonSelected(normal.bg).bg, fg = normal.fg })

I don't think there's any reason to have lighter fg for this group, keeping the same fg as for other results will offer more consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, apparently, FzfLuaCursorLine is intended for the preview window. I was confused about the naming convention and removed it.

hl.set("FzfLuaScrollFloatFull", { link = "FzfLuaScrollBorderFull" })
hl.set("FzfLuaScrollFloatEmpty", { link = "FzfLuaScrollBorderEmpty" })
hl.set("FzfLuaHeaderBind", { fg = colors.ui_purple })
hl.set("FzfLuaHeaderText", { fg = colors.fg4 })
Copy link
Owner

Choose a reason for hiding this comment

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

This group goes nicely here:

    hl.set("FzfLuaFzfHeader", { fg = colors.fg4 })

@ramojus
Copy link
Owner

ramojus commented Dec 14, 2024

Nice work, really. I like that we also retain some similarity with fzf cli, mainly by keeping that FzfLuaFzfPointer visible, but also not distracting.
One more issue here:
image
The background of that title doesn't match with normal background. Not sure which highlight group sets it, as I don't get this title by default.

@strash
Copy link
Contributor Author

strash commented Dec 14, 2024

  • Current result being bold (seems unnecessary - distracting).
    image

Not sure how to fix this. I've explicitly unset bold and set nocombine, but it seems to be ignored.

  • Backdrop removed when searching colorschemes. I get it, it's for colorscheme preview, but ideally fzf background would then be lighter when searching colorschemes.
    image

I think it's fine. And probably there's nothing we can do about it.

  • Line numbers in preview having their default dark background. (This is probably fine as long as the preview has Normal background)
    image
    I was thinking of making the backgrounds lighter, but with this issue, probably better to leave it as is.

Fixed.

One more issue here:
image
The background of that title doesn't match with normal background. Not sure which highlight group sets it, as I don't get this title by default.

Nice catch! Fixed.

@strash
Copy link
Contributor Author

strash commented Dec 14, 2024

Screenshot 2024-12-14 at 23 51 09 Screenshot 2024-12-14 at 23 51 51

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.

3 participants