Experimental Workspace Symbols overhaul

[jwortmann]

Here is another hack from me. Or maybe let's call it a creative way of using the ST API, rather than a hack ;-)

I prefer the ST native "Goto Symbol in Project" over the LSP variant, because for the latter you have to type the query text first, before the results are shown. So I basically never use the one from LSP.

This PR allows to fetch and show the results of the "LSP: Goto Symbol in Project" command before you hit Enter. And I think the PR could also make it possible to support partial results for the workspace symbols request, though I haven't looked into that yet.

This was a bit of an experiment and it might have some strange behaviors. So for now I created this PR as a draft, and we can see if it can be improved, or otherwise close it and hope that one day ST will implement such a panel natively (sublimehq/sublime_text#3338). But I think it is an interesting approach and has some potential. Maybe I will revisit it at a later time.

Current drawbacks in this implementation are:

• Sometimes parts of the input text might vanish... I don't know what causes this or how to reproduce it, but this is certainly a blocker.
• ST will select the first item of the server response, but then reorders the items in the list according to its fuzzy matching. So the selected item will not be at the top. I think we need to sort the server response first, according to Sublime's fuzzy matching with the query text, to prevent this. Or do we even need to keep track of the selected item before fetching the updated results, so that it won't suddenly change (basically what @rchl describes in Extend quick panel API to support language servers sublimehq/sublime_text#3338 (comment))? I'm not sure about what the expected behavior should be, but I think this definitely needs some improvements.
• The command palette changes its width when items are updated, dependent on the length of the longest item text.
• Possibly other bugs which I haven't noticed?

[jwortmann]

Update:

• Sometimes parts of the input text might vanish...

  I think this might have been fixed already. But I will keep an eye open to see whether it can still happen.

• ST will select the first item of the server response, but then reorders the items in the list according to its fuzzy matching. So the selected item will not be at the top.

  It will now select the topmost row in the list (i.e. best match according too ST fuzzy matching). It still means that the selected item might change when the items are updated after you typed some more characters. But I think this could actually be the desired behavior. Because usually you roughly know the symbol name to type, and then after that possibly scroll through the list with the arrow keys, before you hit Enter. And if you type something more, a currently selected item might end up somewhere else down in the list, so if the selection was stable even after the update you would manually need to scroll to the top to see the most relevant items. But I'm not settled on this point and open for suggestions.

• The command palette changes its width when items are updated, dependent on the length of the longest item text.

  This is still the case, but it is only an aesthetic issue. And I think there might be a way to prevent this somehow.

• Requests are now sent to all sessions in the window, instead of only to the session of the active view. This should be closer to what the built-in "Goto Symbol in Project" does, which also shows the results from all relevant syntaxes for different filetypes.

  In case you have multiple workspaceSymbolProvider sessions running for the same filetype, this means that there might be duplicated results. But I assume that for such a multi-session setup for a filetype, the servers are chosen to complement each other, for example LSP-typescript for general features and then quick-lint-js only for additional diagnostics. And even if not, you can still use "disabled_capabilities" for the workspaceSymbolProvider of the unneeded sessions (I think a similar thing would happen with duplicate completion items from multiple running sessions).

• Added support for workspaceSymbol/resolve (lazy evaluation of the location).

Has anybody tried out this branch yet, or is the implementation too "hacky"? I think it could be a nice UX improvement.

[predragnikolic]

No, I haven't tried it.
I will give it a go

[rchl]

I'm impressed with your persistence in getting this working against all odds (ST limitations) :)

After quick testing I've noticed that we are loosing fuzzy filtering in the process. Previously you would search for a symbol (in a non-fuzzy way typically) and get a list with results that could be filtered in a fuzzy way. Now the query both does a query against the server and filters so no longer works that way.

Not sure how acceptable that is.

[jwortmann]

After quick testing I've noticed that we are loosing fuzzy filtering in the process. Previously you would search for a symbol (in a non-fuzzy way typically) and get a list with results that could be filtered in a fuzzy way. Now the query both does a query against the server and filters so no longer works that way.

Yeah, that is right. But as you say, with the previous way there is also no fuzzy matching when you type the query (unless implemented by the server itself), so if you made a typo there or don't really know the exact symbol name, it might also show no relevant results from the server. And in general I would not say that the usability of the previous implementation with the two-step procedure is better in any way. If you want to make use of the fuzzy matching, then you have to type the input twice; first in the query field and after that again when the results are show. And there is no quick way to get back to the query text.
With the new implementation from this PR you can simply press backspace a few times if there are no results from the server for the current input text. So in my opinion this is much easier to use and saves a bit of time, when the results for the current input are immediately shown and you don't really have to first think about what exact text you want to put as the input query.

I've checked VS Code too, and there seems to also be no fuzzy matching for the "Go to Symbol in Workspace" feature (at least when it uses the workspace/symbol results from the server; the Python extension does have fuzzy matching, but it doesn't seem to send any server requests - maybe they use syntax based results like native ST "Goto Symbol in Project" or so).

[predrag-codetribe]

I noticed that there there is an empty ListInputItem when I delete the last char:

Screen.Recording.2023-10-18.at.11.03.50.mov

Also it would be nice to display "No results" when there are no search results

        return [symbol_to_list_input_item(item, session_name=session_name) for item in response] if response else [sublime.ListInputItem('No results', value=None)] 

I like this PR more than the current behaviour :)

