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

[#23] Refresh open editors when compile_commands.json changes #24

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

ddscharfe
Copy link
Contributor

Update open editors when compile_commands.json file of the project of the edited file is added/removed/changed.

This makes use of clangd hot-reload, see https://reviews.llvm.org/D92663.
I thought it should be possible to notify clangd via didChangeWatchedFiles, however clangd receives the command but doesn't seem to update the compilation data base.

@ghentschke
Copy link
Contributor

I thought it should be possible to notify clangd via didChangeWatchedFiles, however clangd receives the command but doesn't seem to update the compilation data base

They mentioned in the commit message of llvm/llvm-project@9899319 :

Slight behavior change: we now only look for build/compile_commands.json
rather than trying every CDB strategy under build subdirectories.

I'am not sure, but it seems that the compile_commands.json file has to be in the subdirectory /build relative to the source code file e.g. src/build/compile_commands.json

@ddscharfe
Copy link
Contributor Author

Slight behavior change: we now only look for build/compile_commands.json
rather than trying every CDB strategy under build subdirectories.

I'am not sure, but it seems that the compile_commands.json file has to be in the subdirectory /build relative to the source code file e.g. src/build/compile_commands.json

I think this comment just means that the auto refresh only checks the compile_commands.json in the build directory, meaning the toplevel directory. AFAIK you can put compile_commands.json files anywhere in the sub hierarchy but they are not covered by this mechanism.

@ghentschke
Copy link
Contributor

I think this comment just means that the auto refresh only checks the compile_commands.json in the build directory, meaning the toplevel directory.
Have you tried this?

@ddscharfe
Copy link
Contributor Author

I think this comment just means that the auto refresh only checks the compile_commands.json in the build directory, meaning the toplevel directory.
Have you tried this?

I just did. It doesn't seem to make a difference at all if the json file is at the toplevel or under toplevel/build.

Btw I just tested using org.eclipse.lsp4e 0.15.0 instead of org.eclipse.lsp4e 0.16.0 and with the 0.15.0 version it seems to work without the delay of 5 seconds. However with 0.15.0 I get an exception

INFO: Failed to send notification message.
org.eclipse.lsp4j.jsonrpc.JsonRpcException: java.io.IOException: Stream closed
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageConsumer.consume(StreamMessageConsumer.java:72)
	at org.eclipse.lsp4e.LanguageServerWrapper.lambda$3(LanguageServerWrapper.java:287)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.notify(RemoteEndpoint.java:126)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.sendCancelNotification(RemoteEndpoint.java:180)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint$1.cancel(RemoteEndpoint.java:150)
	at org.eclipse.lsp4e.outline.LSSymbolsContentProvider.refreshTreeContentFromLS(LSSymbolsContentProvider.java:303)
	at org.eclipse.lsp4e.outline.LSSymbolsContentProvider$ReconcilerOutlineUpdater.process(LSSymbolsContentProvider.java:156)
	at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:207)

@ghentschke
Copy link
Contributor

interesting, I'am working with the LSP4E sources in in my workspace. I'll try it

@ghentschke
Copy link
Contributor

Btw I just tested using org.eclipse.lsp4e 0.15.0 instead of org.eclipse.lsp4e 0.16.0 and with the 0.15.0 version it seems to work without the delay of 5 seconds. However with 0.15.0 I get an exception

Do you set schedule the UIJob immediately:

		UIJob.create("Refresh Editors", monitor -> {
			int rangeOffset = textViewer.getTopIndexStartOffset();
			int rangeLength = textViewer.getBottomIndexEndOffset() - rangeOffset;
			editor.getSite().getPage().reuseEditor((IReusableEditor) editor, editor.getEditorInput());
			textViewer.revealRange(rangeOffset, rangeLength);
		}).schedule();

@ghentschke
Copy link
Contributor

ghentschke commented Mar 3, 2023

@ddscharfe Please rebase your PR to let the CI build run

@ddscharfe ddscharfe force-pushed the compile-commands-monitor branch from 8f82abb to 6f48f62 Compare March 3, 2023 11:18
@ghentschke
Copy link
Contributor

ghentschke commented Mar 3, 2023

I did the folowing with the LSP4E build form sources:

  1. schedule refreshEditor without delay
  2. Open IDE
  3. Open c/c++ file in LSP editor
  4. LSP server gets started
  5. Delete compile_commands.json in c++ project
    The LS gets terminated:
I[12:12:20.481] --> reply:textDocument/documentSymbol("4") 0 ms, error: -32602: trying to get AST for non-added document
I[12:12:24.972] <-- shutdown("5")
I[12:12:24.972] --> reply:shutdown("5") 0 ms
I[12:12:24.973] <-- exit
I[12:12:24.973] LSP finished, exiting with status 0

With a 5 sec schedule delay it works. After re-build of the project an a newly created compile_commands.json the editor gets refreshed after 5 sec

@ddscharfe
Copy link
Contributor Author

Btw I just tested using org.eclipse.lsp4e 0.15.0 instead of org.eclipse.lsp4e 0.16.0 and with the 0.15.0 version it seems to work without the delay of 5 seconds. However with 0.15.0 I get an exception

Do you set schedule the UIJob immediately:

		UIJob.create("Refresh Editors", monitor -> {
			int rangeOffset = textViewer.getTopIndexStartOffset();
			int rangeLength = textViewer.getBottomIndexEndOffset() - rangeOffset;
			editor.getSite().getPage().reuseEditor((IReusableEditor) editor, editor.getEditorInput());
			textViewer.revealRange(rangeOffset, rangeLength);
		}).schedule();

Yes.

@ghentschke ghentschke self-requested a review March 3, 2023 15:43
@ghentschke ghentschke assigned ghentschke and ddscharfe and unassigned ghentschke Mar 3, 2023
@ddscharfe ddscharfe force-pushed the compile-commands-monitor branch from 6f48f62 to 32d1d75 Compare March 6, 2023 09:15
@ddscharfe ddscharfe requested a review from ghentschke March 6, 2023 09:16
@ddscharfe ddscharfe force-pushed the compile-commands-monitor branch from 32d1d75 to b3b9bea Compare March 6, 2023 09:48
@ddscharfe ddscharfe requested a review from ghentschke March 6, 2023 09:50
Copy link
Contributor

@ghentschke ghentschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could you provide test for this feature?

@ghentschke ghentschke merged commit 1f3aa8c into eclipse-cdt:master Mar 6, 2023
@ddscharfe
Copy link
Contributor Author

I could add a few integration tests, this would probably make sense. I'd also like to have unit tests, for which using dependency injection and mocking would be helpful. Would you mind using google guice in this project and mockito for the tests?

@ghentschke
Copy link
Contributor

Would you mind using google guice in this project and mockito for the tests?

Mockito is fine. I'am not sure about google guice, since it should be a CDT feature in the future. We should ask @jonahgraham regarding this.

@ddscharfe
Copy link
Contributor Author

I asked this question to @jonahgraham a few years ago: https://www.eclipse.org/lists/cdt-dev/msg31411.html
So I think having guice as dependency wouldn't be a problem, it's more a design choice, I think. I like to use dependency injection for my projects I think it helps having less coupling between classes, however it does add some kind of complexity.

@ghentschke
Copy link
Contributor

I like to use dependency injection for my projects I think it helps having less coupling between classes, however it does add some kind of complexity.

Yepp, we do this in our projects as well.

@ddscharfe ddscharfe deleted the compile-commands-monitor branch March 6, 2023 16:36
@jonahgraham jonahgraham added this to the 1.0.0 milestone Sep 18, 2023
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

Successfully merging this pull request may close these issues.

3 participants