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

sdedic · 2022-01-05T14:56:50Z

This PR allows LSP clients to "rebrand" or alter icons supplied byt he NBLS server for exported nodes. It's not exactly branding, since the LSP client may have its own ideas or icon schemes that do not fit to the NB Node implementations - which shall not be affected at all. The vscode client, in addition, uses fontified icons (bcs they scale), which more or less prevent branding - except maybe if I'd extract the font glyphs (possible license issues) and converted them to SVG branding resources.

Instead, the URL of the image's resource will be extracted from Node's icon and transmitted. As it acts like an unique ID (not exactly, see notes below), the client may replace it with its own icon. Icons that are the same on NB side (NB design) may be matched and replaced based on contextValue or name exported node's metadata (which is actually more powerful than just branding). Branding can be still used, if appropriate.

Note that certain prominent icons do not come from URL resources, but from UIManager.getDefaults() - the exact shape is defined by LaF. So these icons either do not have URLs at all, or their URLs differ between LaFs, which is unfortunate if the LaF-specific URL was used an ID.

The 1st commit in this PR changes openide ImageUtilities, it's probably the most important one from this PR.

Images and icons produced by ImageUtilities maintain not only url (see Image.getProperty API) but also uri. As url property must be typed as URL, and non-location IDs cannot be represented as URLs (no URLStreamHandler registered, and the underlying resource bits are really not known) - another property is invented. Since the URI syntax is more flexible, even badges or filters can be represented as URIs (see at the end) in the future.
During implementation it turned out that if ToolkitImage is wrapped in a delegating Image instance weird things happen: the SurfaceManager is not initialized properly so an attempt to graphics.drawImage will fail with IllegalArgumentException. So super-hacky originalImage or originalIcon backdoor is invented instead - so the anticipated delegating implementation may pass the surface-compatible real image as appropriate.

This part of impl is not finished: IMHO some convenience accessor methods for the URL (already was there) and URI should be added to ImageUtilities, the URI of regular resource-based icons should not be affected by branding or localization (it is now) and eventual badges should be somehow accessible as URI[] and also the possibly applied filter od disabled icon filter...
That's why I did not (yet) documented the added properties in arch.xml. I plan to do so after the implementation proves worthy hopefuly before NB13 finalizes.

The UIDefaultsIconMetadata is a NBLS startup hack, that replaces UIDefault images for delegates that provide the metadata (URL, URI). These are then preserved during ImageUtilities.mergeImages so a combined image inherits the base ID from the base image.

Finally the URI is transmitted to the LSP client in a ImageDescriptor structure that will grow in the future (badge and filter URIs).

The vscode client reads icon replacement definitions from package.json of all vscode extensions. It matches the transmitted image based on base URI and possibly contextValue items and also name as it is sometimes used as programmatic ID (in nodes that represent containers). If a match is found, icon data is ignored and vscode's ThemeIcon or a local icon (provided by an extension) is used.
This way the vscode can alter icons to match its visual style and ensure consistency if an icon was defined already in the vscode world. Other LSP clients can do just the same.

Note that the tree LSP messages need some optimization. For example, the server should only transmit image URI/descriptor - and the client should call back for URL's contents (image data) if it wants to display the original image. This will be solved in a future PRs.

dbalek · 2022-01-06T10:09:21Z

java/java.lsp.server/vscode/package.json

