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

Semantic Tokens ext point #1683

Merged
merged 15 commits into from
Dec 9, 2024
Merged

Semantic Tokens ext point #1683

merged 15 commits into from
Dec 9, 2024

Conversation

BoykoAlex
Copy link
Contributor

Fixes #1594

Extension point to contribute semantic tokens into the JDT Java editor.

@BoykoAlex BoykoAlex mentioned this pull request Sep 28, 2024
Closed
@BoykoAlex BoykoAlex changed the title Sync jdt syntax highlighting Semantic Tokens ext point Sep 28, 2024
@BoykoAlex BoykoAlex force-pushed the sync-jdt-syntax-highlighting branch from 3e87ca0 to 9332408 Compare October 1, 2024 15:23
@martinlippert
Copy link

martinlippert commented Oct 14, 2024 •
edited
Loading

After the initial discussion in #1594, this is the corresponding PR with the implementation. Would be extremely awesome if someone could have a look and provide feedback, so that we can get this merged whenever it makes sense.

Interested @noopur2507, @iloveeclipse, @HannesWell ?

@HannesWell
Copy link
Contributor

After the initial discussion in #1594, this is the corresponding PR with the implementation. Would be extremely awesome if someone could have a look and provide feedback, so that we can get this merged whenever it makes sense.

Interested @noopur2507, @iloveeclipse, @HannesWell ?

Unfortunately I'm not a JDT committer and cannot say with certainty who would be the best person to review this.
Besides the other one you mentioned, maybe @mickaelistria can have a look since he also works on LS integration?

@mickaelistria
Copy link
Contributor

I'm not a committer on JDT either ( https://projects.eclipse.org/projects/eclipse.jdt/elections/election-mickael-istria-committer-eclipse-jdt-java-development-tools ). I've put some generic comments about documentation and API that needs to be polished.

@BoykoAlex
Copy link
Contributor Author

@mickaelistria @martinlippert updated the PR for Mickael's comments

@martinlippert
Copy link

@BoykoAlex Thanks for incorporating the comments from @mickaelistria (and thanks a lot @mickaelistria for taking a look, much appreciated).

Now looking for a JDT UI committer to pick this up and provide feedback, merge, or whatever makes sense to move this forward.

jukzi
jukzi reviewed Oct 28, 2024
View reviewed changes
...eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/SemanticHighlightingReconciler.java Outdated

/**
* XXX Hack for performance reasons (should loop over fJobSemanticHighlightings can call consumes(*))
* @since 3.5
*/
private Highlighting fJobDeprecatedMemberHighlighting;

private Highlighting[] fEditorHighlightings;
Copy link
Contributor

Choose a reason for hiding this comment

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

does this duplicate a hack? better find a solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think this code needs to move up and be collocated with with the similar definition for semantic highlightings. There is one field for whatever came in from the viewer definition and one for whatever the current reconciling job can use. To be honest this comment relates more to fJobSemanticHighlightings rather than fJobDeprecatedMemberHighlighting... Reconciling job may have highlightings different from the reconciler if colour settings have changed while reconciling text chunk in the editor, hence I think this is okay.

@jukzi
Copy link
Contributor

jukzi commented Oct 28, 2024

This change is rather complex to review, fails the automated tests and does not come with junit testcases.
At best the added API is tested with junit and used within jdt.ui itself or comes with a examples, so it will be actually tested by users. Please also add description how to systematically test including screenshots what to expect. If you have fixed all issues you may ping me for review. The merge may have to wait until an M1.

@BoykoAlex BoykoAlex force-pushed the sync-jdt-syntax-highlighting branch from 43ebedf to 336cf12 Compare October 28, 2024 20:44
@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Oct 29, 2024 •
edited
Loading

@jukzi thank you for the feedback. I have added an example extnesion point in the .text.tests plugin and a simple unit test. The build fails on "plugins not loaded" test sometimes... I don't understand why it fails sometimes... I don't see what I could hav e broken there yet...

@jukzi
Copy link
Contributor

jukzi commented Oct 30, 2024

If you are are sure PluginsNotLoadedTest is unrelated (likely) then please create an issue for it and link it. i have not found an existing issue for it.

@iloveeclipse
Copy link
Member

If you are are sure PluginsNotLoadedTest is unrelated

I believe it is related, the test is not known as unstable.

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Oct 30, 2024 •
edited
Loading

@iloveeclipse I don't think the test failure is related as other PRs such as:

All have the same test failure. Likely something has changed in the platform upstream.
I'll open a new issue for the test failure.

@BoykoAlex
Copy link
Contributor Author

BoykoAlex commented Oct 30, 2024 •
edited
Loading

If you are are sure PluginsNotLoadedTest is unrelated (likely) then please create an issue for it and link it. i have not found an existing issue for it.

@jukzi I have created #1754 for the failed test

@BoykoAlex
Copy link
Contributor Author

@jukzi the PR should be okay to review now. Could you please have another look at it when you get a chance? :-)

@BoykoAlex BoykoAlex requested a review from jukzi December 5, 2024 15:19
@BoykoAlex
Copy link
Contributor Author

The test failures seem unrelated. I noticed the same number of failed tests in the recently created PRs...

@jukzi jukzi force-pushed the sync-jdt-syntax-highlighting branch from a148cd0 to 6617a4f Compare December 6, 2024 07:09
@jukzi
Copy link
Contributor

jukzi commented Dec 6, 2024

rebased to get rid of build errors

@BoykoAlex BoykoAlex requested a review from jukzi December 6, 2024 15:53
@jukzi jukzi force-pushed the sync-jdt-syntax-highlighting branch from 5db63af to c544c78 Compare December 9, 2024 06:54
@jukzi jukzi dismissed their stale review December 9, 2024 06:57

review applied

@jukzi
Copy link
Contributor

jukzi commented Dec 9, 2024

unrelated SmokeViewsTest.testOpenSourceView can be ignored #1843

@jukzi
Copy link
Contributor

jukzi commented Dec 9, 2024

let's give it a try

@jukzi jukzi merged commit d0563dc into eclipse-jdt:master Dec 9, 2024
7 of 10 checks passed
@martinlippert
Copy link

So many thanks to @jukzi for reviewing and guiding us through this PR, so much appreciated. 1000 Thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickaelistria mickaelistria mickaelistria requested changes

@jukzi jukzi Awaiting requested review from jukzi

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

Successfully merging this pull request may close these issues.

Java editor syntax highlighting via extension point
6 participants
@BoykoAlex @martinlippert @HannesWell @mickaelistria @jukzi @iloveeclipse