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

Where I'm up to #8

Open
elextr opened this issue Nov 2, 2023 · 16 comments
Open

Where I'm up to #8

elextr opened this issue Nov 2, 2023 · 16 comments

Comments

@elextr
Copy link
Contributor

elextr commented Nov 2, 2023

See comments in code

foo.cpp:

#include <string>
#include <iostream>
#include <memory>
#include "foo.hpp"

using namespace std;

class Cpp {
	std::string s;
public:
	Cpp(std::string&& st):s(move(st)){}
	std::string str(){ return s; }
	void str(const string& st){ s = st; }
	// string is highlighted without std:: because of the using
	size_t size();
	~Cpp(){}
};

size_t Cpp::size(){
	return s.size();
}

size_t Hpp::size(){
	return s.size();
}

int main(){
	Hpp h("foo");
	Cpp c("bar");
	auto cp = make_unique<Cpp>("abc");
	std::cout<<"h="<<h.str()<<":"<<h.size()<<" c="<<c.str()<<":"<<c.size()<<std::endl;
	c.str("abc");
	cout<<"cp="<<cp->str()<<endl; 
	// cp-> works, and type of cp was inferred from a template at L29!!!
	// cp->str( correctly shows two overloads
	int Hpp = 0; ++Hpp;
	//  ^^^ should not be highlighted
	// I guess this is Scintilla (once a name is in a keyword list its always a type)
	return 0;
}
@techee
Copy link
Owner

techee commented Nov 2, 2023

	int Hpp = 0; ++Hpp;
	//  ^^^ should not be highlighted
	// I guess this is Scintilla (once a name is in a keyword list its always a type)

So yeah, the implementation of semantic tokens is a little dumb right now but I don't know if there's a simple way to fix it with Scintilla. The LSP server returns correctly the token types exactly with the right positions in the code that could then be colorized at those positions only. In Scintilla I found

https://www.scintilla.org/ScintillaDoc.html#Styling

but when I try to use SCI_STARTSTYLING followed by SCI_SETSTYLING, all the other colorization is lost. It appears to be used as an alternative to lexers when you want to colorize the whole document by yourself but it doesn't seem to be possible to colorize just some selected words in the document (but maybe I didn't do something right and I'm wrong here).

So what I did is that I check the positions of semantic tokens that LSP returns, get the strings at those positions from Scintilla, remove duplicates and pass it to Scinitlla the same way like for TM:

https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea/src/document.c#L2761

It's kind of backwards and we lose the information about the exact position of semantic tokens, but I'm not sure if there's anything we can do here (and before you ask, no, I currently don't plan implementing a whole lexer based on semantic tokens :-).

@elextr
Copy link
Contributor Author

elextr commented Nov 2, 2023

SCI_STARTSTYLING followed by SCI_SETSTYLING

IIUC thats for mainly for styling triggered by a SCN_STYLENEEDED notification, thats how the apps that use regex styling work with Scintila. I don't think it can safely be combined with a conventional lexer.

and before you ask, no, I currently don't plan implementing a whole lexer based on semantic tokens

I hate mind readers ;-P

Basically there needs to be a way of having syntax highlighting by the lexer (fast and synchronous) overridden by semantic stuff from the LSP (slow and asynchronous).

@elextr
Copy link
Contributor Author

elextr commented Nov 2, 2023

What do you use to trigger LSP queries? Vscode uses a slow(ish) timer (slow enough to be visible, maybe 1/4 sec Edit: actually its probably several secs, maybe I will find the setting one day) or when the cursor moves lines.

Maybe enable discussions on the repo for stuff like this?

@techee
Copy link
Owner

techee commented Nov 2, 2023

What do you use to trigger LSP queries? Vscode uses a slow(ish) timer (slow enough to be visible, maybe 1/4 sec) or when the cursor moves lines.

Depends on what LSP queries you have in mind. Autocompletion, symbol tree, and semantic tokens are triggered by the same code like in Geany itself so it should be (if I remember correctly) the default 300ms timeout.

Diagnostic messages (underlined errors) are displayed immediately now and this isn't very efficient and could cause performance problems - there are multiple things I could do in this respect:

#4

It just worked OK for editing Geany itself so I didn't do anything about it yet.

Maybe enable discussions on the repo for stuff like this?

Should be enabled now (I hope).

@elextr
Copy link
Contributor Author

elextr commented Nov 2, 2023

It just worked OK for editing Geany itself so I didn't do anything about it yet.

Did you try in Scintilla (ie C++)? Remember how much slower compiling Scintilla files is than Geany files.

@techee
Copy link
Owner

techee commented Nov 2, 2023

Did you try in Scintilla (ie C++)? Remember how much slower compiling Scintilla files is than Geany files.

I tried but didn't see anything too horrible. If you run into performance issues, would you try disabling various LSP features to check exactly which one it is? For performance, my suspects are:

diagnostics_enable=true
symbol_highlight_available=true

