Add kind filter for Goto Symbol command

[jwortmann]

Closes #2055.

I removed the colored region box highlighting for now, because I don't really like it personally and ST's built-in Goto Symbol doesn't have something like that. But it could be added back easily.

The implementation is a bit hacky, and there is one significant drawback: a user key binding for this command would need to be adjusted in the following way:

[
    // Document Symbols (a replacement for ST's "Goto Symbol")
    {
        "keys": ["primary+r"],
        "command": "show_overlay",
        "args": {"overlay": "command_palette", "command": "lsp_document_symbols"},
        "context": [{"key": "lsp.session_with_capability", "operand": "documentSymbolProvider"}]
    },
]

Otherwise it won't work due to the input handlers in the command palette. I'm unsure if this breaking change would be acceptable. Maybe I can find a workaround to make it work with the former key binding too.

Other than that I think it's pretty cool and very practical to use.

Preview video:

preview.webm

"All Kinds" will be selected by default, so that the behavior if you just want to run the command and navigate the unfiltered results is the same as before. Oh and there seems to be still a bug with the sorting sometimes, I'll see if I can fix that.

[rchl]

Otherwise it won't work due to the input handlers in the command palette. I'm unsure if this breaking change would be acceptable. Maybe I can find a workaround to make it work with the former key binding too.

I'd rather not introduce a breaking change...
Let's try to add a workaround so that the old one still works.

[rchl]

Having 3 lines per item can look a bit ugly.

Before:

After:

Could we format like before -- combine two of those properties into one line?

[jwortmann]

I found a workaround to keep the current key bindings working (actually the logic is a bit more straightforward now).

But there is still a bug in case you add custom binding with the kind argument like

    {
        "keys": ["primary+r"],
        "command": "lsp_document_symbols",
        "args": {"kind": 5},
        "context": [{"key": "lsp.session_with_capability", "operand": "documentSymbolProvider"}]
    },

It works fine if this kind exists in the response, but otherwise there will be an infinite loop. I'll take a look later.

I've reverted the item formatting to be similar as before now, with only one details line.

The sorting bug I mentioned above is not really a bug; it is just that ST reorders the items according to fuzzy matching when you type something in the input - but this is also the case with the quick panel which is currently used, and afaik there is no way to prevent that.

And I've noticed that the colored region box highlighting on main is even buggy when there are only some items in the response which have the corresponding range information, then the highlighting will be "stuck" on those items (you can see that for example with LSP-css and @font-face rules). So I think it's a good thing to remove this part.

[jwortmann]

I think everything should be fixed now.

It is now possible to use the optional kind argument in key bindings (or command palette entries) to start with a specific symbol kind preselected for filtering, instead of "All Kinds".

For example for kind "Class":

    {
        "keys": ["primary+r"],
        "command": "lsp_document_symbols",
        "args": {"kind": 5},
        "context": [{"key": "lsp.session_with_capability", "operand": "documentSymbolProvider"}]
    },

I have one remaining question; is "All Kinds" a good name for the default label, or would "Any Kind" or maybe even just "Any" be better?

[rchl]

I have one remaining question; is "All Kinds" a good name for the default label, or would "Any Kind" or maybe even just "Any" be better?

I think it works now.
I've tried to forget about the LSP terminology and think of how ST would call it but I couldn't think of anything better.

BTW. When testing, I thought it would be nice to be able to exclude certain kinds like "Variable" and "Constant" while including all other but it might be impossible with how list handlers work, I suppose.

I will do a bit more testing and spend some time on looking at the code before accepting but it looks nice.

[jwortmann]

BTW. When testing, I thought it would be nice to be able to exclude certain kinds like "Variable" and "Constant" while including all other but it might be impossible with how list handlers work, I suppose.

I did think about this too, and my approach would be to use an array like "kinds": [1, 4, 12] instead of the single kind. But this would only work for custom key bindings or command palette entries, but not dynamically when you select the kind via the input handler in the command palette, because there is no multi-select. For such key bindings it could then show something like "3 Kinds" instead of the different kind names (which would be too long) for the label. But I didn't like the asymetrical behavior in that case, that when you hit backspace after triggering such a key binding, you could not get to this state again where are multiple kinds selected. So I eventually discarded this idea and I think the approach with the single kind argument should be easier and good enough to use. In practice you probably always know anyways whether you look for a function or a variable or something else, and it's super quick to just hit backspace and then select the appropriate kind name to list only the matching symbols. So I'm pretty satisfied with the behavior from this PR now.

When you look at the source code you will see that the implementation is a bit of an hack, where the command calls itself a second time and saves state between those calls, but this is necessary due to the async nature of the request/response to fetch the results for the list input panel, which is not a consideration in the ST input() API method design. And it might be slightly complex to follow the logic that is needed to accound for in what order and whether ST calls the input() and run() method, depending on different ways how the command was invoked. I think the only part which could theoretically go wrong is if we get to this part of the code in the input() method (line 180):

            if not window:
                return None

because then it would accidentally run the same request a second time. But it should still be safe to not end up in an infinite loop, because it cannot call the command again another time after the response, when there is no more window. Not sure if this description was somewhat understandable or if you were still able to follow, but in other words I didn't find anymore bugs when testing and I think that the logic should be good now.

In general the implementation is a complete rewrite of the LspDocumentSymbolsCommand, but I think especially the code to convert from LSP response to ST list items is quite a bit simpler now.