Improve occurrences highlighting #1941

0dinD · 2021-11-15T00:02:13Z

Fixes redhat-developer/vscode-java#2211
Fixes redhat-developer/vscode-java#2212
Fixes redhat-developer/vscode-java#2213
Fixes redhat-developer/vscode-java#972

This PR improves on the occurrence highlighting by porting the following occurrence finders from jdt.ui:

MethodExitsFinder

ExceptionOccurrencesFinder

BreakContinueTargetFinder

ImplementOccurrencesFinder

It also registers the document highlight capability in the syntax server.

Lastly, I fixed a bug which could cause the wrong DocumentHighlightKind to be set. The bug was caused by incorrect logic when checking the bit flags set on an OccurrenceLocation. If, for example, there were no flags set (0), the condition would still be true, which means DocumentHighlightKind.Write would be set even though it shouldn't be.

Initially I thought these occurrence finders were available from jdt.core.manipulation (seeing as OccurrencesFinder is), but it turns out they are located in jdt.ui. So I copied the needed classes from there, seeing as they have no external dependencies and as such could easily be ported.

With that said, I understand that copying from jdt.ui is not ideal, and I think these classes should be moved to jdt.core.manipulation. But right now I don't have time to spend on that, especially since I'll need to learn how Gerrit works and how to set up jdt.ui for development.

If anyone wants to help with that to avoid copying, please go ahead. But I also think it would be valuable for me to learn how to contribute to upstream (and this seems like a fairly simple contribution to start with), so another alternative is to wait for that (might be a few more weeks until I have time though). Of course, we could also merge this PR with the copied files and then remove them later once they've been moved to jdt.core.manipulation.

rgrunber

this . change . is . awesome .

I was happy to make the requests upstream since I would just ping a contact there to make them, but if you want to do them, I'm happy with that.

CC'ing @jjohnstn for awareness : @0dinD would like to contribute to JDT.UI by moving the copied classes mentioned here into jdt.core.manipulation. It should be a simple move (the dependent classes are already in place).

As for this change : I've tried it out and although it does work, but I see one main protocol issue :

~~Java types, that would normally display with document highlighting (a more visually noticeable highlight at least on vscode) now displays with a fainter hover highlighting.~~ <-- This is just what OccurencesFinder did. The other finders use a different kind resulting in a different highlight.

...ipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/DocumentHighlightHandler.java

rgrunber · 2022-01-28T15:45:52Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/search/MethodExitsFinder.java