(well, symbol_highlight_available should have been called symbol_highlight_enable, I'll probably rename the settings name soon).

@techee
Copy link
Owner

techee commented Nov 2, 2023

What do you use to trigger LSP queries? Vscode uses a slow(ish) timer (slow enough to be visible, maybe 1/4 sec) or when the cursor moves lines.

But I just noticed that I send textDocument/didChange notification immediately which makes clangd re-parse the current file all the time. I think I should batch those (but make sure that if I need to send some other request to the server, the didChange notifications get sent before).

@elextr
Copy link
Contributor Author

elextr commented Nov 3, 2023

Well if you wait until you need it to send changes and initiate parsing you are making whatever needs the parsing wait on the parsing before it gets done, so maybe its not so good since it may make the user wait on it as well. Since the parsing is in another process having it do stuff in the background should not be a bad thing ™️

@techee
Copy link
Owner

techee commented Nov 3, 2023

Well if you wait until you need it to send changes and initiate parsing you are making whatever needs the parsing wait on the parsing before it gets done, so maybe its not so good since it may make the user wait on it as well.

I was thinking about batching change notifications in something like 250ms intervals unless something else might need them in which case they'd be sent earlier. From what I have seen, autocompletion for instance seems to return results before the compilation happens so it wouldn't be blocked by it. But maybe not worth the work I don't know.

I don't know how vscode behaves in this respect, it's just that while typing, the background clangd process seems to consume something like 60% CPU when editing some C++ Scintilla file (it's around 15% for plain C).

@elextr
Copy link
Contributor Author

elextr commented Nov 3, 2023

autocompletion for instance seems to return results before the compilation happens

That makes sense, if its triggering autocompletion then it is, by definition, not complete, so it possibly won't compile at that moment, so no point in waiting for a compile that might fail.

something like 60% CPU when editing some C++ Scintilla file

I tried vscode opening editor.cxx took about 40% for a while, then editing editor.cxx and renaming a symbol that needed it to edit several other unopened files, and it never went above 10-15% whereas just opening that file in geany-lsp has clangd at 70%, but after that it also never seems to get above 10-15%.

Edit: obviously it was a meson build of Geany to have the compile commands file, and vscode offered to work it all out itself, or read that :-)

@techee
Copy link
Owner

techee commented Nov 3, 2023

I tried vscode opening editor.cxx took about 40% for a while, then editing editor.cxx and renaming a symbol that needed it to edit several other unopened files, and it never went above 10-15% whereas just opening that file in geany-lsp has clangd at 70%, but after that it also never seems to get above 10-15%.

Alright, if vscode (the aspirational benchmark of what Geany could become) behaves similar, it's probably alright :-).

@techee
Copy link
Owner

techee commented Nov 3, 2023

Edit: obviously it was a meson build of Geany to have the compile commands file, and vscode offered to work it all out itself, or read that :-)

You don't even have to build it with Geany, just meson setup build is enough and you can keep using autotools.

@elextr
Copy link
Contributor Author

elextr commented Nov 3, 2023

and you can keep using autotools.

Why? 😛

@elextr
Copy link
Contributor Author

elextr commented Jan 14, 2024

Just a new year update, have been using this intermittently and been quiet because its "just worked" for me. Congratulations, well done.

Edit: to be clear "this" means "geany-lsp" and mostly used it for answering support questions on Geany, its so nice to click a name and goto ... and have it right every time. And the hover is useful at times. C++ is still mostly Vscode due to the multi-window capability, sorry its just soooo useful.

@techee
Copy link
Owner

techee commented Jan 20, 2024

Just a new year update, have been using this intermittently and been quiet because its "just worked" for me. Congratulations, well done.

Thanks, good to hear! I haven't had much time recently but I'd like to finish some things and have a look at #32.

Also, I've been thinking about some "non-modified-Geany" mode of the plugin in case we don't find any agreement in geany/geany#3571. Basically, users would have to manually disable the ctags parser for the language they use the LSP plugin for in the filetype's config file using:

[settings]
tag_parser=

Then the plugin would have to detect ctrl+click and perform tag definition/declaration goto independently of Geany. There would also have to be separate keybindings for these actions. The symbol tree wouldn't work (which I wouldn't miss much myself). But then I believe I could make all the rest of the LSP functionality work fine as ctags wouldn't interfere as it would be disabled and since the Scintilla API is fully exposed to plugins.

@elextr
Copy link
Contributor Author

elextr commented Jan 20, 2024

I don't think its worth having a half baked version, all that will happen is users will ask "why doesn't it work like vscode/emacs/...".

I think we need to work through geany/geany#3571 or some equivalent of it, provided lsp can be disabled and ctags left in its current state there is absolutely no excuse for anybody saying "I don't want it so nobody can have it". That is just unacceptable behaviour, of course they are not expected to contribute, but active blocking or attack without proposing any alternative should not happen.

Clearly we need to communicate the reasoning behind the changes, remember when it first happened I suggested using TM until I understood your reasoning of "simply replace specific Geany functions" as a less intrusive method of introducing new capabilities.

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

No branches or pull requests

2 participants