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

feat: update lsp4mp to 0.7.1 #895

Merged
merged 1 commit into from
Jun 6, 2023
Merged

feat: update lsp4mp to 0.7.1 #895

merged 1 commit into from
Jun 6, 2023

Conversation

fbricon
Copy link
Contributor

@fbricon fbricon commented May 26, 2023 •
edited
Loading

The PropertiesManagerForJava.javaCursorContext(...) implementation is much simpler than the JDT one, that's where you need to pay extra attention.

I haven't looked at unit tests yet.

Currently includes #894 already merged in main

Part of #806

@angelozerr
Copy link
Contributor

Wow it looks very cool We!

We have a test in JDT with java cusrsor context please add it to check your implémentation is good.

@angelozerr angelozerr mentioned this pull request May 30, 2023
Closed
turkeylurkey
turkeylurkey reviewed May 30, 2023
View reviewed changes
src/main/java/com/redhat/devtools/intellij/lsp4mp4ij/psi/core/PropertiesManagerForJava.java
*/
public JavaCursorContextResult javaCursorContext(MicroProfileJavaCompletionParams params, IPsiUtils utils) {
String uri = params.getUri();
PsiFile typeRoot = resolveTypeRoot(uri, utils);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need "ApplicationManager.getApplication().runReadAction()" to avoid exceptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you exlain why we could have some exceptions please.

Copy link
Contributor

@turkeylurkey turkeylurkey May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm referring to exceptions like this:

java.lang.Throwable: Read access is allowed from inside read-action (or EDT) only (see com.intellij.openapi.application.Application.runReadAction())
Current thread: Thread[JobScheduler FJ pool 0/7,4,main] 869433486 (EventQueue.isDispatchThread()=false)
SystemEventQueueThread: Thread[AWT-EventQueue-0,6,main] 1514231839
	at com.intellij.openapi.diagnostic.Logger.error(Logger.java:202)
	at com.intellij.openapi.application.impl.ApplicationImpl.assertReadAccessAllowed(ApplicationImpl.java:1004)
	at com.intellij.workspaceModel.core.fileIndex.impl.WorkspaceFileIndexDataImpl.ensureIsUpToDate(WorkspaceFileIndexDataImpl.kt:127)
	at com.intellij.workspaceModel.core.fileIndex.impl.WorkspaceFileIndexDataImpl.getFileInfo(WorkspaceFileIndexDataImpl.kt:70)
	at com.intellij.workspaceModel.core.fileIndex.impl.WorkspaceFileIndexImpl.getFileInfo(WorkspaceFileIndexImpl.kt:220)
	at com.intellij.workspaceModel.core.fileIndex.impl.WorkspaceFileIndexImpl.findFileSetWithCustomData(WorkspaceFileIndexImpl.kt:205)
	at com.intellij.openapi.roots.impl.ProjectFileIndexImpl.getModuleForFile(ProjectFileIndexImpl.java:133)
	at io.openliberty.tools.intellij.lsp4mp4ij.psi.internal.core.ls.PsiUtilsLSImpl.getModule(PsiUtilsLSImpl.java:79)
	at io.openliberty.tools.intellij.lsp4mp4ij.psi.internal.core.ls.PsiUtilsLSImpl.resolveCompilationUnit(PsiUtilsLSImpl.java:177)
	at io.openliberty.tools.intellij.lsp4jakarta.lsp4ij.PropertiesManagerForJakarta.resolveTypeRoot(PropertiesManagerForJakarta.java:293)
	at io.openliberty.tools.intellij.lsp4jakarta.lsp4ij.PropertiesManagerForJakarta.javaCursorContext(PropertiesManagerForJakarta.java:208)
...

turkeylurkey
turkeylurkey reviewed May 31, 2023
View reviewed changes
src/main/java/com/redhat/devtools/intellij/lsp4mp4ij/psi/core/PropertiesManagerForJava.java
PsiField psiField = (PsiField) parent;
@Nullable PsiTypeElement fieldType = psiField.getTypeElement();
if (fieldType != null && completionOffset <= fieldType.getTextOffset()) {
return JavaCursorContextKind.BEFORE_FIELD;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this will work according to the definition in JavaCursorContextKind line 87. Before_Field means the cursor is in whitespace before a field. E.g. int a; | int b; In this code the parent of the whitespace is the containing class rather than a field (b in this example). The same concept applies to methods also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the whole JavaCursor thing is mostly wrong for now, I'm working on it. I used chatGPT to quickly generate the initial PSI-based implem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turkeylurkey can you check how the latest commit works for you?

@fbricon fbricon force-pushed the lsp4mp-071 branch 2 times, most recently from 0b6e94b to 6d71448 Compare May 31, 2023 15:00
@fbricon fbricon marked this pull request as ready for review May 31, 2023 15:17
@fbricon
Copy link
Contributor Author

fbricon commented May 31, 2023

@angelozerr I had to downgrade quarkus-qute-ls to from 0.14.1 down to 0.14.0, the following test fails:

testquteQuickStart
org.junit.ComparisonFailure: Expect org.acme.qute.ItemResource but was org.acme.qute.ItemResourceWithFragment for null:discountedPrice(item : org.acme.qute.Item) : java.math.BigDecimal expected:<...me.qute.ItemResource[]> but was:<...me.qute.ItemResource[WithFragment]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at com.redhat.devtools.intellij.qute.psi.template.TemplateGetDataModelProjectTest.assertValueResolver(TemplateGetDataModelProjectTest.java:380)
	at com.redhat.devtools.intellij.qute.psi.template.TemplateGetDataModelProjectTest.assertValueResolver(TemplateGetDataModelProjectTest.java:369)
	at com.redhat.devtools.intellij.qute.psi.template.TemplateGetDataModelProjectTest.assertValueResolver(TemplateGetDataModelProjectTest.java:364)
	at com.redhat.devtools.intellij.qute.psi.template.TemplateGetDataModelProjectTest.testValueResolversFromTemplateExtension(TemplateGetDataModelProjectTest.java:210)
	at com.redhat.devtools.intellij.qute.psi.template.TemplateGetDataModelProjectTest.testquteQuickStart(TemplateGetDataModelProjectTest.java:59)

@angelozerr
Copy link
Contributor

@fbricon the CI build on main fails https://github.com/redhat-developer/intellij-quarkus/actions/runs/5133444014/jobs/9236030715 I think it is the same error that you have.

@angelozerr
Copy link
Contributor

angelozerr commented May 31, 2023 •
edited
Loading

@fbricon I created a PR #903 which should fix the CI build which fails randomly

@fbricon fbricon force-pushed the lsp4mp-071 branch 5 times, most recently from 5602f58 to 271c65e Compare June 2, 2023 13:14
angelozerr
angelozerr reviewed Jun 2, 2023
View reviewed changes
...java/com/redhat/devtools/intellij/lsp4mp4ij/psi/core/snippets/JavaFileCursorContextTest.java Outdated
}

// @Test
// public void testOneWord() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it normal that thoses testes are commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are used when snippets are supported

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 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 angelozerr merged commit 7a28229 into redhat-developer:main Jun 6, 2023
@angelozerr
Copy link
Contributor

It works like a charm! Great job @fbricon !

@angelozerr angelozerr added enhancement New feature or request quarkus Quarkus support labels Jun 6, 2023
@angelozerr angelozerr added this to the 1.20.0 milestone Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turkeylurkey turkeylurkey turkeylurkey left review comments

@angelozerr angelozerr angelozerr left review comments

Assignees

@fbricon fbricon

Labels
enhancement New feature or request quarkus Quarkus support
Projects
IDE Cloudaptors
Archived in project
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fbricon @angelozerr @turkeylurkey