[WIP] Allow plugin fragments to supply alternate icons #14

AObuchow · 2022-04-19T00:56:10Z

Allow plugin fragments to contribute alternate icons that will be loaded
through the URLImageDescriptor class.

The icons must be placed within a directory titled "theme"
located at the root of the plugin fragment, ie. "/theme/"

The file path of the replacement icons must match the file path of the
icon to be replaced.

For example, in order to replace an icon used by plugin org.eclipse.xxx
where the path of the icon is "/path/To/icon.png" in the plugin's bundle
one must:

Create a plugin fragment with org.eclipse.xxx as the Host Plug-in
Create a directory "/theme/" at the root of the plugin
fragment
Place an alternate icon with the correct file path in the theme
directory, ie. "/theme/path/To/icon.png"

Allow plugin fragments to contribute alternate icons that will be loaded through the URLImageDescriptor class. The icons must be placed within a directory titled "theme" located at the root of the plugin fragment, ie. "/theme/" The file path of the replacement icons must match the file path of the icon to be replaced. For example, in order to replace an icon used by plugin org.eclipse.xxx where the path of the icon is "/path/To/icon.png" in the plugin's bundle one must: - Create a plugin fragment with org.eclipse.xxx as the Host Plug-in - Create a directory "/theme/" at the root of the plugin fragment - Place an alternate icon with the correct file path in the theme directory, ie. "/theme/path/To/icon.png" Signed-off-by: Andrew Obuchowicz <andrew@aobuchow.com>

AObuchow · 2022-04-19T00:57:03Z

Here is the original Gerrit (it has some comments which are still valuable).

AObuchow · 2022-04-19T01:01:22Z

In order to test out this PR:

Checkout the PR locally, having the JFace Eclipse bundle imported
Clone my sample icon-providing fragment
Import the sample fragment into your Eclipse workspace
Ensure the Eclipse application has all workspace plugins enabled (Or atleast, JFace and the sample fragment)

The observed result should be that the "run" and "debug" icons in the toolbar have been replaced:

mickaelistria · 2022-04-19T14:36:36Z

The overall code seems good. What particularly is left TODO (apart from the documentation TODO in code) ?

laeubi

I think this makes to much specific assumptions (icons are always local bundles, they can be resolved to a path, we know how image URLs are constructed,...).

I think it should more work with a delegation model where we construct an new URL for the theming and using the original one as a fallback image in case of errors.

laeubi · 2022-04-19T14:53:57Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

+		if (
+		// !JFaceResources.getString(JFacePreferences.CUSTOM_RESOURCE_THEME).equals(JFacePreferences.CUSTOM_RESOURCE_THEME)
+		// &&
+		resultingURL.getFile().contains(PLUGIN_URL)) {


I don't think it is suitable to explicitly target the (internal) plugin URL format. We should use a scene that is independent from the source URL format like it is done with the zoom images.
The one don't needs to fiddle around with bundle ids and nls and alike...

+1 with that

I'm +1 for doing that, as it simplifies things a lot.
However, I'm a bit confused on how to do this, since the requirements and assumptions for zoom images are different.

For zoom images, resolution-specifying directories (eg icons/32x32/) only need to come from the same bundle as the original icon. Thus it is possible to preprend the icon filename with the resolution-specifying directory, and keep the rest of the icon URL/filepath the same.

With themed icons, the requirements is to allow another bundle (that is different from the original bundle which provides the icon) to provide an alternative icon URL. Thus, we need to change the bundle in the icons URL. My original idea was to provide a JFace preference (CUSTOM_RESOURCE_THEME) which would specify which bundle is providing the alternative icon.

For zoom images, resolution-specifying directories (eg icons/32x32/) only need to come from the same bundle as the original icon.

This depends on the URL type used. You can use http urls as well or even custom url types (like mvn:)

With themed icons, the requirements is to allow another bundle (that is different from the original bundle which provides the icon) to provide an alternative icon URL. Thus, we need to change the bundle in the icons URL.

If you want to replace icons from a bundle, the most flexibel way would be to require that providers supply a fragment to this bundle, that is how the translation bundles work (it would even allow to provide different sizes right now) and then you can simply modify the name.

This of course would mean there is not only one icon bundle but different ones, on the other hand it is a standard way to enhance an existing bundle and would make it clear what icons are included, so one don not need to ask the icon supplier to add support for my special eclipse plugin but I can supply an own one (probably once included in the original icon pack).

If you want to replace icons from a bundle, the most flexibel way would be to require that providers supply a fragment to this bundle, that is how the translation bundles work (it would even allow to provide different sizes right now) and then you can simply modify the name.

This of course would mean there is not only one icon bundle but different ones, on the other hand it is a standard way to enhance an existing bundle and would make it clear what icons are included, so one don not need to ask the icon supplier to add support for my special eclipse plugin but I can supply an own one (probably once included in the original icon pack).

My worry with making the icon provider have to supply a fragment to the original icons bundle is that it would require many icon bundles to replace all of Eclipse's standard icons. This would probably demotivate icon pack creators as it'd be very tedious, rather than creating a single plugin that replaces icons from various bundles (even though my idea contradicts the name of this PR).

With that being said, I am open to your approach as a "first step" towards supporting icon replacement in Eclipse. Perhaps a script/tool could be developed to automate the process of creating the required bundle fragments and populating them with the replacement icons.

@gayanper @mickaelistria if either of you have any input on this, feel free to chime in :)