[jwortmann]

I noticed that there there is an empty ListInputItem when I delete the last char:

Yes, this empty item is needed as a trick to select the topmost row instead of the first item of the list, see sublimehq/sublime_text#6162 (comment). If my suggestion from the issue report maybe gets implemented in the future, we could remove this empty item, but for now this was the only workaround I found.

Also it would be nice to display "No results" when there are no search results

Nice idea, added.

[rwols]

I tried this a bit and it’s already a big improvement over the previous implementation! I vote to merge this.

[jwortmann]

I tried to fix the volatile width of the command palette after updating the list items, but I couldn't find a way to do it. The command palette keeps a constant width when you manually go back in an input handler with Backspace and then open the command again, and even if the length of the longest list item is now different. But I found no way to do that programatically, because there is no command to pop an input handler from the input stack. I tried to workaround that via select_all, right_delete (to delete the input text), then select on an empty item which triggers a BackInputHandler, and then insert the command name again and select, but it turned out to not work because select only works with sublime.set_timeout, and it unfortunately seems to only work a single time per chained actions.

Nevertheless I'll mark the PR as ready, so you could review if you want.

[predragnikolic]

Sometimes parts of the input text might vanish... I don't know what causes this or how to reproduce it, but this is certainly a blocker.

Sometimes, it can happen that the text in the palette gets deleted:
Screencast from 2023-10-22 11-31-37.webm

For example I was typing a s d f g h j k l really slowly,
and in the video you can se that a s d were removed when I typed f

[jwortmann]

Sometimes, it can happen that the text in the palette gets deleted

I think I know what causes this bug. When an InputHandler is shown in the command palette, with a non-empty initial_text, then the full input text is automatically selected in the input field. There was an initial_selection method introduced in ST 4081, but it doesn't seem to work. So I added this workaround to manually put the caret at the end of the input text, after the input handler is created:

        # --- Hack needed because the initial_selection method is not supported on Python 3.3 API
        selection = self.input_view.sel()
        selection.clear()
        selection.add(len(self.text))
        # --- End of hack

But this cannot run in __init__ or initial_text(), because at that time the input view (text field) doesn't exist yet. So I must run it with sublime.set_timeout (see attach_listener method which finds the input field view).

Now the problem is that apparently there is a very small timespan where the input field was just created, with the whole text still selected, and a key press event when typing another character gets recognized at the same time. In that case it will replace the whole input text, before the attach_listener runs (due to set_timeout). I've confirmed this by adding some debug printing in DynamicListInputHandler.on_modified, and indeed this function doesn't run in exactly those cases when a part of the input text gets removed.

I have opened an issue for the initial_selection bug at sublimehq/sublime_text#6175 and it appears to be easy to fix; you can even do it manually by modifying sublime_plugin.py. After a quick test with the described fix I can't reproduce the bug anymore - it might only still miss to send a new request to the server for updated input text, but that should happen very rarely and "fixes" itself if you continue typing.

Removed the workaround from above (meaning that the PR is only usable with a fixed sublime_plugin.py), and converting back to draft until it gets fixed in ST.

[predrag-codetribe]

I just wanted to tell that the UX is gold.

[rchl]

I still think we are loosing something (fuzzy filtering) in the process which makes me a little hesitant about this change.

Before we could do a basic query to get a lot of symbols and then filter it more with fuzzy matching.

Now the fuzzy matching is suppressed by server results.

Screen.Recording.2023-11-09.at.23.36.55.mov

Though to be fair i pretty much never use LSP's "Goto in project" because of that two-step flow and because Sublime's own implementation is good enough typically so I'm fine with others overriding me on this one.

[jwortmann]

I still think we are loosing something (fuzzy filtering) in the process

Yes, I already partially answered in #2333 (comment). But it depends on the server, for example Pyright does support fuzzy matching. Some servers might even support special query patterns, though I don't know if there is any example for that. This would probably not work with this PR due to being filtered out by ST matching. Another related discussion: microsoft/language-server-protocol#1674

So the question is which UX is the better one, the previous two-step behavior with fuzzy matching after the query string, or the dynamic input from this PR, possibly without the fuzzy matching.

[predragnikolic]

I like this more than the current behavior

[jwortmann]

I've marked the PR as ready again, so you could decide whether it should be merged or not, or maybe there are still some suggestions for modifications.

The bug mentioned above with the deleted input should be fixed now both on stable and dev ST builds, and it should happen only very rarely for older builds.

One point which I think was not discussed above, is that with this PR there is no way anymore to send an empty query string to the server. In theory the behavior could easily be changed to always send a request with empty query when the command is run, but I decided to not do that because probably in 99% of cases the user will start typing anyway when the input handler gets opened, and when the response from the empty query arrives it would most likely already be outdated. And I guess that the request with empty query, i.e. all symbols in the project, is rather expensive, so it might not be the best idea to do this.

[predragnikolic]

I would merge this till the end of the week if no one has anything against it.