-
Notifications
You must be signed in to change notification settings - Fork 147
Add SVGs to org.eclipse.debug.ui #1797
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
Conversation
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.
There seems to be an issue with how SVGs are generated for a specific zoom since I face the same issue as mentioned in eclipse-platform/eclipse.platform.ui#2643 (review) for icons from Platform UI now also with the ones exchanged with SVGs in this PR:
How to reproduce:
- Two monitors, primary at 100% and secondary at 175%, monitor-specific scaling enabled
- Move window to secondary monitor
You can ignore my previous comment. I found the issue for the wrongly scaled SVGs: the Image implementation was erroneous. If the data for an image at the zoom level of the file (usually 100) was already loaded, this data is scaled instead of loading the file in proper zoom anew (as required for SVGs). That's easy to fix with a PR I provided today, so I added a fix that PR: |
64274a2
to
26558b5
Compare
When we merge this, we should also have eclipse-platform/eclipse.platform.swt#1936 available, shouldn't we? Otherwise I expect the disabled SVGs to not look good. |
On the other hand I recall a (private) discussion with Heiko where he said, that the current state of that SWT PR is not yet fully ready and needs further refinement. |
The disabled icons are not affected by this. I only changed the paths of the normal icons to SVG and added the SVGs. I ignored all disabled icons so they will look exactly as before. |
This commit add all SVGs of org.eclipse.debug.ui except for the following as these are not available as SVG yet: obj16: launchConfiguration.svg ov15: quickaccess_debug quickaccess_profile quickaccess_run wizban: adddir_wiz addsrcloc_wiz debug_wiz editdir_wiz edtsrclkup_wiz export_brkpts_wizban export_config_wizban import_brkpts_wizban import_config_wizban profile_wiz run_wiz
26558b5
to
7f33b86
Compare
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.
Thank you for this work! The change looks good and it's great to finally have the first SVG icons in real use!
Altough this contributon is much larger than 1000 LOC (which usually requires an IP-review), this is fine because all the SVG files originate from https://github.com/eclipse-platform/eclipse.platform.images.
Eventually I think all SVG files that have been moved into the using plugin should be removed from that repo to avoid maintaining it at two different places.
As soon as eclipse-platform/eclipse.platform.ui#2593 is submitted, this can be submitted too.
helpContextId="run_last_action_context" | ||
label="%ContextLaunchingRunMenu.name" | ||
menubarPath="org.eclipse.ui.run/relaunchGroup"> | ||
</action> | ||
<action | ||
id="org.eclipse.debug.internal.ui.actions.RunDropDownAction" | ||
toolbarPath="org.eclipse.debug.ui.launchActionSet/debug" | ||
hoverIcon="$nl$/icons/full/etool16/run_exc.png" | ||
hoverIcon="$nl$/icons/full/etool16/run_exc.svg" |
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.
As the doc of hoverIcon says:
a relative path of an icon used to visually represent the action in its context when the mouse pointer is over the action.
If omitted, the normal icon will be used. The path is relative to the location of the plugin.xml file of the contributing plug-in.
I think we could just remove the hoverIcon
attribute with a value equal to the icon
attribute.
But since the current state doesn't hurt, I think this can be done in a follow-up.
maybe a stupid question: Do I need to activate the rendering of the SVG icons somehow on macOS? |
No you do not need to activate something, every icon will automatically be rendered using SVGs but to see an improvement it is important to use the right autoScaling mode. On Windows the default autoScaling mode is set to I did not test it yet on MacOs but can you try adding the following line in your Please note that for now only the bundle |
I did this (added to the launch config not the ini file) but I could not see a difference on my macOS retina display. Is the scaling on macOS so good? |
I think so. In the discussions I read, before I implemented this feature, it seemed like people using macOS had less problems with scaling icons. The new smooth scaling shouldn't be activated for macOS as far as I know. Can you please try also adding |
MacOS does only support 100% and 200% scaling. Since we had already provided the rasterized versions for those two scale values, you will not see any difference on MacOS. They will be equally sharp or blurry (like everything on MacOS) as they were before. |
This PR adds all SVGs of
org.eclipse.debug.ui
and changes related paths except for the following as these are not available as SVG yet:obj16:
launchConfiguration.svg
ovr16:
quickaccess_debug
quickaccess_profile
quickaccess_run
wizban:
adddir_wiz
addsrcloc_wiz
debug_wiz
editdir_wiz
edtsrclkup_wiz
export_brkpts_wizban
export_config_wizban
import_brkpts_wizban
import_config_wizban
profile_wiz
run_wiz
The changes can be seen in the toolbar of the debug perspective:
Default (PNG):

Updated (SVG):

We should add this change in M1 so we can properly test the SVG feature that was introduced in this PR with a bundle that contains SVGs.