chrisant996 / clink-fzf

Fzf integration for Clink
MIT License
77 stars 10 forks source link

Show descriptions when filtering arg completions #23

Closed fiso64 closed 1 month ago

fiso64 commented 1 month ago

I have added inline descriptions for arguments (if available) similar to how the complete menu shows them. There are two new settings, fzf.show_descriptions (true by default) and fzf.color_descriptions which is false by default since fzf does not preserve ansi color codes unless it is run with --ansi.

chrisant996 commented 1 month ago

First of all, I like the idea!

I applied the PR to my workspace, and I found several issues, most of which I was able to solve.

Here are some issues that I don't have a solution for yet:

  1. When a description is highlighted, it can be hard to read -- especially if the description color has been customized. image

  2. Descriptions breaks preview -- note that descriptions can be present for files and directories as well, not just for flags. Also, it's unsafe to pass description strings to the previewer, as depending on the content in a description string it could get misinterpreted and executed as an actual command! I'm not sure this will realistically be solvable, but I'm working on it. image

chrisant996 commented 1 month ago

This PR also led me to find some pre-existing bugs in the fzf.lua script, which I'm working on fixing.

For example, when file preview is configured and also matchicons.lua from clink-gizmos is loaded and enabled, then the preview breaks for any matches that don't have an icon (including files or directories that don't have an icon, which can happen especially with custom match generators).

And that particular example is very difficult to solve, similar to the issue noted earlier where showing descriptions breaks preview.

fiso64 commented 1 month ago

For the first problem I don't think it's that bad since both the description color and the fzf highlight color can be customized.

As for the second issue you mentioned, maybe it should check that m.type == 'arg' in addition (line 790 in the PR), in case there is no safe solution? Also, how does a file or directory get a description?

chrisant996 commented 1 month ago

For the first problem I don't think it's that bad since both the description color and the fzf highlight color can be customized.

If fzf.color_descriptions is disabled by default then it's ok because the user had to take custom action. I was exploring whether it's possible to make it enabled by default, but that would create an accessibility problem by default. So, it has to be disabled by default.

As for the second issue you mentioned, maybe it should check that m.type == 'arg' in addition (line 790 in the PR), in case there is no safe solution?

The match type doesn't matter. The issue is if the description string contains certain patterns of " characters. I came up with a solution, which involves detecting unsafe quotes and translating them into and characters instead (notice they're angled/curved, not straight -- they still look like quotes, but CMD doesn't treat them like quotes).

Also, how does a file or directory get a description?

A script can do it.

An argmatcher can use Functions As Argument Options to add a function that returns a match with type "file" plus a description.

A match generator can specify type and description when adding matches.

Simplistic example in an argmatcher:

local function func()
    return {
        { match="filename.txt", type="file", description="This is a file" },
    }
end

clink.argmatcher("foo"):addarg(func)

Also, for example, in the fish shell, executable completion (for the first word in a command) includes descriptions. I've considered adding that in Clink, but I dislike it because it clutters the completion list, and it's really just adding descriptions for the sake of adding them -- they don't provide much actual value. (Who cares how big an .exe file is? Why duplicate the extension (e.g. describing foo.txt as "TXT file")? Etc.)

chrisant996 commented 1 month ago

Do you mind if I commit a different implementation, and close this PR without merging it?

Or, if you want credit on GitHub for the contribution, then I could merge it and then commit the different implementation. It's slightly messier in the source history, but it's the only way I know to make sure GitHub gives you credit.

Let me know which you prefer! I'm happy with either.

fiso64 commented 1 month ago

Thanks, closing this without merging is fine!

chrisant996 commented 1 month ago

The description support commits have been pushed -- give it a try!

fiso64 commented 1 month ago

When fzf.color_descriptions is true, pressing enter to accept does not insert the item anymore in my case. I have encountered the same issue in my implementation and it seems to be because r:read('*line') does not preserve color codes, which is why I remove all color codes before inserting the lines into the table. I was able to fix this bug by replacing text with text:gsub('\x1b%[[%d;]*m', '') here:

https://github.com/chrisant996/clink-fzf/blob/main/fzf.lua#L887-L888

fiso64 commented 1 month ago

I've noticed another pre-existing bug. Some items are never inserted when the user accepts. For instance if clink-completions is installed and I type winget, then press ctrl+space there will be some items in the list that have no description (e.g 'complete' or 'add'). None of these are inserted when I press enter (they also don't appear in the usual complete menu).