+import org.eclipse.jdt.internal.corext.util.Messages;
+
+
+public class MethodExitsFinder extends ASTVisitor implements IOccurrencesFinder {


A simple move request for this class against JDT.UI should eliminate needing to copy.

rgrunber · 2022-01-28T15:48:00Z

...lipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/search/BreakContinueTargetFinder.java

+ *
+ * @since 3.2
+ */
+public class BreakContinueTargetFinder extends ASTVisitor implements IOccurrencesFinder {


I checked the imports and they all reside in jdt.core / jdt.core.manipulation, except of course this class :) But that means all we need to do is request JDT.UI move it accordingly.

~~I can do this, and we drastically reduce the patch size.~~
I'll let you handle this :)

rgrunber · 2022-01-28T15:49:51Z

...ipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/search/ExceptionOccurrencesFinder.java

+import org.eclipse.jdt.internal.corext.dom.Bindings;
+import org.eclipse.jdt.internal.corext.util.Messages;
+
+public class ExceptionOccurrencesFinder extends ASTVisitor implements IOccurrencesFinder {


A simple move request for this class against JDT.UI should eliminate needing to copy.

rgrunber · 2022-01-28T15:50:28Z

...ipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/search/ImplementOccurrencesFinder.java

+ *
+ * @since 3.1
+ */
+public class ImplementOccurrencesFinder implements IOccurrencesFinder {


A simple move request for this class against JDT.UI should eliminate needing to copy.

rgrunber · 2022-01-28T15:51:13Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/search/SearchMessages.java

+
+import org.eclipse.osgi.util.NLS;
+
+public final class SearchMessages extends NLS {


We can probably request this be moved as well along with the properties file.

0dinD · 2022-01-28T22:32:28Z

I was happy to make the requests upstream since I would just ping a contact there to make them, but if you want to do them, I'm happy with that.

I'm hoping to have time for this sometime next week. Any tips or pointers are appreciated, otherwise I'll just follow https://wiki.eclipse.org/JDT_UI/How_to_Contribute.

Java types, that would normally display with document highlighting (a more visually noticeable highlight at least on vscode) now displays with a fainter hover highlighting.

Not sure what exactly you mean (screenshot would help), but I'm guessing it's because I fixed the bug where DocumentHighlightKind.Write would be set on type names (which does give them a more noticeable highlight in the default theme). But it's not semantically correct (except maybe for type declarations), see for example the document highlight of types in TypeScript.

rgrunber · 2022-01-31T17:36:01Z

I'm hoping to have time for this sometime next week. Any tips or pointers are appreciated, otherwise I'll just follow https://wiki.eclipse.org/JDT_UI/How_to_Contribute.

@0dinD In particular https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Contributing_a_change, since the git:// protocol is/was retired. For the Change-Id header in the commit, you can follow https://git.eclipse.org/r/Documentation/user-changeid.html#creation . The Eclipse URL for the script is https://git.eclipse.org/r/tools/hooks/commit-msg . It is used to identify your change set when you make a 2nd, 3rd, etc. update.

Make sure to CC myself and @jjohnstn on the Gerrit pull request. You'll see your change on https://git.eclipse.org/r/q/project:jdt%252Feclipse.jdt.ui+status:open .

rgrunber · 2022-02-03T16:59:40Z

Not sure what exactly you mean (screenshot would help), but I'm guessing it's because I fixed the bug where DocumentHighlightKind.Write would be set on type names (which does give them a more noticeable highlight in the default theme). But it's not semantically correct (except maybe for type declarations), see for example the document highlight of types in TypeScript.

Right again. Looks like a combination of the LSP only supports Read, Write, Text for the DocumentHighlightKind and the fact that the JDT API supports more. In IOccurrencesFinder , the finder will return some of those flags (eg. F_EXCEPTION_DECLARATION), but we only look for Read, or Write and otherwise it's all treated as Text. I guess its up to the LSP to come up with some more descriptive token kinds.

Update : I though some of those K_* values get returned in the flags, but they don't, only F_* values, so we really are doing the right thing.

0dinD · 2022-02-04T14:09:54Z

but we only look for Read, or Write and otherwise it's all treated as Text.

Oh, but now that you say that I think the default is supposed to be Read for symbols, Text is for fuzzy/text matches (which none of the finders do). See https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_documentHighlight:

Symbol matches usually have a DocumentHighlightKind of Read or Write whereas fuzzy or textual matches use Text as the kind.

We do explicitly need to set it, since the default kind (if not specified) is Text:

export interface DocumentHighlight {

    // ...

    /**
     * The highlight kind, default is DocumentHighlightKind.Text.
     */
    kind?: DocumentHighlightKind;

}

See also implementation in other language servers:

I've made the change to set DocumentHighlightKind.Read instead of null in a71de36.

rgrunber · 2022-02-07T17:45:51Z

Looks good on this end. In terms of contributing to JDT, the only real obstacle now is the 2022-03 M3 deadline. I've spoken to @akurtakov and although it is Feb 18th, it would be ideal to get it in by the end of this week to ensure no issues.

0dinD · 2022-02-07T20:37:15Z

Yeah I didn't have much time last week, but I should have time to submit a Gerrit PR during this week. Thanks for the help!

0dinD · 2022-02-11T13:04:40Z

I've submitted a Gerrit PR now: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/190713

rgrunber · 2022-02-11T16:45:36Z

Merged. Once an I-build from Feb 11 (or later) is available, it should be possible to remove the copied code and simply update the target platform to use that. We can keep an eye on https://download.eclipse.org/eclipse/downloads/ for when that might happen.

You could also technically play with the change before an I-build is available (and this is often useful for confirming something will work prior to proposing it) by doing a mvn clean install -DskipTests -f org.eclipse.jdt.core.manipulation/pom.xml from eclipse.jdt.ui . This will install the artifacts locally, so when you build eclipse.jdt.ls, you'll want to look for something in the logs like :

[WARNING] The following locally built units have been used to resolve project dependencies:
[WARNING]   org.eclipse.jdt.core.manipulation/1.15.200.v20220211-1633

Just don't forget to remove the artifacts from the local repository or else you'd always be using them.

0dinD · 2022-02-11T17:12:07Z

You could also technically play with the change before an I-build is available

Thanks, that's really good to know!

rgrunber · 2022-02-16T21:37:43Z

In fact, now you won't need to update the target platform. Once #2009 is merged (which fixes another issue), you should just be able to rebase on top of it, remove the copied code, and use the new classes available in jdt.core.manipulation.

Signed-off-by: Odin Dahlström <zerodind@gmail.com>

0dinD · 2022-02-17T23:08:22Z

test this please

0dinD · 2022-02-17T23:20:09Z

I've removed the copied classes now, so should be ready to merge if it looks good to you. There was a random test failure, but I see it's already been reported as #1271.

rgrunber · 2022-02-21T05:14:49Z

Thanks for taking the time to get this in.

rgrunber self-requested a review January 28, 2022 15:17

rgrunber reviewed Jan 28, 2022

View reviewed changes

rgrunber added this to the End February 2022 milestone Jan 28, 2022

rgrunber added enhancement semantic highlighting labels Jan 28, 2022

0dinD force-pushed the improve-occurrences-highlighting branch from bd6268a to a71de36 Compare February 4, 2022 14:43

Improve occurrences highlighting

1c2cc40

Signed-off-by: Odin Dahlström <zerodind@gmail.com>

0dinD force-pushed the improve-occurrences-highlighting branch from a71de36 to 1c2cc40 Compare February 17, 2022 22:40

rgrunber approved these changes Feb 21, 2022

View reviewed changes

rgrunber merged commit 60d01c7 into eclipse-jdtls:master Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve occurrences highlighting #1941

Improve occurrences highlighting #1941

0dinD commented Nov 15, 2021 •

edited

rgrunber left a comment •

edited

rgrunber Jan 28, 2022

rgrunber Jan 28, 2022 •

edited

rgrunber Jan 28, 2022

rgrunber Jan 28, 2022

rgrunber Jan 28, 2022

0dinD commented Jan 28, 2022 •

edited

rgrunber commented Jan 31, 2022

rgrunber commented Feb 3, 2022 •

edited

0dinD commented Feb 4, 2022 •

edited

rgrunber commented Feb 7, 2022

0dinD commented Feb 7, 2022

0dinD commented Feb 11, 2022

rgrunber commented Feb 11, 2022 •

edited

0dinD commented Feb 11, 2022

rgrunber commented Feb 16, 2022

0dinD commented Feb 17, 2022

0dinD commented Feb 17, 2022 •

edited

rgrunber commented Feb 21, 2022

		import org.eclipse.jdt.internal.corext.util.Messages;


		public class MethodExitsFinder extends ASTVisitor implements IOccurrencesFinder {


		import org.eclipse.osgi.util.NLS;

		public final class SearchMessages extends NLS {

Improve occurrences highlighting #1941

Improve occurrences highlighting #1941

Conversation

0dinD commented Nov 15, 2021 • edited

rgrunber left a comment • edited

Choose a reason for hiding this comment

rgrunber Jan 28, 2022

Choose a reason for hiding this comment

rgrunber Jan 28, 2022 • edited

Choose a reason for hiding this comment

rgrunber Jan 28, 2022

Choose a reason for hiding this comment

rgrunber Jan 28, 2022

Choose a reason for hiding this comment

rgrunber Jan 28, 2022

Choose a reason for hiding this comment

0dinD commented Jan 28, 2022 • edited

rgrunber commented Jan 31, 2022

rgrunber commented Feb 3, 2022 • edited

0dinD commented Feb 4, 2022 • edited

rgrunber commented Feb 7, 2022

0dinD commented Feb 7, 2022

0dinD commented Feb 11, 2022

rgrunber commented Feb 11, 2022 • edited

0dinD commented Feb 11, 2022

rgrunber commented Feb 16, 2022

0dinD commented Feb 17, 2022

0dinD commented Feb 17, 2022 • edited

rgrunber commented Feb 21, 2022

0dinD commented Nov 15, 2021 •

edited

rgrunber left a comment •

edited

rgrunber Jan 28, 2022 •

edited

0dinD commented Jan 28, 2022 •

edited

rgrunber commented Feb 3, 2022 •

edited

0dinD commented Feb 4, 2022 •

edited

rgrunber commented Feb 11, 2022 •

edited

0dinD commented Feb 17, 2022 •

edited