-
Notifications
You must be signed in to change notification settings - Fork 193
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.
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
base: master
Are you sure you want to change the base?
Conversation
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 <[email protected]>
Here is the original Gerrit (it has some comments which are still valuable). |
In order to test out this PR:
The observed result should be that the "run" and "debug" icons in the toolbar have been replaced: |
The overall code seems good. What particularly is left TODO (apart from the documentation TODO in code) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if ( | ||
// !JFaceResources.getString(JFacePreferences.CUSTOM_RESOURCE_THEME).equals(JFacePreferences.CUSTOM_RESOURCE_THEME) | ||
// && | ||
resultingURL.getFile().contains(PLUGIN_URL)) { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with that
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
PLUGIN_URL + bundleID + THEME_REPLACEMENTS_DIR + originalFilePath); | ||
String found = getFilePath(themedIconURL, false); | ||
if (found != null) { | ||
resultingURL = themedIconURL; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
try { | ||
URL themedIconURL = new URL(resultingURL.getProtocol(), resultingURL.getHost(), resultingURL.getPort(), | ||
PLUGIN_URL + bundleID + THEME_REPLACEMENTS_DIR + originalFilePath); | ||
String found = getFilePath(themedIconURL, false); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
@@ -42,6 +42,9 @@ | |||
*/ | |||
class URLImageDescriptor extends ImageDescriptor { | |||
|
|||
private static final String THEME_REPLACEMENTS_DIR = "/theme"; //$NON-NLS-1$ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Thanks @laeubi for the review; I agree with those points, and trying to stick to (any form) of URL should work better. |
Thanks for the review @mickaelistria :)
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:
|
@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. |
@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 :) |
@@ -42,6 +42,9 @@ | |||
*/ | |||
class URLImageDescriptor extends ImageDescriptor { | |||
|
|||
private static final String THEME_REPLACEMENTS_DIR = "/theme"; //$NON-NLS-1$ |
There was a problem hiding this comment.
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
if ( | ||
// !JFaceResources.getString(JFacePreferences.CUSTOM_RESOURCE_THEME).equals(JFacePreferences.CUSTOM_RESOURCE_THEME) | ||
// && | ||
resultingURL.getFile().contains(PLUGIN_URL)) { |
There was a problem hiding this comment.
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.
// 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$ |
There was a problem hiding this comment.
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.
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:
|
…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 <[email protected]> * 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 <[email protected]>
@Michael5601 This might be interesting for you as you deal with icons now. |
Thanks for the info. Seems like a great project. :) Yesterday I talked with a colleague of mine about this topic as he searched for a new bigger project. This would fit perfectly with our new initiative for a new modern icon pack in the Eclipse IDE. @gayanper maybe you are interested in participating as you talked about new icon packs? I will forward this PR to my colleague as I probably won't have time due to other projects. |
Hi guys, I was asked if I could continue with this PR. First of all I would like to know if my ideas are realistic before starting to implement them. |
I think how users interact with this feature is a next step. IMO it can be skipped for the moment so we can focus on the more critical underlying design. Once we have the ability to switch to another icon set, one way or the other, then we'll more easily figure out how far we want to push it in term of usability. But anyway, usability cannot be implemented until underlying strategy is available in JFace.
Sounds good.
Note that it's about improbable that all plugins change their icons dir. So the
The current proposal of a fragment is convenient with plugin URLs because from a plugin it's easy to infer the path of the plugin and the path of the icon to inject the |
If we use a URL scheme than all kind of URLs should/could be supported, I don't see that a specific scheme is mandatory for this. Anyways for me the most interesting part would be how to derive the theme id (in a platform independent way) and one should keep in mind that this can increase the load time of images a lot. |
Imagine
I guess it would be more the other way round with the given proposal: there would be a named icon set, existing in the wild independently from the theme, and any Eclipse theme could declare to use that set. |
As we do for other schemes as well, e.g currently we support
so for sure one can take your provided example here, extract the name part and try
or even
or whatever, I think this is the most important part is should/must work with all kinds of URIs. If you only want support for resources loaded from bundles, then a Resource Hook would be much better suited as I mentioned already somewhere before. |
Of course there is already built-in support for Anyway, I'm not sure why we are discussing Translation support has similar issues about looking things up at alternative locations and to which fragment can contribute "overrides"... |
This is a different topic, because here we are talking about |
That does seem very interesting! |
Hi guys, i am currently working on this issue and encountered a problem with the bundleentry - URL´s. Some of the URL´s are not of the type Thanks for the help! |
As mentioned already above, one can not make any assumptions about the actual URL, as it is a
Also assume it can be converted to a file is plainly wrong, from my point of view as again there is no guarantee about what this actually return. So I can only strongly suggest to only operate on the URL itself (as it is done for the |
FYI, I think toFileURL would unzip and jar to some local location... |
What is the workflow if the My experience with Eclipse icons was that they are either loaded via a |
They are not converted to a path, they are just loaded form the
Just look at |
Note that both these schemes are hierarchical just like file: scheme is hierarchical so I would imagine you can do the same/similar things with the URIs as you are trying to do with the paths... |
I'll try to setup an example using bundle transformer hooks the next days, this looks actually more useful than having even more URLs to check and it should even use for all kind of ways to access the icons. |
Makes sense, thank you for the clarification. :) |
I have now setup an example locally and technically we have everything we need already in platform but it could be improved so it works a little bit smoother for the case of an icon pack, the only thing we need here is some kind of documentation and an example. From that we can start creating icon packs and probably improve the process further. The good thing about this is that it is totally flexible regarding the icons to replace and the layout to be used, does not require "special" things like fragments and one can choose to replace icons for one bundle or even all bundles. The only thing is that when one installs multiple packs that try to override the same icons then of course only one can win, but that's more a deployment issue. |
Ideally, we could start with some test cases. Using an eg blue square icon by default in an ImageDescriptor and testing it to be overridden as a red icon by a theme could be a relatively easy way to test we get the right image. Do I get it right that your proposal then would be purely available for URLs from Equinox, and not for general SWT then? (note that I don't think it's an issue, having theming implemented in Platform rather than in SWT makes at least as much sense as the other way round) |
It has nothing to with So it won't work if you use SWT/JFace in a plain setup without OSGi, but as there you have a flat classpath it is even simpler: Just make sure your icon pack appears before the one you want to replace an image. |
Thanks for that. I don't have the technical knowledge the really understand your implementation on a technical level.
For the "Icon Packs" I see two use-cases:
folder structure. And you want to simply add new monochrom versions of "log" and "fancyType" directly inside the com.acme.fancyEditor" plugin.
From the user's perspective the icon pack should be something he selects in the preference dialog. For example a new setting next to the "Theme" drop down box on the "Appearance" preference page. This drop down box would list all available icon packs and the user can select which one to use. So only one icon pack should be "active" at a time. Can you explain e.g. based on my example if and how these use-cases can be achieved? What does an plugin-developer do to leverage this feature? |
I'll prepare an example how this can be used once the changes are available but don't wanted to rush them in due to holiday seasons and give people some more time to possibly give feedback. In any case things like configurability / UI is something that has to be provided on top of this and we might want/need to further enhance the "core" for different use-case, but both of what you have described is possible. |
I want to share/refresh one conclusion we had at last year's Eclipse IDE community day at SAP when discussing the icon exchange/pack/theme topic: Due to the general independence of plugins, being provided as extensions to a product like the IDE, it is unlikely to achieve one consistent set of new icons under the same icon pack/theme ID (or even multiple ones, if new types of icons are provided). If we consider "icon packs" as complete sets of icons, all provided for the same ID of an icon pack, this probably only work fine for the product use case, where the product delivers a complete and consistent icon pack on its own, but not when considering an extensible platform. Last year, we thus agreed on following the approach of making icon packs modular and choosing multiple of them to form something like an icon theme. For example, the Eclipse platform may deliver a "flat" and "monochrome" icon pack. JDT does the same. Some extension provider only provides a "flat" pack. Now you can select those packs you want to combine to your theme. One reasonable theme would be the combination of all "flat" icon packs. But given that the extension provider has no "monochrome" pack so far, you could combine its "flat" pack with the other "monochrom" ones, rather than using the default (non-flat) icons, which do integrate with monochrome icons even worse, when just allowing to contribute to a complete icon theme. In addition, icon packs may not be mutually exclusive, as an icon pack may want to override an icon that is also provided by another icon pack, like the one with the basic icons for the platform. This may be resolved by defining a precedence order for the icon packs. And to make it easier to configure, you may still think of a concept of "themes" that have a default configuration for specific icon packs or icon pack IDs, but still give users the flexibility to insert the desired icon pack with icons of an extensions for which no "properly matching" icon pack is provided. |
Yes multiple packs can work together, one pack might even be able to override the ones from another, but as mentioned these are more "organizational" things, e.g. m2e allows to install so called "connectors", the same is possible for team providers (SVN, GIT) so maybe that would be a better approach than a plain preference. I'll leave that to the interested parties after the technical thing is available :-) |
But how can we make "technical things" available "before" we agreed on what we want to offer to the user? |
If the requirement is that we can replace icons by alternative ones through other bundles without the help of the original bundle author, then this can be solved independent on how these "icon bundles" later being installed or how many of them there are and how the UI looks like. And as @HeikoKlare correctly summarized, as Eclipse is extensible there is actually no way to have "one" icon pack that solves that all... maybe there will be one that supports many famous extensions but it can't support everything. This is not necessarily a problem, as still a dedicated product or EPP on the other hand still can benefit from such a feature. |
Fix #13
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:
fragment
directory, ie. "/theme/path/To/icon.png"