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.

Sign up for GitHub

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

Jump to bottom

fix: debounce classpath/source change events #959

Merged
merged 1 commit into from
Jun 23, 2023
Merged

fix: debounce classpath/source change events #959

merged 1 commit into from
Jun 23, 2023

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Jun 15, 2023 •
edited by fbricon
Loading

fix: debounce classpath/source change events (#916)

Fixes #916
Fixes #958

Signed-off-by: azerr [email protected]

@angelozerr angelozerr force-pushed the aggregate-events branch 5 times, most recently from 4e4e68e to e9c7d3c Compare June 15, 2023 16:35
@fbricon fbricon changed the title fix: events agregtate fix: debounce classpath/source change events Jun 16, 2023
fbricon
fbricon requested changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

please also fix the commit message

...main/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangeManager.java Outdated Show resolved Hide resolved
...main/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangeManager.java Outdated
Comment on lines 58 to 59
this.connection.disconnect();
PsiManager.getInstance(project).removePsiTreeChangeListener(listener);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the listener be removed first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

gut feeling. Feel free to ignore that question

...n/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathRessourceChangedListener.java Outdated Show resolved Hide resolved
...main/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangeManager.java Outdated
@@ -0,0 +1,61 @@
package com.redhat.devtools.intellij.quarkus.classpath;
Copy link
Contributor

Choose a reason for hiding this comment

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

header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and I add Javadoc in class header

src/main/java/com/redhat/devtools/intellij/quarkus/QuarkusProjectService.java Outdated
public void dispose() {
connection.disconnect();
executor.shutdown();
private class ResourcesChangeListener extends PsiTreeChangeAdapter implements WorkspaceModelChangeListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove if ClasspathResourceChangeManager is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I clean the QuarkusprojectService which is used to create JSON schema and it seems it force loading of Quarkus deployment library if I have understood correctly.

...n/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathRessourceChangedListener.java Outdated
}

private void handleChangedPsiTree(PsiTreeChangeEvent event) {
PsiFile psiFile = event.getFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you check psiFile.getVirtualFile() instead?, since it's checked in tryToAddSourceFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I don't understand what you mean

Copy link
Contributor

Choose a reason for hiding this comment

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

you're checking event.getFile() for null, why not psiFile.getVirtualFile() instead, since this is what is being used later on?

src/main/java/com/redhat/devtools/intellij/quarkus/classpath/SourceFileChangeNotifier.java Outdated Show resolved Hide resolved
src/main/java/com/redhat/devtools/intellij/quarkus/classpath/SourceFileChangeNotifier.java Outdated Show resolved Hide resolved
src/main/java/com/redhat/devtools/intellij/qute/lsp/QuteLanguageClient.java Outdated Show resolved Hide resolved
src/main/java/com/redhat/devtools/intellij/qute/lsp/QuteLanguageClient.java Outdated Show resolved Hide resolved
@angelozerr angelozerr changed the title fix: debounce classpath/source change events fix: Unreasonable amount of requests sent when saving pom.xml file Jun 16, 2023
@angelozerr angelozerr changed the title fix: Unreasonable amount of requests sent when saving pom.xml file fix: debounce classpath/source change events Jun 16, 2023
@angelozerr angelozerr force-pushed the aggregate-events branch 4 times, most recently from 6a64f58 to 579b51b Compare June 16, 2023 13:03
@angelozerr angelozerr force-pushed the aggregate-events branch 3 times, most recently from d80a7d0 to f21055f Compare June 16, 2023 14:26
@angelozerr angelozerr marked this pull request as ready for review June 16, 2023 14:27
@angelozerr angelozerr force-pushed the aggregate-events branch 4 times, most recently from b1e3953 to a89d8d1 Compare June 16, 2023 16:04
fbricon
fbricon reviewed Jun 20, 2023
View reviewed changes
...ain/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathLibraryChangedListener.java Outdated
libraryClass = getLibraryClass();
// here Java reflection is used to try to support several IJ versions
if (libraryClass == null) {
LOGGER.warn("Cannot find LibraryEntity class '" + String.join(",", LIBRARY_ENTITY_CLASSES) + "' in classpath.");
Copy link
Contributor

Choose a reason for hiding this comment

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

also LOGGER.warn("Reacting to classpath changes is not supported in this version of Intellij IDEA")

fbricon
fbricon reviewed Jun 20, 2023
View reviewed changes
...ain/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangedManager.java Outdated
connection = project.getMessageBus().connect();
// Track end of Java libraries update
WorkspaceModelTopicsBridge workspaceModelTopicsBridge = new WorkspaceModelTopicsBridge();
Object libraryChangedListener = new ClasspathLibraryChangedListener(project).getProxyInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to create a proxy instance if workspaceModelTopicsBridge.CHANGED == null

fbricon
fbricon reviewed Jun 20, 2023
View reviewed changes
...java/com/redhat/devtools/intellij/quarkus/classpath/bridge/VersionedStorageChangeBridge.java Outdated
if ("getChanges".equals(method.getName())) {
method.setAccessible(true);
getChangesMethod = method;
getChangesMethods.put(clazz, getChangesMethod);
Copy link
Contributor

Choose a reason for hiding this comment

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

break here

@fbricon
Copy link
Contributor

fbricon commented Jun 20, 2023

Tests fail:

Test testConfigPropertyNameResolveExpression FAILED (14.7s)

  java.lang.AssertionError: expected:<MarkupContent [
    kind = "markdown"
    value = "`greeting.message = salutations` *in* [META-INF/microprofile-config.properties](file:///tmp/unitTest_configPropertyNameResolveExpression_2RTchAIsIGwgDevZZaFmFJk7Ek9/testConfigPropertyNameResolveExpression_2RTch4wMbMG5NIFipP377WAhq0t/config-hover23/src/main/resources/META-INF/microprofile-config.properties)"
  ]> but was:<MarkupContent [
    kind = "markdown"
    value = "`greeting.message = hello` *in* [META-INF/microprofile-config.properties](file:///tmp/unitTest_configPropertyNameResolveExpression_2RTchAIsIGwgDevZZaFmFJk7Ek9/testConfigPropertyNameResolveExpression_2RTch4wMbMG5NIFipP377WAhq0t/config-hover23/src/main/resources/META-INF/microprofile-config.properties)"
  ]>
      at com.redhat.devtools.intellij.lsp4mp4ij.psi.core.config.java.MicroProfileConfigJavaHoverTest.testConfigPropertyNameResolveExpression(MicroProfileConfigJavaHoverTest.java:269)

and

Test testUrlCodeLensPropertiesWithPropertyExpression FAILED (19s)

  org.junit.ComparisonFailure: expected:<...//restcountries.url/[${asdf}]/v2/name/{name}> but was:<...//restcountries.url/[rest]/v2/name/{name}>
      at com.redhat.devtools.intellij.lsp4mp4ij.psi.core.restclient.java.MicroProfileRestClientJavaCodeLensTest.assertCodeLenses(MicroProfileRestClientJavaCodeLensTest.java:244)

@angelozerr angelozerr force-pushed the aggregate-events branch 2 times, most recently from 6e350a7 to 84b7035 Compare June 21, 2023 09:21
fbricon
fbricon reviewed Jun 21, 2023
View reviewed changes
...in/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangedListener.java Outdated
public CompletableFuture<Void> processModules() {
return CompletableFuture.runAsync(() -> {
for (var module : ModuleManager.getInstance(manager.getProject()).getModules()) {
LOGGER.info("Calling ensure from processModules");
Copy link
Contributor

Choose a reason for hiding this comment

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

calling checkQuarkusLibrary rather?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must not use Quarkus.

fbricon
fbricon reviewed Jun 21, 2023
View reviewed changes
...in/java/com/redhat/devtools/intellij/quarkus/classpath/ClasspathResourceChangedListener.java Outdated

private final ClasspathResourceChangedManager manager;

private final Set<Module> modulesBeingEnsured = new HashSet<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

quarkusModules or checkedQuarkusModules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must not use Quarkus as it is MicroProfile.

@angelozerr angelozerr force-pushed the aggregate-events branch 15 times, most recently from ff3d875 to 7ac7f4e Compare June 22, 2023 15:40
@sonarcloud
Copy link

sonarcloud bot commented Jun 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@angelozerr
Copy link
Contributor Author

@fbricon CI build is working again!

@angelozerr angelozerr requested a review from fbricon June 23, 2023 05:58
@angelozerr angelozerr merged commit 3e86087 into redhat-developer:main Jun 23, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon Awaiting requested review from fbricon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1000s of LSP events in Qute console, after reopening a project Debounce classpath/source change events
2 participants
@angelozerr @fbricon