I'll try to clarify my position on this PR. I didn't really receive any valuable input regarding the implementation itself (not blaming anyone, I completely understand each of us has other duties and limited time for Geany). The feedback from @elextr and @b4n was mostly related to the functionality of the plugin which I tried to address more or less (of course, I can't do much with the behavior of LSP servers themselves).
The feedback from @kugel- was IMO based on some assumptions of how LSP works which weren't based on reality. LSP servers are black boxes that provide the given functionality and aren't meant to be used as a source of symbols that are used for some advanced logic of the client - the necessary information just isn't there. If anyone wants to go this direction, the person should first study LSP and Geany and give concrete suggestions about how to implement it (which I'm not convinced is possible). There's also the possibility of a "hybrid" implementation so that we use the LSP API for everything but still inject the symbols into the TM but I'm really worried that the outputs of various servers will vary greatly, even among the same server's versions (e.g., while testing, @elextr ran into an issue that one clang version reported what we call "scope" but some older version didn't) and I don't believe that having to configure the behavior of LSP servers for every possible server version is the way we should go. Moreover, we can't easily get all the symbols from the whole workspace using LSP (apart from querying it individually for each file) so I'm not sure if we could really mirror what we do for the TM. So the way I see it is to do something similar to this PR but of course there are tons of ways to implement it and I'm open to any suggestions. I needed something relatively quickly so I could create the plugin and also wanted some feedback first before doing some possible multi-thousand LOC refactoring unnecessarily. This PR is just around 500 LOCs and more or less demonstrates the things that are affected by LSP in Geany with not too many diffs involved. This doesn't mean it couldn't be implemented the way this PR suggests - I think it's mostly OK and not too intrusive, only the symbol tree implementation should definitely be changed to something better because the current way is way too hacky. But before doing that, I'd prefer some feedback in case someone has a feeling the whole LSP integration should be implemented in some other way so I don't waste my time. I don't want to introduce something that causes some friction among Geany developers. On the other hand, I don't want the plugin be a vaporware - it has around 7000 LOCs (the Geany part is the trivial one in comparison), it cost me a significant amount of time to develop and I just don't want to throw it to the garbage bin. So what I'm proposing is to have some version of the plugin for the time being until (if) the Geany portion is acceptable for everyone where the plugin doesn't depend on any special support in Geany. For such a version I could create a PR against geany-plugins and it could receive a broader testing among users. The thing will be clumsy in many ways - there will have to be separate keybindings for identical things that are present in both Geany and the plugin (such as going to definition/declaration), TM will have to be disabled for the given file type manually by users and some things like the symbol tree wouldn't work at all. But there will be something usable and useful. This shouldn't be much work for me and the plugin will live at least in some form. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3571#issuecomment-1979139769 You are receiving this because you are subscribed to this thread. Message ID: <geany/geany/pull/3571/[email protected]>
