Support to redefine icons for LSP client #3459

sdedic · 2022-01-15T11:39:23Z

I reworked the stuff originally in #3414 to a state hopefully acceptable for NB13 before freeze/branching. Overview

Openide changes

ImageUtilities just formalize PROPERTY_URL in API + implementation bugfixes

LSP / VSCode changes

UIDefaults are patched at startup, with ImageUtilities.loadImages converted to icons for keys that are actually present. Replacement table is in a resource file. Stub image is replicated at build time, and mapped to images not mapped explicitly (just listed).
regexps in vscode client adjusted.

eirikbakke

Thanks for splitting out this digestible PR; looks good. I read over everything, though assuming that the LSP stuff was reviewed before by someone more familiar with that part of the codebase.

See in-code comments... main changes requested was fixing a double equals in a properties file and adding a comment to UIDefaultsIconMetadata to make it clear that it doesn't run during normal IDE execution.

...integration/src/org/netbeans/modules/nbcode/integration/resources/uimanager-icons.properties

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/explorer/TreeItem.java

...integration/src/org/netbeans/modules/nbcode/integration/resources/uimanager-icons.properties

...r/nbcode/integration/src/org/netbeans/modules/nbcode/integration/UIDefaultsIconMetadata.java

eirikbakke

Thanks for the updates!

neilcsmith-net

Can you double check the static initialization logic in ImageUtilities. Then please squash and merge when ready. Will be branching for NB13 later today.

platform/openide.util.ui/src/org/openide/util/ImageUtilities.java

eirikbakke · 2022-01-17T13:31:23Z

