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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+60 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions bundles/org.eclipse.jface/src/org/eclipse/jface/preference/JFacePreferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ public final class JFacePreferences {
*/
public static final String REVISION_OLDEST_COLOR = "org.eclipse.jface.REVISION_OLDEST_COLOR"; //$NON-NLS-1$

/**
* TODO: Add javadoc
*
* @since 3.26
*/
public static final String CUSTOM_RESOURCE_THEME = "org.eclipse.jface.CUSTOM_RESOURCE_THEME"; //$NON-NLS-1$

private static IPreferenceStore preferenceStore;

/**
Expand Down
58 changes: 53 additions & 5 deletions bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
*/
class URLImageDescriptor extends ImageDescriptor {

private static final String THEME_REPLACEMENTS_DIR = "/theme"; //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

private static final String PLUGIN_URL = "/plugin/"; //$NON-NLS-1$

private static class URLImageFileNameProvider implements ImageFileNameProvider {
private String url;

Expand Down Expand Up @@ -198,9 +201,17 @@ public String toString() {
}

private static URL getxURL(URL url, int zoom) {
if (zoom == 100) {
return url;
}

URL resultingURL = url;
// First we check for an image with increased zoom, if desired

// TODO: Ideally, we shouldn't be commenting out this line. Figure out a way
// around removing it.

// if (zoom == 100) {
// return url;
// }

String path = url.getPath();
int dot = path.lastIndexOf('.');
if (dot != -1 && (zoom == 150 || zoom == 200)) {
Expand All @@ -215,12 +226,49 @@ private static URL getxURL(URL url, int zoom) {
if (url.getQuery() != null) {
file += '?' + url.getQuery();
}
return new URL(url.getProtocol(), url.getHost(), url.getPort(), file);
resultingURL = new URL(url.getProtocol(), url.getHost(), url.getPort(), file);
} catch (MalformedURLException e) {
Policy.getLog().log(new Status(IStatus.ERROR, Policy.JFACE, e.getLocalizedMessage(), e));
}
}

// Next we check if there's an alternate image that we should use
// TODO: I'm not exactly sure how we should be accessing the JFace preference
// Should a IPreferenceStore instance be retrieved?
// Or is that considered part of Eclipse RCP and not JFace?
if (
// !JFaceResources.getString(JFacePreferences.CUSTOM_RESOURCE_THEME).equals(JFacePreferences.CUSTOM_RESOURCE_THEME)
// &&
resultingURL.getFile().contains(PLUGIN_URL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// The third path segment is the bundleID
String originalFilePath = resultingURL.getFile();
for (int i = 0; i < 2; i++) {
originalFilePath = originalFilePath.substring(originalFilePath.indexOf('/') + 1);
}
// TODO: The original bundleID should only be used in the case of plugin
// fragment image replacements.
// We need an additional (optional) preference that will hold the bundleID of
// the currently used theme plugin
String bundleID = originalFilePath.substring(0, originalFilePath.indexOf('/'));
originalFilePath = originalFilePath.substring(originalFilePath.indexOf('/'), originalFilePath.length());

// 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$
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (found != null) {
resultingURL = themedIconURL;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

}
} catch (MalformedURLException e) {
Policy.getLog().log(new Status(IStatus.ERROR, Policy.JFACE, e.getLocalizedMessage(), e));
}
}
return null;
return resultingURL;

}

Expand Down