Bugzilla – Full Text Bug Listing
| Summary: | [Graphics] [Themes] fix up o.e.ui.images structure | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Paul Webster <pwebster> | ||||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Paul Webster <pwebster> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | daniel.rolka, daniel_megert, john.arthorne, Lars.Vogel, nilskp, tmccrary, tmccrary | ||||||||
| Version: | 4.4 | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 422036 | ||||||||||
| Attachments: |
|
||||||||||
+1 from me. As discussed please feel free to adjust the structure. I was insure about a good layout, also the existing folder structure of the icons have little meaning to me. (In reply to Paul Webster from comment #0) > - Standard bundle i18n > - set the BREE to 1.5 or 1.4 > - add the SVG to the source plugin Done in http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=17d6330e7ba6fb7f7838c09e37f193e791349d5d (In reply to Paul Webster from comment #0) > - move the PNG folders to the /src folder > - flatten the PNG structure. Maybe just src/bundle/icons.... ? Maybe > src/bundle/obj16{etc)... ? Right now the patch to a SVG file is /org.eclipse.ui.images/eclipse-svg/eclipse.platform.ui/bundles/org.eclipse.ui/icons/full/obj16/info_tsk.svg (it reflects where it actually came from). I think that this is OK, and it might even make sense to make eclipse-svg a source folder (if we had a builder that could turn SVG into PNG). The path for a PNG file is /org.eclipse.ui.images/eclipse.platform.ui/bundles/org.eclipse.ui/icons/full/obj16/info_tsk.png If we copy the PNG files into each bundle they come from, we have duplication but they're easy to consume. If we want to just delegate to the org.eclipse.ui.images bundle then I think we need to shorten the path. Option 1: platform:/plugin/org.eclipse.ui.images/org.eclipse.ui/icons/full/obj16/info_tsk.png We remove the repo information, as the bundle name is enough to keep the icons separate. Still a little long. Option 2: platform:/plugin/org.eclipse.ui.images/icons/full/obj16/info_tsk.png We remove the bundle name as well, so it really does mirror the icon layout in the bundles we have today. The downside is that some images have duplicate names in the different bundles. This might not be a problem if the images are identical. ex: in the plugin, there are 1535 PNG files. If I copy them all into a flatter structure, I get 1374 PNG files. PW > Option 2:
> platform:/plugin/org.eclipse.ui.images/icons/full/obj16/info_tsk.png
>
> We remove the bundle name as well, so it really does mirror the icon layout
> in the bundles we have today. The downside is that some images have
> duplicate names in the different bundles. This might not be a problem if
> the images are identical. ex: in the plugin, there are 1535 PNG files. If
> I copy them all into a flatter structure, I get 1374 PNG files.
>
> PW
Tony, as you created the icons, do you know if the icons with the same name show the same icon or are they different in some cases?
We have a svg->png rendering target in the Google Code project's ant build. The structure of the icons was setup to match the existing bundles. Typically, the icons with the same name are the same icon. However, there are so many icons, it's possible there are some exceptions. I was able to convert SVG to PNG using convert (ImageMagick, but not a good conversion) and Inkscape (pretty close to what's in git). One option might be to standardize on one of those freely available tools. PW Another possibility would be to use the batik maven plugin: http://mojo.codehaus.org/batik-maven-plugin/usage.html It's not clear 1) how parameterizable it is and 2) if its defaults produce the kind of images that we want. PW (In reply to Paul Webster from comment #7) > Another possibility would be to use the batik maven plugin: > http://mojo.codehaus.org/batik-maven-plugin/usage.html > > It's not clear 1) how parameterizable it is and 2) if its defaults produce > the kind of images that we want. > > PW That's basically what the Google Code ant target is doing, it uses batik. (In reply to Tony McCrary from comment #8) > (In reply to Paul Webster from comment #7) > > Another possibility would be to use the batik maven plugin: > > http://mojo.codehaus.org/batik-maven-plugin/usage.html > > > > It's not clear 1) how parameterizable it is and 2) if its defaults produce > > the kind of images that we want. > > > > PW > > That's basically what the Google Code ant target is doing, it uses batik. The code needs to be cleaned up but it works: https://code.google.com/p/eclipse-svg-icons/source/browse/src/main/java/RasterizerUtil.java Created attachment 237317 [details]
converted using ImageMagick
Created attachment 237318 [details]
converted using InkScape
(In reply to Tony McCrary from comment #9) > > The code needs to be cleaned up but it works: > > https://code.google.com/p/eclipse-svg-icons/source/browse/src/main/java/ > RasterizerUtil.java OK, so the maven plugin might not be far off. We can't easily grab those libraries and consume them as in your project on code.google.com. But we can use easily installable linux tools (although that's not as portable) or existing maven plugins in maven central (like the batik-maven-plugin) PW Originally, I was going to set it up as a maven build but later switched to pure ant thinking full maven wouldn't be necessary for rendering icons. :) I will convert it to maven so we can use it in this project. (In reply to Paul Webster from comment #12) > (In reply to Tony McCrary from comment #9) > > > > The code needs to be cleaned up but it works: > > > > https://code.google.com/p/eclipse-svg-icons/source/browse/src/main/java/ > > RasterizerUtil.java > > OK, so the maven plugin might not be far off. > > We can't easily grab those libraries and consume them as in your project on > code.google.com. But we can use easily installable linux tools (although > that's not as portable) or existing maven plugins in maven central (like the > batik-maven-plugin) > > PW We're building the bundle with maven, so hopefully it shouldn't be that hard: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org.eclipse.ui.images/pom.xml PW (In reply to Paul Webster from comment #14) > We're building the bundle with maven, so hopefully it shouldn't be that > hard: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org. > eclipse.ui.images/pom.xml > > PW The current dependencies are batik (svg renderer), jhlabs filters (image processing) and java imaging scaling (lanczos resampling), which all appear to be in maven central and are licensed either ASL or BSD. There may be an issue with the jhlabs filters, as IIRC I had to build it from their revision control system for some newer features. I will compare it to the version in central. If batik-maven-plugin is good enough that might be the first thing to try, as it's already set up. PW (In reply to Paul Webster from comment #16) > If batik-maven-plugin is good enough that might be the first thing to try, > as it's already set up. > > PW ok cool, then I will hold off on converting the google code project. Created attachment 237320 [details]
A sample SVG to PNG conversion in maven
Here is an example maven conversion using org.codehaus.mojo:batik-maven-plugin. It's not part of the build, but is done with "mvn [options] batik:rasterize"
Things I learned:
1) The conversion for my example file seems reasonable to me, but we would have to see how they fair once they start getting used.
2) The converter doesn't maintain the path structure from srcDir to destDir. That means we need multiple executions of the plugin with different, more specific src/dest in the one pom. Someone with more maven knowledge will have to chime in here.
3) There are both multiple copies of icons that are the same *and* a few same-filename-but-different-icons in our group of icons.
PW
(In reply to Paul Webster from comment #18) > Created attachment 237320 [details] > A sample SVG to PNG conversion in maven > > Here is an example maven conversion using > org.codehaus.mojo:batik-maven-plugin. It's not part of the build, but is > done with "mvn [options] batik:rasterize" > > Things I learned: > > 1) The conversion for my example file seems reasonable to me, but we would > have to see how they fair once they start getting used. > > 2) The converter doesn't maintain the path structure from srcDir to destDir. > That means we need multiple executions of the plugin with different, more > specific src/dest in the one pom. Someone with more maven knowledge will > have to chime in here. > > 3) There are both multiple copies of icons that are the same *and* a few > same-filename-but-different-icons in our group of icons. > > PW One of the things you'll notice is that Batik distorts images when they're rendered at low resolutions. It's very subtle, but curves become angular, circles get corners, etc. The icons become wonky looking, I assume it some type of rounding precision issue. So to get accurate images, you'll need to render the icon at a larger size and scale it down. But be careful, you can't use any old scaling algorithm when doing this. Also, there are no desaturated icons, so you'll need to coax the maven plugin into generating them. I've created a new version of the RasterizerUtil that uses pure maven for it's dependencies from central (which are ASL and BSD, should be EPL compatible). I've tried to clean it up a bit and document a little more (just noticed some tabs got in there): RasterizerUtil.java - http://goo.gl/8KQLhQ pom.xml - http://goo.gl/txur5Q It's available at the following repo, in the "tmccrary" branch: git clone http://git.l33tlabs.org/eclipse.org/platform/eclipse.platform.ui.git git checkout tmccrary Right now, the rasterizer doesn't build on the command line due to some issue with how the tycho-compiler-plugin sets up the dependencies, but with mvn eclipse:eclipse the project builds within Eclipse itself. Once eclipse builds it, you can run it with: mvn exec:java I'll work on resolving that issue soon. I still have to figure how to get the RasterizerUtil class itself building via maven, executing as part of a phase like generate-resources and then continue with the tycho eclipse-plugin profile. One way would be to create a maven plugin project that this one depends on, another would be include a prebuilt jar w/ source that can be directly executed. I'm not sure what's feasible given the infrastructure and process constraints. I've pushed a maven plugin via gerrit that supplies the SVG renderer as a maven mojo. It adds a step in the generate-resources phase that renders the SVG icons. I was hoping you could include the plugin module in the build and have reactor sort the dependencies so that the plugin is available when building modules that depend on it. However, Maven ignores plugin dependencies when computing the build order. I'm not sure if it's a maven bug or if it does that intentionally for some reason. So right now, if you "mvn install" the plugin first and then run the eclipse.platform.ui build, the images get generated as you would expect. If you don't install the plugin first, the build fails because the plugin can't be resolved (obviously it isn't in central). I tried to find resources about anyone else who had this problem, but haven't found much so far: http://stackoverflow.com/questions/15040606/maven-plugin-as-module The solution of building the plugin first via profiles would work but I'm not sure how acceptable that will be, because it will make the build more complicated than "mvn install". (In reply to Tony McCrary from comment #21) > I've pushed a maven plugin via gerrit that supplies the SVG renderer as a > maven mojo. It adds a step in the generate-resources phase that renders the > SVG icons. > AFAIK plugins that are used for a build must already be "in" maven before that build starts, they can't be in the current build reactor. We're fine with checking in the PNGs, so could we modify the usecase for this? 1) The developer can use mvn clean install to install o.e.ui.images.renderer in their local cache 2) Then the developer uses mvn <somethingSpecific> in o.e.ui.images to regenerate the images into their appropriate locations in o.e.ui.images. 3) The developer checks in the changed PNGs 4) the regular mvn clean install in a build works as it does today. PW Sounds great. I'll get that ready and provide a new submission to gerrit. > The path for a PNG file is
> /org.eclipse.ui.images/eclipse.platform.ui/bundles/org.eclipse.ui/icons/full/
> obj16/info_tsk.png
>
> If we copy the PNG files into each bundle they come from, we have
> duplication but they're easy to consume. If we want to just delegate to the
> org.eclipse.ui.images bundle then I think we need to shorten the path.
>
> Option 1:
> platform:/plugin/org.eclipse.ui.images/org.eclipse.ui/icons/full/obj16/
> info_tsk.png
>
> We remove the repo information, as the bundle name is enough to keep the
> icons separate. Still a little long.
>
> Option 2:
> platform:/plugin/org.eclipse.ui.images/icons/full/obj16/info_tsk.png
>
> We remove the bundle name as well, so it really does mirror the icon layout
> in the bundles we have today. The downside is that some images have
> duplicate names in the different bundles. This might not be a problem if
> the images are identical. ex: in the plugin, there are 1535 PNG files. If
> I copy them all into a flatter structure, I get 1374 PNG files.
>
> PW
I like Option 2. This also reduces the complexity for people which wants to consume the icons. If Paul gives his OK for this option, I move some of the icons soon to the new folder to have a starting point. We would still have to review the duplicates and decided what to do with them.
Looks to me that we also have multiple duplicates with different file names:
find -not -empty -type f -name "*.svg" -printf "%s\n" | sort -rn | uniq -d | xargs -I{} -n1 find -type f -size {}c -print0 | xargs -0 md5sum | sort | uniq -w32 --all-repeated=separate
I think as we start using them, we can remove the redundant svg images and regenerate the resulting png files.
I think Option 1 is a good first step even if we later implement Option 2. I prepare a patch for it. Do we really need "/icons/full/"? Not all plug-ins are using that, some are only using "full" others using only "icons". I suggest to drop that from the path. Observation: Currently we have 827 svg files. I used the following command to determine this. find eclipse-svg -type f | wc -l (In reply to Lars Vogel from comment #28) > Observation: Currently we have 827 svg files. I used the following command > to determine this. > > find eclipse-svg -type f | wc -l Correction 684 files. Remove Java nature from plug-in as it is only a resource bundle: https://git.eclipse.org/r/18600 I suggest to change the current structure so that we group the generated output in one folder and remove the repo info from eclipse-svg. I also plan to leave the "icons/full" path segment in, this will make it easier to copy the files to the other plug-ins if required.
So we would have:
eclipse-svg
- org.eclipse.ant.ui
- icons
-full
- org.eclipse.debug.ui
- full
- ...
eclipse-png
- org.eclipse.ant.ui
- icons
-full
- org.eclipse.debug.ui
- full
- ...
I will adjust the Mojo from Tony and the folder structure if no one disagrees.
Suggest new structure: https://git.eclipse.org/r/18605 Fixed with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1348cdaa673e042ea615e7b4c7014d3d05f9aa8b https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4856d83f06e4683f31c63e3d41a32349212c3904 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=abe2bf29f49f93c0eb4b130e49fae16f60f3c126 See also Bug 422139, we potentially move the plug-in and rename it. |
For the new bundle, org.eclipse.ui.images, we should make some changes. - Standard bundle i18n - set the BREE to 1.5 or 1.4 - add the SVG to the source plugin - move the PNG folders to the /src folder - flatten the PNG structure. Maybe just src/bundle/icons.... ? Maybe src/bundle/obj16{etc)... ? PW