My worry with making the icon provider have to supply a fragment to the original icons bundle is that it would require many icon bundles to replace all of Eclipse's standard icons.

The problem is, that each bundle has its own class/resource space so bundle A and B can have both icon/editor.png also note that there is no such thing as "all" icons because Eclipse can be extended and different combinations are used.

So it would be better to be more flexible here and tyr to give icon-pack creators tools to easier organize/automate this process, e.g. I can think of something like the babel-project for icon, where a tool simply scans a folder of plugins and discovers all image resources, then this is presented to the user as a big table and icons can be replaced and sutable fragments are generated, an update site is prepared and then one can install the packs required for the current eclipse install.

Beside this, I tried a while back to get SVG support into SWT (with little success because of concerns having a dependency to AWT), that would allow to style icons with CSS and could be even more profitable as you do not need to edit/replace every icon ...

For a first iteration, as there is no API, I think it's OK to restrict the them-ability to plugins/fragments and consider opening it to other URI schemes later.

laeubi · 2022-04-19T14:54:35Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

+						PLUGIN_URL + bundleID + THEME_REPLACEMENTS_DIR + originalFilePath);
+				String found = getFilePath(themedIconURL, false);
+				if (found != null) {
+					resultingURL = themedIconURL;


I think it makes more sense to simply return here instead of fall through the default return.

laeubi · 2022-04-19T14:56:23Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

+			try {
+				URL themedIconURL = new URL(resultingURL.getProtocol(), resultingURL.getHost(), resultingURL.getPort(),
+						PLUGIN_URL + bundleID + THEME_REPLACEMENTS_DIR + originalFilePath);
+				String found = getFilePath(themedIconURL, false);


I don't think that filepath is suitable here, this further limits the usage of this.

My intention here was to validate that the newly constructed icon URL actually resolves, and if not, fallback to the default icon. Do you know if there is a better way to see if a URL resolves, that doesn't rely on the assumption that bundles are local?

Ah maybe instead of getFilePath(themedIconURL, false) I could do something like URLImageDescriptor.getImageData(themedIconURL)? I'm investigating your commit for help :)

The only way that works for all cases is try open the URL, and maybe fall back to either the error or the original URL.

laeubi · 2022-04-19T15:01:50Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

@@ -42,6 +42,9 @@
 */
 class URLImageDescriptor extends ImageDescriptor {

+	private static final String THEME_REPLACEMENTS_DIR = "/theme"; //$NON-NLS-1$


Why limit to one particular folder here and not use what is provided by the CUSTOM_RESOURCE_THEME

Agreed, the parent directory of the icon should be as customizable as possible for the theme creator/provider.

+1 with @laeubi proposal to not mandate a root directory and let path just be /mySuperIconTheme/path/to/image.png

mickaelistria · 2022-04-19T15:08:34Z

Thanks @laeubi for the review; I agree with those points, and trying to stick to (any form) of URL should work better.

AObuchow · 2022-05-21T04:35:42Z

Thanks for the review @mickaelistria :)

The overall code seems good. What particularly is left TODO (apart from the documentation TODO in code) ?

I think asides from addressing @laeubi's comments, there are some followup tasks that will come from this change.

Some that come to mind are:

Set defaults for the newly created JFace Preferences (one preference should probably specify the icon theme bundle, and another will specify the parent directory of the themed icon)
Add an extension point to set these above two preferences in an icon theme bundle
Add an extension point to associate an icon theme to an IDE theme (this would allow specifying icons to use for the dark theme, and different icons to use for the light theme)
Add a UI preference to select the icon theme, similar to how IDE themes are chosen.

AObuchow · 2022-05-21T04:37:04Z

@laeubi Thank you very much for the review :) I apologize for being so slow on this topic. I hoped it was just a matter of me implementing your requested changes, but it turns out I needed further clarification.

gayanper · 2022-08-03T19:29:53Z

@AObuchow are you planning to continue on this PR ? it would be really good to have this functionality to start creating some icons packs with the themes.

AObuchow · 2022-08-05T01:54:53Z

@AObuchow are you planning to continue on this PR ? it would be really good to have this functionality to start creating some icons packs with the themes.

Thanks for pinging me on this @gayanper :) Yes I am planning on continuing it, I just got caught up with life shortly after my last comments. Feel free to help contribute to this topic in any way, it is always appreciated :)

mickaelistria · 2022-09-15T13:35:18Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