+				"codeicon": "file-submodule"
+			},
+			{
+				"uriExpression": "nbres:/org/netbeans/modules/java/resources/class.png",


What about abstract_class_file.png, main-class.png, and enum_file.png ?

Added in eec8fe1

JaroslavTulach

I'd prefer to not introduce the "uri" Image key. Should there be one, please add a utility method to extract URI from an Image.

A bit of documentation and some tests (Java as well as TypeScript) would be ideal at the end.

JaroslavTulach · 2022-01-06T16:33:36Z

java/java.lsp.server/vscode/package.json

+		},
+
+		"netbeans.iconMapping" : [
+			{


APIs, APIs...

JaroslavTulach · 2022-01-06T16:35:03Z

java/java.lsp.server/vscode/schemas/package.schema.json

+            "contributes": {
+                    "type": "object",
+                    "properties": {
+                            "netbeans.iconMapping" : {


Let's consider this the description of the API. Please state the stability category of the API at least in this PR.

What API category should that be ? Layer ;) ?

package.json!?

No need to add it into arch.xml - this is really VSNetBeans specific. However other extension writers should know how much they can rely on this. I'd also like to know whether we want to support this iconMapping forever or rewrite it the next version?

I would mark it as a highly experimental feature at this moment.

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

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/explorer/api/TreeItemData.java

...ava.lsp.server/src/org/netbeans/modules/java/lsp/server/explorer/DefaultDecorationsImpl.java

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

JaroslavTulach · 2022-01-06T17:05:54Z

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

+    public static final String PROP_ORIGINAL_ICON = "originalIcon"; // NOI18N
+    public static final String PROP_ORIGINAL_IMAGE = "originalImage"; // NOI18N
+
+    private static final String URN_DEFAULTS_PREFIX = "urn:uidefaults:"; // NOI18N


Is the whole introduction of "uri" because you want to use this URN_DEFAULTS_PREFIX!? I understand that when starting from scratch the ability to have real URI with its "magical strings" might be beneficial, but we are not starting from scratch. We have "url" on images - it'd be more hidden if we could reuse it and just enhance it.

Resolved using @eirikbakke suggestion to use Strings. The client instanceof duality was resolved by helper functions.

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

sdedic · 2022-01-06T22:20:26Z

I'd prefer to not introduce the "uri" Image key. Should there be one, please add a utility method to extract URI from an Image.

Let's get a little philosophical (there are more hints to use URL, so let's sort that in one place). The goal is to provide an ID. And there are images that one cannot get the physical bits. Most importantly the images in the UIDefaults, but possibly other ones as well.
The distinction between URI and URL is that the URI is really an identifier ... which sometimes might be the location at the same time (URL). But some URIs are not (= URN). The most important functional distinction is that URL allows to locate the physical resource = access its bits. That's what URLStreamHandlers are for.

I don't have the bits for images from UIDefaults. They can be forged, by painting the Icon + encoding as PNG, for example - then the special URL with an associated URLStreamHandler instance could eventually work. But such URLs would not be usable anywhere - an attempt to pass them around will eventually lead to .toString() and back - which will fail, since the protocol would not be probably registered - and the referenced bits are ephemeral.

I can invent an additional protocol, 'uidefaults' and register a handler for it that would access the LaF (authority) icon/image (path). But still the resource's content would be computed. Allowing for URIs does not preclude that, but allows true identifiers for ephemeral objects. So IMHO there should have been URIs from the start - so that the image was not required to be a locatable, but just identifiable.

Does that explain the introduction - does it seem acceptable ? If there's strong disagreement, then my preferred best solution is that special protocol.

JaroslavTulach · 2022-01-07T07:10:54Z

First of all I'd like to point out I have already approved your PR. The changes are sane and acceptable from some architecture perspective. My following comments are just result of viewing them from a different angle...

Let's get a little philosophical...

Thanks for writing down your thoughts. Yes, I expected you had something like that in mind. You worked in Systinet where URL, URI, URN were essential. Your focus on the nuances between them is understandable.

I also know that mailto: isn't technically URL. But for me java.net.URL is just a class, I have no mental problem to encode mailto: into java.net.URL and later throw an org.openide.text.UserTypeSomeTextException.

I am more scared of duplicating the existing API by a (95% similar) concept, just to be pure and absolutely correct.

... goal is ... an ID ... images ... (without) ... the physical bits... URI is really an identifier ... URL allows to locate ... that's what URLStreamHandlers are for...invent an additional protocol, 'uidefaults'

From a point of Java programmer and maintainer (creator) of the previous (messy?) system, I'd call the URI vs. URL debate an unimportant difference and I'd just encode UIDefaults & co. images in java.net.URL and later throw an exception (or paint the icon into data:, if useful).

the protocol would not be probably registered - and the referenced bits are ephemeral.

Yes, there are problems passing Java URLs into OS or browser. Each has its own set of handlers and the intersection is small (http, file, ftp?).

So IMHO there should have been URIs from the start

Indeed!

However there was no java.net.URI when NetBeans started. The java.net package contained just java.net.URL and other APIs like Class.getResource used it to refer to other objects than just those on World Wide Web.

If we started from scratch right now, I'd support your idea to use java.net.URI. However we have java.net.URL and I'd rather fit into existing design than complicate the conceptual area (from a Java programmer perspective) just because of little (philosophical) differences between URL and URI.

Does that explain the introduction - does it seem acceptable ?

Yes, your position is acceptable. It just leads to more complicated system. If we pretended URI could be represented by java.net.URL (as in the early days of Java), we'd get simpler result.

sdedic · 2022-01-07T07:43:52Z

First of all I'd like to point out I have already approved your PR. The changes are sane and acceptable from some architecture

I know. This discussion is about coming to an agreement, or a practical compromise. Maybe 3rd view would help.

I am more scared of duplicating the existing API by a (95% similar) concept, just to be pure and absolutely correct.

Well, not exactly. When pseudoURL.open*() throw an exception, you can't really know (if there's a mix of real+ephemeral URLs), if something broke or you just hit the ephemeral one, for example. If ever someone prints fakeURL.toString() and will try to convert it back, an exception is likely (handler not registered). fakeURL.toURI() will work, but fakeURL.toURI().toURL() will fail. We break the java.net.UR(I|L) behaviour at runtime ... but yes, the API would be simpler.

However there was no java.net.URI when NetBeans started. The java.net package contained just java.net.URL and other APIs like Class.getResource used it to refer to other objects than just those on World Wide Web.

The Locator was not required to point to WWW. It is required to locate the resource so its content can be accessed (other ops possible). The ClassLoader methods were right to use URLs, as the purpose was to access content (to a lesser degree identify the resource, but as each well-formed URL is URI, it's satisfied by the contract).
I believe we have a different situation here.

Yes, your position is acceptable. It just leads to more complicated system. If we pretended URI could be represented by java.net.URL (as in the early days of Java), we'd get simpler result.

Hm, let's let some other party to weigh in :) there are yet some other reviewers available. Functionally, I am mostly afraid of weird bugs from circulating URLs, that are not URLs and break the usual URL conversion/usage patterns, through the system at runtime.

eirikbakke · 2022-01-07T08:34:52Z

I started looking through this, but I'm boarding a plane now and haven't finished reviewing yet...

Adding URI in addition to URL seems to add a lot of complexity, and potential for confusion. Perhaps just add an optional String identifier, and then let icons be looked up by either the URL (if present) or the optional identifier?

There's a number of hacks that could add complexity here... I remember dealing with icon/image wrappers when adding SVG support, and it was a source of pain.

I will look more closely through this patch again in a day or two.

sdedic · 2022-01-07T08:50:50Z

Adding URI in addition to URL seems to add a lot of complexity, and potential for confusion. Perhaps just add an optional String identifier, and then let icons be looked up by either the URL (if present) or the optional identifier?

This ("url" + "id") is the conceptually the same as "url" + "uri", it is that dichotomy itself that is the centre of the heated philosophical debate.

BTW as a improvement, there could be always an ID (String, URI), possibly derived from a supplied load URL but unaffected by branding / localization computed internally by ImageUtilities (hey, another benefit of the duplication !) and sometimes possibly URL. Then a client may choose to get just the property it is interested in (location, identification). The current client code in the patch queries both, which is confusing; will fix.

Except that, URI has at least some structure :) compared to String, with Strings, we would still need to invent some convention (e.g. prefixing) so that ids from different sources remain unique (i.e. I should not probably provide just "Tree.openIcon", that's likely to clash, but "something.Tree.openIcon" ... which is quite equivalent to an URL/URI scheme + host / path).

neilcsmith-net · 2022-01-07T09:18:33Z

Except that, URI has at least some structure :) compared to String, with Strings, we would still need to invent some convention (e.g. prefixing) so that ids from different sources remain unique (i.e. I should not probably provide just "Tree.openIcon", that's likely to clash, but "something.Tree.openIcon" ... which is quite equivalent to an URL/URI scheme + host / path).

That's an interesting point about specificity. What would the path levels mean? I've said before that I'd wished NetBeans had a logical icon spec, something akin to xdg icon spec https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html#guidelines Here the level of specificity has meaning, and the most specific option found will be used, by removing the lower level and looking again. eg. Tree.openIcon and Tree.openIcon.something would make sense.

eirikbakke · 2022-01-07T14:48:24Z

As long as there is an "url" field, having an "uri" field as well be confusing. People will not be able to guess what they are used for, and if something starts with "http://", then anyone looking at it will assume it points to a physical file somewhere. (Unless they are intimately familiar with XML namespace conventions.)

Having an optional String property called "logicalIconID" would be clearer. Then we don't have to try to convert between URL and URI either, and the code for setting the "url" parameter can remain untouched.

sdedic · 2022-01-08T17:05:22Z

That's an interesting point about specificity. What would the path levels mean? I've said before that I'd wished NetBeans had a

I wouldn't drag this discussion into a PR, maybe mailing list would be better. For the purpose of this PR (and accompanying documentation), I'd say that a module contributed to Apache NetBeans repository should coordinate its IDs (through a review, for example), and external modules must use DNSlike prefix as if URI was created. That will satisfy uniqueness property required here, additional semantics should be IMHO solved separately

sdedic · 2022-01-08T17:12:21Z

@JaroslavTulach @eirikbakke it seems that some consensus is bulding finally. Let me recap:

I need some 'identifier' to identify icon for replacement; that can be sometimes derived from resource URL, but sometimes not.
Client should make one call or extraction, not a series of get + instanceof
URI is rather confusable with URL for other people than XML veterans, so people could choose wrong thing from URL/URI alternatives (especially when most of the time URI would be URL)

So ... I am going to:

change the type to String, document minimal rules for uniqueness
provide a helper static method to get URL and an (String) identifier (the instanceof cascade will be here)
propagate "id" and "url" separately, not derive one form the other in ImageUtilities impl.

All OK ?

neilcsmith-net · 2022-01-08T17:33:51Z

@sdedic you mentioned "invent some convention". I'm just suggesting ad(o/a)pting one that already exists. And that branding by unique IDs across modules is one of the flaws in NetBeans as it is.

eirikbakke · 2022-01-08T22:51:17Z

@sdedic Yes, that would cover my own main concerns with ImageUtilities.

eirikbakke

I submitted a few in-code comments that I started yesterday... happy to review again when PR is updated with the latest proposed changes.

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

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

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

JaroslavTulach

If you are OK with it Sváťo, feel free to proceed. Kudos to XML veterans!

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

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

sdedic · 2022-01-09T21:50:38Z

Will squash & merge early morning (CET) tomorrow.

eirikbakke

Thanks for the changes in ImageUtilities! I'm not familiar with the VSCode, LSP stuff etc., but I had a look at everything.

My biggest question is about UIDefaultsIconMetadata. Is this going to be run as part of the regular NetBeans IDE now? If so I want to take a closer look at it...

In particular, how does UIDefaultsIconMetadata handle Icon implementations other than ImageIcon, or ImageIcon instances that use a MultiResolutionImage?

eirikbakke · 2022-01-09T21:59:32Z

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

+            return null;
+        }
+        try {
+            if (s.contains(":")) {


I'm not sure I understand the convention that is being established here. Would be good to have it documented somewhere.

eirikbakke · 2022-01-09T22:03:45Z

java/java.lsp.server/vscode/package.json

+		"netbeans.iconMapping" : [
+			{
+				"uriExpression": "nbres:/org/netbeans/modules/gradle/resources/gradle.png",
+				"codeicon": "project"


nit: Should probably be codeIcon (with capital I).

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

eirikbakke · 2022-01-09T22:09:52Z

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

@@ -1112,6 +1201,7 @@ public ToolTipImage(Icon delegateIcon, String toolTipText, URL url, int imageTyp
            this.delegateIcon = delegateIcon;
            this.toolTipText = toolTipText;
            this.url = url;
+            this.imageId = null;


Do we not need to pass the imageId along here?

The other constructor is called from assignToolTipToImageInternal that reads imageID property from the image. That one is called from the image2Icon, which is used by mergeImages.In addition

icon2Image(image2Icon(image_with_imageId))

ought to provide an image that still returns imageID property. So I think yes, TooltipImage should remember it.

eirikbakke · 2022-01-09T22:11:58Z

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

+
+/**
+ * Goes through UIDefaults and replaces Icons and Images with variants that
+ * return image's URL from the 'url' property.


How do we know that this is run after the UIDefaults customizations in o.n.swing.plaf or o.n.swing.laf.flatlaf? Does the invokeLater call really guarantee this?

Hm. I think I can force the configured LaF initialization by getting some UIManager's value. Anyway, the manipulation with UIDefaults should run from the EDT.

Edit: fixed in 37f6c1b

After the first sweep, I hook to PropertyChangeEvent from UIDefaults, so later changes should be caught and patched.

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

eirikbakke · 2022-01-09T22:22:22Z

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

+        BufferedImage newImage = new MetadataImage(image, model, raster, bitmask, props);
+
+        java.awt.Graphics g = newImage.createGraphics();
+        g.drawImage(image, 0, 0, null);


This doesn't work for HiDPI icons or MultiResolutionImage? What kind of icons is this used for?

In this first version (which affects just NBLS headless distribution) - icons are exported from Nodes only. Could you please suggest how to improve this part in the future so the HiDPI works OK ?

In this first version (which affects just NBLS headless distribution)

Is a future version of UIDefaultsIconMetadata supposed to actually run as part of the NetBeans IDE? Or is it always just in this headless mode?

(In any case, I'll have a look at the HiDPI implications here...)

At this moment, we need to replace certain icons bcs their style does not match the LSP client's appearance. This does not apply to NB -- the icons would be simply changed or rebranded.

The client (vscode) has some icons already that use font glyphs, rebranding is not that much possible. Extracting glyphs, converting (e.g.) to SVGs for branding could bring some licensing issues I'd like to avoid. You're right (see other question) that the implementation is not optimal - this will hopefully improve.

Re. usage in NB apps that actually display the icons - I can think of possible usage for A11Y purposes, but did not went deep that way.

eirikbakke · 2022-01-09T23:21:39Z

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

+        });
+    }
+
+    static void replaceIconsAndImages(UIDefaults defs) {


Could you explain why the UIDefaultsIconMetadata wrapping is necessary? As far as I know, icons that are loaded from files already have the "url" property set by ImageUtilities.

There's a lot of LAF customizations (and hacks) that interact with UIManager, and hooking up to it this way seems fragile and likely to cause problems in the future. Furthermore, the Icon wrapping logic in ImageUtilities is already hard to understand and debug, and I'd hate to wrap all the Icon instances in yet another layer...

My goal was to provide "some identification" for an Image now exported to LSP, so the client can decide what to do with it. Ideally without changing all the places where an image is used or loaded. In addition images are loaded from UIDefaults by Swing as well (which I can not change).

It may be the case that eventually I'll go through NB codebase and change the UIManager.getIcon() calls one by one (after I make up some alternative and for a SPI)... then this wrapper code code would eventually vanish without impact on its clients.

This place is at this moment the central and only one that knows, for a image, some ID - the hashmap key. It seemed as a way to enhance the images that flow throughout the system in a single central place. So far this layer is only used in a headless mode, where image rendering is not that big issue.

I share Eirik's concerns, and also have concerns about changing the UIManager calls without some coordination. An API / SPI approach is certainly where I'd like to see this evolve, but that overlaps with a bunch of other conversations going on at the moment (inc. hidpi, svg, dark icons, branding). As you said before, there's a discussion to be had on dev@ about this. I still think this PR is potentially addressing the issue in the wrong place, but also that it's a case of needs must right now.

@neilcsmith-net Oh, I always assumed that this PR was part of the effort to let icons be branded for LAF purposes. Is there any hope of satisfying both needs with one API change?

It may be the case that eventually I'll go through NB codebase and change the UIManager.getIcon() calls one by one (after I make up some alternative and for a SPI)... then this wrapper code code would eventually vanish without impact on its clients.

Perhaps this might actually be preferable to hacking UIManager. How many of the icons would actually require this? It's only the select few that don't have an URL to a PNG or GIF for some reason, right?

@eirikbakke note that this 'replacement' solves also icons queried by Swing codebase that might flow through the system: even if Swing calls getIcon(), it gets a wrapper ... and if that image/icon is passed around the meta-data are present.

Other approach would be to introduce a SPI, that would supply metadata (in general) for an image/icon. ImageUtilities could ensure the SPIs would be called just once/instance (as the images should be immutable anyway). Then wrappers could just vanish from UIDefaultsIconMetadata. The drawback would be that if one instance is shared into multiple keys, the metadata (icon id) provider would just pick one of the keys as an identification - I don't know how much could that be an issue.

Other approach would be to introduce a SPI, that would supply metadata (in general) for an image/icon

That sounds a bit backwards. "Metadata" here just means "logical icon ID", right? And surely it's the site that actually loads the icon that should provide the logical icon ID to look up by?

By the way, do we have an actual use case for this patch yet, i.e. actual icons that will be substituted in a future patch? Which icons are those, and how many of them are there?

note that this 'replacement' solves also icons queried by Swing codebase that might flow through the system: even if Swing calls getIcon(), it gets a wrapper

For Swing-loaded icons, can't clients just update the relevant icons directly in UIDefaults, like LAFs do?

Oh, I always assumed that this PR was part of the effort to let icons be branded for LAF purposes. Is there any hope of satisfying both needs with one API change?

@eirikbakke I think so, but not necessarily with this PR. And the question of keeping the metadata in the icon I can understand given the Swing internals at play too. API discussions are probably better on dev@

That sounds a bit backwards. "Metadata" here just means "logical icon ID", right? And surely it's the site that actually loads the icon that should provide the logical icon ID to look up by?

I agree with that. IMO we need an API whereby icon loading can use a logical ID and request an icon for X. And an SPI that provides the icons for X. In the LSP client you'd implement the same. Any such API/SPI could use the existing URLs (file locations) as a backup ID to aid transition. So both needs would benefit from migrating to a logical ID, as well as being able to query that ID, without any icon being loaded.

@neilcsmith-net ... that would require to change UIManager.getIcon|Image to "something different" that would involve that loading SPI (an implementation could do UIManager.get* and return the image/icon with 'logical ID' ) - the change would need to be done throughout the codebase, right ?

Good; and what should we do with Swing calling UIManager.get() from its own code ?

IMO we need an API whereby icon loading can use a logical ID and request an icon for X. And an SPI that provides the icons for X. In the LSP client you'd implement the same. Any such API/SPI could use the existing URLs (file locations) as a backup ID to aid transition. So both needs would benefit from migrating to a logical ID, as well as being able to query that ID, without any icon being loaded.

Yes, something like this would be exactly what I'd have in mind for this.

that would require to change UIManager.getIcon|Image to "something different" that would involve that loading SPI (an implementation could do UIManager.get* and return the image/icon with 'logical ID' ) - the change would need to be done throughout the codebase, right ?

I think the existing APIs could be just fine initially. Just keep loading icons by URL; after all, there needs to be a default icon file in the NetBeans codebase for each logical icon ID in any case. But there could indeed be an SPI that ImageUtilities will check to see if a given URL should be resolved to a different Icon instance.

Good; and what should we do with Swing calling UIManager.get() from its own code ?

For this, just modify the icon in UIDefaults, like Swing intended. We already do this e.g. from Windows8LFCustoms.

@sdedic I somewhat suspect there will be fallbacks very much along the lines of what you do here too! I only mentioned in the first place so we sync up conversations on the various uses for IDs / don't lock its spec to something that can't meet more needs in future.

mbien · 2022-01-10T03:30:49Z

i am a bit late to the URI vs URL discussion but i only wanted to add that I am avoiding using URLs whenever possible. The class has several non obvious problems, e.g it does dns lookups in the background. This means that equals and hash code has to do IO which causes a chain of issues. All what is needed is someone putting URLs as keys into a map.

sdedic · 2022-01-10T05:09:12Z

Rebased on master & consolidated fixes.

sdedic · 2022-01-10T06:29:40Z

@mbien this is another philosophical one ;) if interested, let's take it on the mailing list.

JaroslavTulach · 2022-01-10T07:15:22Z

avoiding using URLs whenever possible

Great idea when you write new code or refactor internals of existing one. However here we are dealing with 20 years old already existing API. Just replacing java.net.URL with java.net.URI would cause uncountable disruptions in NetBeans own source code as well as applications using the NetBeans Platform.

mbien · 2022-01-10T14:24:59Z

avoiding using URLs whenever possible

Great idea when you write new code or refactor internals of existing one. However here we are dealing with 20 years old already existing API. Just replacing java.net.URL with java.net.URI would cause uncountable disruptions in NetBeans own source code as well as applications using the NetBeans Platform.

@JaroslavTulach I was not proposing to change any existing API. (although JDK 8 would have all tools it needs for a migration, default methods and the fact that there are easy ways to convert between URL and URI - its basically a best case scenario).

But this should be considered for new APIs and impls which use the actual classes - since its easy to overlook. I also use the word "avoiding" in the "try not to use if possible" sense, not in the "its forbidden" sense.

@mbien this is another philosophical one ;) if interested, let's take it on the mailing list.

@sdedic the philosophical discussion probably happened when URL was implemented, now we can only try to find technical solutions to deal with the non-obvious side effects of the URL class (e.g. inspections). JEP 418 might help mitigating some side effects, but thats JDK 18 and would be only a mitigation.

graemerocher · 2022-01-14T09:14:00Z

Is there a snapshot build of the vsix for this that can be tested?

sdedic · 2022-01-14T09:48:18Z

The artifacts currently being voted on (https://dist.apache.org/repos/dist/dev/netbeans/netbeans-vscode-ext/12.6.301/) should contain this change.

eirikbakke · 2022-01-14T13:06:00Z

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

+                props.put(meta[i], meta[i+1]);
+            }
+            props.put(ImageUtilities.PROPERTY_ID, URN_DEFAULTS_PREFIX + key);
+            return createNew(image, icon, props);


OK, I'm actually confused about what UIDefaultsIconMetadata does. Is it trying to use the UIManager key to identify the icon?

OK, I should change the (now after the changes) obsolete javadoc. The only purpose of that is to 'somehow add metadata' to the Icons and Images available from UIManager.getIcon/Image(). It does so by wrapping the originals into a delegate that provides appropriate value(s) from Image.getProperty - and that's all.

Why can't whichever client which needs to get the ID do this instead, at the time when the ID is actually needed?

I maybe do not understand the question: The client receives an Image instance. That instance may be possibly badged from some base Image instance the client has no access to. How could the client compute that the (base) image comes from UIDefault key xxx when it does not know the base image instance, or how the base image has been obtained (maybe even not from UIDefaults) ?

Then there is the Icon > Image conversion that happens sometimes in the infrastructure (i.e. FolderNode) which also needs (somehow) to carry over the ID; in this case, the converting code does not even know that someone will query an ID from the resulting image (or an image badged from that).

The client receives an Image instance. That instance may be possibly badged from some base Image instance the client has no access to. How could the client compute that the (base) image comes from UIDefault key xxx when it does not know the base image instance, or how the base image has been obtained (maybe even not from UIDefaults) ?

I think this is backwards. By the time you have an Image or an Icon instance, it's already too late to try to substitute another icon. The substitution needs to happen when you load the Image or the Icon. That's when the code loading the icon should be supplying an ID.

Then there is the Icon > Image conversion that happens sometimes in the infrastructure (i.e. FolderNode) which also needs (somehow) to carry over the ID

There was a similar problem when I implemented HiDPI icon support in ImageUtilities. The solution was to make sure that ImageUtilities.icon2image and ImageUtilities/image2icon were reversible. So the metadata is carried over.

I think this is backwards. By the time you have an Image or an Icon instance, it's already too late to try to substitute another icon. The substitution needs to happen when you load the Image or the Icon. That's when the code loading the icon should be supplying an ID.

The use case for headless LSP server is different. The icon, for example in our case, resides in a completely other binary/product with a completely different loading and painting mechanism. Its glyph or bits are loaded in a different process, actually; but in order to do that, I need to identify somehow the icon. I might eventually brand those icons - but the mechanism will be the same: image bits thrown away or unused, handled by the client based on "some ID".
We don't want to change the loading code in NetBeans because of "just some LSP client" needs (unless necessary).

For Images obtained as ImageUtilities.loadImage it's rather easy as there's the URL (which can be mangled by localization etc, but that can be handled). The imageID stuff supplies some ID for non-URL images, most notably those from UIDefaults (not loaded by loadImage)

OK, so as long as all of this only ever runs in the headless LSP use case, I have a lot fewer concerns. Adding the string id field to ImageUtilities also seems fine, as long as there is no extra instance wrapping in the regular IDE.

The use case for headless LSP server is different. The icon, for example in our case, resides in a completely other binary/product with a completely different loading and painting mechanism.

So the icons to be replaced are not icons in the NetBeans codebase? Could you give an example of such an icon, in context, with a screenshot? (A screenshot would make it a lot easier for me to understand what, exactly, the LSP server is...) What are we replacing these icons with?

The imageID stuff supplies some ID for non-URL images, most notably those from UIDefaults (not loaded by loadImage)

Right, but UIDefaults icons can be replaced by changing the UIDefaults itself, no?

The use case for headless LSP server is different.

My general point is that the use case for the headless LSP server is different to what we have now, but that the IDE would benefit from fixing the process in a similar way.

For Images obtained as ImageUtilities.loadImage it's rather easy as there's the URL (which can be mangled by localization etc, but that can be handled).

And the URL has _been_mangled to address branding, and dark variants, and sizes, and hidpi, and svg conversion ... Some of which interfere badly with each other, and spread files (some duplicates IIRC) all over the code base. For the purposes of loading an icon at least, there are benefits to not mangling based on URL in the first place.

neilcsmith-net · 2022-01-14T17:24:40Z

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 JButton();


Judging from the comment, this should be JCheckBox too?!

sdedic · 2022-01-15T11:54:22Z

FYI - I removed NB13 milestone from here, bcs. of pending discussion. I created #3459 which does not introduce new things to API (I've retained the URL property, it's there and using just "url" string is not well traceable)

Chris2011 · 2022-06-26T13:26:20Z

I prefer to add a comment why PRs or Issues where closed. If a short description is possible, that would be nice :)

sdedic changed the title ~~Support for image IDs, vscode client can redefine images/icons~~ Support for image IDs, LSP client can redefine images/icons Jan 5, 2022

sdedic self-assigned this Jan 5, 2022

sdedic added enhancement LSP [ci] enable Language Server Protocol tests Platform [ci] enable platform tests (platform/*) VSCode Extension [ci] enable VSCode Extension tests labels Jan 5, 2022

sdedic added this to the NB13 milestone Jan 5, 2022

sdedic requested review from JaroslavTulach and dbalek January 5, 2022 15:50

dbalek reviewed Jan 6, 2022

View reviewed changes

JaroslavTulach requested a review from eirikbakke January 6, 2022 16:07

JaroslavTulach approved these changes Jan 6, 2022

View reviewed changes

dbalek approved these changes Jan 7, 2022

View reviewed changes

eirikbakke reviewed Jan 8, 2022

View reviewed changes

sdedic requested review from eirikbakke and JaroslavTulach January 9, 2022 16:57

sdedic added the Need Squashing label Jan 9, 2022

JaroslavTulach approved these changes Jan 9, 2022

View reviewed changes

eirikbakke reviewed Jan 9, 2022

View reviewed changes

sdedic added 6 commits January 10, 2022 06:08

Support URIs for images, i.e. images from UIDefaults.

6abd60d

Added URI metadata to images from UIDefaults.

9511922

Explicitly mark physical files the client can see.

0c38d11

Send image URI on the protocol.

95c7849

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

e4d65f8

Licenses fixed.

cae803b

sdedic force-pushed the vscode/redefined-icons branch from 37f6c1b to cae803b Compare January 10, 2022 05:09

eirikbakke reviewed Jan 14, 2022

View reviewed changes

neilcsmith-net reviewed Jan 14, 2022

View reviewed changes

sdedic removed this from the NB13 milestone Jan 15, 2022

sdedic mentioned this pull request Jan 15, 2022

Support to redefine icons for LSP client #3459

Merged

sdedic marked this pull request as draft February 22, 2022 18:12

sdedic closed this Jun 25, 2022

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

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

Conversation

sdedic commented Jan 5, 2022 • edited

dbalek Jan 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic commented Jan 6, 2022

JaroslavTulach commented Jan 7, 2022

sdedic commented Jan 7, 2022

eirikbakke commented Jan 7, 2022

sdedic commented Jan 7, 2022 • edited

neilcsmith-net commented Jan 7, 2022

eirikbakke commented Jan 7, 2022

sdedic commented Jan 8, 2022 • edited

sdedic commented Jan 8, 2022

neilcsmith-net commented Jan 8, 2022

eirikbakke commented Jan 8, 2022

eirikbakke left a comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

sdedic commented Jan 9, 2022

eirikbakke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic Jan 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic Jan 9, 2022 • edited

Choose a reason for hiding this comment

eirikbakke Jan 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic Jan 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikbakke Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbien commented Jan 10, 2022

sdedic commented Jan 10, 2022

sdedic commented Jan 10, 2022

JaroslavTulach commented Jan 10, 2022

mbien commented Jan 10, 2022 • edited

graemerocher commented Jan 14, 2022

sdedic commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikbakke Jan 14, 2022 • edited

Choose a reason for hiding this comment

sdedic Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikbakke Jan 14, 2022 • edited

Choose a reason for hiding this comment

sdedic Jan 14, 2022 • edited

Choose a reason for hiding this comment

eirikbakke Jan 14, 2022 • edited

Choose a reason for hiding this comment

sdedic commented Jan 5, 2022 •

edited

dbalek Jan 6, 2022 •

edited

sdedic commented Jan 7, 2022 •

edited

sdedic commented Jan 8, 2022 •

edited

sdedic Jan 9, 2022 •

edited

sdedic Jan 9, 2022 •

edited

eirikbakke Jan 9, 2022 •

edited

sdedic Jan 10, 2022 •

edited

eirikbakke Jan 14, 2022 •

edited

sdedic Jan 14, 2022 •

edited

mbien commented Jan 10, 2022 •

edited

eirikbakke Jan 14, 2022 •

edited

sdedic Jan 14, 2022 •

edited

eirikbakke Jan 14, 2022 •

edited

sdedic Jan 14, 2022 •

edited

eirikbakke Jan 14, 2022 •

edited

Chris2011 commented Jun 26, 2022 •

edited