platform/openide.util.ui/src/org/openide/util/ImageUtilities.java


    static {
        /* Could have used Mutex.EVENT.writeAccess here, but it doesn't seem to be available during
        testing. */
        if (EventQueue.isDispatchThread()) {
-            dummyIconComponent = new JLabel();
+            dummyIconComponentLabel = new JLabel();
+            dummyIconComponentButton = new JCheckBox();


If we're doing a JCheckBox here instead of a JButton, the variable should be called dummyIconComponentCheckBox.

But remind me why we are doing a JCheckBox? I know it extends JButton but unless you know of specific cases that need it, I'd prefer to just have a JButton. Otherwise the hacks start getting too specific, and we'll be afraid to change them in the future.

During development of the UIDefaults replacing code, I used ImageUtilities on real icons served by UIManager (default LaF). Several of them had special implementations that downcast the Component to JCheckbox in their paint(). Having a JCheckbox satisfies more icons than just JButton (there are such icons as well) I tried to choose the class that covered most of the errors.

OK, as long as you saw actual cases, that's fine. What do you mean by "using ImageUtilities"; which method(s) specifically? And which LAF was this? (Which OS?) Why haven't these cases come up before?

which method(s) specifically

ImageUtilities.icon2Image; that ultimately calls paintIcon() to get the icon rendered to a BufferedImage. OS Linux / KDE, JDK 8+11, default L&F.

Why haven't these cases come up before?

Nobody called icon2Image on those particular UIManager icons ?

I think the reasoning is captured fine in the comment above too.

Yeah, it's fine with me. Just wondering if these were problems caused due to aggressive manual testing only, or whether they actually occur during real use.

eirikbakke · 2022-01-17T13:34:57Z

platform/openide.util.ui/src/org/openide/util/ImageUtilities.java

+     */
+    public static URL findImageBaseURL(Image image) {
+      Object o = image.getProperty(PROPERTY_URL, null);
+      return o instanceof URL ? (URL)o : null;


Space after ")", if you're doing another round of changes...

neilcsmith-net

OK, let's get it in before branching.

neilcsmith-net · 2022-01-17T15:34:58Z

@sdedic actually, hold on! Travis CI VSCode test is broken. Lots of logged java.lang.NoClassDefFoundError: Could not initialize class org.openide.util.ImageUtilities.

java.lang.NullPointerException: The image parameter cannot be null
    [junit] 	at org.openide.util.Parameters.notNull(Parameters.java:64)
    [junit] 	at org.openide.util.ImageUtilities.assignToolTipToImageInternal(ImageUtilities.java:397)
    [junit] 	at org.openide.util.ImageUtilities.image2Icon(ImageUtilities.java:328)
    [junit] 	at org.netbeans.modules.nbcode.integration.UIDefaultsIconMetadata$1.createValue(UIDefaultsIconMetadata.java:94)
    [junit] 	at javax.swing.UIDefaults.getFromHashtable(UIDefaults.java:216)
    [junit] 	at javax.swing.UIDefaults.get(UIDefaults.java:161)
    ...
    [junit] 	at javax.swing.JCheckBox.<init>(JCheckBox.java:174)
    ...
    [junit] Caused: java.lang.ExceptionInInitializerError
    [junit] 	at org.netbeans.modules.nbcode.integration.UIDefaultsIconMetadata$1.createValue(UIDefaultsIconMetadata.java:94)

Coincidence that that's with JCheckbox, or because of previous fix?

eirikbakke · 2022-01-17T15:48:04Z

@neilcsmith-net ImageUtilities.image2Icon(ImageUtilities.loadImage(resImage)) is suspect in any case, since loadImage can return null and image2Icon does not accept null.

neilcsmith-net · 2022-01-17T17:05:30Z

@sdedic I need to proceed with branching. Once available, please rebase this on delivery.

sdedic · 2022-01-17T17:08:41Z

Got it, running tests. Will rebase.

sdedic · 2022-01-17T17:17:57Z

Seems that when the 1st call to ImageUtilities happened in the EDT (seemed not the case when running as a headless NBLS server or as an IDE, but did in the test), UIDefaultsIconMetadata attempted to call back to stil-not-initialized class ImageUtilities that resulted in CDNFE; at least when running locally.

neilcsmith-net · 2022-01-17T18:44:04Z

Thanks! See, I knew that would cause a bug somewhere, or that fixing it would. 😆

Bit of a backlog in tests with all the branching and merging today. Please merge when it goes green.

sdedic requested review from entlicher, neilcsmith-net, JaroslavTulach and eirikbakke and removed request for entlicher January 15, 2022 11:39

sdedic force-pushed the vscode/redefined-icons3 branch from ea07e73 to ae3bade Compare January 15, 2022 11:42

sdedic changed the title ~~Vscode/redefined icons3~~ Support to redefine icons for LSP client Jan 15, 2022

sdedic mentioned this pull request Jan 15, 2022

Support for image IDs, LSP client can redefine images/icons #3414

Closed

sdedic force-pushed the vscode/redefined-icons3 branch from ae3bade to cfddb16 Compare January 15, 2022 12:10

eirikbakke requested changes Jan 15, 2022

View reviewed changes

eirikbakke approved these changes Jan 16, 2022

View reviewed changes

jtulach approved these changes Jan 16, 2022

View reviewed changes

sdedic self-assigned this Jan 16, 2022

sdedic added this to the NB13 milestone Jan 16, 2022

sdedic added enhancement LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests API Change [ci] enable extra API related tests Need Squashing labels Jan 16, 2022

neilcsmith-net requested changes Jan 17, 2022

View reviewed changes

platform/openide.util.ui/src/org/openide/util/ImageUtilities.java Outdated Show resolved Hide resolved

sdedic force-pushed the vscode/redefined-icons3 branch from a49f9be to 5791131 Compare January 17, 2022 12:46

sdedic removed the Need Squashing label Jan 17, 2022

sdedic requested a review from neilcsmith-net January 17, 2022 12:47

eirikbakke reviewed Jan 17, 2022

View reviewed changes

neilcsmith-net approved these changes Jan 17, 2022

View reviewed changes

neilcsmith-net self-requested a review January 17, 2022 15:39

sdedic added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 17, 2022

sdedic added 6 commits January 17, 2022 18:12

Formalize ImageUtilities URL property.

f40062a

Replace UIDefaults icons with metadata ones.

c860c94

Explicitly mark physical files the client can see.

d1ccb36

Send image URI on the protocol.

bf0aecf

Allow to match and replace icons based on icon+node data.

b60a3e0

Licenses fixed.

2fbce60

sdedic force-pushed the vscode/redefined-icons3 branch from 5791131 to 2fbce60 Compare January 17, 2022 17:13

sdedic changed the base branch from master to delivery January 17, 2022 17:13

neilcsmith-net approved these changes Jan 17, 2022

View reviewed changes

neilcsmith-net merged commit 2b91be1 into apache:delivery Jan 18, 2022

neilcsmith-net removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to redefine icons for LSP client #3459

Support to redefine icons for LSP client #3459

sdedic commented Jan 15, 2022 •

edited

eirikbakke left a comment

eirikbakke left a comment

neilcsmith-net left a comment

eirikbakke Jan 17, 2022 •

edited

sdedic Jan 17, 2022 •

edited

eirikbakke Jan 17, 2022

sdedic Jan 17, 2022 •

edited

neilcsmith-net Jan 17, 2022

eirikbakke Jan 17, 2022

eirikbakke Jan 17, 2022

neilcsmith-net left a comment

neilcsmith-net commented Jan 17, 2022 •

edited

eirikbakke commented Jan 17, 2022

neilcsmith-net commented Jan 17, 2022

sdedic commented Jan 17, 2022

sdedic commented Jan 17, 2022

neilcsmith-net commented Jan 17, 2022

Support to redefine icons for LSP client #3459

Support to redefine icons for LSP client #3459

Conversation

sdedic commented Jan 15, 2022 • edited

Openide changes

LSP / VSCode changes

eirikbakke left a comment

Choose a reason for hiding this comment

eirikbakke left a comment

Choose a reason for hiding this comment

neilcsmith-net left a comment

Choose a reason for hiding this comment

eirikbakke Jan 17, 2022 • edited

Choose a reason for hiding this comment

sdedic Jan 17, 2022 • edited

Choose a reason for hiding this comment

eirikbakke Jan 17, 2022

Choose a reason for hiding this comment

sdedic Jan 17, 2022 • edited

Choose a reason for hiding this comment

neilcsmith-net Jan 17, 2022

Choose a reason for hiding this comment

eirikbakke Jan 17, 2022

Choose a reason for hiding this comment

eirikbakke Jan 17, 2022

Choose a reason for hiding this comment

neilcsmith-net left a comment

Choose a reason for hiding this comment

neilcsmith-net commented Jan 17, 2022 • edited

eirikbakke commented Jan 17, 2022

neilcsmith-net commented Jan 17, 2022

sdedic commented Jan 17, 2022

sdedic commented Jan 17, 2022

neilcsmith-net commented Jan 17, 2022

sdedic commented Jan 15, 2022 •

edited

eirikbakke Jan 17, 2022 •

edited

sdedic Jan 17, 2022 •

edited

sdedic Jan 17, 2022 •

edited

neilcsmith-net commented Jan 17, 2022 •

edited