@@ -42,6 +42,9 @@
 */
 class URLImageDescriptor extends ImageDescriptor {

+	private static final String THEME_REPLACEMENTS_DIR = "/theme"; //$NON-NLS-1$


+1 with @laeubi proposal to not mandate a root directory and let path just be /mySuperIconTheme/path/to/image.png

mickaelistria · 2022-09-15T13:37:47Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

+		if (
+		// !JFaceResources.getString(JFacePreferences.CUSTOM_RESOURCE_THEME).equals(JFacePreferences.CUSTOM_RESOURCE_THEME)
+		// &&
+		resultingURL.getFile().contains(PLUGIN_URL)) {


For a first iteration, as there is no API, I think it's OK to restrict the them-ability to plugins/fragments and consider opening it to other URI schemes later.

mickaelistria · 2022-09-15T13:41:28Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

+			// TODO: This is a workaround and should probably be removed
+			if (originalFilePath.contains("/$nl$/")) { //$NON-NLS-1$
+
+				originalFilePath = originalFilePath.replace("/$nl$/", "/"); //$NON-NLS-1$ //$NON-NLS-2$


Why is $nl$ removed here? I think it should be kept and resolved as usual without particular case.

akurtakov · 2023-01-25T12:56:35Z

Is there any hope for this one still?

AObuchow · 2023-01-26T16:45:19Z

Is there any hope for this one still?

Yes, I apologize for the extremely long delay. I believe the remaining changes that are required are relatively simple, I've just been putting it off due to personal priorities. I will try and make these changes over the next week.

Since the consensus is to keep things relatively simple and rely on plugin fragments, I will follow this approach:

So it would be better to be more flexible here and try to give icon-pack creators tools to easier organize/automate this process, e.g. I can think of something like the babel-project for icon, where a tool simply scans a folder of plugins and discovers all image resources, then this is presented to the user as a big table and icons can be replaced and sutable fragments are generated, an update site is prepared and then one can install the packs required for the current eclipse install.

…platform#14) * Bug 579509 - IES423:(ICT) Missing space between two strings - Fixed missing space between strings - Added missing Eclipse Copy right note Change-Id: Ie3bbd2ff42172d1cd5beb0269900cda1039e07d4 Signed-off-by: Niraj Modi <niraj.modi@in.ibm.com> * Bug 579509 - IES423:(ICT) Missing space between two strings - Fixed missing space between strings - Added missing Eclipse Copy right note Change-Id: Ie3bbd2ff42172d1cd5beb0269900cda1039e07d4 Signed-off-by: Niraj Modi <niraj.modi@in.ibm.com>

laeubi requested changes Apr 19, 2022

View reviewed changes

laeubi reviewed Apr 19, 2022

View reviewed changes

vogella mentioned this pull request Jun 16, 2022

Remove System theme #142

Merged

mickaelistria requested changes Sep 15, 2022

View reviewed changes

HannesWell pushed a commit to HannesWell/eclipse.platform.ui that referenced this pull request Jul 1, 2023

Add timestamp and stacktrace (eclipse-platform#14)

5c46894

WJSchakel mentioned this pull request Feb 13, 2024

Refactor rename of method named 'copy' freezes Eclipse #1675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Allow plugin fragments to supply alternate icons #14

[WIP] Allow plugin fragments to supply alternate icons #14

AObuchow commented Apr 19, 2022

AObuchow commented Apr 19, 2022

AObuchow commented Apr 19, 2022

mickaelistria commented Apr 19, 2022

laeubi left a comment

laeubi Apr 19, 2022

mickaelistria Apr 19, 2022

AObuchow May 21, 2022

laeubi May 22, 2022

AObuchow Aug 5, 2022

laeubi Aug 5, 2022

mickaelistria Sep 15, 2022

laeubi Apr 19, 2022

AObuchow May 21, 2022

laeubi Apr 19, 2022

AObuchow May 21, 2022

AObuchow May 21, 2022

laeubi May 22, 2022

laeubi Apr 19, 2022

AObuchow May 21, 2022

mickaelistria Sep 15, 2022

mickaelistria commented Apr 19, 2022

AObuchow commented May 21, 2022

AObuchow commented May 21, 2022

gayanper commented Aug 3, 2022

AObuchow commented Aug 5, 2022

mickaelistria Sep 15, 2022

mickaelistria Sep 15, 2022

mickaelistria Sep 15, 2022

akurtakov commented Jan 25, 2023

AObuchow commented Jan 26, 2023

[WIP] Allow plugin fragments to supply alternate icons #14

Are you sure you want to change the base?

[WIP] Allow plugin fragments to supply alternate icons #14

Conversation

AObuchow commented Apr 19, 2022

AObuchow commented Apr 19, 2022

AObuchow commented Apr 19, 2022

mickaelistria commented Apr 19, 2022

laeubi 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

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

Choose a reason for hiding this comment

mickaelistria commented Apr 19, 2022

AObuchow commented May 21, 2022

AObuchow commented May 21, 2022

gayanper commented Aug 3, 2022

AObuchow commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akurtakov commented Jan 25, 2023

AObuchow commented Jan 26, 2023