Edit: Actually this seems unrelated to clink-fzf. Even without clink-fzf if I type 'winget compl' it suggests 'complete' but pressing tab does nothing. Only accepting the suggestion with right arrow inserts it.

chrisant996 commented 1 month ago

I've noticed another pre-existing bug. Some items are never inserted when the user accepts. For instance if clink-completions is installed and I type winget, then press ctrl+space there will be some items in the list that have no description (e.g 'complete' or 'add'). None of these are inserted when I press enter (they also don't appear in the usual complete menu).

Edit: Actually this seems unrelated to clink-fzf. Even without clink-fzf if I type 'winget compl' it suggests 'complete' but pressing tab does nothing. Only accepting the suggestion with right arrow inserts it.

It's not a bug. Auto-Suggestions are different from completions. It's suggesting winget complete because it found that as an entry in your history file.

Since commit af01e484cdcc634fda83bff6f2b1ed287bd2db63 in January 2023 (a year and a half ago), typing winget compl should not have any completions, because the complete subcommand is hidden from completions (because it's intended for scripted use, not really command line use). If you type complete it will get colored as a recognized command (and it has a subparser), but it is not included in the list of completions.

If you're seeing some without descriptions, then you might be using an old version of either Clink or clink-completions. Since commit 7c22a6f0a3456c95a7472730dd835b22aecd8156 in January 2023, the winget argmatcher includes descriptions for all subcommands.

UPDATE: Oh, I see: Auto-Suggestions "see" all available completions, even hidden ones. The hiding happens when displaying the completions, and completion scripts have to do the work of hiding the completions at display time, and the Auto-Suggestions have no way to tell whether a script would choose to hide a completion during display. It's designed that way so that input line coloring is able to color recognized arguments even if they're hidden when displaying completions. So Auto-Suggestions ends up including even "hidden" completions. So it might not be coming from a command in your history.

chrisant996 commented 1 month ago

When fzf.color_descriptions is true, pressing enter to accept does not insert the item anymore in my case. I have encountered the same issue in my implementation and it seems to be because r:read('*line') does not preserve color codes, which is why I remove all color codes before inserting the lines into the table. I was able to fix this bug by replacing text with text:gsub('\x1b%[[%d;]*m', '') here:

https://github.com/chrisant996/clink-fzf/blob/main/fzf.lua#L887-L888

Confirmed, and a fix has been pushed. Note that I use console.plaintext() rather than a regex pattern, because it's more accurate. The regex can potentially misfire on some escape codes.

fiso64 commented 1 month ago

It wasn't coming from my history since I have cleared my config directory for testing. I know that completions and suggestions are not the same, but I thought that if the suggestion was displayed for some command then the completion should work as well, but now I understand why it does that. Anyways, clink and all the scripts and are up-to-date (I have only been using it for a week or so). clink-fzf is displaying hidden completions like winget's 'complete' command, and for these completions there are no descriptions and they are not inserted when I press enter.

chrisant996 commented 1 month ago

Oh... I get it now, thanks. I think I can probably get clink-fzf to not list the hidden completions.

(And maybe I can even figure out a way to get suggestions to ignore them... We'll see!)

chrisant996 commented 1 month ago

Yes, I can get clink-fzf to not list the "hidden" completions.

The reason selecting one of the "hidden" completions has no effect, is because it gets filtered from the list of available matches after fzf gets a crack at it. So fzf returns for example "complete", but then "complete" gets removed from the available matches, and so Clink rejects it as invalid, and so nothing gets inserted.

The clink-fzf script needs to use a high priority match generator to hook into match filtering. Otherwise it would sometimes completely fail to invoke fzf at all.

But using a high priority match generator means fzf ends up being the first match filter that gets registered. And it needs to run as the last match filter.

So, it really needs to use two steps:

  1. Register a match filter function (which will run first), and that function must only register another match filter function (which will run last).
  2. The second match filter function is the point at which it must invoke fzf.

That seems to have solved the problem.

chrisant996 commented 1 month ago

(But the only way to make Auto-Suggestions not "see" the "hidden" completions would be to build into Clink the concept of "hidden" matches. Currently the "hidden" concept is completely external -- Clink has no such concept, instead it's done by scripts through match filtering.)

(And Auto-Suggestions cannot invoke match filter functions, because that could greatly break things. Match filters are allowed to be arbitrarily interactive. For example that's how clink-fzf works -- fzf is invoked from a match filter function!)

fiso64 commented 1 month ago

Thanks for addressing this, it works!